Re: [dpdk-dev] [PATCH 26/29] net/ena: reuse 0 length Rx descriptor

["Michał Krawczyk" <mk@semihalf.com>]

pt., 27 mar 2020 o 12:30 Andrew Rybchenko <arybchenko@solarflare.com>
napisał(a):
>
> On 3/27/20 1:18 PM, Michal Krawczyk wrote:
> > Some ENA devices can pass to the driver descriptor with length 0. To
> > avoid extra allocation, the descriptor can be reused by simply putting
> > it back to the device.
> >
> > Signed-off-by: Michal Krawczyk <mk@semihalf.com>
> > Reviewed-by: Igor Chauskin <igorch@amazon.com>
> > Reviewed-by: Guy Tzalik <gtzalik@amazon.com>
> > ---
> >  drivers/net/ena/ena_ethdev.c | 74 ++++++++++++++++++++++++++++--------
> >  1 file changed, 59 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> > index 54bd2760e5..9b95772e9e 100644
> > --- a/drivers/net/ena/ena_ethdev.c
> > +++ b/drivers/net/ena/ena_ethdev.c
> > @@ -191,6 +191,8 @@ static struct rte_mbuf *ena_rx_mbuf(struct ena_ring
*rx_ring,
> >                                   uint8_t offset);
> >  static uint16_t eth_ena_recv_pkts(void *rx_queue,
> >                                 struct rte_mbuf **rx_pkts, uint16_t
nb_pkts);
> > +static int ena_add_single_rx_desc(struct ena_com_io_sq *io_sq,
> > +                               struct rte_mbuf *mbuf, uint16_t id);
> >  static int ena_populate_rx_queue(struct ena_ring *rxq, unsigned int
count);
> >  static void ena_init_rings(struct ena_adapter *adapter,
> >                          bool disable_meta_caching);
> > @@ -1403,6 +1405,24 @@ static int ena_rx_queue_setup(struct rte_eth_dev
*dev,
> >       return 0;
> >  }
> >
> > +static int ena_add_single_rx_desc(struct ena_com_io_sq *io_sq,
> > +                               struct rte_mbuf *mbuf, uint16_t id)
> > +{
> > +     struct ena_com_buf ebuf;
> > +     int rc;
> > +
> > +     /* prepare physical address for DMA transaction */
> > +     ebuf.paddr = mbuf->buf_iova + RTE_PKTMBUF_HEADROOM;
> > +     ebuf.len = mbuf->buf_len - RTE_PKTMBUF_HEADROOM;
> > +
> > +     /* pass resource to device */
> > +     rc = ena_com_add_single_rx_desc(io_sq, &ebuf, id);
> > +     if (unlikely(rc))
>
> DPDK style says to compare to 0 [1] and [2].
>
> [1] https://doc.dpdk.org/guides/contributing
> /coding_style.html#function-calls
> [2]
https://doc.dpdk.org/guides/contributing/coding_style.html#null-pointer
>

I will fix that. I can see that the whole driver is using invalid style for
checking rc. However, I'll fix the style of the conditional checks that are
touched by this patch and fix the rest of them in the future, in the
separate patch.

> > +             PMD_DRV_LOG(WARNING, "failed adding rx desc\n");
> > +
> > +     return rc;
> > +}
> > +
> >  static int ena_populate_rx_queue(struct ena_ring *rxq, unsigned int
count)
> >  {
> >       unsigned int i;
> > @@ -1430,7 +1450,6 @@ static int ena_populate_rx_queue(struct ena_ring
*rxq, unsigned int count)
> >
> >       for (i = 0; i < count; i++) {
> >               struct rte_mbuf *mbuf = mbufs[i];
> > -             struct ena_com_buf ebuf;
> >               struct ena_rx_buffer *rx_info;
> >
> >               if (likely((i + 4) < count))
> > @@ -1443,16 +1462,10 @@ static int ena_populate_rx_queue(struct
ena_ring *rxq, unsigned int count)
> >
> >               rx_info = &rxq->rx_buffer_info[req_id];
> >
> > -             /* prepare physical address for DMA transaction */
> > -             ebuf.paddr = mbuf->buf_iova + RTE_PKTMBUF_HEADROOM;
> > -             ebuf.len = mbuf->buf_len - RTE_PKTMBUF_HEADROOM;
> > -             /* pass resource to device */
> > -             rc = ena_com_add_single_rx_desc(rxq->ena_com_io_sq,
> > -                                             &ebuf, req_id);
> > -             if (unlikely(rc)) {
> > -                     PMD_DRV_LOG(WARNING, "failed adding rx desc\n");
> > +             rc = ena_add_single_rx_desc(rxq->ena_com_io_sq, mbuf,
req_id);
> > +             if (unlikely(rc))
>
> same here
>
> >                       break;
> > -             }
> > +
> >               rx_info->mbuf = mbuf;
> >               next_to_use = ENA_IDX_NEXT_MASKED(next_to_use,
rxq->size_mask);
> >       }
> > @@ -2105,13 +2118,44 @@ static struct rte_mbuf *ena_rx_mbuf(struct
ena_ring *rx_ring,
> >               rx_info = &rx_ring->rx_buffer_info[req_id];
> >               RTE_ASSERT(rx_info->mbuf != NULL);
> >
> > -             /* Create an mbuf chain. */
> > -             mbuf->next = rx_info->mbuf;
> > -             mbuf = mbuf->next;
> > +             if (unlikely(len == 0)) {
> > +                     /*
> > +                      * Some devices can pass descriptor with the
length 0.
> > +                      * To avoid confusion, the PMD is simply putting
the
> > +                      * descriptor back, as it was never used. We'll
avoid
> > +                      * mbuf allocation that way.
> > +                      */
> > +                     int rc =
ena_add_single_rx_desc(rx_ring->ena_com_io_sq,
> > +                             rx_info->mbuf, req_id);
> > +                     if (unlikely(rc)) {
>
> same here
>
> > +                             /* Free the mbuf in case of an error. */
> > +                             rte_mbuf_raw_free(rx_info->mbuf);
> > +                     } else {
> > +                             /*
> > +                              * If there was no error, just exit the
loop as
> > +                              * 0 length descriptor is always the last
one.
> > +                              */
> > +                             break;
> > +                     }
> > +             } else {
> > +                     /* Create an mbuf chain. */
> > +                     mbuf->next = rx_info->mbuf;
> > +                     mbuf = mbuf->next;
> >
> > -             ena_init_rx_mbuf(mbuf, len);
> > -             mbuf_head->pkt_len += len;
> > +                     ena_init_rx_mbuf(mbuf, len);
> > +                     mbuf_head->pkt_len += len;
> > +             }
> >
> > +             /*
> > +              * Mark the descriptor as depleted and perform necessary
> > +              * cleanup.
> > +              * This code will execute in two cases:
> > +              *  1. Descriptor len was greater than 0 - normal
situation.
> > +              *  2. Descriptor len was 0 and we failed to add the
descriptor
> > +              *     to the device. In that situation, we should try to
add
> > +              *     the mbuf again in the populate routine and mark the
> > +              *     descriptor as used up by the device.
> > +              */
> >               rx_info->mbuf = NULL;
> >               rx_ring->empty_rx_reqs[ntc] = req_id;
> >               ntc = ENA_IDX_NEXT_MASKED(ntc, rx_ring->size_mask);
> >
>