| Message ID | 20240724164840.2536605-4-larysa.zaremba@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: fix synchronization between .ndo_bpf() and reset | expand |
On 7/24/2024 9:48 AM, Larysa Zaremba wrote: > If VSI rebuild is pending, .ndo_bpf() can attach/detach the XDP program on > VSI without applying new ring configuration. When unconfiguring the VSI, we > can encounter the state in which there is an XDP program but no XDP rings > to destroy or there will be XDP rings that need to be destroyed, but no XDP > program to indicate their presence. > > When unconfiguring, rely on the presence of XDP rings rather then XDP > program, as they better represent the current state that has to be > destroyed. > > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > --- Right. When operating on the rings, we should be checking xdp_rings. Ok. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
>-----Original Message----- >From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of >Zaremba, Larysa >Sent: Wednesday, July 24, 2024 10:19 PM >To: intel-wired-lan@lists.osuosl.org >Cc: Drewek, Wojciech <wojciech.drewek@intel.com>; Fijalkowski, Maciej ><maciej.fijalkowski@intel.com>; Jesper Dangaard Brouer <hawk@kernel.org>; >Daniel Borkmann <daniel@iogearbox.net>; Zaremba, Larysa ><larysa.zaremba@intel.com>; netdev@vger.kernel.org; John Fastabend ><john.fastabend@gmail.com>; Alexei Starovoitov <ast@kernel.org>; linux- >kernel@vger.kernel.org; Eric Dumazet <edumazet@google.com>; Kubiak, >Michal <michal.kubiak@intel.com>; Nguyen, Anthony L ><anthony.l.nguyen@intel.com>; Nambiar, Amritha ><amritha.nambiar@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; >Jakub Kicinski <kuba@kernel.org>; bpf@vger.kernel.org; Paolo Abeni ><pabeni@redhat.com>; David S. Miller <davem@davemloft.net>; Karlsson, >Magnus <magnus.karlsson@intel.com> >Subject: [Intel-wired-lan] [PATCH iwl-net v2 3/6] ice: check for XDP rings >instead of bpf program when unconfiguring > >If VSI rebuild is pending, .ndo_bpf() can attach/detach the XDP program on VSI >without applying new ring configuration. When unconfiguring the VSI, we can >encounter the state in which there is an XDP program but no XDP rings to >destroy or there will be XDP rings that need to be destroyed, but no XDP >program to indicate their presence. > >When unconfiguring, rely on the presence of XDP rings rather then XDP >program, as they better represent the current state that has to be destroyed. > >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> >--- > drivers/net/ethernet/intel/ice/ice_lib.c | 4 ++-- >drivers/net/ethernet/intel/ice/ice_main.c | 4 ++-- >drivers/net/ethernet/intel/ice/ice_xsk.c | 6 +++--- > 3 files changed, 7 insertions(+), 7 deletions(-) > Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
On Wed, Jul 24, 2024 at 06:48:34PM +0200, Larysa Zaremba wrote: > If VSI rebuild is pending, .ndo_bpf() can attach/detach the XDP program on > VSI without applying new ring configuration. When unconfiguring the VSI, we > can encounter the state in which there is an XDP program but no XDP rings > to destroy or there will be XDP rings that need to be destroyed, but no XDP > program to indicate their presence. > > When unconfiguring, rely on the presence of XDP rings rather then XDP > program, as they better represent the current state that has to be > destroyed. > No Fixes: tag? > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_lib.c | 4 ++-- > drivers/net/ethernet/intel/ice/ice_main.c | 4 ++-- > drivers/net/ethernet/intel/ice/ice_xsk.c | 6 +++--- > 3 files changed, 7 insertions(+), 7 deletions(-) [...] > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c > index 2c1a843ba200..5dd50a2866cc 100644 > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c > @@ -39,7 +39,7 @@ static void ice_qp_reset_stats(struct ice_vsi *vsi, u16 q_idx) > sizeof(vsi_stat->rx_ring_stats[q_idx]->rx_stats)); > memset(&vsi_stat->tx_ring_stats[q_idx]->stats, 0, > sizeof(vsi_stat->tx_ring_stats[q_idx]->stats)); > - if (ice_is_xdp_ena_vsi(vsi)) > + if (vsi->xdp_rings) > memset(&vsi->xdp_rings[q_idx]->ring_stats->stats, 0, > sizeof(vsi->xdp_rings[q_idx]->ring_stats->stats)); > } > @@ -52,7 +52,7 @@ static void ice_qp_reset_stats(struct ice_vsi *vsi, u16 q_idx) > static void ice_qp_clean_rings(struct ice_vsi *vsi, u16 q_idx) > { > ice_clean_tx_ring(vsi->tx_rings[q_idx]); > - if (ice_is_xdp_ena_vsi(vsi)) { > + if (vsi->xdp_rings) { > synchronize_rcu(); > ice_clean_tx_ring(vsi->xdp_rings[q_idx]); > } > @@ -189,7 +189,7 @@ static int ice_qp_dis(struct ice_vsi *vsi, u16 q_idx) > err = ice_vsi_stop_tx_ring(vsi, ICE_NO_RESET, 0, tx_ring, &txq_meta); > if (err) > return err; > - if (ice_is_xdp_ena_vsi(vsi)) { > + if (vsi->xdp_rings) { From XSK POV these checks are false positive and I will be sending a patch that gets rid of it (I had this on my tree when working on timeout issues but I pulled this out as it was not a -net candidate IMHO). Just a heads up, this can go as-is currently. > struct ice_tx_ring *xdp_ring = vsi->xdp_rings[q_idx]; > > memset(&txq_meta, 0, sizeof(txq_meta)); > -- > 2.43.0 >
On Mon, Aug 12, 2024 at 02:58:00PM +0200, Maciej Fijalkowski wrote: > On Wed, Jul 24, 2024 at 06:48:34PM +0200, Larysa Zaremba wrote: > > If VSI rebuild is pending, .ndo_bpf() can attach/detach the XDP program on > > VSI without applying new ring configuration. When unconfiguring the VSI, we > > can encounter the state in which there is an XDP program but no XDP rings > > to destroy or there will be XDP rings that need to be destroyed, but no XDP > > program to indicate their presence. > > > > When unconfiguring, rely on the presence of XDP rings rather then XDP > > program, as they better represent the current state that has to be > > destroyed. > > > > No Fixes: tag? > This is basically a fixup for a previous patch. The need to replace program check with a ring check emerges from the existence of an in-between state that my synchronization introduces. So it does not fix anything from a referencable patch. I have separated this because it was seriously getting in the way when trying to read the synchronization logic. > > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_lib.c | 4 ++-- > > drivers/net/ethernet/intel/ice/ice_main.c | 4 ++-- > > drivers/net/ethernet/intel/ice/ice_xsk.c | 6 +++--- > > 3 files changed, 7 insertions(+), 7 deletions(-) > > [...] > > > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c > > index 2c1a843ba200..5dd50a2866cc 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c > > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c > > @@ -39,7 +39,7 @@ static void ice_qp_reset_stats(struct ice_vsi *vsi, u16 q_idx) > > sizeof(vsi_stat->rx_ring_stats[q_idx]->rx_stats)); > > memset(&vsi_stat->tx_ring_stats[q_idx]->stats, 0, > > sizeof(vsi_stat->tx_ring_stats[q_idx]->stats)); > > - if (ice_is_xdp_ena_vsi(vsi)) > > + if (vsi->xdp_rings) > > memset(&vsi->xdp_rings[q_idx]->ring_stats->stats, 0, > > sizeof(vsi->xdp_rings[q_idx]->ring_stats->stats)); > > } > > @@ -52,7 +52,7 @@ static void ice_qp_reset_stats(struct ice_vsi *vsi, u16 q_idx) > > static void ice_qp_clean_rings(struct ice_vsi *vsi, u16 q_idx) > > { > > ice_clean_tx_ring(vsi->tx_rings[q_idx]); > > - if (ice_is_xdp_ena_vsi(vsi)) { > > + if (vsi->xdp_rings) { > > synchronize_rcu(); > > ice_clean_tx_ring(vsi->xdp_rings[q_idx]); > > } > > @@ -189,7 +189,7 @@ static int ice_qp_dis(struct ice_vsi *vsi, u16 q_idx) > > err = ice_vsi_stop_tx_ring(vsi, ICE_NO_RESET, 0, tx_ring, &txq_meta); > > if (err) > > return err; > > - if (ice_is_xdp_ena_vsi(vsi)) { > > + if (vsi->xdp_rings) { > > From XSK POV these checks are false positive and I will be sending a patch > that gets rid of it (I had this on my tree when working on timeout issues > but I pulled this out as it was not a -net candidate IMHO). > > Just a heads up, this can go as-is currently. > Ok, thanks. > > struct ice_tx_ring *xdp_ring = vsi->xdp_rings[q_idx]; > > > > memset(&txq_meta, 0, sizeof(txq_meta)); > > -- > > 2.43.0 > >
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index bbef5ec67cae..f9852f1a136e 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -2419,7 +2419,7 @@ void ice_vsi_decfg(struct ice_vsi *vsi) dev_err(ice_pf_to_dev(pf), "Failed to remove RDMA scheduler config for VSI %u, err %d\n", vsi->vsi_num, err); - if (ice_is_xdp_ena_vsi(vsi)) + if (vsi->xdp_rings) /* return value check can be skipped here, it always returns * 0 if reset is in progress */ @@ -2521,7 +2521,7 @@ static void ice_vsi_release_msix(struct ice_vsi *vsi) for (q = 0; q < q_vector->num_ring_tx; q++) { ice_write_itr(&q_vector->tx, 0); wr32(hw, QINT_TQCTL(vsi->txq_map[txq]), 0); - if (ice_is_xdp_ena_vsi(vsi)) { + if (vsi->xdp_rings) { u32 xdp_txq = txq + vsi->num_xdp_txq; wr32(hw, QINT_TQCTL(vsi->txq_map[xdp_txq]), 0); diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index e50526b491fc..d7cc641643f8 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -7244,7 +7244,7 @@ int ice_down(struct ice_vsi *vsi) if (tx_err) netdev_err(vsi->netdev, "Failed stop Tx rings, VSI %d error %d\n", vsi->vsi_num, tx_err); - if (!tx_err && ice_is_xdp_ena_vsi(vsi)) { + if (!tx_err && vsi->xdp_rings) { tx_err = ice_vsi_stop_xdp_tx_rings(vsi); if (tx_err) netdev_err(vsi->netdev, "Failed stop XDP rings, VSI %d error %d\n", @@ -7261,7 +7261,7 @@ int ice_down(struct ice_vsi *vsi) ice_for_each_txq(vsi, i) ice_clean_tx_ring(vsi->tx_rings[i]); - if (ice_is_xdp_ena_vsi(vsi)) + if (vsi->xdp_rings) ice_for_each_xdp_txq(vsi, i) ice_clean_tx_ring(vsi->xdp_rings[i]); diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c index 2c1a843ba200..5dd50a2866cc 100644 --- a/drivers/net/ethernet/intel/ice/ice_xsk.c +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c @@ -39,7 +39,7 @@ static void ice_qp_reset_stats(struct ice_vsi *vsi, u16 q_idx) sizeof(vsi_stat->rx_ring_stats[q_idx]->rx_stats)); memset(&vsi_stat->tx_ring_stats[q_idx]->stats, 0, sizeof(vsi_stat->tx_ring_stats[q_idx]->stats)); - if (ice_is_xdp_ena_vsi(vsi)) + if (vsi->xdp_rings) memset(&vsi->xdp_rings[q_idx]->ring_stats->stats, 0, sizeof(vsi->xdp_rings[q_idx]->ring_stats->stats)); } @@ -52,7 +52,7 @@ static void ice_qp_reset_stats(struct ice_vsi *vsi, u16 q_idx) static void ice_qp_clean_rings(struct ice_vsi *vsi, u16 q_idx) { ice_clean_tx_ring(vsi->tx_rings[q_idx]); - if (ice_is_xdp_ena_vsi(vsi)) { + if (vsi->xdp_rings) { synchronize_rcu(); ice_clean_tx_ring(vsi->xdp_rings[q_idx]); } @@ -189,7 +189,7 @@ static int ice_qp_dis(struct ice_vsi *vsi, u16 q_idx) err = ice_vsi_stop_tx_ring(vsi, ICE_NO_RESET, 0, tx_ring, &txq_meta); if (err) return err; - if (ice_is_xdp_ena_vsi(vsi)) { + if (vsi->xdp_rings) { struct ice_tx_ring *xdp_ring = vsi->xdp_rings[q_idx]; memset(&txq_meta, 0, sizeof(txq_meta));