Re: [PATCH net-next v6 3/4] gve: Rx Buffer Recycling

[Alexander Duyck <alexander.duyck@gmail.com>]

On Wed, Nov 18, 2020 at 2:50 PM David Awogbemila <awogbemila@google.com> wrote:
>
> On Wed, Nov 11, 2020 at 9:20 AM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > On Mon, Nov 9, 2020 at 3:39 PM David Awogbemila <awogbemila@google.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@google.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.

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

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