Re: [PATCH 2/4] gve: Add support for raw addressing to the rx path

[David Awogbemila <awogbemila@google.com>]

On Tue, Nov 3, 2020 at 3:19 PM Saeed Mahameed <saeed@kernel.org> wrote:
>
> On Tue, 2020-11-03 at 09:46 -0800, David Awogbemila wrote:
> > From: Catherine Sullivan <csully@google.com>
> >
> > Add support to use raw dma addresses in the rx path. Due to this new
> > support we can alloc a new buffer instead of making a copy.
> >
> > RX buffers are handed to the networking stack and are
> > re-allocated as needed, avoiding the need to use
> > skb_copy_to_linear_data() as in "qpl" mode.
> >
> > Reviewed-by: Yangchun Fu <yangchun@google.com>
> > Signed-off-by: Catherine Sullivan <csully@google.com>
> > Signed-off-by: David Awogbemila <awogbemila@google.com>
> > ---
> >  drivers/net/ethernet/google/gve/gve.h        |   9 +-
> >  drivers/net/ethernet/google/gve/gve_adminq.c |  14 +-
> >  drivers/net/ethernet/google/gve/gve_desc.h   |  10 +-
> >  drivers/net/ethernet/google/gve/gve_main.c   |   3 +-
> >  drivers/net/ethernet/google/gve/gve_rx.c     | 220 +++++++++++++++
> > ----
> >  5 files changed, 203 insertions(+), 53 deletions(-)
> >
>
> ...
>
> >  static inline u32 gve_num_rx_qpls(struct gve_priv *priv)
> >  {
> > -     return priv->rx_cfg.num_queues;
> > +     if (priv->raw_addressing)
> > +             return 0;
> > +     else
> > +             return priv->rx_cfg.num_queues;
>
> else statement is redundant.
Thanks, I'll take the else statement away.
>
>
> >
> >  static int gve_prefill_rx_pages(struct gve_rx_ring *rx)
> >  {
> >       struct gve_priv *priv = rx->gve;
> >       u32 slots;
> > +     int err;
> >       int i;
> >
> >       /* Allocate one page per Rx queue slot. Each page is split into
> > two
> > @@ -71,12 +96,31 @@ static int gve_prefill_rx_pages(struct
> > gve_rx_ring *rx)
> >       if (!rx->data.page_info)
> >               return -ENOMEM;
> >
> > -     rx->data.qpl = gve_assign_rx_qpl(priv);
> > -
> > +     if (!rx->data.raw_addressing)
> > +             rx->data.qpl = gve_assign_rx_qpl(priv);
> >       for (i = 0; i < slots; i++) {
> > -             struct page *page = rx->data.qpl->pages[i];
> > -             dma_addr_t addr = i * PAGE_SIZE;
> > +             struct page *page;
> > +             dma_addr_t addr;
> > +
> > +             if (rx->data.raw_addressing) {
> > +                     err = gve_alloc_page(priv, &priv->pdev->dev,
> > &page,
> > +                                          &addr, DMA_FROM_DEVICE);
> > +                     if (err) {
> > +                             int j;
> >
>
> the code is skewed right, 5 level indentation is a lot.
> you can just goto alloc_err; and handle the rewind on the exit path of
> the function .
>
> BTW you could split this loop to two independent flows if you utilize
> gve_rx_alloc_buffer()
>
> if (!raw_Addressing) {
>         page = rx->data.qpl->pages[i];
>         addr = i * PAGE_SIZE;
>         gve_setup_rx_buffer(...);
>         continue;
> }
>
> /* raw addressing mode */
> err = gve_rx_alloc_buffer(...);
> if (err)
>       goto alloc_err;
>

Thanks, I'll eliminate the skew andI'll take the suggestion to
separate the independent flows.