* [PATCH net 0/2] ibmvnic: fixes in reset path @ 2020-10-28 5:57 Lijun Pan 2020-10-28 5:57 ` [PATCH net 1/2] ibmvnic: notify peers when failover and migration happen Lijun Pan 2020-10-28 5:57 ` [PATCH net 2/2] ibmvnic: skip tx timeout reset while in resetting Lijun Pan 0 siblings, 2 replies; 5+ messages in thread From: Lijun Pan @ 2020-10-28 5:57 UTC (permalink / raw) To: netdev; +Cc: Lijun Pan Patch 1/2 notifies peers in failover and migration reset. Patch 2/2 skips timeout reset if it is already resetting. Lijun Pan (2): ibmvnic: notify peers when failover and migration happen ibmvnic: skip tx timeout reset while in resetting drivers/net/ethernet/ibm/ibmvnic.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) -- 2.23.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net 1/2] ibmvnic: notify peers when failover and migration happen 2020-10-28 5:57 [PATCH net 0/2] ibmvnic: fixes in reset path Lijun Pan @ 2020-10-28 5:57 ` Lijun Pan 2020-10-30 20:27 ` Jakub Kicinski 2020-10-28 5:57 ` [PATCH net 2/2] ibmvnic: skip tx timeout reset while in resetting Lijun Pan 1 sibling, 1 reply; 5+ messages in thread From: Lijun Pan @ 2020-10-28 5:57 UTC (permalink / raw) To: netdev; +Cc: Lijun Pan, Brian King, Pradeep Satyanarayana, Dany Madden We need to notify peers only when failover and migration happen. It is unnecessary to call that in other events like FATAL, NON_FATAL, CHANGE_PARAM, and TIMEOUT resets since in those scenarios the MAC address and ip address mapping does not change. Originally all the resets except CHANGE_PARAM are processed by do_reset such that we need to find out failover and migration cases in do_reset and call notifier functions. We only need to notify peers in do_reset and do_hard_reset. We don't need notify peers in do_change_param_reset since it is a CHANGE_PARAM reset. In a nested reset case, it will finally call into do_hard_reset with reasons other than failvoer and migration. So, we don't need to check the reset reason in do_hard_reset and just call notifier functions anyway. netdev_notify_peers calls below two functions with rtnl lock(). call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, dev); call_netdevice_notifiers(NETDEV_RESEND_IGMP, dev); When netdev_notify_peers was substituted in commit 986103e7920c ("net/ibmvnic: Fix RTNL deadlock during device reset"), call_netdevice_notifiers(NETDEV_RESEND_IGMP, dev) was missed. Fixes: 61d3e1d9bc2a ("ibmvnic: Remove netdev notify for failover resets") Fixes: 986103e7920c ("net/ibmvnic: Fix RTNL deadlock during device reset") Suggested-by: Brian King <brking@linux.vnet.ibm.com> Suggested-by: Pradeep Satyanarayana <pradeeps@linux.vnet.ibm.com> Signed-off-by: Dany Madden <drt@linux.ibm.com> Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 1b702a43a5d0..718da39f5ae4 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -2067,8 +2067,11 @@ static int do_reset(struct ibmvnic_adapter *adapter, for (i = 0; i < adapter->req_rx_queues; i++) napi_schedule(&adapter->napi[i]); - if (adapter->reset_reason != VNIC_RESET_FAILOVER) + if (adapter->reset_reason == VNIC_RESET_FAILOVER || + adapter->reset_reason == VNIC_RESET_MOBILITY) { call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, netdev); + call_netdevice_notifiers(NETDEV_RESEND_IGMP, netdev); + } rc = 0; @@ -2138,6 +2141,9 @@ static int do_hard_reset(struct ibmvnic_adapter *adapter, if (rc) return IBMVNIC_OPEN_FAILED; + call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, netdev); + call_netdevice_notifiers(NETDEV_RESEND_IGMP, netdev); + return 0; } -- 2.23.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net 1/2] ibmvnic: notify peers when failover and migration happen 2020-10-28 5:57 ` [PATCH net 1/2] ibmvnic: notify peers when failover and migration happen Lijun Pan @ 2020-10-30 20:27 ` Jakub Kicinski 2020-11-05 4:54 ` drt 0 siblings, 1 reply; 5+ messages in thread From: Jakub Kicinski @ 2020-10-30 20:27 UTC (permalink / raw) To: Lijun Pan; +Cc: netdev, Brian King, Pradeep Satyanarayana, Dany Madden On Wed, 28 Oct 2020 00:57:41 -0500 Lijun Pan wrote: > We need to notify peers only when failover and migration happen. > It is unnecessary to call that in other events like > FATAL, NON_FATAL, CHANGE_PARAM, and TIMEOUT resets > since in those scenarios the MAC address and ip address mapping > does not change. Originally all the resets except CHANGE_PARAM > are processed by do_reset such that we need to find out > failover and migration cases in do_reset and call notifier functions. > We only need to notify peers in do_reset and do_hard_reset. > We don't need notify peers in do_change_param_reset since it is > a CHANGE_PARAM reset. In a nested reset case, it will finally > call into do_hard_reset with reasons other than failvoer and > migration. So, we don't need to check the reset reason in > do_hard_reset and just call notifier functions anyway. You're completely undoing the commit you linked to: commit 61d3e1d9bc2a1910d773cbf4ed6f587a7a6166b5 Author: Nathan Fontenot <nfont@linux.vnet.ibm.com> Date: Mon Jun 12 20:47:45 2017 -0400 ibmvnic: Remove netdev notify for failover resets When handling a driver reset due to a failover of the backing server on the vios, doing the netdev_notify_peers() can cause network traffic to stall or halt. Remove the netdev notify call for failover resets. Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com> Signed-off-by: David S. Miller <davem@davemloft.net> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index fd3ef3005fb0..59ea7a5ae776 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -1364,7 +1364,9 @@ static int do_reset(struct ibmvnic_adapter *adapter, for (i = 0; i < adapter->req_rx_queues; i++) napi_schedule(&adapter->napi[i]); - netdev_notify_peers(netdev); + if (adapter->reset_reason != VNIC_RESET_FAILOVER) + netdev_notify_peers(netdev); + return 0; } But you don't seem to address why this change was unnecessary. AFAIK you're saying "we only need this event for FAILOVER and MOBILITY" but the previous commit _excluded_ FAILOVER for some vague reason. If the previous commit was incorrect you need to explain that in the commit message. > netdev_notify_peers calls below two functions with rtnl lock(). > call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, dev); > call_netdevice_notifiers(NETDEV_RESEND_IGMP, dev); > When netdev_notify_peers was substituted in > commit 986103e7920c ("net/ibmvnic: Fix RTNL deadlock during device reset"), > call_netdevice_notifiers(NETDEV_RESEND_IGMP, dev) was missed. That should be a separate patch. > Fixes: 61d3e1d9bc2a ("ibmvnic: Remove netdev notify for failover resets") > Fixes: 986103e7920c ("net/ibmvnic: Fix RTNL deadlock during device > reset") Please don't line-wrap fixes tags. > Suggested-by: Brian King <brking@linux.vnet.ibm.com> > Suggested-by: Pradeep Satyanarayana <pradeeps@linux.vnet.ibm.com> > Signed-off-by: Dany Madden <drt@linux.ibm.com> > Signed-off-by: Lijun Pan <ljp@linux.ibm.com> > --- > drivers/net/ethernet/ibm/ibmvnic.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c > index 1b702a43a5d0..718da39f5ae4 100644 > --- a/drivers/net/ethernet/ibm/ibmvnic.c > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > @@ -2067,8 +2067,11 @@ static int do_reset(struct ibmvnic_adapter *adapter, > for (i = 0; i < adapter->req_rx_queues; i++) > napi_schedule(&adapter->napi[i]); > > - if (adapter->reset_reason != VNIC_RESET_FAILOVER) > + if (adapter->reset_reason == VNIC_RESET_FAILOVER || > + adapter->reset_reason == VNIC_RESET_MOBILITY) { > call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, netdev); > + call_netdevice_notifiers(NETDEV_RESEND_IGMP, netdev); > + } > > rc = 0; > > @@ -2138,6 +2141,9 @@ static int do_hard_reset(struct ibmvnic_adapter *adapter, > if (rc) > return IBMVNIC_OPEN_FAILED; > > + call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, netdev); > + call_netdevice_notifiers(NETDEV_RESEND_IGMP, netdev); > + > return 0; > } > ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net 1/2] ibmvnic: notify peers when failover and migration happen 2020-10-30 20:27 ` Jakub Kicinski @ 2020-11-05 4:54 ` drt 0 siblings, 0 replies; 5+ messages in thread From: drt @ 2020-11-05 4:54 UTC (permalink / raw) To: Jakub Kicinski Cc: Lijun Pan, netdev, Brian King, Pradeep Satyanarayana, Dany Madden On 2020-10-30 13:27, Jakub Kicinski wrote: > On Wed, 28 Oct 2020 00:57:41 -0500 Lijun Pan wrote: >> We need to notify peers only when failover and migration happen. >> It is unnecessary to call that in other events like >> FATAL, NON_FATAL, CHANGE_PARAM, and TIMEOUT resets >> since in those scenarios the MAC address and ip address mapping >> does not change. Originally all the resets except CHANGE_PARAM >> are processed by do_reset such that we need to find out >> failover and migration cases in do_reset and call notifier functions. >> We only need to notify peers in do_reset and do_hard_reset. >> We don't need notify peers in do_change_param_reset since it is >> a CHANGE_PARAM reset. In a nested reset case, it will finally >> call into do_hard_reset with reasons other than failvoer and >> migration. So, we don't need to check the reset reason in >> do_hard_reset and just call notifier functions anyway. > > You're completely undoing the commit you linked to: Testing is underway. We will clarify the description in the next version. Thank you for your review and feedback. Dany > > commit 61d3e1d9bc2a1910d773cbf4ed6f587a7a6166b5 > Author: Nathan Fontenot <nfont@linux.vnet.ibm.com> > Date: Mon Jun 12 20:47:45 2017 -0400 > > ibmvnic: Remove netdev notify for failover resets > > When handling a driver reset due to a failover of the backing > server on the vios, doing the netdev_notify_peers() can cause > network traffic to stall or halt. Remove the netdev notify call > for failover resets. > > Signed-off-by: Nathan Fontenot <nfont@linux.vnet.ibm.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c > b/drivers/net/ethernet/ibm/ibmvnic.c > index fd3ef3005fb0..59ea7a5ae776 100644 > --- a/drivers/net/ethernet/ibm/ibmvnic.c > +++ b/drivers/net/ethernet/ibm/ibmvnic.c > @@ -1364,7 +1364,9 @@ static int do_reset(struct ibmvnic_adapter > *adapter, > for (i = 0; i < adapter->req_rx_queues; i++) > napi_schedule(&adapter->napi[i]); > > - netdev_notify_peers(netdev); > + if (adapter->reset_reason != VNIC_RESET_FAILOVER) > + netdev_notify_peers(netdev); > + > return 0; > } > > But you don't seem to address why this change was unnecessary. > > AFAIK you're saying "we only need this event for FAILOVER and MOBILITY" > but the previous commit _excluded_ FAILOVER for some vague reason. > > If the previous commit was incorrect you need to explain that in the > commit message. > >> netdev_notify_peers calls below two functions with rtnl lock(). >> call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, dev); >> call_netdevice_notifiers(NETDEV_RESEND_IGMP, dev); >> When netdev_notify_peers was substituted in >> commit 986103e7920c ("net/ibmvnic: Fix RTNL deadlock during device >> reset"), >> call_netdevice_notifiers(NETDEV_RESEND_IGMP, dev) was missed. > > That should be a separate patch. > >> Fixes: 61d3e1d9bc2a ("ibmvnic: Remove netdev notify for failover >> resets") >> Fixes: 986103e7920c ("net/ibmvnic: Fix RTNL deadlock during device >> reset") > > Please don't line-wrap fixes tags. > >> Suggested-by: Brian King <brking@linux.vnet.ibm.com> >> Suggested-by: Pradeep Satyanarayana <pradeeps@linux.vnet.ibm.com> >> Signed-off-by: Dany Madden <drt@linux.ibm.com> >> Signed-off-by: Lijun Pan <ljp@linux.ibm.com> >> --- >> drivers/net/ethernet/ibm/ibmvnic.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c >> b/drivers/net/ethernet/ibm/ibmvnic.c >> index 1b702a43a5d0..718da39f5ae4 100644 >> --- a/drivers/net/ethernet/ibm/ibmvnic.c >> +++ b/drivers/net/ethernet/ibm/ibmvnic.c >> @@ -2067,8 +2067,11 @@ static int do_reset(struct ibmvnic_adapter >> *adapter, >> for (i = 0; i < adapter->req_rx_queues; i++) >> napi_schedule(&adapter->napi[i]); >> >> - if (adapter->reset_reason != VNIC_RESET_FAILOVER) >> + if (adapter->reset_reason == VNIC_RESET_FAILOVER || >> + adapter->reset_reason == VNIC_RESET_MOBILITY) { >> call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, netdev); >> + call_netdevice_notifiers(NETDEV_RESEND_IGMP, netdev); >> + } >> >> rc = 0; >> >> @@ -2138,6 +2141,9 @@ static int do_hard_reset(struct ibmvnic_adapter >> *adapter, >> if (rc) >> return IBMVNIC_OPEN_FAILED; >> >> + call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, netdev); >> + call_netdevice_notifiers(NETDEV_RESEND_IGMP, netdev); >> + >> return 0; >> } >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH net 2/2] ibmvnic: skip tx timeout reset while in resetting 2020-10-28 5:57 [PATCH net 0/2] ibmvnic: fixes in reset path Lijun Pan 2020-10-28 5:57 ` [PATCH net 1/2] ibmvnic: notify peers when failover and migration happen Lijun Pan @ 2020-10-28 5:57 ` Lijun Pan 1 sibling, 0 replies; 5+ messages in thread From: Lijun Pan @ 2020-10-28 5:57 UTC (permalink / raw) To: netdev; +Cc: Lijun Pan, Brian King Sometimes it takes longer than 5 seconds to complete failover, migration, and other resets. In stead of scheduling another timeout reset, we wait for the current one to complete. Suggested-by: Brian King <brking@linux.vnet.ibm.com> Signed-off-by: Lijun Pan <ljp@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index 718da39f5ae4..eb0eb1e8ac22 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -2336,6 +2336,12 @@ static void ibmvnic_tx_timeout(struct net_device *dev, unsigned int txqueue) { struct ibmvnic_adapter *adapter = netdev_priv(dev); + if (test_bit(0, &adapter->resetting)) { + netdev_err(adapter->netdev, + "adapter is resetting, skip timeout reset\n"); + return; + } + ibmvnic_reset(adapter, VNIC_RESET_TIMEOUT); } -- 2.23.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-05 4:54 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-28 5:57 [PATCH net 0/2] ibmvnic: fixes in reset path Lijun Pan 2020-10-28 5:57 ` [PATCH net 1/2] ibmvnic: notify peers when failover and migration happen Lijun Pan 2020-10-30 20:27 ` Jakub Kicinski 2020-11-05 4:54 ` drt 2020-10-28 5:57 ` [PATCH net 2/2] ibmvnic: skip tx timeout reset while in resetting Lijun Pan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).