thr3ads.net - Xen devel - [Xen-devel] [PATCH] make protocol specific usage of shared sring explicit [Jul 2010]

If this information is useful, please help other people find it:
Share via:

Ian Campbell

2010-Jul-01 09:22 UTC

[Xen-devel] [PATCH] make protocol specific usage of shared sring explicit

I don''t think protocol specific data''t really belongs in this
header
but since it is already there and we seem to be stuck with it lets at
least make the users explicit lest people get caught out by future new
fields moving the pad field around.

This is the Xen portion of this change. The kernel portion will be
sent separately. There is no dependency between the two.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Daniel Stodden <daniel.stodden@citrix.com>
Cc: Dongxiao Xu <dongxiao.xu@intel.com>

diff -r aecf092da748 -r 9fb06660a061 tools/blktap2/drivers/tapdisk-vbd.c
--- a/tools/blktap2/drivers/tapdisk-vbd.c	Wed Jun 30 22:12:54 2010 +0100
+++ b/tools/blktap2/drivers/tapdisk-vbd.c	Thu Jul 01 10:20:07 2010 +0100
@@ -1684,7 +1684,7 @@
 	if (!vbd->ring.sring)
 		return -EINVAL;
 
-	switch (vbd->ring.sring->pad[0]) {
+	switch (vbd->ring.sring->private.tapif_user.msg) {
 	case 0:
 		return 0;
 
diff -r aecf092da748 -r 9fb06660a061 xen/include/public/io/ring.h
--- a/xen/include/public/io/ring.h	Wed Jun 30 22:12:54 2010 +0100
+++ b/xen/include/public/io/ring.h	Thu Jul 01 10:20:07 2010 +0100
@@ -103,8 +103,16 @@
 struct __name##_sring {                                                 \
     RING_IDX req_prod, req_event;                                       \
     RING_IDX rsp_prod, rsp_event;                                       \
-    uint8_t  netfront_smartpoll_active;                                 \
-    uint8_t  pad[47];                                                   \
+    union {                                                             \
+        struct {                                                        \
+            uint8_t smartpoll_active;                                   \
+        } netif;                                                        \
+        struct {                                                        \
+            uint8_t msg;                                                \
+        } tapif_user;                                                   \
+        uint8_t pvt_pad[4];                                             \
+    } private;                                                          \
+    uint8_t pad[44];                                                    \
     union __name##_sring_entry ring[1]; /* variable-length */           \
 };                                                                      \
                                                                         \

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Jul-01 09:22 UTC

head link

[Xen-devel] [PATCH] make protocol specific usage of shared sring explicit

I don''t think protocol specific data''t really belongs in this
header
but since it is already there and we seem to be stuck with it lets at
least make the users explicit lest people get caught out by future new
fields moving the pad field around.

This is the kernel portion of this change. The Xen portion was sent
separately. There is no dependency between the two.

I was unable to find the netfront portion of the smartpoll code in the
merge branches. Looks like it is in the xen/netfront branch but not
merged anywhere.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Daniel Stodden <daniel.stodden@citrix.com>
Cc: Dongxiao Xu <dongxiao.xu@intel.com>
---
 drivers/xen/blktap/ring.c       |    4 ++--
 drivers/xen/blktap/sysfs.c      |    3 ++-
 drivers/xen/netback/netback.c   |   13 ++++++-------
 include/xen/interface/io/ring.h |   12 ++++++++++--
 4 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/xen/blktap/ring.c b/drivers/xen/blktap/ring.c
index b49c478..d7d0c79 100644
--- a/drivers/xen/blktap/ring.c
+++ b/drivers/xen/blktap/ring.c
@@ -335,7 +335,7 @@ blktap_ring_set_message(struct blktap *tap, int msg)
 	struct blktap_ring *ring = &tap->ring;
 
 	if (ring->ring.sring)
-		ring->ring.sring->pad[0] = msg;
+		ring->ring.sring->private.tapif_user.msg = msg;
 }
 
 static int
@@ -400,7 +400,7 @@ static unsigned int blktap_ring_poll(struct file *filp,
poll_table *wait)
 	up_read(&current->mm->mmap_sem);
 
 	if (work ||
-	    ring->ring.sring->pad[0])
+	    ring->ring.sring->private.tapif_user.msg)
 		return POLLIN | POLLRDNORM;
 
 	return 0;
