Thanks for the quick review Tony!
On Fri, Apr 25, 2025 at 6:41 PM Tony Nguyen <anthony.l.ngu...@intel.com> wrote:
>
> 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
>
Correct, will send to iwl-net then and add the Fixes tag.
> > 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....@intel.com>
> > Signed-off-by: Brian Vazquez <bria...@google.com>
> > Signed-off-by: Luigi Rizzo <lri...@google.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.
acked
> > 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()
will fix in v2
>
> 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_ */
>
