| Message ID | 20240822-igb_xdp_tx_lock-v1-1-718aecc753da@linutronix.de |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [iwl-net] igb: Always call igb_xdp_ring_update_tail() under Tx lock | expand |
On Thu, Aug 22, 2024 at 09:42:07AM +0200, Kurt Kanzenbach wrote: > From: Sriram Yagnaraman <sriram.yagnaraman@est.tech> > > Always call igb_xdp_ring_update_tail() under __netif_tx_lock, add a comment > and lockdep assert to indicate that. This is needed to share the same TX > ring between XDP, XSK and slow paths. Furthermore, the current XDP > implementation is racy on tail updates. > > Fixes: 9cbc948b5a20 ("igb: add XDP support") > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech> > [Kurt: Add lockdep assert and fixes tag] > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Thanks! > --- > drivers/net/ethernet/intel/igb/igb_main.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c > index 33a42b4c21e0..c71eb2bbb23d 100644 > --- a/drivers/net/ethernet/intel/igb/igb_main.c > +++ b/drivers/net/ethernet/intel/igb/igb_main.c > @@ -33,6 +33,7 @@ > #include <linux/bpf_trace.h> > #include <linux/pm_runtime.h> > #include <linux/etherdevice.h> > +#include <linux/lockdep.h> > #ifdef CONFIG_IGB_DCA > #include <linux/dca.h> > #endif > @@ -2914,8 +2915,11 @@ static int igb_xdp(struct net_device *dev, struct netdev_bpf *xdp) > } > } > > +/* This function assumes __netif_tx_lock is held by the caller. */ > static void igb_xdp_ring_update_tail(struct igb_ring *ring) > { > + lockdep_assert_held(&txring_txq(ring)->_xmit_lock); > + > /* Force memory writes to complete before letting h/w know there > * are new descriptors to fetch. > */ > @@ -3000,11 +3004,11 @@ static int igb_xdp_xmit(struct net_device *dev, int n, > nxmit++; > } > > - __netif_tx_unlock(nq); > - > if (unlikely(flags & XDP_XMIT_FLUSH)) > igb_xdp_ring_update_tail(tx_ring); > > + __netif_tx_unlock(nq); > + > return nxmit; > } > > @@ -8854,12 +8858,14 @@ static void igb_put_rx_buffer(struct igb_ring *rx_ring, > > static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget) > { > + unsigned int total_bytes = 0, total_packets = 0; > struct igb_adapter *adapter = q_vector->adapter; > struct igb_ring *rx_ring = q_vector->rx.ring; > - struct sk_buff *skb = rx_ring->skb; > - unsigned int total_bytes = 0, total_packets = 0; > u16 cleaned_count = igb_desc_unused(rx_ring); > + struct sk_buff *skb = rx_ring->skb; > + int cpu = smp_processor_id(); > unsigned int xdp_xmit = 0; > + struct netdev_queue *nq; > struct xdp_buff xdp; > u32 frame_sz = 0; > int rx_buf_pgcnt; > @@ -8987,7 +8993,10 @@ static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget) > if (xdp_xmit & IGB_XDP_TX) { > struct igb_ring *tx_ring = igb_xdp_tx_queue_mapping(adapter); > > + nq = txring_txq(tx_ring); > + __netif_tx_lock(nq, cpu); > igb_xdp_ring_update_tail(tx_ring); > + __netif_tx_unlock(nq); > } > > u64_stats_update_begin(&rx_ring->rx_syncp); > > --- > base-commit: a0b4a80ed6ce2cf8140fe926303ba609884b5d9b > change-id: 20240822-igb_xdp_tx_lock-b6846fddc758 > > Best regards, > -- > Kurt Kanzenbach <kurt@linutronix.de> >
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Kurt > Kanzenbach > Sent: Thursday, August 22, 2024 12:42 AM > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com> > Cc: Jesper Dangaard Brouer <hawk@kernel.org>; Daniel Borkmann > <daniel@iogearbox.net>; Sriram Yagnaraman > <sriram.yagnaraman@ericsson.com>; Sebastian Andrzej Siewior > <bigeasy@linutronix.de>; Kurt Kanzenbach <kurt@linutronix.de>; John Fastabend > <john.fastabend@gmail.com>; Alexei Starovoitov <ast@kernel.org>; Sriram > Yagnaraman <sriram.yagnaraman@est.tech>; Benjamin Steinke > <benjamin.steinke@woks-audio.com>; Eric Dumazet <edumazet@google.com>; > netdev@vger.kernel.org; Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; intel- > wired-lan@lists.osuosl.org; Jakub Kicinski <kuba@kernel.org>; > bpf@vger.kernel.org; Paolo Abeni <pabeni@redhat.com>; David S. Miller > <davem@davemloft.net> > Subject: [Intel-wired-lan] [PATCH iwl-net] igb: Always call igb_xdp_ring_update_tail() > under Tx lock > > From: Sriram Yagnaraman <sriram.yagnaraman@est.tech> > > Always call igb_xdp_ring_update_tail() under __netif_tx_lock, add a comment and > lockdep assert to indicate that. This is needed to share the same TX ring between > XDP, XSK and slow paths. Furthermore, the current XDP implementation is racy on > tail updates. > > Fixes: 9cbc948b5a20 ("igb: add XDP support") > Signed-off-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech> > [Kurt: Add lockdep assert and fixes tag] > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > drivers/net/ethernet/intel/igb/igb_main.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index 33a42b4c21e0..c71eb2bbb23d 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -33,6 +33,7 @@ #include <linux/bpf_trace.h> #include <linux/pm_runtime.h> #include <linux/etherdevice.h> +#include <linux/lockdep.h> #ifdef CONFIG_IGB_DCA #include <linux/dca.h> #endif @@ -2914,8 +2915,11 @@ static int igb_xdp(struct net_device *dev, struct netdev_bpf *xdp) } } +/* This function assumes __netif_tx_lock is held by the caller. */ static void igb_xdp_ring_update_tail(struct igb_ring *ring) { + lockdep_assert_held(&txring_txq(ring)->_xmit_lock); + /* Force memory writes to complete before letting h/w know there * are new descriptors to fetch. */ @@ -3000,11 +3004,11 @@ static int igb_xdp_xmit(struct net_device *dev, int n, nxmit++; } - __netif_tx_unlock(nq); - if (unlikely(flags & XDP_XMIT_FLUSH)) igb_xdp_ring_update_tail(tx_ring); + __netif_tx_unlock(nq); + return nxmit; } @@ -8854,12 +8858,14 @@ static void igb_put_rx_buffer(struct igb_ring *rx_ring, static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget) { + unsigned int total_bytes = 0, total_packets = 0; struct igb_adapter *adapter = q_vector->adapter; struct igb_ring *rx_ring = q_vector->rx.ring; - struct sk_buff *skb = rx_ring->skb; - unsigned int total_bytes = 0, total_packets = 0; u16 cleaned_count = igb_desc_unused(rx_ring); + struct sk_buff *skb = rx_ring->skb; + int cpu = smp_processor_id(); unsigned int xdp_xmit = 0; + struct netdev_queue *nq; struct xdp_buff xdp; u32 frame_sz = 0; int rx_buf_pgcnt; @@ -8987,7 +8993,10 @@ static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget) if (xdp_xmit & IGB_XDP_TX) { struct igb_ring *tx_ring = igb_xdp_tx_queue_mapping(adapter); + nq = txring_txq(tx_ring); + __netif_tx_lock(nq, cpu); igb_xdp_ring_update_tail(tx_ring); + __netif_tx_unlock(nq); } u64_stats_update_begin(&rx_ring->rx_syncp);