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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.


Wei.

> Ian.

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