RE: [PATCH] VNIF: Using smart polling instead of event notification.

RE: [PATCH] VNIF: Using smart polling instead of event notification.

[Xu, Dongxiao]

    We found that the event notification frequency is still high in some network cases. NAPI polls only for a little
time slot and does not efficient enough in our backend/frontend case. Actually our patch repeated calling NAPI
interface to do more polling, and netback will NOT notify netfront during this period. Once netfront polling out
all the data, and finds that there is no more data arrive/send during the next 100ms, the timer will stop working 
to end the polling. 
    This filed 'smart_poll_active' is shared by netfront and netback, to indicate whether netfront is polling data. 
So this filed is necessary for netback to notify netfront if this flag is not set. 
This field is different from the flag in xenstore, which indicates whether other-end has this new feature. If other-end doesn't support the new feature, everything goes in the original way. 

Thanks!
Dongxiao

________________________________________
From: Ian Campbell [Ian.Campbell@citrix.com]
Sent: Thursday, October 01, 2009 3:03 AM
To: Xu, Dongxiao
Cc: xen-devel@lists.xensource.com; Keir Fraser
Subject: Re: [Xen-devel][PATCH] VNIF: Using smart polling instead of event      notification.

You are adding a netfront specific field to the generic ring structure?
That seems rather ugly.

Is this even necessary as a piece of shared state? Once netback and
netfront have agreed, via xenstore, to use the feature netback seems to
set the flag every time it would have previously notified netfront.

Ian.

On Thu, 2009-10-01 at 01:22 +0100, Xu, Dongxiao wrote:
> Resend and put the patch in attachment.
>
> Patch the Xen version of ring.h
>
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
>
> diff -r 8fc927798476 xen/include/public/io/ring.h
> --- a/xen/include/public/io/ring.h      Tue Sep 01 11:36:51 2009 +0100
> +++ b/xen/include/public/io/ring.h      Thu Oct 01 02:11:45 2009 +0800
> @@ -97,7 +97,8 @@ struct __name##_sring {
>  struct __name##_sring {                                                 \
>      RING_IDX req_prod, req_event;                                       \
>      RING_IDX rsp_prod, rsp_event;                                       \
> -    uint8_t  pad[48];                                                   \
> +    uint8_t  netfront_smartpoll_active;                                 \
> +    uint8_t  pad[47];                                                   \
>      union __name##_sring_entry ring[1]; /* variable-length */           \
>  };                                                                      \
>                                                                          \
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] VNIF: Using smart polling instead of event notification.

[Paul Durrant]

Xu, Dongxiao wrote:
>     We found that the event notification frequency is still high in some network cases. NAPI polls only for a little
> time slot and does not efficient enough in our backend/frontend case. Actually our patch repeated calling NAPI
> interface to do more polling, and netback will NOT notify netfront during this period. Once netfront polling out
> all the data, and finds that there is no more data arrive/send during the next 100ms, the timer will stop working 
> to end the polling. 
>     This filed 'smart_poll_active' is shared by netfront and netback, to indicate whether netfront is polling data. 
> So this filed is necessary for netback to notify netfront if this flag is not set. 
> This field is different from the flag in xenstore, which indicates whether other-end has this new feature. If other-end doesn't support the new feature, everything goes in the original way. 
> 

If you want to prevent event notification, why not simply mask out your 
port?

   Paul

RE: [PATCH] VNIF: Using smart polling instead of event notification.

[Ian Campbell]

On Thu, 2009-10-01 at 15:02 +0100, Xu, Dongxiao wrote:
> We found that the event notification frequency is still high in some network cases. NAPI polls only for a little
> time slot and does not efficient enough in our backend/frontend case. Actually our patch repeated calling NAPI
> interface to do more polling, and netback will NOT notify netfront during this period. Once netfront polling out
> all the data, and finds that there is no more data arrive/send during the next 100ms, the timer will stop working 
> to end the polling. 

Isn't that an argument for improving NAPI rather than coding workarounds
into each driver? As physical NICs increase in speed it seems like NAPI
will need to increase its efficiency in a similar manner.

