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 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.


Wei.

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