Re: [patch 11/35] net: ionic: Replace in_interrupt() usage.
On Mon, Sep 28 2020 at 12:51, Shannon Nelson wrote:
> On 9/28/20 10:24 AM, Shannon Nelson wrote:
>>> ionic_lif_addr() can be called from:
>>>
>>> 1) ->ndo_set_rx_mode() which is under netif_addr_lock_bh()) so it
>>> must not
>>> sleep.
>>>
>>> 2) Init and setup functions which are in fully preemptible task
>>> context.
>>>
>>> _ionic_lif_rx_mode() has only one call path with BH disabled.
>
> Now that I've had my coffee, let's look at this again - there are
> multiple paths that get us to _ionic_lif_rx_mode():
>
> .ndo_set_rx_mode
> ionic_set_rx_mode,
> _ionic_lif_rx_mode
>
> { ionic_open, ionic_lif_handle_fw_up, ionic_start_queues_reconfig }
> ionic_txrx_init
> ionic_set_rx_mode
> _ionic_lif_rx_mode
Hrm. Let me stare at it again...
Re: [patch 11/35] net: ionic: Replace in_interrupt() usage.
On 9/28/20 10:24 AM, Shannon Nelson wrote:
On 9/27/20 12:48 PM, Thomas Gleixner wrote:
From: Sebastian Andrzej Siewior
The in_interrupt() usage in this driver tries to figure out which
context
may sleep and which context may not sleep. in_interrupt() is not really
suitable as it misses both preemption disabled and interrupt disabled
invocations from task context.
Conditionals like that in driver code are frowned upon in general
because
invocations of functions from invalid contexts might not be detected
as the conditional papers over it.
ionic_lif_addr() can be called from:
1) ->ndo_set_rx_mode() which is under netif_addr_lock_bh()) so it
must not
sleep.
2) Init and setup functions which are in fully preemptible task
context.
_ionic_lif_rx_mode() has only one call path with BH disabled.
Now that I've had my coffee, let's look at this again - there are
multiple paths that get us to _ionic_lif_rx_mode():
.ndo_set_rx_mode
ionic_set_rx_mode,
_ionic_lif_rx_mode
{ ionic_open, ionic_lif_handle_fw_up, ionic_start_queues_reconfig }
ionic_txrx_init
ionic_set_rx_mode
_ionic_lif_rx_mode
We may not want to change this one.
sln
ionic_link_status_check_request() has two call paths:
1) NAPI which obviously cannot sleep
2) Setup which is again fully preemptible task context
Add 'can_sleep' arguments to the affected functions and let the callers
provide the context instead of letting the functions deduce it.
Signed-off-by: Sebastian Andrzej Siewior
Signed-off-by: Thomas Gleixner
Cc: Shannon Nelson
Cc: Pensando Drivers
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
Acked-by: Shannon Nelson
---
While reviewing the callpaths, a couple of things were observed which
could
be improved:
- ionic_lif_deferred_work() can iterate over the list. There is no need
to schedule the work item after each iteration
I think the original writer's intent was to avoid monopolizing the
work thread for very long on any one cycle, with the thought that we'd
be making more use of this than we currently are. I'll address this.
- ionic_link_status_check_request() could have ionic_deferred_work
within
ionic_lif(). This would avoid memory allocation from NAPI. More
important, once IONIC_LIF_F_LINK_CHECK_REQUESTED is set and that
alloc
fails, the link check never happens.
Thanks, I'll fix up that error condition.
- ionic_lif_handle_fw_down() sets IONIC_LIF_F_FW_RESET. Invokes then
ionic_lif_deinit() which only invokes cancel_work_sync() if
IONIC_LIF_F_FW_RESET is not set. I think the logic is wrong here as
the work must always be cancled. Also the list with ionic_deferred
work items needs a clean up.
I'll look at that, thanks.
sln
Re: [patch 11/35] net: ionic: Replace in_interrupt() usage.
On 9/27/20 12:48 PM, Thomas Gleixner wrote: From: Sebastian Andrzej Siewior The in_interrupt() usage in this driver tries to figure out which context may sleep and which context may not sleep. in_interrupt() is not really suitable as it misses both preemption disabled and interrupt disabled invocations from task context. Conditionals like that in driver code are frowned upon in general because invocations of functions from invalid contexts might not be detected as the conditional papers over it. ionic_lif_addr() can be called from: 1) ->ndo_set_rx_mode() which is under netif_addr_lock_bh()) so it must not sleep. 2) Init and setup functions which are in fully preemptible task context. _ionic_lif_rx_mode() has only one call path with BH disabled. ionic_link_status_check_request() has two call paths: 1) NAPI which obviously cannot sleep 2) Setup which is again fully preemptible task context Add 'can_sleep' arguments to the affected functions and let the callers provide the context instead of letting the functions deduce it. Signed-off-by: Sebastian Andrzej Siewior Signed-off-by: Thomas Gleixner Cc: Shannon Nelson Cc: Pensando Drivers Cc: "David S. Miller" Cc: Jakub Kicinski Cc: net...@vger.kernel.org Acked-by: Shannon Nelson --- While reviewing the callpaths, a couple of things were observed which could be improved: - ionic_lif_deferred_work() can iterate over the list. There is no need to schedule the work item after each iteration I think the original writer's intent was to avoid monopolizing the work thread for very long on any one cycle, with the thought that we'd be making more use of this than we currently are. I'll address this. - ionic_link_status_check_request() could have ionic_deferred_work within ionic_lif(). This would avoid memory allocation from NAPI. More important, once IONIC_LIF_F_LINK_CHECK_REQUESTED is set and that alloc fails, the link check never happens. Thanks, I'll fix up that error condition. - ionic_lif_handle_fw_down() sets IONIC_LIF_F_FW_RESET. Invokes then ionic_lif_deinit() which only invokes cancel_work_sync() if IONIC_LIF_F_FW_RESET is not set. I think the logic is wrong here as the work must always be cancled. Also the list with ionic_deferred work items needs a clean up. I'll look at that, thanks. sln
[patch 11/35] net: ionic: Replace in_interrupt() usage.
From: Sebastian Andrzej Siewior
The in_interrupt() usage in this driver tries to figure out which context
may sleep and which context may not sleep. in_interrupt() is not really
suitable as it misses both preemption disabled and interrupt disabled
invocations from task context.
Conditionals like that in driver code are frowned upon in general because
invocations of functions from invalid contexts might not be detected
as the conditional papers over it.
ionic_lif_addr() can be called from:
1) ->ndo_set_rx_mode() which is under netif_addr_lock_bh()) so it must not
sleep.
2) Init and setup functions which are in fully preemptible task context.
_ionic_lif_rx_mode() has only one call path with BH disabled.
ionic_link_status_check_request() has two call paths:
1) NAPI which obviously cannot sleep
2) Setup which is again fully preemptible task context
Add 'can_sleep' arguments to the affected functions and let the callers
provide the context instead of letting the functions deduce it.
Signed-off-by: Sebastian Andrzej Siewior
Signed-off-by: Thomas Gleixner
Cc: Shannon Nelson
Cc: Pensando Drivers
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: net...@vger.kernel.org
---
While reviewing the callpaths, a couple of things were observed which could
be improved:
- ionic_lif_deferred_work() can iterate over the list. There is no need
to schedule the work item after each iteration
- ionic_link_status_check_request() could have ionic_deferred_work within
ionic_lif(). This would avoid memory allocation from NAPI. More
important, once IONIC_LIF_F_LINK_CHECK_REQUESTED is set and that alloc
fails, the link check never happens.
- ionic_lif_handle_fw_down() sets IONIC_LIF_F_FW_RESET. Invokes then
ionic_lif_deinit() which only invokes cancel_work_sync() if
IONIC_LIF_F_FW_RESET is not set. I think the logic is wrong here as
the work must always be cancled. Also the list with ionic_deferred
work items needs a clean up.
---
drivers/net/ethernet/pensando/ionic/ionic_dev.c |2 -
drivers/net/ethernet/pensando/ionic/ionic_lif.c | 43 +++-
drivers/net/ethernet/pensando/ionic/ionic_lif.h |2 -
3 files changed, 22 insertions(+), 25 deletions(-)
--- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
@@ -22,7 +22,7 @@ static void ionic_watchdog_cb(struct tim
hb = ionic_heartbeat_check(ionic);
if (hb >= 0 && ionic->master_lif)
- ionic_link_status_check_request(ionic->master_lif);
+ ionic_link_status_check_request(ionic->master_lif, false);
}
void ionic_init_devinfo(struct ionic *ionic)
--- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
+++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
@@ -135,7 +135,7 @@ static void ionic_link_status_check(stru
clear_bit(IONIC_LIF_F_LINK_CHECK_REQUESTED, lif->state);
}
-void ionic_link_status_check_request(struct ionic_lif *lif)
+void ionic_link_status_check_request(struct ionic_lif *lif, bool can_sleep)
{
struct ionic_deferred_work *work;
@@ -143,7 +143,7 @@ void ionic_link_status_check_request(str
if (test_and_set_bit(IONIC_LIF_F_LINK_CHECK_REQUESTED, lif->state))
return;
- if (in_interrupt()) {
+ if (!can_sleep) {
work = kzalloc(sizeof(*work), GFP_ATOMIC);
if (!work)
return;
@@ -751,7 +751,7 @@ static bool ionic_notifyq_service(struct
switch (le16_to_cpu(comp->event.ecode)) {
case IONIC_EVENT_LINK_CHANGE:
- ionic_link_status_check_request(lif);
+ ionic_link_status_check_request(lif, false);
break;
case IONIC_EVENT_RESET:
work = kzalloc(sizeof(*work), GFP_ATOMIC);
@@ -928,7 +928,8 @@ static int ionic_lif_addr_del(struct ion
return 0;
}
-static int ionic_lif_addr(struct ionic_lif *lif, const u8 *addr, bool add)
+static int ionic_lif_addr(struct ionic_lif *lif, const u8 *addr, bool add,
+ bool can_sleep)
{
struct ionic *ionic = lif->ionic;
struct ionic_deferred_work *work;
@@ -957,7 +958,7 @@ static int ionic_lif_addr(struct ionic_l
lif->nucast--;
}
- if (in_interrupt()) {
+ if (!can_sleep) {
work = kzalloc(sizeof(*work), GFP_ATOMIC);
if (!work) {
netdev_err(lif->netdev, "%s OOM\n", __func__);
@@ -983,12 +984,12 @@ static int ionic_lif_addr(struct ionic_l
static int ionic_addr_add(struct net_device *netdev, const u8 *addr)
{
- return ionic_lif_addr(netdev_priv(netdev), addr, true);
+ return ionic_lif_addr(netdev_priv(netdev), addr, true, false);
}
static int ionic_addr_del(struct net_device *netdev, const u8 *addr)
{
- return ionic_lif_addr(netdev_priv(netdev), addr, false);
+ return ionic_lif_addr(netdev_priv(netdev), addr, false, false);
}
