Re: [PATCH net-next 2/5] sfc: separate channel->tx_queue and efx->xdp_tx_queue mappings

["Íñigo Huguet" <ihuguet@redhat.com>]

On Wed, May 11, 2022 at 9:52 AM Martin Habets <habetsm.xilinx@gmail.com> wrote:
>
> On Tue, May 10, 2022 at 10:44:40AM +0200, Íñigo Huguet wrote:
> > Channels that contains tx queues need to determine the mapping of this
> > queue structures to hw queue numbers. This applies both to all tx
> > queues, no matter if they are normal tx queues, xdp_tx queues or both at
> > the same time.
>
> In my thinking XDP-only channels are a different channel type, so it
> would be cleaner to define a separate struct efx_channel_type for those.

That would be changes far deeper than this patch series intended to
do. However, I totally agree with you, but maybe it's something better
to be included in the channels rework you're working on? I'm happy to
help on that, if possible.

>
> >
> > Also, a lookup table to map each cpu to a xdp_tx queue is created,
> > containing pointers to the xdp_tx queues, that should already be
> > allocated in one or more channels. This lookup table is global to all
> > efx_nic structure.
>
> I'm not keen on a direct CPU to queue mapping, but rather map the
> specific XDP-only channels to CPUs. Also for such a mapping there is
> XPS already. Ideally that configuration will be used.

But that mapping is not introduced by me, it already existed, and it's
used in efx_xdp_tx_buffers to have a dedicated TX queue for each CPU,
avoiding locking contention (at least when there are enough channels).

XPS is not an option here, at least not one I can think of, for 2 reasons:
1. When doing XDP_TX action, there doesn't even exist an skb, so there
is no information to be used by XPS
2. XPS create mappings to the normal TX queues, for normal traffic,
not for the XDP ones

In fact, when I sent the patches to allow using normal TX queues for
XDP if there are no free channels for XDP, I already tried to use XPS
to do the CPU mapping, and I didn't find the way to do it, because of
point 1.

I think this is because XDP is still very limited in some things,
hopefully it will improve in the future. AFAIK, all drivers are doing
the same or similar approach of one CPU mapped to one dedicated XDP_TX
queue.