diff --git a/drivers/xen/blktap/sysfs.c b/drivers/xen/blktap/sysfs.c
index 49f5e67..53f3b95 100644
--- a/drivers/xen/blktap/sysfs.c
+++ b/drivers/xen/blktap/sysfs.c
@@ -97,6 +97,7 @@ blktap_sysfs_remove_device(struct device *dev,
 			   const char *buf, size_t size)
 {
 	struct blktap *tap = (struct blktap *)dev_get_drvdata(dev);
+	struct blktap_ring *ring = &tap->ring;
 
 	if (!tap->ring.dev)
 		return size;
@@ -105,7 +106,7 @@ blktap_sysfs_remove_device(struct device *dev,
 		return -EBUSY;
 
 	BTDBG("sending tapdisk close message\n");
-	tap->ring.ring.sring->pad[0] = BLKTAP2_RING_MESSAGE_CLOSE;
+	ring->sring->private.tapif_user.msg = BLKTAP2_RING_MESSAGE_CLOSE;
 	blktap_ring_kick_user(tap);
 	wait_event_interruptible(tap->wq,
 				 !test_bit(BLKTAP_CONTROL, &tap->dev_inuse));
diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
index 9a7ada2..29b60c8 100644
--- a/drivers/xen/netback/netback.c
+++ b/drivers/xen/netback/netback.c
@@ -729,11 +729,10 @@ static void net_rx_action(unsigned long data)
 		 * netfront_smartpoll_active indicates whether
 		 * netfront timer is active.
 		 */
-		if ((netif->smart_poll == 1)) {
-			if (!(netif->rx.sring->netfront_smartpoll_active)) {
-				notify_remote_via_irq(irq);
-				netif->rx.sring->netfront_smartpoll_active = 1;
-			}
+		if ((netif->smart_poll == 1) &&
+		    !(netif->rx.sring->private.netif.smartpoll_active)) {
+			notify_remote_via_irq(irq);
+			netif->rx.sring->private.netif.smartpoll_active = 1;
 		}
 
 		netif_put(netif);
@@ -1612,9 +1611,9 @@ static void make_tx_response(struct xen_netif *netif,
 	 * is active.
 	 */
 	if ((netif->smart_poll == 1)) {
-		if (!(netif->rx.sring->netfront_smartpoll_active)) {
+		if (!(netif->rx.sring->private.netif.smartpoll_active)) {
 			notify_remote_via_irq(netif->irq);
-			netif->rx.sring->netfront_smartpoll_active = 1;
+			netif->rx.sring->private.netif.smartpoll_active = 1;
 		}
 	} else if (notify)
 		notify_remote_via_irq(netif->irq);
diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 865dcf0..7b301fa 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -73,8 +73,16 @@ union __name##_sring_entry {						\
 struct __name##_sring {							\
     RING_IDX req_prod, req_event;					\
     RING_IDX rsp_prod, rsp_event;					\
-    uint8_t  netfront_smartpoll_active;					\
-    uint8_t  pad[47];							\
+    union {								\
+        struct {							\
+            uint8_t smartpoll_active;					\
+        } netif;							\
+        struct {							\
+            uint8_t msg;						\
+        } tapif_user;							\
+        uint8_t pvt_pad[4];						\
+    } private;								\
+    uint8_t pad[44];							\
     union __name##_sring_entry ring[1]; /* variable-length */		\
 };									\
 									\
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Jul-01 09:39 UTC

head link

[Xen-devel] Re: [PATCH] make protocol specific usage of shared sring explicit

On Thu, 2010-07-01 at 10:22 +0100, Ian Campbell wrote:> I don''t think protocol specific data''t really belongs in
this header
> but since it is already there and we seem to be stuck with it lets at
> least make the users explicit lest people get caught out by future new
> fields moving the pad field around.
The blktap use of the pad field was/is used as a back channel of sorts
to communicate device shutdown from the toolstack to the userspace
tapdisk process via a sysfs write which set the flag in pad[0] which the
tapdisk userspace picked up. This has now been replaced by a unix domain
socket control interface (tap-ctl) allowing communication directly
between the toolstack and the tapdisk process (a much cleaner and
preferable interface). 

However the interface between userspace and kernel was inadvertently
changed when the netfront smartpoll feature was added to netback due to
the new field in the shared ring which caused the the poll[0] field to
move up by one in the structure.

On the kernel side smartpoll change was apparently merged into
xen/master in October 2009 and on the hypervisor side the it is present
in Xen 4.0. So it seems that we have combinations in the wild of kernel
blktap and tapdisk userspace both independently using one of two
possible offsets of pad[0].

Perhaps this interface would be better deprecated and removed sooner
rather than later? Is the tap-ctl baked enough to consider for Xen
4.0.1?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2010-Jul-01 09:58 UTC

head link

Re: [Xen-devel] [PATCH] make protocol specific usage of shared sring explicit

>>> On 01.07.10 at 11:22, Ian Campbell <ian.campbell@citrix.com>
wrote:
> --- a/xen/include/public/io/ring.h	Wed Jun 30 22:12:54 2010 +0100
> +++ b/xen/include/public/io/ring.h	Thu Jul 01 10:20:07 2010 +0100
> @@ -103,8 +103,16 @@
>  struct __name##_sring {                                                 \
>      RING_IDX req_prod, req_event;                                       \
>      RING_IDX rsp_prod, rsp_event;                                       \
> -    uint8_t  netfront_smartpoll_active;                                 \
> -    uint8_t  pad[47];                                                   \
> +    union {                                                             \
> +        struct {                                                        \
> +            uint8_t smartpoll_active;                                   \
> +        } netif;                                                        \
> +        struct {                                                        \
> +            uint8_t msg;                                                \
> +        } tapif_user;                                                   \
> +        uint8_t pvt_pad[4];                                             \
> +    } private;                                                          \
> +    uint8_t pad[44];                                                    \
Since this is source incompatible for consumers anyway, wouldn''t
it be better to rename the new pad[] to e.g. _pad[], so that builds
of unchanged consumers fail instead of producing runtime
incompatible code?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Jul-01 10:09 UTC

head link

Re: [Xen-devel] [PATCH] make protocol specific usage of shared sring explicit

On Thu, 2010-07-01 at 10:58 +0100, Jan Beulich wrote:> >>> On 01.07.10 at 11:22, Ian Campbell
<ian.campbell@citrix.com> wrote:
> > --- a/xen/include/public/io/ring.h	Wed Jun 30 22:12:54 2010 +0100
> > +++ b/xen/include/public/io/ring.h	Thu Jul 01 10:20:07 2010 +0100
> > @@ -103,8 +103,16 @@
> >  struct __name##_sring {                                              
\
> >      RING_IDX req_prod, req_event;                                    
\
> >      RING_IDX rsp_prod, rsp_event;                                    
\
> > -    uint8_t  netfront_smartpoll_active;                              
\
> > -    uint8_t  pad[47];                                                
\
> > +    union {                                                          
\
> > +        struct {                                                     
\
> > +            uint8_t smartpoll_active;                                
\
> > +        } netif;                                                     
\
> > +        struct {                                                     
\
> > +            uint8_t msg;                                             
\
> > +        } tapif_user;                                                
\
> > +        uint8_t pvt_pad[4];                                          
\
> > +    } private;                                                       
\
> > +    uint8_t pad[44];                                                 
\
> 
> Since this is source incompatible for consumers anyway, wouldn''t
> it be better to rename the new pad[] to e.g. _pad[], so that builds
> of unchanged consumers fail instead of producing runtime
> incompatible code?
Yes, that''s a good idea. I''ll follow up with a patch to use
__pad (I
think the single _ namespace is reserved for libc or some such)

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Jul-01 10:22 UTC

head link

[Xen-devel] [PATCH] make shared ring pad field less enticing for users by adding a __ prefix

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Daniel Stodden <daniel.stodden@citrix.com>
Cc: Dongxiao Xu <dongxiao.xu@intel.com>

diff -r 0918ea363872 -r 15ff04882377 xen/include/public/io/ring.h
--- a/xen/include/public/io/ring.h	Thu Jul 01 11:18:21 2010 +0100
+++ b/xen/include/public/io/ring.h	Thu Jul 01 11:19:33 2010 +0100
@@ -112,7 +112,7 @@
         } tapif_user;                                                   \
         uint8_t pvt_pad[4];                                             \
     } private;                                                          \
-    uint8_t pad[44];                                                    \
+    uint8_t __pad[44];                                                  \
     union __name##_sring_entry ring[1]; /* variable-length */           \
 };                                                                      \
                                                                         \
@@ -156,7 +156,8 @@
 #define SHARED_RING_INIT(_s) do {                                       \
     (_s)->req_prod  = (_s)->rsp_prod  = 0;                              \
     (_s)->req_event = (_s)->rsp_event = 1;                              \
-    (void)memset((_s)->pad, 0, sizeof((_s)->pad));                      \
+    (void)memset((_s)->private.pvt_pad, 0,
sizeof((_s)->private.pvt_pad)); \
+    (void)memset((_s)->__pad, 0, sizeof((_s)->__pad));                  \
 } while(0)
 
 #define FRONT_RING_INIT(_r, _s, __size) do {                            \

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell

