Date:   Thu, 19 Nov 2020 13:11:29 -0800
From:   David Awogbemila <awogbemila@...gle.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v6 3/4] gve: Rx Buffer Recycling

On Thu, Nov 19, 2020 at 8:55 AM Alexander Duyck
<alexander.duyck@...il.com> wrote:
>
> On Wed, Nov 18, 2020 at 2:50 PM David Awogbemila <awogbemila@...gle.com> wrote:
> >
> > On Wed, Nov 11, 2020 at 9:20 AM Alexander Duyck
> > <alexander.duyck@...il.com> wrote:
> > >
> > > On Mon, Nov 9, 2020 at 3:39 PM David Awogbemila <awogbemila@...gle.com> wrote:
> > > >
> > > > This patch lets the driver reuse buffers that have been freed by the
> > > > networking stack.
> > > >
> > > > In the raw addressing case, this allows the driver avoid allocating new
> > > > buffers.
> > > > In the qpl case, the driver can avoid copies.
> > > >
> > > > Signed-off-by: David Awogbemila <awogbemila@...gle.com>
> > > > ---
>
> <snip>
>
> > > > +static int gve_rx_can_recycle_buffer(struct page *page)
> > > > +{
> > > > +       int pagecount = page_count(page);
> > > > +
> > > > +       /* This page is not being used by any SKBs - reuse */
> > > > +       if (pagecount == 1)
> > > > +               return 1;
> > > > +       /* This page is still being used by an SKB - we can't reuse */
> > > > +       else if (pagecount >= 2)
> > > > +               return 0;
> > > > +       WARN(pagecount < 1, "Pagecount should never be < 1");
> > > > +       return -1;
> > > > +}
> > > > +
> > >
> > > So using a page count of 1 is expensive. Really if you are going to do
> > > this you should probably look at how we do it currently in ixgbe.
> > > Basically you want to batch the count updates to avoid expensive
> > > atomic operations per skb.
> >
> > A separate patch will be coming along to change the way the driver
> > tracks page count.
> > I thought it would be better to have that reviewed separately since
> > it's a different issue from what this patch addresses.
>
> Okay, you might want to call that out in your patch description then
> that this is just a temporary placeholder. Back when I did this for
> ixgbe I think we had it doing a single page update for a few years,
> however that code had a bug in it that would cause page count
> corruption as I wasn't aware at the time that the mm tree had
> functions that would take a reference on the page without us ever
> handing it out.

Ok, I'll add that to the patch description since even though this
aspect of the driver's behavior is not changing in this patch it is being
separated into its own function.

>
> > >
> > > >  static struct sk_buff *
> > > >  gve_rx_raw_addressing(struct device *dev, struct net_device *netdev,
> > > >                       struct gve_rx_slot_page_info *page_info, u16 len,
> > > >                       struct napi_struct *napi,
> > > > -                     struct gve_rx_data_slot *data_slot)
> > > > +                     struct gve_rx_data_slot *data_slot, bool can_flip)
> > > >  {
> > > > -       struct sk_buff *skb = gve_rx_add_frags(napi, page_info, len);
> > > > +       struct sk_buff *skb;
> > > >
> > > > +       skb = gve_rx_add_frags(napi, page_info, len);
> > >
> > > Why split this up?It seemed fine as it was.
> >
> > It was based on a comment from v3 of the patchset.
>
> Do you recall the comment? This just seems like noise to me since it
> is moving code and doesn't seem to address either a formatting issue,
> nor a functional issue.

The comment suggested that it might be weird to initialize a variable
with a function call and error-check after an empty line.
I realize now that what I should have done here is introduce the split
in the patch that introduced the function so
it doesn't appear as an unnecessary isolated change - I'll do that.

>
> > >
> > > >         if (!skb)
> > > >                 return NULL;
> > > >
> > > > +       /* Optimistically stop the kernel from freeing the page by increasing
> > > > +        * the page bias. We will check the refcount in refill to determine if
> > > > +        * we need to alloc a new page.
> > > > +        */
> > > > +       get_page(page_info->page);
> > > > +       page_info->can_flip = can_flip;
> > > > +
> > >
> > > Why pass can_flip and page_info only to set it here? Also I don't get
> > > why you are taking an extra reference on the page without checking the
> > > can_flip variable. It seems like this should be set in the page_info
> > > before you call this function and then you call get_page if
> > > page_info->can_flip is true.
> >
> > I think it's important to call get_page here even for buffers we know
> > will not be flipped so that if the skb does a put_page twice we would
> > not run into the WARNing in gve_rx_can_recycle_buffer when trying to
> > refill buffers.
> > (Also please note that a future patch changes the way the driver uses
> > page refcounts)
>
> It was your buffer recycling bit that I hadn't noticed. So you have
> cases where if your page count gets to 2 you push it to 3 in the hopes
> that the 2 that are already out there will be freed and you are left
> holding the one remaining count. However I am not sure how much of an
> advantage there is to that. If the page flipping is already failing I
> wonder what percentage of the time you are able to recover from that.
> It might be worthwhile to look at adding a couple of counters to track
> the number of times you couldn't flip versus the number of times you
> recycled the frame. You may find that the buffer recycling is adding
> more overhead for very little gain versus just doing the page
> flipping.

Thanks for the suggestion. Metrics tracking those sound like a good idea.
I'll look into getting that change into the patch which changes the driver's
refcount tracking method and eliminates get_page entirely.

Powered by blists - more mailing lists