>
> > Mappings to hw queues and xdp lookup table creation were done at the
> > same time in efx_set_channels, but it had a bit messy and not very clear
> > code. Then, commit 059a47f1da93 ("net: sfc: add missing xdp queue
> > reinitialization") moved part of that initialization to a separate
> > function to fix a bug produced because the xdp_tx queues lookup table
> > was not reinitialized after channels reallocation, leaving it pointing
> > to deallocated queues. Not all of that initialization needs to be
> > redone, but only the xdp_tx queues lookup table, and not the mappings to
> > hw queues. So this resulted in even less clear code.
> >
> > This patch moves back the part of that code that doesn't need to be
> > reinitialized. That is, the mapping of tx queues with hw queues numbers.
> > As a result, xdp queues lookup table creation and this are done in
> > different places, conforming to single responsibility principle and
> > resulting in more clear code.
> >
> > Signed-off-by: Íñigo Huguet <ihuguet@redhat.com>
> > ---
> >  drivers/net/ethernet/sfc/efx_channels.c | 69 +++++++++++++------------
> >  1 file changed, 37 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/sfc/efx_channels.c b/drivers/net/ethernet/sfc/efx_channels.c
> > index 3f28f9861dfa..8feba80f0a34 100644
> > --- a/drivers/net/ethernet/sfc/efx_channels.c
> > +++ b/drivers/net/ethernet/sfc/efx_channels.c
> > @@ -767,6 +767,19 @@ void efx_remove_channels(struct efx_nic *efx)
> >       kfree(efx->xdp_tx_queues);
> >  }
> >
> > +static inline int efx_alloc_xdp_tx_queues(struct efx_nic *efx)
> > +{
> > +     if (efx->xdp_tx_queue_count) {
> > +             EFX_WARN_ON_PARANOID(efx->xdp_tx_queues);
> > +             efx->xdp_tx_queues = kcalloc(efx->xdp_tx_queue_count,
> > +                                          sizeof(*efx->xdp_tx_queues),
> > +                                          GFP_KERNEL);
>
> efx_set_channels() can be called multiple times. In that case
> the previous memory allocated is not freed and thus it is leaked.

This was an already existing bug, that I didn't see, but you're right
we should fix it now. I will submit it to `net`.

>
> Martin
>
> > +             if (!efx->xdp_tx_queues)
> > +                     return -ENOMEM;
> > +     }
> > +     return 0;
> > +}
> > +
> >  static int efx_set_xdp_tx_queue(struct efx_nic *efx, int xdp_queue_number,
> >                               struct efx_tx_queue *tx_queue)
> >  {
> > @@ -789,44 +802,29 @@ static void efx_set_xdp_channels(struct efx_nic *efx)
> >       int xdp_queue_number = 0;
> >       int rc;
> >
> > -     /* We need to mark which channels really have RX and TX
> > -      * queues, and adjust the TX queue numbers if we have separate
> > -      * RX-only and TX-only channels.
> > -      */
> >       efx_for_each_channel(channel, efx) {
> >               if (channel->channel < efx->tx_channel_offset)
> >                       continue;
> >
> >               if (efx_channel_is_xdp_tx(channel)) {
> >                       efx_for_each_channel_tx_queue(tx_queue, channel) {
> > -                             tx_queue->queue = next_queue++;
> >                               rc = efx_set_xdp_tx_queue(efx, xdp_queue_number,
> >                                                         tx_queue);
> >                               if (rc == 0)
> >                                       xdp_queue_number++;
> >                       }
> > -             } else {
> > -                     efx_for_each_channel_tx_queue(tx_queue, channel) {
> > -                             tx_queue->queue = next_queue++;
> > -                             netif_dbg(efx, drv, efx->net_dev,
> > -                                       "Channel %u TXQ %u is HW %u\n",
> > -                                       channel->channel, tx_queue->label,
> > -                                       tx_queue->queue);
> > -                     }
> > +             } else if (efx->xdp_txq_queues_mode == EFX_XDP_TX_QUEUES_BORROWED) {
> >
> >                       /* If XDP is borrowing queues from net stack, it must
> >                        * use the queue with no csum offload, which is the
> >                        * first one of the channel
> >                        * (note: tx_queue_by_type is not initialized yet)
> >                        */
> > -                     if (efx->xdp_txq_queues_mode ==
> > -                         EFX_XDP_TX_QUEUES_BORROWED) {
> > -                             tx_queue = &channel->tx_queue[0];
> > -                             rc = efx_set_xdp_tx_queue(efx, xdp_queue_number,
> > -                                                       tx_queue);
> > -                             if (rc == 0)
> > -                                     xdp_queue_number++;
> > -                     }
> > +                     tx_queue = &channel->tx_queue[0];
> > +                     rc = efx_set_xdp_tx_queue(efx, xdp_queue_number,
> > +                                               tx_queue);
> > +                     if (rc == 0)
> > +                             xdp_queue_number++;
> >               }
> >       }
> >       WARN_ON(efx->xdp_txq_queues_mode == EFX_XDP_TX_QUEUES_DEDICATED &&
> > @@ -952,31 +950,38 @@ int efx_realloc_channels(struct efx_nic *efx, u32 rxq_entries, u32 txq_entries)
> >
> >  int efx_set_channels(struct efx_nic *efx)
> >  {
> > +     struct efx_tx_queue *tx_queue;
> >       struct efx_channel *channel;
> > +     unsigned int queue_num = 0;
> >       int rc;
> >
> >       efx->tx_channel_offset =
> >               efx_separate_tx_channels ?
> >               efx->n_channels - efx->n_tx_channels : 0;
> >
> > -     if (efx->xdp_tx_queue_count) {
> > -             EFX_WARN_ON_PARANOID(efx->xdp_tx_queues);
> > -
> > -             /* Allocate array for XDP TX queue lookup. */
> > -             efx->xdp_tx_queues = kcalloc(efx->xdp_tx_queue_count,
> > -                                          sizeof(*efx->xdp_tx_queues),
> > -                                          GFP_KERNEL);
> > -             if (!efx->xdp_tx_queues)
> > -                     return -ENOMEM;
> > -     }
> > -
> > +     /* We need to mark which channels really have RX and TX queues, and
> > +      * adjust the TX queue numbers if we have separate RX/TX only channels.
> > +      */
> >       efx_for_each_channel(channel, efx) {
> >               if (channel->channel < efx->n_rx_channels)
> >                       channel->rx_queue.core_index = channel->channel;
> >               else
> >                       channel->rx_queue.core_index = -1;
> > +
> > +             if (channel->channel >= efx->tx_channel_offset) {
> > +                     efx_for_each_channel_tx_queue(tx_queue, channel) {
> > +                             tx_queue->queue = queue_num++;
> > +                             netif_dbg(efx, drv, efx->net_dev,
> > +                                       "Channel %u TXQ %u is HW %u\n",
> > +                                       channel->channel, tx_queue->label,
> > +                                       tx_queue->queue);
> > +                     }
> > +             }
> >       }
> >
> > +     rc = efx_alloc_xdp_tx_queues(efx);
> > +     if (rc)
> > +             return rc;
> >       efx_set_xdp_channels(efx);
> >
> >       rc = netif_set_real_num_tx_queues(efx->net_dev, efx->n_tx_channels);
> > --
> > 2.34.1
>


-- 
Íñigo Huguet