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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2013-08-05 at 03:31 +0100, Wei Liu wrote:
> On Fri, Aug 02, 2013 at 01:20:52PM +0100, Ian Campbell wrote:
> > On Wed, 2013-05-29 at 12:43 +0100, Wei Liu wrote:
> > > 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.
> > 
> > Any interesting numbers from this change?
> > 
> 
> [extracted from 0/2 of this series]

Oops, missed that, sorry.

> 
> IRQs are distributed to 4 cores by hand in the new model, while in the
> old model vifs are automatically distributed to 4 kthreads.
> 
> * New model
> %Cpu0  :  0.5 us, 20.3 sy,  0.0 ni, 28.9 id,  0.0 wa,  0.0 hi, 24.4 si, 25.9 
> st
> %Cpu1  :  0.5 us, 17.8 sy,  0.0 ni, 28.8 id,  0.0 wa,  0.0 hi, 27.7 si, 25.1 
> st
> %Cpu2  :  0.5 us, 18.8 sy,  0.0 ni, 30.7 id,  0.0 wa,  0.0 hi, 22.9 si, 27.1 
> st
> %Cpu3  :  0.0 us, 20.1 sy,  0.0 ni, 30.4 id,  0.0 wa,  0.0 hi, 22.7 si, 26.8 
> st
> Throughputs: 2027.89 2025.95 2018.57 2016.23 aggregated: 8088.64
> 
> * Old model
> %Cpu0  :  0.5 us, 68.8 sy,  0.0 ni, 16.1 id,  0.5 wa,  0.0 hi,  2.8 si, 11.5 
> st
> %Cpu1  :  0.4 us, 45.1 sy,  0.0 ni, 31.1 id,  0.4 wa,  0.0 hi,  2.1 si, 20.9 
> st
> %Cpu2  :  0.9 us, 44.8 sy,  0.0 ni, 30.9 id,  0.0 wa,  0.0 hi,  1.3 si, 22.2 
> st
> %Cpu3  :  0.8 us, 46.4 sy,  0.0 ni, 28.3 id,  1.3 wa,  0.0 hi,  2.1 si, 21.1 
> st
> Throughputs: 1899.14 2280.43 1963.33 1893.47 aggregated: 8036.37

The CPU load seems to drop pretty significantly with the new model,
nice. I suppose that is the impact of NAPI rather than the 1:1 side of
things.

> 
> > > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > ---
> > >  drivers/net/xen-netback/common.h    |  105 +++--
> > >  drivers/net/xen-netback/interface.c |  122 +++---
> > >  drivers/net/xen-netback/netback.c   |  742 
> > > ++++++++++++-----------------------
> > >  3 files changed, 389 insertions(+), 580 deletions(-)
> > > 
> > > diff --git a/drivers/net/xen-netback/common.h 
> > > b/drivers/net/xen-netback/common.h
> > > index 8a4d77e..2e270cf 100644
> > > --- a/drivers/net/xen-netback/common.h
> > > +++ b/drivers/net/xen-netback/common.h
> > > @@ -45,15 +45,43 @@
> > >  #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)
> > > +
> > > +struct pending_tx_info {
> > > + struct xen_netif_tx_request req; /* coalesced tx request */
> > > + pending_ring_idx_t head; /* head != INVALID_PENDING_RING_IDX
> > > +                           * if it is head of one or more tx
> > > +                           * reqs
> > 
> 
> This is only code movement, no funcational change.
> 
> > So this sounds like it is only valid for the pending tx corresponding to
> > the first slot in a frame, right?
> > 
> 
> No. It can also be interpreted as continuation of several merged tx
> requests.
> 
> > It does not contain a link from any arbitrary slot to the slot
> > containing the head of that frame, right?
> > 
> > So for the head it is effectively that entries index into the pending_tx
> > array?
> > 
> 
> No. It's a flag to tell if this pending_tx_info is the head of one or
> more merged tx requests.

OK, then I'm totally confused.

I think a comment with an example of the representation of a single
multislot frame (2-3 slots if that suffices as an illustration) in a
comment would be useful.

> > > +static int xenvif_poll(struct napi_struct *napi, int budget)
> > > +{
> > > + struct xenvif *vif = container_of(napi, struct xenvif, napi);
> > > + int work_done;
> > > +
> > > + work_done = xenvif_tx_action(vif, budget);
> > > +
> > > + if (work_done < budget) {
> > > +         int more_to_do = 0;
> > > +         unsigned long flags;
> > > +
> > > +         local_irq_save(flags);
> > 
> > What does this protect against? Maybe it's a napi requirement not an Xen
> > netif one?
> > 
> 
> This is sort of a NAPI requirement. If we don't do local_irq_save we
> would lose event.

How come? This is a race against this code vs. a new event coming in and
calling napi_schedule? Do real drivers all disable interrupts in this
way too or do they get this for free via some other means?

I think a comment is at least required.

Ian.


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