Re: [Xen-devel] [PATCH V4 2/3] xen-netback: switch to NAPI + kthread 1:1 model

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-08-06 at 16:33 +0100, Wei Liu wrote:
> On Tue, Aug 06, 2013 at 04:24:10PM +0100, Wei Liu wrote:
> > On Tue, Aug 06, 2013 at 04:21:38PM +0100, Ian Campbell wrote:
> > > On Tue, 2013-08-06 at 16:05 +0100, Wei Liu wrote:
> > > > On Tue, Aug 06, 2013 at 03:56:46PM +0100, Ian Campbell wrote:
> > > > > On Tue, 2013-08-06 at 15:50 +0100, Wei Liu wrote:
> > > > > > On Tue, Aug 06, 2013 at 02:50:04PM +0100, Ian Campbell wrote:
> > > > > > > On Tue, 2013-08-06 at 14:33 +0100, Wei Liu wrote:
> > > > > > > > I sent this patch right before Ian requested more comments in 
> > > > > > > > code. Now
> > > > > > > > I've updated this one. Not going to resend the whole series as 
> > > > > > > > this is
> > > > > > > > only changes in comments.
> > > > > > > > 
> > > > > > > > Sorry for my bad English, I don't know whether I've made them 
> > > > > > > > clear
> > > > > > > > enough. Suggestions are welcomed.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Wei.
> > > > > > > > 
> > > > > > > > ---8<---
> > > > > > > > From bb33027fe4bafeea546352cd3e409466f8bd7aa4 Mon Sep 17 
> > > > > > > > 00:00:00 2001
> > > > > > > > From: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > > > > > > Date: Tue, 6 Aug 2013 06:59:15 +0100
> > > > > > > > Subject: [PATCH] xen-netback: switch to NAPI + kthread 1:1 model
> > > > > > > > 
> > > > > > > > This patch implements 1:1 model netback. NAPI and kthread are 
> > > > > > > > utilized
> > > > > > > > to do the weight-lifting job:
> > > > > > > > 
> > > > > > > > - NAPI is used for guest side TX (host side RX)
> > > > > > > > - kthread is used for guest side RX (host side TX)
> > > > > > > > 
> > > > > > > > Xenvif and xen_netbk are made into one structure to reduce code 
> > > > > > > > size.
> > > > > > > > 
> > > > > > > > This model provides better scheduling fairness among vifs. It 
> > > > > > > > is also
> > > > > > > > prerequisite for implementing multiqueue for Xen netback.
> > > > > > > > 
> > > > > > > > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > >  drivers/net/xen-netback/common.h    |  128 +++++---
> > > > > > > >  drivers/net/xen-netback/interface.c |  119 ++++---
> > > > > > > >  drivers/net/xen-netback/netback.c   |  607 
> > > > > > > > ++++++++++-------------------------
> > > > > > > >  3 files changed, 347 insertions(+), 507 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/net/xen-netback/common.h 
> > > > > > > > b/drivers/net/xen-netback/common.h
> > > > > > > > index 8a4d77e..184ae0a 100644
> > > > > > > > --- a/drivers/net/xen-netback/common.h
> > > > > > > > +++ b/drivers/net/xen-netback/common.h
> > > > > > > > @@ -45,31 +45,105 @@
> > > > > > > >  #include <xen/grant_table.h>
> > > > > > > >  #include <xen/xenbus.h>
> > > > > > > >  
> > > > > > > > -struct xen_netbk;
> > > > > > > > +typedef unsigned int pending_ring_idx_t;
> > > > > > > > +#define INVALID_PENDING_RING_IDX (~0U)
> > > > > > > > +
> > > > > > > > +/* For the head field in pending_tx_info: It is used to 
> > > > > > > > indicate
> > > > > > > > + * whether this tx info is the head of one or more coalesced 
> > > > > > > > requests.
> > > > > > > > + *
> > > > > > > > + * When head != INVALID_PENDING_RING_IDX, it means the start 
> > > > > > > > of a new
> > > > > > > > + * tx requests queue and the end of previous queue.
> > > > > > > > + *
> > > > > > > > + * An example sequence of head fields (I = 
> > > > > > > > INVALID_PENDING_RING_IDX):
> > > > > > > > + *
> > > > > > > > + * ... 0 I I I 5 I 9 I I I ...
> > > > > > > > + *
> > > > > > > > + * After consming the first packet we have:
> > > > > > > 
> > > > > > > "consuming"
> > > > > > > 
> > > > > > 
> > > > > > Oops...
> > > > > > 
> > > > > > > The first packet here is "0 I I I"? Consisting of the head (0) 
> > > > > > > and 3
> > > > > > > extra data slots (the 3xI)?
> > > > > > > 
> > > > > > 
> > > > > > Yes. But the term "extra data slot" is not acurate. "0 I I I" means
> > > > > > we've coalesced 3 more slots into the first slot. And the index of 
> > > > > > first
> > > > > > slot into various arrays is 0.
> > > > > 
> > > > > Got it, I think.
> > > > > 
> > > > > > 
> > > > > > > > + *
> > > > > > > > + * ... V V V V 5 I 9 I I I ...
> > > > > > > > + *
> > > > > > > > + * where V stands for "valid pending ring index", any number 
> > > > > > > > other
> > > > > > > > + * than INVALID_PENDING_RING_IDX is OK. In practice we use 0.
> > > > > > > 
> > > > > > > OK, this is where I get confused, because 0 is also valid in a 
> > > > > > > different
> > > > > > > state, this one:
> > > > > > > > + * The non-INVALID_PENDING_RING_IDX (say 0, 5 and 9 in the 
> > > > > > > > above
> > > > > > > > + * example) number is the index into pending_tx_info and 
> > > > > > > > mmap_pages
> > > > > > > > + * arrays.
> > > > > > > 
> > > > > > > So what does V mean and how to you distinguish this from a
> > > > > > > non-INVALID_PENDING_RING_IDX which happens to correspond to the 0 
> > > > > > > you
> > > > > > > use in practice for V?
> > > > > > > 
> > > > > > 
> > > > > > Oh, V is just another way of saying non-INVALID_PENDING_RING_IDX. 
> > > > > > :-/
> > > > > > 
> > > > > > The head field is always manipulated when coalescing slots and 
> > > > > > releasing
> > > > > > slots. So if the routine sees 0 when processing a slot is actually 
> > > > > > means
> > > > > > index 0.
> > > > > 
> > > > > But you say "In practice we use 0", but you could use any arbitrary
> > > > > integer which is != INVALID_PENDING_RING_IDX?
> > > > > 
> > > > 
> > > > Yes. This is only useful when releasing slots. When the routine sees a
> > > > number not equal to INVALID_PENDING_RING_IDX it knows the previous
> > > > sequence has ended. So setting head to 0 is as good as any.
> > > > 
> > > > > > > I'm also not sure how 0 is considered a "valid pending ring 
> > > > > > > index". I
> > > > > > > suppose it is not INVALID_PENDING_RING_IDX but it appears that it
> > > > > > > doesn't have any actual meaning? So it's just an arbitrary valid 
> > > > > > > but
> > > > > > > meaningless number perhaps?
> > > > > > > 
> > > > > > 
> > > > > > Yes. 0 is as good as any number other than INVALID_PENDING_RING_IDX.
> > > > > 
> > > > > So I still have this confusion between a 'V' entry and an entry which
> > > > > contains an actual index into various arrays. How are they different?
> > > > > 
> > > > > AIUI each entry can be one of three types:
> > > > > 
> > > > > a) Contains INVALID_PENDING_RING_IDX (is a coalesced into a previous 
> > > > > head)
> > > > > b) Contains an index into various arrays (an integer)
> > > > > c) Contains 'V', a "valid pending ring index", usually 0.
> > > > > 
> > > > > So how are b) and c) distinguished? I think b) is the head, so what is
> > > > > c)?
> > > > 
> > > > c) state can only be reached *after* releasing a slot. When the slot is
> > > > reused into b) state (after coalescing), the head field is then set to
> > > > the actual number of the index. b) and c) are mutual exclusive.
> > > 
> > > Ah, so these 'V' entries are free entries, I think I've got it now.
> > > 
> > > So:
> > > ...|0 I I I|5 I|9 I I I|...
> > > -->|<-INUSE----------------
> > > 
> > > Then you consume "0 I I I" and get:
> > > 
> > > ...|V V V V|5 I|9 I I I|...
> > > -----FREE->|<-INUSE--------
> > > 
> > 
> > Right, this is it. :-)
> > 
> > I will use the above graph in the comment.
> > 
> 
> /* For the head field in pending_tx_info: it is used to indicate
>  * whether this tx info is the head of one or more coalesced requests.
>  *
>  * When head != INVALID_PENDING_RING_IDX, it means the start of a new
>  * tx requests queue and the end of previous queue.
>  *
>  * An example sequence of head fields (I = INVALID_PENDING_RING_IDX):
>  *
>  * ...|0 I I I|5 I|9 I I I|...
>  * -->|<-INUSE----------------
>  *
>  * After consuming the first slot(s) we have:
>  *
>  * ...|V V V V|5 I|9 I I I|...
>  * -----FREE->|<-INUSE--------
>  *
>  * where V stands for "valid pending ring index". Any number other
>  * than INVALID_PENDING_RING_IDX is OK. In practice we use 0.

Perhaps add "These entries are considered free and can contain any
number other than INVALID_PENDING_RING_IDX" ?

Do you have a #define, like FREE_PENDING_RING_IDX for this value? Should
mention it if so I think.

>  *
>  * The non-INVALID_PENDING_RING_IDX (say 0, 5 and 9 in the above
        ^ add "in use"?

>  * example) number is the index into pending_tx_info and mmap_pages
>  * arrays.
>  */
> 
> > 
> > Wei.
> > 
> > > Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel