| Message ID | 20240819100606.15383-5-larysa.zaremba@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: fix synchronization between .ndo_bpf() and reset | expand |
On Mon, Aug 19, 2024 at 12:05:41PM +0200, Larysa Zaremba wrote: > Consider the following scenario: > > .ndo_bpf() | ice_prepare_for_reset() | > ________________________|_______________________________________| > rtnl_lock() | | > ice_down() | | > | test_bit(ICE_VSI_DOWN) - true | > | ice_dis_vsi() returns | > ice_up() | | > | proceeds to rebuild a running VSI | > > .ndo_bpf() is not the only rtnl-locked callback that toggles the interface > to apply new configuration. Another example is .set_channels(). > > To avoid the race condition above, act only after reading ICE_VSI_DOWN > under rtnl_lock. > > Fixes: 0f9d5027a749 ("ice: Refactor VSI allocation, deletion and rebuild flow") > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_lib.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c > index b72338974a60..94029e446b99 100644 > --- a/drivers/net/ethernet/intel/ice/ice_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c > @@ -2665,8 +2665,7 @@ int ice_ena_vsi(struct ice_vsi *vsi, bool locked) > */ > void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > { > - if (test_bit(ICE_VSI_DOWN, vsi->state)) > - return; > + bool already_down = test_bit(ICE_VSI_DOWN, vsi->state); > > set_bit(ICE_VSI_NEEDS_RESTART, vsi->state); > > @@ -2674,15 +2673,16 @@ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > if (netif_running(vsi->netdev)) { > if (!locked) > rtnl_lock(); > - > - ice_vsi_close(vsi); > + already_down = test_bit(ICE_VSI_DOWN, vsi->state); > + if (!already_down) > + ice_vsi_close(vsi); ehh sorry for being sloppy reviewer. we still are testing ICE_VSI_DOWN in ice_vsi_close(). wouldn't all of this be cleaner if we would bail out of the called function when bit was already set? > > if (!locked) > rtnl_unlock(); > - } else { > + } else if (!already_down) { > ice_vsi_close(vsi); > } > - } else if (vsi->type == ICE_VSI_CTRL) { > + } else if (vsi->type == ICE_VSI_CTRL && !already_down) { > ice_vsi_close(vsi); > } > } > -- > 2.43.0 >
On Thu, Aug 22, 2024 at 01:34:33PM +0200, Maciej Fijalkowski wrote: > On Mon, Aug 19, 2024 at 12:05:41PM +0200, Larysa Zaremba wrote: > > Consider the following scenario: > > > > .ndo_bpf() | ice_prepare_for_reset() | > > ________________________|_______________________________________| > > rtnl_lock() | | > > ice_down() | | > > | test_bit(ICE_VSI_DOWN) - true | > > | ice_dis_vsi() returns | > > ice_up() | | > > | proceeds to rebuild a running VSI | > > > > .ndo_bpf() is not the only rtnl-locked callback that toggles the interface > > to apply new configuration. Another example is .set_channels(). > > > > To avoid the race condition above, act only after reading ICE_VSI_DOWN > > under rtnl_lock. > > > > Fixes: 0f9d5027a749 ("ice: Refactor VSI allocation, deletion and rebuild flow") > > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > > Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_lib.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c > > index b72338974a60..94029e446b99 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_lib.c > > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c > > @@ -2665,8 +2665,7 @@ int ice_ena_vsi(struct ice_vsi *vsi, bool locked) > > */ > > void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > > { > > - if (test_bit(ICE_VSI_DOWN, vsi->state)) > > - return; > > + bool already_down = test_bit(ICE_VSI_DOWN, vsi->state); > > > > set_bit(ICE_VSI_NEEDS_RESTART, vsi->state); > > > > @@ -2674,15 +2673,16 @@ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > > if (netif_running(vsi->netdev)) { > > if (!locked) > > rtnl_lock(); > > - > > - ice_vsi_close(vsi); > > + already_down = test_bit(ICE_VSI_DOWN, vsi->state); > > + if (!already_down) > > + ice_vsi_close(vsi); > > ehh sorry for being sloppy reviewer. we still are testing ICE_VSI_DOWN in > ice_vsi_close(). wouldn't all of this be cleaner if we would bail out of > the called function when bit was already set? > I am not sure I see the possibility to rewrite this as you suggest, we cannot bail out for the netif_running() case due to needing to unlock after ice_vsi_close() finishes. This leaves bailing out in case of CTRL VSI and non-running PF, which we could do, but it would require a lengthy if condition, which is not that much better than nested code, IMO. > > > > if (!locked) > > rtnl_unlock(); > > - } else { > > + } else if (!already_down) { > > ice_vsi_close(vsi); > > } > > - } else if (vsi->type == ICE_VSI_CTRL) { > > + } else if (vsi->type == ICE_VSI_CTRL && !already_down) { > > ice_vsi_close(vsi); > > } > > } > > -- > > 2.43.0 > >
On Thu, Aug 22, 2024 at 02:56:50PM +0200, Larysa Zaremba wrote: > On Thu, Aug 22, 2024 at 01:34:33PM +0200, Maciej Fijalkowski wrote: > > On Mon, Aug 19, 2024 at 12:05:41PM +0200, Larysa Zaremba wrote: > > > Consider the following scenario: > > > > > > .ndo_bpf() | ice_prepare_for_reset() | > > > ________________________|_______________________________________| > > > rtnl_lock() | | > > > ice_down() | | > > > | test_bit(ICE_VSI_DOWN) - true | > > > | ice_dis_vsi() returns | > > > ice_up() | | > > > | proceeds to rebuild a running VSI | > > > > > > .ndo_bpf() is not the only rtnl-locked callback that toggles the interface > > > to apply new configuration. Another example is .set_channels(). > > > > > > To avoid the race condition above, act only after reading ICE_VSI_DOWN > > > under rtnl_lock. > > > > > > Fixes: 0f9d5027a749 ("ice: Refactor VSI allocation, deletion and rebuild flow") > > > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > > > Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > --- > > > drivers/net/ethernet/intel/ice/ice_lib.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c > > > index b72338974a60..94029e446b99 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_lib.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c > > > @@ -2665,8 +2665,7 @@ int ice_ena_vsi(struct ice_vsi *vsi, bool locked) > > > */ > > > void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > > > { > > > - if (test_bit(ICE_VSI_DOWN, vsi->state)) > > > - return; > > > + bool already_down = test_bit(ICE_VSI_DOWN, vsi->state); > > > > > > set_bit(ICE_VSI_NEEDS_RESTART, vsi->state); > > > > > > @@ -2674,15 +2673,16 @@ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > > > if (netif_running(vsi->netdev)) { > > > if (!locked) > > > rtnl_lock(); > > > - > > > - ice_vsi_close(vsi); > > > + already_down = test_bit(ICE_VSI_DOWN, vsi->state); > > > + if (!already_down) > > > + ice_vsi_close(vsi); > > > > ehh sorry for being sloppy reviewer. we still are testing ICE_VSI_DOWN in > > ice_vsi_close(). wouldn't all of this be cleaner if we would bail out of > > the called function when bit was already set? > > > > I am not sure I see the possibility to rewrite this as you suggest, we cannot > bail out for the netif_running() case due to needing to unlock after > ice_vsi_close() finishes. This leaves bailing out in case of CTRL VSI and > non-running PF, which we could do, but it would require a lengthy if condition, > which is not that much better than nested code, IMO. Hmm. I meant to move bit checking onto ice_vsi_close() only, so you would bail out of it in case bit has already been set. overall, ice_dis_vsi() is a very cumbersome way of calling ice_vsi_close() :( I guess we can progress with what you have but i'd like to brainstorm later about some simplification around it. I prototyped something but not tested that, just to maybe spark a discussion. Feels easier to read and swallow in the end. Not sure if functionality is kept:) >From 706289d5c37c41cd3021997e0d5e64d7496e5536 Mon Sep 17 00:00:00 2001 From: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Date: Thu, 22 Aug 2024 16:33:37 +0200 Subject: [PATCH] ice: simplify ice_dis_vsi() Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- drivers/net/ethernet/intel/ice/ice_lib.c | 46 +++++++++++++----------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index f559e60992fa..8ccdda69a1d4 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -2625,14 +2625,34 @@ void ice_vsi_free_rx_rings(struct ice_vsi *vsi) */ void ice_vsi_close(struct ice_vsi *vsi) { - if (!test_and_set_bit(ICE_VSI_DOWN, vsi->state)) - ice_down(vsi); + if (test_bit(ICE_VSI_DOWN, vsi->state)) + return; + + set_bit(ICE_VSI_DOWN, vsi->state); + ice_down(vsi); ice_vsi_free_irq(vsi); ice_vsi_free_tx_rings(vsi); ice_vsi_free_rx_rings(vsi); } +/** + * __ice_vsi_close - variant of shutting down a VSI that takes care of + * rtnl_lock + * @vsi: the VSI being shut down + * @take_lock: to lock or not to lock + */ +static void __ice_vsi_close(struct ice_vsi *vsi, bool take_lock) +{ + if (take_lock) + rtnl_lock(); + + ice_vsi_close(vsi); + + if (take_lock) + rtnl_unlock(); +} + /** * ice_ena_vsi - resume a VSI * @vsi: the VSI being resume @@ -2671,26 +2691,12 @@ int ice_ena_vsi(struct ice_vsi *vsi, bool locked) */ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) { - if (test_bit(ICE_VSI_DOWN, vsi->state)) - return; - set_bit(ICE_VSI_NEEDS_RESTART, vsi->state); - if (vsi->type == ICE_VSI_PF && vsi->netdev) { - if (netif_running(vsi->netdev)) { - if (!locked) - rtnl_lock(); - - ice_vsi_close(vsi); - - if (!locked) - rtnl_unlock(); - } else { - ice_vsi_close(vsi); - } - } else if (vsi->type == ICE_VSI_CTRL) { - ice_vsi_close(vsi); - } + if (vsi->type == ICE_VSI_PF && vsi->netdev) + __ice_vsi_close(vsi, !locked && netif_running(vsi->netdev)); + else if (vsi->type == ICE_VSI_CTRL) + __ice_vsi_close(vsi, false); } /**
On Thu, Aug 22, 2024 at 04:42:44PM +0200, Maciej Fijalkowski wrote: > On Thu, Aug 22, 2024 at 02:56:50PM +0200, Larysa Zaremba wrote: > > On Thu, Aug 22, 2024 at 01:34:33PM +0200, Maciej Fijalkowski wrote: > > > On Mon, Aug 19, 2024 at 12:05:41PM +0200, Larysa Zaremba wrote: > > > > Consider the following scenario: > > > > > > > > .ndo_bpf() | ice_prepare_for_reset() | > > > > ________________________|_______________________________________| > > > > rtnl_lock() | | > > > > ice_down() | | > > > > | test_bit(ICE_VSI_DOWN) - true | > > > > | ice_dis_vsi() returns | > > > > ice_up() | | > > > > | proceeds to rebuild a running VSI | > > > > > > > > .ndo_bpf() is not the only rtnl-locked callback that toggles the interface > > > > to apply new configuration. Another example is .set_channels(). > > > > > > > > To avoid the race condition above, act only after reading ICE_VSI_DOWN > > > > under rtnl_lock. > > > > > > > > Fixes: 0f9d5027a749 ("ice: Refactor VSI allocation, deletion and rebuild flow") > > > > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > > > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > > > > Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > > --- > > > > drivers/net/ethernet/intel/ice/ice_lib.c | 12 ++++++------ > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c > > > > index b72338974a60..94029e446b99 100644 > > > > --- a/drivers/net/ethernet/intel/ice/ice_lib.c > > > > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c > > > > @@ -2665,8 +2665,7 @@ int ice_ena_vsi(struct ice_vsi *vsi, bool locked) > > > > */ > > > > void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > > > > { > > > > - if (test_bit(ICE_VSI_DOWN, vsi->state)) > > > > - return; > > > > + bool already_down = test_bit(ICE_VSI_DOWN, vsi->state); > > > > > > > > set_bit(ICE_VSI_NEEDS_RESTART, vsi->state); > > > > > > > > @@ -2674,15 +2673,16 @@ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > > > > if (netif_running(vsi->netdev)) { > > > > if (!locked) > > > > rtnl_lock(); > > > > - > > > > - ice_vsi_close(vsi); > > > > + already_down = test_bit(ICE_VSI_DOWN, vsi->state); > > > > + if (!already_down) > > > > + ice_vsi_close(vsi); > > > > > > ehh sorry for being sloppy reviewer. we still are testing ICE_VSI_DOWN in > > > ice_vsi_close(). wouldn't all of this be cleaner if we would bail out of > > > the called function when bit was already set? > > > > > > > I am not sure I see the possibility to rewrite this as you suggest, we cannot > > bail out for the netif_running() case due to needing to unlock after > > ice_vsi_close() finishes. This leaves bailing out in case of CTRL VSI and > > non-running PF, which we could do, but it would require a lengthy if condition, > > which is not that much better than nested code, IMO. > > Hmm. I meant to move bit checking onto ice_vsi_close() only, so you would > bail out of it in case bit has already been set. > > overall, ice_dis_vsi() is a very cumbersome way of calling ice_vsi_close() > :( > > I guess we can progress with what you have but i'd like to brainstorm > later about some simplification around it. > > I prototyped something but not tested that, just to maybe spark a > discussion. Feels easier to read and swallow in the end. Not sure if > functionality is kept:) > Ok, now I get it. Yes, this is something worth considering for a -next patch. Opting out of closing the VSI based on a down state seems not very nice though :/ I am not even sure if such approach is correct in ice_dis_vsi or is it just some ancient atrifact. Seems like it needs some VSI state changes analysis. > From 706289d5c37c41cd3021997e0d5e64d7496e5536 Mon Sep 17 00:00:00 2001 > From: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > Date: Thu, 22 Aug 2024 16:33:37 +0200 > Subject: [PATCH] ice: simplify ice_dis_vsi() > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_lib.c | 46 +++++++++++++----------- > 1 file changed, 26 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c > index f559e60992fa..8ccdda69a1d4 100644 > --- a/drivers/net/ethernet/intel/ice/ice_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c > @@ -2625,14 +2625,34 @@ void ice_vsi_free_rx_rings(struct ice_vsi *vsi) > */ > void ice_vsi_close(struct ice_vsi *vsi) > { > - if (!test_and_set_bit(ICE_VSI_DOWN, vsi->state)) > - ice_down(vsi); > + if (test_bit(ICE_VSI_DOWN, vsi->state)) > + return; > + > + set_bit(ICE_VSI_DOWN, vsi->state); > > + ice_down(vsi); > ice_vsi_free_irq(vsi); > ice_vsi_free_tx_rings(vsi); > ice_vsi_free_rx_rings(vsi); > } > > +/** > + * __ice_vsi_close - variant of shutting down a VSI that takes care of > + * rtnl_lock > + * @vsi: the VSI being shut down > + * @take_lock: to lock or not to lock > + */ > +static void __ice_vsi_close(struct ice_vsi *vsi, bool take_lock) > +{ > + if (take_lock) > + rtnl_lock(); > + > + ice_vsi_close(vsi); > + > + if (take_lock) > + rtnl_unlock(); > +} > + > /** > * ice_ena_vsi - resume a VSI > * @vsi: the VSI being resume > @@ -2671,26 +2691,12 @@ int ice_ena_vsi(struct ice_vsi *vsi, bool locked) > */ > void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > { > - if (test_bit(ICE_VSI_DOWN, vsi->state)) > - return; > - > set_bit(ICE_VSI_NEEDS_RESTART, vsi->state); > > - if (vsi->type == ICE_VSI_PF && vsi->netdev) { > - if (netif_running(vsi->netdev)) { > - if (!locked) > - rtnl_lock(); > - > - ice_vsi_close(vsi); > - > - if (!locked) > - rtnl_unlock(); > - } else { > - ice_vsi_close(vsi); > - } > - } else if (vsi->type == ICE_VSI_CTRL) { > - ice_vsi_close(vsi); > - } > + if (vsi->type == ICE_VSI_PF && vsi->netdev) > + __ice_vsi_close(vsi, !locked && netif_running(vsi->netdev)); > + else if (vsi->type == ICE_VSI_CTRL) > + __ice_vsi_close(vsi, false); > } > > /** > -- > 2.34.1 > > > > > > > > > > > > > if (!locked) > > > > rtnl_unlock(); > > > > - } else { > > > > + } else if (!already_down) { > > > > ice_vsi_close(vsi); > > > > } > > > > - } else if (vsi->type == ICE_VSI_CTRL) { > > > > + } else if (vsi->type == ICE_VSI_CTRL && !already_down) { > > > > ice_vsi_close(vsi); > > > > } > > > > } > > > > -- > > > > 2.43.0 > > > >
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index b72338974a60..94029e446b99 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -2665,8 +2665,7 @@ int ice_ena_vsi(struct ice_vsi *vsi, bool locked) */ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) { - if (test_bit(ICE_VSI_DOWN, vsi->state)) - return; + bool already_down = test_bit(ICE_VSI_DOWN, vsi->state); set_bit(ICE_VSI_NEEDS_RESTART, vsi->state); @@ -2674,15 +2673,16 @@ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) if (netif_running(vsi->netdev)) { if (!locked) rtnl_lock(); - - ice_vsi_close(vsi); + already_down = test_bit(ICE_VSI_DOWN, vsi->state); + if (!already_down) + ice_vsi_close(vsi); if (!locked) rtnl_unlock(); - } else { + } else if (!already_down) { ice_vsi_close(vsi); } - } else if (vsi->type == ICE_VSI_CTRL) { + } else if (vsi->type == ICE_VSI_CTRL && !already_down) { ice_vsi_close(vsi); } }
Consider the following scenario: .ndo_bpf() | ice_prepare_for_reset() | ________________________|_______________________________________| rtnl_lock() | | ice_down() | | | test_bit(ICE_VSI_DOWN) - true | | ice_dis_vsi() returns | ice_up() | | | proceeds to rebuild a running VSI | .ndo_bpf() is not the only rtnl-locked callback that toggles the interface to apply new configuration. Another example is .set_channels(). To avoid the race condition above, act only after reading ICE_VSI_DOWN under rtnl_lock. Fixes: 0f9d5027a749 ("ice: Refactor VSI allocation, deletion and rebuild flow") Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> --- drivers/net/ethernet/intel/ice/ice_lib.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)