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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Aug 06, 2013 at 10:26:24AM +0100, Ian Campbell wrote:
> 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.
> 

Indeed.

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

Ok.

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

Yes. That's the case. After we manipulate the event pointer the frontend
might generate a new event. Our NAPI handler is still scheduled at that
time, it won't be scheduled again. 

Do real drivers all disable interrupts in this
> way too or do they get this for free via some other means?
> 

Real drivers often use hardware mechanism to prevent generating events.

> I think a comment is at least required.
> 

Sure.


Wei.

> Ian.

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