by Jens Axboe

[permalink] [raw]

Subject: Re: [PATCH AUTOSEL 5.15 20/29] block: reduce kblockd_mod_delayed_work_on() CPU consumption

On 12/21/21 10:58 AM, Sasha Levin wrote:
> On Tue, Dec 21, 2021 at 08:36:33AM -0700, Jens Axboe wrote:
>> On 12/21/21 8:35 AM, Michael Kelley (LINUX) wrote:
>>> From: Sasha Levin <[email protected]> Sent: Monday, December 20, 2021 5:58 PM
>>>>
>>>> From: Jens Axboe <[email protected]>
>>>>
>>>> [ Upstream commit cb2ac2912a9ca7d3d26291c511939a41361d2d83 ]
>>>>
>>>> Dexuan reports that he's seeing spikes of very heavy CPU utilization when
>>>> running 24 disks and using the 'none' scheduler. This happens off the
>>>> sched restart path, because SCSI requires the queue to be restarted async,
>>>> and hence we're hammering on mod_delayed_work_on() to ensure that the work
>>>> item gets run appropriately.
>>>>
>>>> Avoid hammering on the timer and just use queue_work_on() if no delay
>>>> has been specified.
>>>>
>>>> Reported-and-tested-by: Dexuan Cui <[email protected]>
>>>> Link: https://lore.kernel.org/linux-block/BYAPR21MB1270C598ED214C0490F47400BF719@BYAPR21MB1270.namprd21.prod.outlook.com/
>>>> Reviewed-by: Ming Lei <[email protected]>
>>>> Signed-off-by: Jens Axboe <[email protected]>
>>>> Signed-off-by: Sasha Levin <[email protected]>
>>>> ---
>>>> block/blk-core.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index c2d912d0c976c..a728434fcff87 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -1625,6 +1625,8 @@ EXPORT_SYMBOL(kblockd_schedule_work);
>>>> int kblockd_mod_delayed_work_on(int cpu, struct delayed_work *dwork,
>>>> unsigned long delay)
>>>> {
>>>> + if (!delay)
>>>> + return queue_work_on(cpu, kblockd_workqueue, &dwork->work);
>>>> return mod_delayed_work_on(cpu, kblockd_workqueue, dwork, delay);
>>>> }
>>>> EXPORT_SYMBOL(kblockd_mod_delayed_work_on);
>>>> --
>>>> 2.34.1
>>>
>>> Sasha -- there are reports of this patch causing performance problems.
>>> See
>>> https://lore.kernel.org/lkml/1639853092.524jxfaem2.none@localhost/. I
>>> would suggest *not* backporting it to any of the stable branches until
>>> the issues are fully sorted out.
>>
>> Both this and the revert were backported. Which arguably doesn't make a
>> lot of sense, but at least it's consistent and won't cause any issues...
>
> The logic behind it is that it makes it easy for both us as well as
> everyone else to annotate why a certain patch might be "missing" from
> the trees - in this case because it was reverted.
>
> It looks dumb now, but it saves a lot of time as well as mitigates the
> risk of it being picked up again at some point in the future.

It's fine with me, when I saw the first patch yesterday I did get
worried, but then I saw the revert was picked too. As I said, as long
as the end result is sane, then there's no harm in doing it this way.

--
Jens Axboe

2021-12-25 18:23:59

by Sedat Dilek

[permalink] [raw]

Subject: Re: [PATCH AUTOSEL 5.15 07/29] nl80211: reset regdom when reloading regdb

On Tue, Dec 21, 2021 at 2:58 AM Sasha Levin <[email protected]> wrote:
>
> From: Finn Behrens <[email protected]>
>
> [ Upstream commit 1eda919126b420fee6b8d546f7f728fbbd4b8f11 ]
>
> Reload the regdom when the regulatory db is reloaded.
> Otherwise, the user had to change the regulatoy domain
> to a different one and then reset it to the correct
> one to have a new regulatory db take effect after a
> reload.
>
> Signed-off-by: Finn Behrens <[email protected]>
> Link: https://lore.kernel.org/r/YaIIZfxHgqc/[email protected]
> [edit commit message]
> Signed-off-by: Johannes Berg <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>

This requires [1] to fix this warning:

net/wireless/reg.c:1137:23: warning: implicit conversion from
enumeration type 'enum nl80211_user_reg_hint_type' to different
enumeration type 'enum nl80211_reg_
initiator' [-Wenum-conversion]

[PATCH] nl80211: remove reload flag from regulatory_request

- Sedat -

[1] https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git/patch/?id=37d33114240ede043c42463a6347f68ed72d6904

> ---
> include/net/regulatory.h | 1 +
> net/wireless/reg.c | 27 +++++++++++++++++++++++++--
> 2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/regulatory.h b/include/net/regulatory.h
> index 47f06f6f5a67c..0cf9335431e07 100644
> --- a/include/net/regulatory.h
> +++ b/include/net/regulatory.h
> @@ -83,6 +83,7 @@ struct regulatory_request {
> enum nl80211_dfs_regions dfs_region;
> bool intersect;
> bool processed;
> + bool reload;
> enum environment_cap country_ie_env;
> struct list_head list;
> };
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index df87c7f3a0492..61f1bf1bc4a73 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -133,6 +133,7 @@ static u32 reg_is_indoor_portid;
>
> static void restore_regulatory_settings(bool reset_user, bool cached);
> static void print_regdomain(const struct ieee80211_regdomain *rd);
> +static void reg_process_hint(struct regulatory_request *reg_request);
>
> static const struct ieee80211_regdomain *get_cfg80211_regdom(void)
> {
> @@ -1098,6 +1099,8 @@ int reg_reload_regdb(void)
> const struct firmware *fw;
> void *db;
> int err;
> + const struct ieee80211_regdomain *current_regdomain;
> + struct regulatory_request *request;
>
> err = request_firmware(&fw, "regulatory.db", &reg_pdev->dev);
> if (err)
> @@ -1118,8 +1121,27 @@ int reg_reload_regdb(void)
> if (!IS_ERR_OR_NULL(regdb))
> kfree(regdb);
> regdb = db;
> - rtnl_unlock();
>
> + /* reset regulatory domain */
> + current_regdomain = get_cfg80211_regdom();
> +
> + request = kzalloc(sizeof(*request), GFP_KERNEL);
> + if (!request) {
> + err = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + request->wiphy_idx = WIPHY_IDX_INVALID;
> + request->alpha2[0] = current_regdomain->alpha2[0];
> + request->alpha2[1] = current_regdomain->alpha2[1];
> + request->initiator = NL80211_USER_REG_HINT_USER;
> + request->user_reg_hint_type = NL80211_USER_REG_HINT_USER;
> + request->reload = true;
> +
> + reg_process_hint(request);
> +
> +out_unlock:
> + rtnl_unlock();
> out:
> release_firmware(fw);
> return err;
> @@ -2690,7 +2712,8 @@ reg_process_hint_user(struct regulatory_request *user_request)
>
> treatment = __reg_process_hint_user(user_request);
> if (treatment == REG_REQ_IGNORE ||
> - treatment == REG_REQ_ALREADY_SET)
> + (treatment == REG_REQ_ALREADY_SET &&
> + !user_request->reload))
> return REG_REQ_IGNORE;
>
> user_request->intersect = treatment == REG_REQ_INTERSECT;
> --
> 2.34.1
>