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

> > 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)?


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