Skip to content

Commit de41e80

Browse files
PlaidCatPlaidCat
committed
idpf: fix a race in txq wakeup
jira LE-3467 Rebuild_History Non-Buildable kernel-4.18.0-553.58.1.el8_10 commit-author Brian Vazquez <brianvv@google.com> commit 7292af0 Empty-Commit: Cherry-Pick Conflicts during history rebuild. Will be included in final tarball splat. Ref for failed cherry-pick at: ciq/ciq_backports/kernel-4.18.0-553.58.1.el8_10/7292af04.failed 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. Luckily netif_subqueue_maybe_stop macro already allows you to use a function to evaluate the start/stop conditions so the fix only requires 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 separately the conditions for singleq and splitq. Note that idpf_tx_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 changing your 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 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 Fixes: 6818c4d ("idpf: add splitq start_xmit") Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com> Signed-off-by: Josh Hay <joshua.a.hay@intel.com> Signed-off-by: Brian Vazquez <brianvv@google.com> Signed-off-by: Luigi Rizzo <lrizzo@google.com> Reviewed-by: Simon Horman <horms@kernel.org> Tested-by: Samuel Salin <Samuel.salin@intel.com> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com> (cherry picked from commit 7292af0) Signed-off-by: Jonathan Maple <jmaple@ciq.com> # Conflicts: # drivers/net/ethernet/intel/idpf/idpf_txrx.c
1 parent f286fee commit de41e80

File tree

1 file changed

+199
-0
lines changed

1 file changed

+199
-0
lines changed

