| Message ID | 20200609034143.7668-1-snelson@pensando.io |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,1/1] ionic: wait on queue start until after IFF_UP | expand |
From: Shannon Nelson <snelson@pensando.io> Date: Mon, 8 Jun 2020 20:41:43 -0700 > The netif_running() test looks at __LINK_STATE_START which > gets set before ndo_open() is called, there is a window of > time between that and when the queues are actually ready to > be run. If ionic_check_link_status() notices that the link is > up very soon after netif_running() becomes true, it might try > to run the queues before they are ready, causing all manner of > potential issues. Since the netdev->flags IFF_UP isn't set > until after ndo_open() returns, we can wait for that before > we allow ionic_check_link_status() to start the queues. > > On the way back to close, __LINK_STATE_START is cleared before > calling ndo_stop(), and IFF_UP is cleared after. Both of > these need to be true in order to safely stop the queues > from ionic_check_link_status(). > > Fixes: 49d3b493673a ("ionic: disable the queues on link down") > Signed-off-by: Shannon Nelson <snelson@pensando.io> What will make sure the queues actually get started if this event's queue start gets skipped in this scenerio? This code is only invoked when the link status changes or when the firmware is started.
On 6/9/20 12:47 PM, David Miller wrote: > From: Shannon Nelson <snelson@pensando.io> > Date: Mon, 8 Jun 2020 20:41:43 -0700 > >> The netif_running() test looks at __LINK_STATE_START which >> gets set before ndo_open() is called, there is a window of >> time between that and when the queues are actually ready to >> be run. If ionic_check_link_status() notices that the link is >> up very soon after netif_running() becomes true, it might try >> to run the queues before they are ready, causing all manner of >> potential issues. Since the netdev->flags IFF_UP isn't set >> until after ndo_open() returns, we can wait for that before >> we allow ionic_check_link_status() to start the queues. >> >> On the way back to close, __LINK_STATE_START is cleared before >> calling ndo_stop(), and IFF_UP is cleared after. Both of >> these need to be true in order to safely stop the queues >> from ionic_check_link_status(). >> >> Fixes: 49d3b493673a ("ionic: disable the queues on link down") >> Signed-off-by: Shannon Nelson <snelson@pensando.io> > What will make sure the queues actually get started if this > event's queue start gets skipped in this scenerio? > > This code is only invoked when the link status changes or > when the firmware is started. If the link is already seen as up before ionic_open() is called it starts the queues at that point. If link isn't seen until after ionic_open(), then the queues are started in this periodic link check. sln
From: Shannon Nelson <snelson@pensando.io> Date: Tue, 9 Jun 2020 12:51:17 -0700 > On 6/9/20 12:47 PM, David Miller wrote: >> From: Shannon Nelson <snelson@pensando.io> >> Date: Mon, 8 Jun 2020 20:41:43 -0700 >> >>> The netif_running() test looks at __LINK_STATE_START which >>> gets set before ndo_open() is called, there is a window of >>> time between that and when the queues are actually ready to >>> be run. If ionic_check_link_status() notices that the link is >>> up very soon after netif_running() becomes true, it might try >>> to run the queues before they are ready, causing all manner of >>> potential issues. Since the netdev->flags IFF_UP isn't set >>> until after ndo_open() returns, we can wait for that before >>> we allow ionic_check_link_status() to start the queues. >>> >>> On the way back to close, __LINK_STATE_START is cleared before >>> calling ndo_stop(), and IFF_UP is cleared after. Both of >>> these need to be true in order to safely stop the queues >>> from ionic_check_link_status(). >>> >>> Fixes: 49d3b493673a ("ionic: disable the queues on link down") >>> Signed-off-by: Shannon Nelson <snelson@pensando.io> >> What will make sure the queues actually get started if this >> event's queue start gets skipped in this scenerio? >> >> This code is only invoked when the link status changes or >> when the firmware is started. > > If the link is already seen as up before ionic_open() is called it > starts the queues at that point. > > If link isn't seen until after ionic_open(), then the queues are > started in this periodic link check. I'm saying if it happens in the race condition you mention that you are protecting against, where running is true and IFF_UP is not. The link check is periodic? It looks like it triggers when the link state changes to me.
On 6/9/20 1:06 PM, David Miller wrote: > From: Shannon Nelson <snelson@pensando.io> > Date: Tue, 9 Jun 2020 12:51:17 -0700 > >> On 6/9/20 12:47 PM, David Miller wrote: >>> From: Shannon Nelson <snelson@pensando.io> >>> Date: Mon, 8 Jun 2020 20:41:43 -0700 >>> >>>> The netif_running() test looks at __LINK_STATE_START which >>>> gets set before ndo_open() is called, there is a window of >>>> time between that and when the queues are actually ready to >>>> be run. If ionic_check_link_status() notices that the link is >>>> up very soon after netif_running() becomes true, it might try >>>> to run the queues before they are ready, causing all manner of >>>> potential issues. Since the netdev->flags IFF_UP isn't set >>>> until after ndo_open() returns, we can wait for that before >>>> we allow ionic_check_link_status() to start the queues. >>>> >>>> On the way back to close, __LINK_STATE_START is cleared before >>>> calling ndo_stop(), and IFF_UP is cleared after. Both of >>>> these need to be true in order to safely stop the queues >>>> from ionic_check_link_status(). >>>> >>>> Fixes: 49d3b493673a ("ionic: disable the queues on link down") >>>> Signed-off-by: Shannon Nelson <snelson@pensando.io> >>> What will make sure the queues actually get started if this >>> event's queue start gets skipped in this scenerio? >>> >>> This code is only invoked when the link status changes or >>> when the firmware is started. >> If the link is already seen as up before ionic_open() is called it >> starts the queues at that point. >> >> If link isn't seen until after ionic_open(), then the queues are >> started in this periodic link check. > I'm saying if it happens in the race condition you mention that you > are protecting against, where running is true and IFF_UP is not. > > The link check is periodic? It looks like it triggers when the > link state changes to me. Yes, the link check is triggered by the periodic watchdog ionic_watchdog_cb(), every 5 seconds. sln
From: Shannon Nelson <snelson@pensando.io> Date: Tue, 9 Jun 2020 13:11:28 -0700 > Yes, the link check is triggered by the periodic watchdog > ionic_watchdog_cb(), every 5 seconds. Thanks for explaining. Applied and queued up for v5.7 -stable, thank you.
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_lif.c b/drivers/net/ethernet/pensando/ionic/ionic_lif.c index 7321a92f8395..fbc36e9e4729 100644 --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c @@ -116,7 +116,7 @@ static void ionic_link_status_check(struct ionic_lif *lif) netif_carrier_on(netdev); } - if (netif_running(lif->netdev)) + if (lif->netdev->flags & IFF_UP && netif_running(lif->netdev)) ionic_start_queues(lif); } else { if (netif_carrier_ok(netdev)) { @@ -124,7 +124,7 @@ static void ionic_link_status_check(struct ionic_lif *lif) netif_carrier_off(netdev); } - if (netif_running(lif->netdev)) + if (lif->netdev->flags & IFF_UP && netif_running(lif->netdev)) ionic_stop_queues(lif); }
The netif_running() test looks at __LINK_STATE_START which gets set before ndo_open() is called, there is a window of time between that and when the queues are actually ready to be run. If ionic_check_link_status() notices that the link is up very soon after netif_running() becomes true, it might try to run the queues before they are ready, causing all manner of potential issues. Since the netdev->flags IFF_UP isn't set until after ndo_open() returns, we can wait for that before we allow ionic_check_link_status() to start the queues. On the way back to close, __LINK_STATE_START is cleared before calling ndo_stop(), and IFF_UP is cleared after. Both of these need to be true in order to safely stop the queues from ionic_check_link_status(). Fixes: 49d3b493673a ("ionic: disable the queues on link down") Signed-off-by: Shannon Nelson <snelson@pensando.io> --- drivers/net/ethernet/pensando/ionic/ionic_lif.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)