2010-Jul-01 10:23 UTC

head link

[Xen-devel] [PATCH] make shared ring pad field less enticing for users by adding a __ prefix

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: Daniel Stodden <daniel.stodden@citrix.com>
Cc: Dongxiao Xu <dongxiao.xu@intel.com>
---
 include/xen/interface/io/ring.h |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
index 7b301fa..fae3296 100644
--- a/include/xen/interface/io/ring.h
+++ b/include/xen/interface/io/ring.h
@@ -82,7 +82,7 @@ struct __name##_sring {							\
         } tapif_user;							\
         uint8_t pvt_pad[4];						\
     } private;								\
-    uint8_t pad[44];							\
+    uint8_t __pad[44];							\
     union __name##_sring_entry ring[1]; /* variable-length */		\
 };									\
 									\
@@ -121,7 +121,8 @@ struct __name##_back_ring {						\
 #define SHARED_RING_INIT(_s) do {					\
     (_s)->req_prod  = (_s)->rsp_prod  = 0;				\
     (_s)->req_event = (_s)->rsp_event = 1;				\
-    memset((_s)->pad, 0, sizeof((_s)->pad));				\
+    memset((_s)->private.pvt_pad, 0, sizeof((_s)->private.pvt_pad));	\
+    memset((_s)->__pad, 0, sizeof((_s)->__pad));			\
 } while(0)
 
 #define FRONT_RING_INIT(_r, _s, __size) do {				\
