| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220511075220.3eut4vp33o4w4qtt@gmail.com>
Date: Wed, 11 May 2022 08:52:20 +0100
From: Martin Habets <habetsm.xilinx@...il.com>
To: Íñigo Huguet <ihuguet@...hat.com>
Cc: ecree.xilinx@...il.com, ap420073@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/5] sfc: separate channel->tx_queue and
efx->xdp_tx_queue mappings
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.
>
> 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.
> 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@...hat.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.
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
Powered by blists - more mailing lists