| Message ID | 20210108071236.123769-3-sukadev@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ibmvnic: Use more consistent locking | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Thu, 7 Jan 2021 23:12:31 -0800 Sukadev Bhattiprolu wrote: > The reset functions need just the 'reset reason' parameter and not > the ibmvnic_rwi list element. Update the functions so we can simplify > the handling of the ->rwi_list in a follow-on patch. > > Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery") > No empty lines after Fixes tags, please. They should also not be wrapped. > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com> Are these patches for net or net-next? It looks like they are fixing races, but at the same time they are rather large. Can you please produce minimal fixes, e.g. patch 3 should just fix the existing leaks rather than refactor the code to not do allocations. 130+ LoC is a lot for a fix.
Jakub Kicinski [kuba@kernel.org] wrote: > On Thu, 7 Jan 2021 23:12:31 -0800 Sukadev Bhattiprolu wrote: > > The reset functions need just the 'reset reason' parameter and not > > the ibmvnic_rwi list element. Update the functions so we can simplify > > the handling of the ->rwi_list in a follow-on patch. > > > > Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery") > > > > No empty lines after Fixes tags, please. They should also not be > wrapped. Ah ok, will fix. > > > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > Are these patches for net or net-next? It looks like they are fixing > races, but at the same time they are rather large. Can you please > produce minimal fixes, e.g. patch 3 should just fix the existing leaks > rather than refactor the code to not do allocations. 130+ LoC is a lot > for a fix. This is a set of bug fixes, but yes a bit large. Should I submit to net-next instead? Thanks, Sukadev
On Sun, 10 Jan 2021 19:12:21 -0800 Sukadev Bhattiprolu wrote: > Jakub Kicinski [kuba@kernel.org] wrote: > > On Thu, 7 Jan 2021 23:12:31 -0800 Sukadev Bhattiprolu wrote: > > > The reset functions need just the 'reset reason' parameter and not > > > the ibmvnic_rwi list element. Update the functions so we can simplify > > > the handling of the ->rwi_list in a follow-on patch. > > > > > > Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery") > > > > > > > No empty lines after Fixes tags, please. They should also not be > > wrapped. > > Ah ok, will fix. > > > > > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > > > Are these patches for net or net-next? It looks like they are fixing > > races, but at the same time they are rather large. Can you please > > produce minimal fixes, e.g. patch 3 should just fix the existing leaks > > rather than refactor the code to not do allocations. 130+ LoC is a lot > > for a fix. > > This is a set of bug fixes, but yes a bit large. Should I submit to > net-next instead? I'd rather you tried to address the problems with minimal patches, then you can refactor the code in net-next.
Jakub Kicinski [kuba@kernel.org] wrote: > On Sun, 10 Jan 2021 19:12:21 -0800 Sukadev Bhattiprolu wrote: > > Jakub Kicinski [kuba@kernel.org] wrote: > > > On Thu, 7 Jan 2021 23:12:31 -0800 Sukadev Bhattiprolu wrote: > > > > The reset functions need just the 'reset reason' parameter and not > > > > the ibmvnic_rwi list element. Update the functions so we can simplify > > > > the handling of the ->rwi_list in a follow-on patch. > > > > > > > > Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery") > > > > > > > > > > No empty lines after Fixes tags, please. They should also not be > > > wrapped. > > > > Ah ok, will fix. > > > > > > > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com> > > > > > > Are these patches for net or net-next? It looks like they are fixing > > > races, but at the same time they are rather large. Can you please > > > produce minimal fixes, e.g. patch 3 should just fix the existing leaks > > > rather than refactor the code to not do allocations. 130+ LoC is a lot > > > for a fix. > > > > This is a set of bug fixes, but yes a bit large. Should I submit to > > net-next instead? > > I'd rather you tried to address the problems with minimal patches, then > you can refactor the code in net-next. I had thought about that but fixing the leaks and then throwing away the code did not seem very useful. The diff stat shows 78 insertions and 59 deletions. The bulk of the new code, about 70 lines including comments, is just the fairly straightforward helper functions: - get_pending_reset() - add_pending_reset() Fixing the leak in the existing code would not reduce the size of these helpers. We are now using a simpler approach with no allocations, so no leaks. Sukadev
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c index d548779561fd..cd8108dbddec 100644 --- a/drivers/net/ethernet/ibm/ibmvnic.c +++ b/drivers/net/ethernet/ibm/ibmvnic.c @@ -1929,17 +1929,17 @@ static int ibmvnic_set_mac(struct net_device *netdev, void *p) * events, or non-zero if we hit a fatal error and must halt. */ static int do_change_param_reset(struct ibmvnic_adapter *adapter, - struct ibmvnic_rwi *rwi, + enum ibmvnic_reset_reason reason, u32 reset_state) { struct net_device *netdev = adapter->netdev; int i, rc; netdev_dbg(adapter->netdev, "Change param resetting driver (%d)\n", - rwi->reset_reason); + reason); netif_carrier_off(netdev); - adapter->reset_reason = rwi->reset_reason; + adapter->reset_reason = reason; ibmvnic_cleanup(netdev); @@ -2015,7 +2015,7 @@ static int do_change_param_reset(struct ibmvnic_adapter *adapter, * non-zero if we hit a fatal error and must halt. */ static int do_reset(struct ibmvnic_adapter *adapter, - struct ibmvnic_rwi *rwi, u32 reset_state) + enum ibmvnic_reset_reason reason, u32 reset_state) { u64 old_num_rx_queues, old_num_tx_queues; u64 old_num_rx_slots, old_num_tx_slots; @@ -2025,7 +2025,7 @@ static int do_reset(struct ibmvnic_adapter *adapter, netdev_dbg(adapter->netdev, "[S:%d FOP:%d] Reset reason %d, reset_state %d\n", adapter->state, adapter->failover_pending, - rwi->reset_reason, reset_state); + reason, reset_state); rtnl_lock(); /* @@ -2033,11 +2033,11 @@ static int do_reset(struct ibmvnic_adapter *adapter, * This will ensure ibmvnic_open() has either completed or will * block until failover is complete. */ - if (rwi->reset_reason == VNIC_RESET_FAILOVER) + if (reason == VNIC_RESET_FAILOVER) adapter->failover_pending = false; netif_carrier_off(netdev); - adapter->reset_reason = rwi->reset_reason; + adapter->reset_reason = reason; old_num_rx_queues = adapter->req_rx_queues; old_num_tx_queues = adapter->req_tx_queues; @@ -2188,16 +2188,16 @@ static int do_reset(struct ibmvnic_adapter *adapter, } static int do_hard_reset(struct ibmvnic_adapter *adapter, - struct ibmvnic_rwi *rwi, u32 reset_state) + enum ibmvnic_reset_reason reason, u32 reset_state) { struct net_device *netdev = adapter->netdev; int rc; netdev_dbg(adapter->netdev, "Hard resetting driver (%d)\n", - rwi->reset_reason); + reason); netif_carrier_off(netdev); - adapter->reset_reason = rwi->reset_reason; + adapter->reset_reason = reason; ibmvnic_cleanup(netdev); release_resources(adapter); @@ -2278,6 +2278,7 @@ static struct ibmvnic_rwi *get_next_rwi(struct ibmvnic_adapter *adapter) static void __ibmvnic_reset(struct work_struct *work) { + enum ibmvnic_reset_reason reason; struct ibmvnic_rwi *rwi; struct ibmvnic_adapter *adapter; bool saved_state = false; @@ -2294,6 +2295,7 @@ static void __ibmvnic_reset(struct work_struct *work) } rwi = get_next_rwi(adapter); + reason = rwi->reset_reason; while (rwi) { spin_lock_irqsave(&adapter->state_lock, flags); @@ -2311,9 +2313,9 @@ static void __ibmvnic_reset(struct work_struct *work) } spin_unlock_irqrestore(&adapter->state_lock, flags); - if (rwi->reset_reason == VNIC_RESET_CHANGE_PARAM) { + if (reason == VNIC_RESET_CHANGE_PARAM) { /* CHANGE_PARAM requestor holds rtnl_lock */ - rc = do_change_param_reset(adapter, rwi, reset_state); + rc = do_change_param_reset(adapter, reason, reset_state); } else if (adapter->force_reset_recovery) { /* * Since we are doing a hard reset now, clear the @@ -2326,11 +2328,11 @@ static void __ibmvnic_reset(struct work_struct *work) if (adapter->wait_for_reset) { /* Previous was CHANGE_PARAM; caller locked */ adapter->force_reset_recovery = false; - rc = do_hard_reset(adapter, rwi, reset_state); + rc = do_hard_reset(adapter, reason, reset_state); } else { rtnl_lock(); adapter->force_reset_recovery = false; - rc = do_hard_reset(adapter, rwi, reset_state); + rc = do_hard_reset(adapter, reason, reset_state); rtnl_unlock(); } if (rc) { @@ -2341,9 +2343,9 @@ static void __ibmvnic_reset(struct work_struct *work) set_current_state(TASK_UNINTERRUPTIBLE); schedule_timeout(60 * HZ); } - } else if (!(rwi->reset_reason == VNIC_RESET_FATAL && + } else if (!(reason == VNIC_RESET_FATAL && adapter->from_passive_init)) { - rc = do_reset(adapter, rwi, reset_state); + rc = do_reset(adapter, reason, reset_state); } kfree(rwi); adapter->last_reset_time = jiffies; @@ -2352,9 +2354,10 @@ static void __ibmvnic_reset(struct work_struct *work) netdev_dbg(adapter->netdev, "Reset failed, rc=%d\n", rc); rwi = get_next_rwi(adapter); + reason = rwi->reset_reason; - if (rwi && (rwi->reset_reason == VNIC_RESET_FAILOVER || - rwi->reset_reason == VNIC_RESET_MOBILITY)) + if (reason && (reason == VNIC_RESET_FAILOVER || + reason == VNIC_RESET_MOBILITY)) adapter->force_reset_recovery = true; }
The reset functions need just the 'reset reason' parameter and not the ibmvnic_rwi list element. Update the functions so we can simplify the handling of the ->rwi_list in a follow-on patch. Fixes: 2770a7984db5 ("ibmvnic: Introduce hard reset recovery") Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com> --- drivers/net/ethernet/ibm/ibmvnic.c | 39 ++++++++++++++++-------------- 1 file changed, 21 insertions(+), 18 deletions(-)