[Intel-wired-lan] [RFC PATCH] ixgbe: delay tail write to every 'n' packets

[Duyck, Alexander H <alexander.h.duyck@intel.com>]

Sounds good. ?We can probably discuss it tomorrow at our 1pm meeting.

Thanks.

- Alex

On Mon, 2017-03-27 at 19:28 +0000, Fastabend, John R wrote:
> FWIW with a revised version of this patch I see 14.6Mpps on RX drop tests and 13.5Mpps on TX test case.
> 
> Alex, maybe we can sync up and squeeze the last mpps out of the design.
> 
> Thanks,
> John
> ________________________________________
> From: Intel-wired-lan [intel-wired-lan-bounces at lists.osuosl.org] on behalf of Alexander Duyck [alexander.duyck at gmail.com]
> Sent: Tuesday, March 14, 2017 6:14 PM
> To: John Fastabend
> Cc: intel-wired-lan; Alexei Starovoitov; Daniel Borkmann; William Tu
> Subject: Re: [Intel-wired-lan] [RFC PATCH] ixgbe: delay tail write to every     'n' packets
> 
> On Tue, Mar 14, 2017 at 4:50 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
> > 
> > On 17-03-13 10:18 AM, Alexander Duyck wrote:
> > > 
> > > On Mon, Mar 13, 2017 at 9:16 AM, John Fastabend
> > > <john.fastabend@gmail.com> wrote:
> > > > 
> > > > Current XDP implementation hits the tail on every XDP_TX return
> > > > code. This patch changes driver behavior to only hit the tail after
> > > > packet processing is complete.
> > > > 
> > > > RFC for now as I test this, it looks promising on my dev box but
> > > > want to do some more tests before official submission.
> > > > 
> > > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> > > > ---
> > > >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   14 +++++++++++---
> > > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > > index bef4e24..2c244b6 100644
> > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> > > > @@ -2282,6 +2282,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
> > > >         unsigned int mss = 0;
> > > >  #endif /* IXGBE_FCOE */
> > > >         u16 cleaned_count = ixgbe_desc_unused(rx_ring);
> > > > +       bool xdp_xmit = false;
> > > > 
> > > >         while (likely(total_rx_packets < budget)) {
> > > >                 union ixgbe_adv_rx_desc *rx_desc;
> > > > @@ -2321,10 +2322,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
> > > >                 }
> > > > 
> > > >                 if (IS_ERR(skb)) {
> > > > -                       if (PTR_ERR(skb) == -IXGBE_XDP_TX)
> > > > +                       if (PTR_ERR(skb) == -IXGBE_XDP_TX) {
> > > > +                               xdp_xmit = true;
> > > >                                 ixgbe_rx_buffer_flip(rx_ring, rx_buffer, size);
> > > > -                       else
> > > > +                       } else {
> > > >                                 rx_buffer->pagecnt_bias++;
> > > > +                       }
> > > >                         total_rx_packets++;
> > > >                         total_rx_bytes += size;
> > > >                 } else if (skb) {
> > > > @@ -2392,6 +2395,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
> > > >                 total_rx_packets++;
> > > >         }
> > > > 
> > > > +       if (xdp_xmit) {
> > > > +               struct ixgbe_ring *ring = adapter->xdp_ring[smp_processor_id()];
> > > > +
> > > > +               writel(ring->next_to_use, ring->tail);
> > > > +       }
> > > > +
> > > 
> > > We will need a wmb here.
> > > 
> > > > 
> > > >         u64_stats_update_begin(&rx_ring->syncp);
> > > >         rx_ring->stats.packets += total_rx_packets;
> > > >         rx_ring->stats.bytes += total_rx_bytes;
> > > > @@ -8251,7 +8260,6 @@ static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter,
> > > >         tx_buffer->next_to_watch = tx_desc;
> > > >         ring->next_to_use = i;
> > > > 
> > > > -       writel(i, ring->tail);
> > > 
> > > So you might want to change the barrier setup for all this to use
> > > smp_wmb instead.  We need the wmb to be paired with the writel.  That
> > > should give you a slight performance boost since smp_wmb breaks down
> > > to just a barrier on x86 systems.
> > > 
> > 
> > Not sure I grok this description entirely, but I think you are just saying
> > replace,
> > 
> >         if (xdp_xmit) {
> >                 ...
> >                 writel(...)
> >         }
> > 
> > with
> > 
> >         if (xdp_xmit) {
> >                 ...
> >                 smp_rmb()
> >                 writel()
> >         }
> > 
> > Correct? Did you have some other change in mind ... "for all this"?
> > 
> > Thanks,
> > John
> 
> 
> No.  Basically what it should be is find and replace wmb with
> smp_wmb() in ixgbe_xmit_xdp_ring.  That way you won't have to worry
> about a race between Tx and clean-up.
> 
> Then the if statement should be:
> if (xdp_xmit) {
>     ...
>     /* wmb required to flush writes to coherent memory before writing
> to non-coherent memory*/
>     wmb();
>     /* writel is a write to non-coherent memory mapped I/O */
>     writel();
> }
> 
> > 
> > > 
> > > > 
> > > >         return IXGBE_XDP_TX;
> > > >  }
> > > > 
> > > > 
> > 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan