| 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: <a1d7a46f-184e-4c01-8613-6cc5d35d2545@intel.com>
Date: Fri, 25 Apr 2025 15:41:18 -0700
From: Tony Nguyen <anthony.l.nguyen@...el.com>
To: Brian Vazquez <brianvv@...gle.com>, Brian Vazquez
<brianvv.kernel@...il.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
<intel-wired-lan@...ts.osuosl.org>
CC: David Decotigny <decot@...gle.com>, Anjali Singhai
<anjali.singhai@...el.com>, Sridhar Samudrala <sridhar.samudrala@...el.com>,
<linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
<emil.s.tantilov@...el.com>, Josh Hay <joshua.a.hay@...el.com>, Luigi Rizzo
<lrizzo@...gle.com>
Subject: Re: [iwl-next PATCH] idpf: fix a race in txq wakeup
On 4/25/2025 10:54 AM, Brian Vazquez wrote:
Should this be a bug fix going to iwl-net/net? If yes, you'll need to
add a Fixes: as well
> Add a helper function to correctly handle the lockless
> synchronization when the sender needs to block. The paradigm is
>
> if (no_resources()) {
> stop_queue();
> barrier();
> if (!no_resources())
> restart_queue();
> }
>
> netif_subqueue_maybe_stop already handles the paradigm correctly, but
> the code split the check for resources in three parts, the first one
> (descriptors) followed the protocol, but the other two (completions and
> tx_buf) were only doing the first part and so race prone.
>
> Luckly netif_subqueue_maybe_stop macro already allows you to use a
s/Luckly/Luckily
> function to evaluate the start/stop conditions so the fix only requires
> to pass the right helper function to evaluate all the conditions at once.
>
> The patch removes idpf_tx_maybe_stop_common since it's no longer needed
> and instead adjusts separetely the conditions for singleq and splitq.
s/separetely/separately
>
> Note that idpf_rx_buf_hw_update doesn't need to check for resources
> since that will be covered in idpf_tx_splitq_frame.
>
> To reproduce:
>
> Reduce the threshold for pending completions to increase the chances of
> hitting this pause by locally changing the kernel:
>
> drivers/net/ethernet/intel/idpf/idpf_txrx.h
>
> -#define IDPF_TX_COMPLQ_OVERFLOW_THRESH(txcq) ((txcq)->desc_count >> 1)
> +#define IDPF_TX_COMPLQ_OVERFLOW_THRESH(txcq) ((txcq)->desc_count >> 4)
>
> Use pktgen to force the host to push small pkts very aggresively:
s/aggresively/aggressively
>
> ./pktgen_sample02_multiqueue.sh -i eth1 -s 100 -6 -d $IP -m $MAC \
> -p 10000-10000 -t 16 -n 0 -v -x -c 64
>
> Signed-off-by: Josh Hay <joshua.a.hay@...el.com>
> Signed-off-by: Brian Vazquez <brianvv@...gle.com>
> Signed-off-by: Luigi Rizzo <lrizzo@...gle.com>
> ---
> .../ethernet/intel/idpf/idpf_singleq_txrx.c | 9 ++--
> drivers/net/ethernet/intel/idpf/idpf_txrx.c | 44 +++++++------------
> drivers/net/ethernet/intel/idpf/idpf_txrx.h | 8 ----
> 3 files changed, 21 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> index c6b927fa9979..fb85270c69d6 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
> @@ -364,15 +364,16 @@ netdev_tx_t idpf_tx_singleq_frame(struct sk_buff *skb,
> struct idpf_tx_buf *first;
> unsigned int count;
> __be16 protocol;
> - int csum, tso;
> + int csum, tso, needed;
This should be moved to be RCT; longest declaration to shortest.
> count = idpf_tx_desc_count_required(tx_q, skb);
> if (unlikely(!count))
> return idpf_tx_drop_skb(tx_q, skb);
>
> - if (idpf_tx_maybe_stop_common(tx_q,
> - count + IDPF_TX_DESCS_PER_CACHE_LINE +
> - IDPF_TX_DESCS_FOR_CTX)) {
> + needed = count + IDPF_TX_DESCS_PER_CACHE_LINE + IDPF_TX_DESCS_FOR_CTX;
> + if (!netif_subqueue_maybe_stop(tx_q->netdev, tx_q->idx,
> + IDPF_DESC_UNUSED(tx_q),
> + needed, needed)) {
> idpf_tx_buf_hw_update(tx_q, tx_q->next_to_use, false);
>
> u64_stats_update_begin(&tx_q->stats_sync);
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> index 970fa9e5c39b..cb41b6fcf03f 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
> @@ -2184,6 +2184,19 @@ void idpf_tx_splitq_build_flow_desc(union idpf_tx_flex_desc *desc,
> desc->flow.qw1.compl_tag = cpu_to_le16(params->compl_tag);
> }
>
> +/* Global conditions to tell whether the txq (and related resources)
> + * has room to allow the use of "size" descriptors.
> + */
> +static inline int txq_has_room(struct idpf_tx_queue *tx_q, u32 size)
no 'inline' in c files please. Also, it's preferred to prepend driver
name to the function i.e. idpf_txq_has_room()
Thanks,
Tony
> +{
> + if (IDPF_DESC_UNUSED(tx_q) < size ||
> + IDPF_TX_COMPLQ_PENDING(tx_q->txq_grp) >
> + IDPF_TX_COMPLQ_OVERFLOW_THRESH(tx_q->txq_grp->complq) ||
> + IDPF_TX_BUF_RSV_LOW(tx_q))
> + return 0;
> + return 1;
> +}
> +
> /**
> * idpf_tx_maybe_stop_splitq - 1st level check for Tx splitq stop conditions
> * @tx_q: the queue to be checked
> @@ -2194,29 +2207,10 @@ void idpf_tx_splitq_build_flow_desc(union idpf_tx_flex_desc *desc,
> static int idpf_tx_maybe_stop_splitq(struct idpf_tx_queue *tx_q,
> unsigned int descs_needed)
> {
> - if (idpf_tx_maybe_stop_common(tx_q, descs_needed))
> - goto out;
> -
> - /* If there are too many outstanding completions expected on the
> - * completion queue, stop the TX queue to give the device some time to
> - * catch up
> - */
> - if (unlikely(IDPF_TX_COMPLQ_PENDING(tx_q->txq_grp) >
> - IDPF_TX_COMPLQ_OVERFLOW_THRESH(tx_q->txq_grp->complq)))
> - goto splitq_stop;
> -
> - /* Also check for available book keeping buffers; if we are low, stop
> - * the queue to wait for more completions
> - */
> - if (unlikely(IDPF_TX_BUF_RSV_LOW(tx_q)))
> - goto splitq_stop;
> -
> - return 0;
> -
> -splitq_stop:
> - netif_stop_subqueue(tx_q->netdev, tx_q->idx);
> + if (netif_subqueue_maybe_stop(tx_q->netdev, tx_q->idx,
> + txq_has_room(tx_q, descs_needed), 1, 1))
> + return 0;
>
> -out:
> u64_stats_update_begin(&tx_q->stats_sync);
> u64_stats_inc(&tx_q->q_stats.q_busy);
> u64_stats_update_end(&tx_q->stats_sync);
> @@ -2242,12 +2236,6 @@ void idpf_tx_buf_hw_update(struct idpf_tx_queue *tx_q, u32 val,
> nq = netdev_get_tx_queue(tx_q->netdev, tx_q->idx);
> tx_q->next_to_use = val;
>
> - if (idpf_tx_maybe_stop_common(tx_q, IDPF_TX_DESC_NEEDED)) {
> - u64_stats_update_begin(&tx_q->stats_sync);
> - u64_stats_inc(&tx_q->q_stats.q_busy);
> - u64_stats_update_end(&tx_q->stats_sync);
> - }
> -
> /* Force memory writes to complete before letting h/w
> * know there are new descriptors to fetch. (Only
> * applicable for weak-ordered memory model archs,
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.h b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> index c779fe71df99..36a0f828a6f8 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> +++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.h
> @@ -1049,12 +1049,4 @@ bool idpf_rx_singleq_buf_hw_alloc_all(struct idpf_rx_queue *rxq,
> u16 cleaned_count);
> int idpf_tso(struct sk_buff *skb, struct idpf_tx_offload_params *off);
>
> -static inline bool idpf_tx_maybe_stop_common(struct idpf_tx_queue *tx_q,
> - u32 needed)
> -{
> - return !netif_subqueue_maybe_stop(tx_q->netdev, tx_q->idx,
> - IDPF_DESC_UNUSED(tx_q),
> - needed, needed);
> -}
> -
> #endif /* !_IDPF_TXRX_H_ */
Powered by blists - more mailing lists