-- 
1.5.6.5


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2010-Jul-05 07:06 UTC

head link

Re: [Xen-devel] [PATCH] make protocol specific usage of shared sring explicit

Could we get the relevant bits (the netback part apparently needs to
be dropped) of this change also into the 2.6.18 tree please, so that
issues won''t arise the next time the public headers get sync-ed over
as a whole?

Thanks, Jan
>>> On 01.07.10 at 11:22, Ian Campbell <ian.campbell@citrix.com>
wrote:
> I don''t think protocol specific data''t really belongs in
this header
> but since it is already there and we seem to be stuck with it lets at
> least make the users explicit lest people get caught out by future new
> fields moving the pad field around.
> 
> This is the kernel portion of this change. The Xen portion was sent
> separately. There is no dependency between the two.
As a side note: This isn''t really true, as the patch moves the field
blktap uses, and hence tools and kernel ought to be changed at
the same time.
 > I was unable to find the netfront portion of the smartpoll code in the
> merge branches. Looks like it is in the xen/netfront branch but not
> merged anywhere.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Jeremy Fitzhardinge <jeremy@goop.org>
> Cc: Daniel Stodden <daniel.stodden@citrix.com>
> Cc: Dongxiao Xu <dongxiao.xu@intel.com>
> ---
>  drivers/xen/blktap/ring.c       |    4 ++--
>  drivers/xen/blktap/sysfs.c      |    3 ++-
>  drivers/xen/netback/netback.c   |   13 ++++++-------
>  include/xen/interface/io/ring.h |   12 ++++++++++--
>  4 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/xen/blktap/ring.c b/drivers/xen/blktap/ring.c
> index b49c478..d7d0c79 100644
> --- a/drivers/xen/blktap/ring.c
> +++ b/drivers/xen/blktap/ring.c
> @@ -335,7 +335,7 @@ blktap_ring_set_message(struct blktap *tap, int msg)
>  	struct blktap_ring *ring = &tap->ring;
>  
>  	if (ring->ring.sring)
> -		ring->ring.sring->pad[0] = msg;
> +		ring->ring.sring->private.tapif_user.msg = msg;
>  }
>  
>  static int
> @@ -400,7 +400,7 @@ static unsigned int blktap_ring_poll(struct file *filp,
> poll_table *wait)
>  	up_read(&current->mm->mmap_sem);
>  
>  	if (work ||
> -	    ring->ring.sring->pad[0])
> +	    ring->ring.sring->private.tapif_user.msg)
>  		return POLLIN | POLLRDNORM;
>  
>  	return 0;
> diff --git a/drivers/xen/blktap/sysfs.c b/drivers/xen/blktap/sysfs.c
> index 49f5e67..53f3b95 100644
> --- a/drivers/xen/blktap/sysfs.c
> +++ b/drivers/xen/blktap/sysfs.c
> @@ -97,6 +97,7 @@ blktap_sysfs_remove_device(struct device *dev,
>  			   const char *buf, size_t size)
>  {
>  	struct blktap *tap = (struct blktap *)dev_get_drvdata(dev);
> +	struct blktap_ring *ring = &tap->ring;
>  
>  	if (!tap->ring.dev)
>  		return size;
> @@ -105,7 +106,7 @@ blktap_sysfs_remove_device(struct device *dev,
>  		return -EBUSY;
>  
>  	BTDBG("sending tapdisk close message\n");
> -	tap->ring.ring.sring->pad[0] = BLKTAP2_RING_MESSAGE_CLOSE;
> +	ring->sring->private.tapif_user.msg = BLKTAP2_RING_MESSAGE_CLOSE;
>  	blktap_ring_kick_user(tap);
>  	wait_event_interruptible(tap->wq,
>  				 !test_bit(BLKTAP_CONTROL, &tap->dev_inuse));
> diff --git a/drivers/xen/netback/netback.c b/drivers/xen/netback/netback.c
> index 9a7ada2..29b60c8 100644
> --- a/drivers/xen/netback/netback.c
> +++ b/drivers/xen/netback/netback.c
> @@ -729,11 +729,10 @@ static void net_rx_action(unsigned long data)
>  		 * netfront_smartpoll_active indicates whether
>  		 * netfront timer is active.
>  		 */
> -		if ((netif->smart_poll == 1)) {
> -			if (!(netif->rx.sring->netfront_smartpoll_active)) {
> -				notify_remote_via_irq(irq);
> -				netif->rx.sring->netfront_smartpoll_active = 1;
> -			}
> +		if ((netif->smart_poll == 1) &&
> +		    !(netif->rx.sring->private.netif.smartpoll_active)) {
> +			notify_remote_via_irq(irq);
> +			netif->rx.sring->private.netif.smartpoll_active = 1;
>  		}
>  
>  		netif_put(netif);
> @@ -1612,9 +1611,9 @@ static void make_tx_response(struct xen_netif *netif,
>  	 * is active.
>  	 */
>  	if ((netif->smart_poll == 1)) {
> -		if (!(netif->rx.sring->netfront_smartpoll_active)) {
> +		if (!(netif->rx.sring->private.netif.smartpoll_active)) {
>  			notify_remote_via_irq(netif->irq);
> -			netif->rx.sring->netfront_smartpoll_active = 1;
> +			netif->rx.sring->private.netif.smartpoll_active = 1;
>  		}
>  	} else if (notify)
>  		notify_remote_via_irq(netif->irq);
> diff --git a/include/xen/interface/io/ring.h
b/include/xen/interface/io/ring.h
> index 865dcf0..7b301fa 100644
> --- a/include/xen/interface/io/ring.h
> +++ b/include/xen/interface/io/ring.h
> @@ -73,8 +73,16 @@ union __name##_sring_entry {						\
>  struct __name##_sring {							\
>      RING_IDX req_prod, req_event;					\
>      RING_IDX rsp_prod, rsp_event;					\
> -    uint8_t  netfront_smartpoll_active;					\
> -    uint8_t  pad[47];							\
> +    union {								\
> +        struct {							\
> +            uint8_t smartpoll_active;					\
> +        } netif;							\
> +        struct {							\
> +            uint8_t msg;						\
> +        } tapif_user;							\
> +        uint8_t pvt_pad[4];						\
> +    } private;								\
> +    uint8_t pad[44];							\
>      union __name##_sring_entry ring[1]; /* variable-length */		\
>  };									\
>  									\



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Jul 2010 - [PATCH] make protocol specific usage of shared sring explicit

[Xen-devel] [PATCH] make protocol specific usage of shared sring explicit

[Xen-devel] [PATCH] make protocol specific usage of shared sring explicit

[Xen-devel] Re: [PATCH] make protocol specific usage of shared sring explicit

Re: [Xen-devel] [PATCH] make protocol specific usage of shared sring explicit

Re: [Xen-devel] [PATCH] make protocol specific usage of shared sring explicit

[Xen-devel] [PATCH] make shared ring pad field less enticing for users by adding a __ prefix

[Xen-devel] [PATCH] make shared ring pad field less enticing for users by adding a __ prefix

Re: [Xen-devel] [PATCH] make protocol specific usage of shared sring explicit