It would be interesting to run the patches past netdev@vger.

>     This filed 'smart_poll_active' is shared by netfront and netback,
> to indicate whether netfront is polling data. 
> So this filed is necessary for netback to notify netfront if this flag
> is not set. 

Right, but that doesn't justify sticking a netfront specific field in a
generic ring structure.

I can't help but wonder if there is some better way to communicate this
shared state. For instance netback sets the flag every time it notifies
the frontend, therefore isn't receiving an interrupt on the frontend
enough to infer that smart polling should be marked as active? Or can
this information be encoded into sring->{req,rsp}_event and the
smartpoll checks integrated into RING_PUSH_*_AND_CHECK_NOTIFY?

At the very least the field should be called {req,rsp}_smartpoll_active
to allow other consumers of the generic ring structure to use it if they
wish.

Ian.

> 
> Thanks!
> Dongxiao
> 
> ________________________________________
> From: Ian Campbell [Ian.Campbell@citrix.com]
> Sent: Thursday, October 01, 2009 3:03 AM
> To: Xu, Dongxiao
> Cc: xen-devel@lists.xensource.com; Keir Fraser
> Subject: Re: [Xen-devel][PATCH] VNIF: Using smart polling instead of event      notification.
> 
> You are adding a netfront specific field to the generic ring structure?
> That seems rather ugly.
> 
> Is this even necessary as a piece of shared state? Once netback and
> netfront have agreed, via xenstore, to use the feature netback seems to
> set the flag every time it would have previously notified netfront.
> 
> Ian.
> 
> On Thu, 2009-10-01 at 01:22 +0100, Xu, Dongxiao wrote:
> > Resend and put the patch in attachment.
> >
> > Patch the Xen version of ring.h
> >
> > Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> >
> > diff -r 8fc927798476 xen/include/public/io/ring.h
> > --- a/xen/include/public/io/ring.h      Tue Sep 01 11:36:51 2009 +0100
> > +++ b/xen/include/public/io/ring.h      Thu Oct 01 02:11:45 2009 +0800
> > @@ -97,7 +97,8 @@ struct __name##_sring {
> >  struct __name##_sring {                                                 \
> >      RING_IDX req_prod, req_event;                                       \
> >      RING_IDX rsp_prod, rsp_event;                                       \
> > -    uint8_t  pad[48];                                                   \
> > +    uint8_t  netfront_smartpoll_active;                                 \
> > +    uint8_t  pad[47];                                                   \
> >      union __name##_sring_entry ring[1]; /* variable-length */           \
> >  };                                                                      \
> >                                                                          \
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel

Re: [PATCH] VNIF: Using smart polling instead of event notification.

[Jeremy Fitzhardinge]

On 10/02/09 02:41, Ian Campbell wrote:
> On Thu, 2009-10-01 at 15:02 +0100, Xu, Dongxiao wrote:
>   
>> We found that the event notification frequency is still high in some network cases. NAPI polls only for a little
>> time slot and does not efficient enough in our backend/frontend case. Actually our patch repeated calling NAPI
>> interface to do more polling, and netback will NOT notify netfront during this period. Once netfront polling out
>> all the data, and finds that there is no more data arrive/send during the next 100ms, the timer will stop working 
>> to end the polling. 
>>     
> Isn't that an argument for improving NAPI rather than coding workarounds
> into each driver? As physical NICs increase in speed it seems like NAPI
> will need to increase its efficiency in a similar manner.
>
> It would be interesting to run the patches past netdev@vger.
>   

Yes.  NAPI is intended to address precisely this issue, so it should be
made to work.

>>     This filed 'smart_poll_active' is shared by netfront and netback,
>> to indicate whether netfront is polling data. 
>> So this filed is necessary for netback to notify netfront if this flag
>> is not set. 
>>     
> Right, but that doesn't justify sticking a netfront specific field in a
> generic ring structure.
>   

This kind of poll/interrupt switch isn't network-specific though; this
just happens to be the first user.   Presumably other devices with a
high rate of small messages could also take advantage of it.

    J

[PATCH] VNIF: Using smart polling instead of event notification.

