netdev - Re: [RFC rdma-next] RDMA/core: Add attribute WQ_MEM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0Uems7Y0hhFmXYcE0Pf2-ZNih=rm6DDALXdwib7de5wqhA@mail.gmail.com>
Date:   Thu, 20 Feb 2020 09:46:02 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Yunsheng Lin <linyunsheng@...wei.com>
Cc:     Leon Romanovsky <leon@...nel.org>, Jason Gunthorpe <jgg@...pe.ca>,
        Lang Cheng <chenglang@...wei.com>,
        Doug Ledford <dledford@...hat.com>,
        David Miller <davem@...emloft.net>,
        Salil Mehta <salil.mehta@...wei.com>, yisen.zhuang@...wei.com,
        LinuxArm <linuxarm@...wei.com>, Netdev <netdev@...r.kernel.org>,
        linux-rdma@...r.kernel.org, Saeed Mahameed <saeedm@...lanox.com>,
        bhaktipriya96@...il.com, Tejun Heo <tj@...nel.org>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Subject: Re: [RFC rdma-next] RDMA/core: Add attribute WQ_MEM_RECLAIM to
 workqueue "infiniband"

On Tue, Feb 18, 2020 at 11:42 PM Yunsheng Lin <linyunsheng@...wei.com> wrote:

> Ok, I may be wrong about the above usecase.
> but the below commit explicitly state that network devices may be used in
> memory reclaim path.
>
> 0a38c17a21a0 ("fm10k: Remove create_workqueue"):
>
> fm10k: Remove create_workqueue
>
> alloc_workqueue replaces deprecated create_workqueue().
>
> A dedicated workqueue has been used since the workitem (viz
> fm10k_service_task, which manages and runs other subtasks) is involved in
> normal device operation and requires forward progress under memory
> pressure.
>
> create_workqueue has been replaced with alloc_workqueue with max_active
> as 0 since there is no need for throttling the number of active work
> items.
>
> Since network devices may be used in memory reclaim path,
> WQ_MEM_RECLAIM has been set to guarantee forward progress.
>
> flush_workqueue is unnecessary since destroy_workqueue() itself calls
> drain_workqueue() which flushes repeatedly till the workqueue
> becomes empty. Hence the call to flush_workqueue() has been dropped.
>
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriya96@...il.com>
> Acked-by: Tejun Heo <tj@...nel.org>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
>
> So:
> 1. Maybe the above commit log is misleading, and network device driver's
>    wq does not need the WQ_MEM_RECLAIM flag, then maybe document what can
>    not be done in the work queued to wq marked with WQ_MEM_RECLAIM, and
>    remove the WQ_MEM_RECLAIM flag for the wq of network device driver.

I am not sure why they added WQ_MEM_RECLAIM to the fm10k service task
thread. It has nothing to do with memory reclaim. If a memory
allocation fails then it will just run to the end and bring the
interface down. The service task is related to dealing with various
one-off events like link up and link down, sorting out hangs, and
updating statistics. The only memory allocation it is involved with is
if it has to reset the interface in which case I believe there may
even be a few GFP_KERNEL calls in there since it is freeing and
reallocating several port related structures.

> 2. If the network device driver's wq does need the WQ_MEM_RECLAIM flag, then
>    hns3 may have tow problems here: WQ_MEM_RECLAIM wq flushing !WQ_MEM_RECLAIM
>    wq problem and GFP_KERNEL allocations in the work queued to WQ_MEM_RECLAIM wq.

It seems like you could solve this by going the other way and dropping
the WQ_MEM_RECLAIM from the original patch you mentioned in your fixes
tag. I'm not seeing anything in hclge_periodic_service_task that
justifies the use of the WQ_MEM_RECLAIM flag. It claims to be involved
with memory reclaim but I don't see where that could be the case.

- Alex