‎ciq/ciq_backports/kernel-4.18.0-553.58.1.el8_10/7292af04.failed

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
idpf: fix a race in txq wakeup
2+
3+
jira LE-3467
4+
Rebuild_History Non-Buildable kernel-4.18.0-553.58.1.el8_10
5+
commit-author Brian Vazquez <brianvv@google.com>
6+
commit 7292af042bcf22e2c18b96ed250f78498a5b28ab
7+
Empty-Commit: Cherry-Pick Conflicts during history rebuild.
8+
Will be included in final tarball splat. Ref for failed cherry-pick at:
9+
ciq/ciq_backports/kernel-4.18.0-553.58.1.el8_10/7292af04.failed
10+
11+
Add a helper function to correctly handle the lockless
12+
synchronization when the sender needs to block. The paradigm is
13+
14+
if (no_resources()) {
15+
stop_queue();
16+
barrier();
17+
if (!no_resources())
18+
restart_queue();
19+
}
20+
21+
netif_subqueue_maybe_stop already handles the paradigm correctly, but
22+
the code split the check for resources in three parts, the first one
23+
(descriptors) followed the protocol, but the other two (completions and
24+
tx_buf) were only doing the first part and so race prone.
25+
26+
Luckily netif_subqueue_maybe_stop macro already allows you to use a
27+
function to evaluate the start/stop conditions so the fix only requires
28+
the right helper function to evaluate all the conditions at once.
29+
30+
The patch removes idpf_tx_maybe_stop_common since it's no longer needed
31+
and instead adjusts separately the conditions for singleq and splitq.
32+
33+
Note that idpf_tx_buf_hw_update doesn't need to check for resources
34+
since that will be covered in idpf_tx_splitq_frame.
35+
36+
To reproduce:
37+
38+
Reduce the threshold for pending completions to increase the chances of
39+
hitting this pause by changing your kernel:
40+
41+
drivers/net/ethernet/intel/idpf/idpf_txrx.h
42+
43+
-#define IDPF_TX_COMPLQ_OVERFLOW_THRESH(txcq) ((txcq)->desc_count >> 1)
44+
+#define IDPF_TX_COMPLQ_OVERFLOW_THRESH(txcq) ((txcq)->desc_count >> 4)
45+
46+
Use pktgen to force the host to push small pkts very aggressively:
47+
48+
./pktgen_sample02_multiqueue.sh -i eth1 -s 100 -6 -d $IP -m $MAC \
49+
-p 10000-10000 -t 16 -n 0 -v -x -c 64
50+
51+
Fixes: 6818c4d5b3c2 ("idpf: add splitq start_xmit")
52+
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
53+
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
54+
Signed-off-by: Josh Hay <joshua.a.hay@intel.com>
55+
Signed-off-by: Brian Vazquez <brianvv@google.com>
56+
Signed-off-by: Luigi Rizzo <lrizzo@google.com>
57+
Reviewed-by: Simon Horman <horms@kernel.org>
58+
Tested-by: Samuel Salin <Samuel.salin@intel.com>
59+
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
60+
(cherry picked from commit 7292af042bcf22e2c18b96ed250f78498a5b28ab)
61+
Signed-off-by: Jonathan Maple <jmaple@ciq.com>
62+
63+
# Conflicts:
64+
# drivers/net/ethernet/intel/idpf/idpf_txrx.c
65+
diff --cc drivers/net/ethernet/intel/idpf/idpf_txrx.c
66+
index 7501a74f8dd9,5cf440e09d0a..000000000000
67+
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
68+
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
69+
@@@ -1971,29 -2184,19 +1971,42 @@@ void idpf_tx_splitq_build_flow_desc(uni
70+
desc->flow.qw1.compl_tag = cpu_to_le16(params->compl_tag);
71+
}
72+
73+
+ /* Global conditions to tell whether the txq (and related resources)
74+
+ * has room to allow the use of "size" descriptors.
75+
+ */
76+
+ static int idpf_txq_has_room(struct idpf_tx_queue *tx_q, u32 size)
77+
+ {
78+
+ if (IDPF_DESC_UNUSED(tx_q) < size ||
79+
+ IDPF_TX_COMPLQ_PENDING(tx_q->txq_grp) >
80+
+ IDPF_TX_COMPLQ_OVERFLOW_THRESH(tx_q->txq_grp->complq) ||
81+
+ IDPF_TX_BUF_RSV_LOW(tx_q))
82+
+ return 0;
83+
+ return 1;
84+
+ }
85+
+
86+
+/**
87+
+ * idpf_tx_maybe_stop_common - 1st level check for common Tx stop conditions
88+
+ * @tx_q: the queue to be checked
89+
+ * @size: number of descriptors we want to assure is available
90+
+ *
91+
+ * Returns 0 if stop is not needed
92+
+ */
93+
+int idpf_tx_maybe_stop_common(struct idpf_queue *tx_q, unsigned int size)
94+
+{
95+
+ struct netdev_queue *nq;
96+
+
97+
+ if (likely(IDPF_DESC_UNUSED(tx_q) >= size))
98+
+ return 0;
99+
+
100+
+ u64_stats_update_begin(&tx_q->stats_sync);
101+
+ u64_stats_inc(&tx_q->q_stats.tx.q_busy);
102+
+ u64_stats_update_end(&tx_q->stats_sync);
103+
+
104+
+ nq = netdev_get_tx_queue(tx_q->vport->netdev, tx_q->idx);
105+
+
106+
+ return netif_txq_maybe_stop(nq, IDPF_DESC_UNUSED(tx_q), size, size);
107+
+}
108+
+
109+
/**
110+
* idpf_tx_maybe_stop_splitq - 1st level check for Tx splitq stop conditions
111+
* @tx_q: the queue to be checked
112+
@@@ -2001,33 -2204,17 +2014,41 @@@
113+
*
114+
* Returns 0 if stop is not needed
115+
*/
116+
-static int idpf_tx_maybe_stop_splitq(struct idpf_tx_queue *tx_q,
117+
+static int idpf_tx_maybe_stop_splitq(struct idpf_queue *tx_q,
118+
unsigned int descs_needed)
119+
{
120+
++<<<<<<< HEAD
121+
+ if (idpf_tx_maybe_stop_common(tx_q, descs_needed))
122+
+ goto splitq_stop;
123+
+
124+
+ /* If there are too many outstanding completions expected on the
125+
+ * completion queue, stop the TX queue to give the device some time to
126+
+ * catch up
127+
+ */
128+
+ if (unlikely(IDPF_TX_COMPLQ_PENDING(tx_q->txq_grp) >
129+
+ IDPF_TX_COMPLQ_OVERFLOW_THRESH(tx_q->txq_grp->complq)))
130+
+ goto splitq_stop;
131+
+
132+
+ /* Also check for available book keeping buffers; if we are low, stop
133+
+ * the queue to wait for more completions
134+
+ */
135+
+ if (unlikely(IDPF_TX_BUF_RSV_LOW(tx_q)))
136+
+ goto splitq_stop;
137+
+
138+
+ return 0;
139+
+
140+
+splitq_stop:
141+
++=======
142+
+ if (netif_subqueue_maybe_stop(tx_q->netdev, tx_q->idx,
143+
+ idpf_txq_has_room(tx_q, descs_needed),
144+
+ 1, 1))
145+
+ return 0;
146+
+
147+
++>>>>>>> 7292af042bcf (idpf: fix a race in txq wakeup)
148+
u64_stats_update_begin(&tx_q->stats_sync);
149+
- u64_stats_inc(&tx_q->q_stats.q_busy);
150+
+ u64_stats_inc(&tx_q->q_stats.tx.q_busy);
151+
u64_stats_update_end(&tx_q->stats_sync);
152+
+ netif_stop_subqueue(tx_q->vport->netdev, tx_q->idx);
153+
154+
return -EBUSY;
155+
}
156+
@@@ -2047,11 -2234,9 +2068,14 @@@ void idpf_tx_buf_hw_update(struct idpf_
157+
{
158+
struct netdev_queue *nq;
159+
160+
- nq = netdev_get_tx_queue(tx_q->netdev, tx_q->idx);
161+
+ nq = netdev_get_tx_queue(tx_q->vport->netdev, tx_q->idx);
162+
tx_q->next_to_use = val;
163+
164+
++<<<<<<< HEAD
165+
+ idpf_tx_maybe_stop_common(tx_q, IDPF_TX_DESC_NEEDED);
166+
+
167+
++=======
168+
++>>>>>>> 7292af042bcf (idpf: fix a race in txq wakeup)
169+
/* Force memory writes to complete before letting h/w
170+
* know there are new descriptors to fetch. (Only
171+
* applicable for weak-ordered memory model archs,
172+
diff --git a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
173+
index 5c3d34d3de8a..464b98b59418 100644
174+
--- a/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
175+
+++ b/drivers/net/ethernet/intel/idpf/idpf_singleq_txrx.c
176+
@@ -356,17 +356,18 @@ static netdev_tx_t idpf_tx_singleq_frame(struct sk_buff *skb,
177+
{
178+
struct idpf_tx_offload_params offload = { };
179+
struct idpf_tx_buf *first;
180+
+ int csum, tso, needed;
181+
unsigned int count;
182+
__be16 protocol;
183+
- int csum, tso;
184+
185+
count = idpf_tx_desc_count_required(tx_q, skb);
186+
if (unlikely(!count))
187+
return idpf_tx_drop_skb(tx_q, skb);
188+
189+
- if (idpf_tx_maybe_stop_common(tx_q,
190+
- count + IDPF_TX_DESCS_PER_CACHE_LINE +
191+
- IDPF_TX_DESCS_FOR_CTX)) {
192+
+ needed = count + IDPF_TX_DESCS_PER_CACHE_LINE + IDPF_TX_DESCS_FOR_CTX;
193+
+ if (!netif_subqueue_maybe_stop(tx_q->netdev, tx_q->idx,
194+
+ IDPF_DESC_UNUSED(tx_q),
195+
+ needed, needed)) {
196+
idpf_tx_buf_hw_update(tx_q, tx_q->next_to_use, false);
197+
198+
return NETDEV_TX_BUSY;
199+
* Unmerged path drivers/net/ethernet/intel/idpf/idpf_txrx.c

0 commit comments

Comments
 (0)