* [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
* [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
* 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
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).