Re: [PATCH 3/4] gve: Rx Buffer Recycling

[David Awogbemila <awogbemila@google.com>]

On Tue, Nov 3, 2020 at 4:01 PM Saeed Mahameed <saeed@kernel.org> wrote:
>
> On Tue, 2020-11-03 at 09:46 -0800, David Awogbemila 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>
> > ---
> >  drivers/net/ethernet/google/gve/gve.h    |  10 +-
> >  drivers/net/ethernet/google/gve/gve_rx.c | 194 +++++++++++++++----
> > ----
>
> > +     if (len <= priv->rx_copybreak) {
> > +             /* Just copy small packets */
> > +             skb = gve_rx_copy(dev, napi, page_info, len);
> > +             u64_stats_update_begin(&rx->statss);
> > +             rx->rx_copied_pkt++;
> > +             rx->rx_copybreak_pkt++;
> > +             u64_stats_update_end(&rx->statss);
> > +     } else {
> > +             bool can_flip = gve_rx_can_flip_buffers(dev);
> > +             int recycle = 0;
> > +
> > +             if (can_flip) {
> > +                     recycle = gve_rx_can_recycle_buffer(page_info-
> > >page);
> > +                     if (recycle < 0) {
> > +                             gve_schedule_reset(priv);
>
> How would a reset solve anything if your driver is handling pages with
> "bad" refcount, i don't agree here that reset is the best course of
> action, all you can do here is warn and leak the page ...
> this is a critical driver bug and not something that user should
> expect.
>
Thanks for pointing this out. For the raw addressing case it would be
fine to warn and leak the page, but for the qpl (non raw addressing)
case, the driver pre-allocates a set of pages which it registers with
the NIC so we'd want to avoid leaking pages - a reset will help here
because we'd allocate a new set of pages and register them with the
NIC.
I will handle both cases separately:
- in raw addressing mode, just warn and leak the page
- in qpl mode, schedule a reset.


> > +
> > +             } else {
> > +                     /* It is possible that the networking stack has
> > already
> > +                      * finished processing all outstanding packets
> > in the buffer
> > +                      * and it can be reused.
> > +                      * Flipping is unnecessary here - if the
> > networking stack still
> > +                      * owns half the page it is impossible to tell
> > which half. Either
> > +                      * the whole page is free or it needs to be
> > replaced.
> > +                      */
> > +                     int recycle =
> > gve_rx_can_recycle_buffer(page_info->page);
> > +
> > +                     if (recycle < 0) {
> > +                             gve_schedule_reset(priv);
> > +                             return false;
>
> Same.
>
>