[Xu, Dongxiao]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

Resend and put the patch in attachment. 

Patch the Xen version of ring.h

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

diff -r 8fc927798476 xen/include/public/io/ring.h
--- a/xen/include/public/io/ring.h      Tue Sep 01 11:36:51 2009 +0100
+++ b/xen/include/public/io/ring.h      Thu Oct 01 02:11:45 2009 +0800
@@ -97,7 +97,8 @@ struct __name##_sring {
 struct __name##_sring {                                                 \
     RING_IDX req_prod, req_event;                                       \
     RING_IDX rsp_prod, rsp_event;                                       \
-    uint8_t  pad[48];                                                   \
+    uint8_t  netfront_smartpoll_active;                                 \
+    uint8_t  pad[47];                                                   \
     union __name##_sring_entry ring[1]; /* variable-length */           \
 };                                                                      \
                                                                         \
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[-- Attachment #2: patch_xen_ring_h.patch --]
[-- Type: text/plain, Size: 916 bytes --]

diff -r 8fc927798476 xen/include/public/io/ring.h
--- a/xen/include/public/io/ring.h	Tue Sep 01 11:36:51 2009 +0100
+++ b/xen/include/public/io/ring.h	Thu Oct 01 08:16:37 2009 +0800
@@ -97,7 +97,8 @@ struct __name##_sring {                 
 struct __name##_sring {                                                 \
     RING_IDX req_prod, req_event;                                       \
     RING_IDX rsp_prod, rsp_event;                                       \
-    uint8_t  pad[48];                                                   \
+    uint8_t  netfront_smartpoll_active;                                 \
+    uint8_t  pad[47];                                                   \
     union __name##_sring_entry ring[1]; /* variable-length */           \
 };                                                                      \
                                                                         \

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] VNIF: Using smart polling instead of event notification.

[Ian Campbell]

You are adding a netfront specific field to the generic ring structure?
That seems rather ugly. 

Is this even necessary as a piece of shared state? Once netback and
netfront have agreed, via xenstore, to use the feature netback seems to
set the flag every time it would have previously notified netfront.

Ian.

On Thu, 2009-10-01 at 01:22 +0100, Xu, Dongxiao wrote:
> Resend and put the patch in attachment. 
> 
> Patch the Xen version of ring.h
> 
> Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>
> 
> diff -r 8fc927798476 xen/include/public/io/ring.h
> --- a/xen/include/public/io/ring.h      Tue Sep 01 11:36:51 2009 +0100
> +++ b/xen/include/public/io/ring.h      Thu Oct 01 02:11:45 2009 +0800
> @@ -97,7 +97,8 @@ struct __name##_sring {
>  struct __name##_sring {                                                 \
>      RING_IDX req_prod, req_event;                                       \
>      RING_IDX rsp_prod, rsp_event;                                       \
> -    uint8_t  pad[48];                                                   \
> +    uint8_t  netfront_smartpoll_active;                                 \
> +    uint8_t  pad[47];                                                   \
>      union __name##_sring_entry ring[1]; /* variable-length */           \
>  };                                                                      \
>                                                                          \
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

[PATCH] VNIF: Using smart polling instead of event notification.

[Xu, Dongxiao]

Patch the Xen version of ring.h

Signed-off-by: Dongxiao Xu <dongxiao.xu@intel.com>

diff -r 8fc927798476 xen/include/public/io/ring.h
--- a/xen/include/public/io/ring.h      Tue Sep 01 11:36:51 2009 +0100
+++ b/xen/include/public/io/ring.h      Thu Oct 01 02:11:45 2009 +0800
@@ -97,7 +97,8 @@ struct __name##_sring {
 struct __name##_sring {                                                 \
     RING_IDX req_prod, req_event;                                       \
     RING_IDX rsp_prod, rsp_event;                                       \
-    uint8_t  pad[48];                                                   \
+    uint8_t  netfront_smartpoll_active;                                 \
+    uint8_t  pad[47];                                                   \
     union __name##_sring_entry ring[1]; /* variable-length */           \
 };                                                                      \
                                                                         \