* Re: [RFC rdma-next] RDMA/core: Add attribute WQ_MEM_RECLAIM to workqueue "infiniband"
2020-02-19 7:40 ` Yunsheng Lin
@ 2020-02-19 11:07 ` Leon Romanovsky
2020-02-20 1:16 ` Yunsheng Lin
2020-02-19 13:04 ` Jason Gunthorpe
2020-02-20 17:46 ` Alexander Duyck
2 siblings, 1 reply; 11+ messages in thread
From: Leon Romanovsky @ 2020-02-19 11:07 UTC (permalink / raw)
To: Yunsheng Lin
Cc: Jason Gunthorpe, Lang Cheng, dledford, davem, salil.mehta,
yisen.zhuang, linuxarm, netdev, linux-rdma, Saeed Mahameed,
bhaktipriya96, tj, Jeff Kirsher
On Wed, Feb 19, 2020 at 03:40:59PM +0800, Yunsheng Lin wrote:
> +cc Bhaktipriya, Tejun and Jeff
>
> On 2020/2/19 14:45, Leon Romanovsky wrote:
> > On Wed, Feb 19, 2020 at 09:13:23AM +0800, Yunsheng Lin wrote:
> >> On 2020/2/18 23:31, Jason Gunthorpe wrote:
> >>> On Tue, Feb 18, 2020 at 11:35:35AM +0800, Lang Cheng wrote:
> >>>> The hns3 driver sets "hclge_service_task" workqueue with
> >>>> WQ_MEM_RECLAIM flag in order to guarantee forward progress
> >>>> under memory pressure.
> >>>
> >>> Don't do that. WQ_MEM_RECLAIM is only to be used by things interlinked
> >>> with reclaimed processing.
> >>>
> >>> Work on queues marked with WQ_MEM_RECLAIM can't use GFP_KERNEL
> >>> allocations, can't do certain kinds of sleeps, can't hold certain
> >>> kinds of locks, etc.
>
> By the way, what kind of sleeps and locks can not be done in the work
> queued to wq marked with WQ_MEM_RECLAIM?
I didn't see this knowledge documented, but I would assume that
everything that can block memory reclaim progress should not be
in such workqueue.
>
> >>
> >> From mlx5 driver, it seems that there is GFP_KERNEL allocations
> >> on wq marked with WQ_MEM_RECLAIM too:
> >>
> >> mlx5e_tx_timeout_work() -> mlx5e_safe_reopen_channels() ->
> >> mlx5e_safe_switch_channels() -> mlx5e_open_channels()
> >>
> >> kcalloc() is called with GFP_KERNEL in mlx5e_open_channels(),
> >> and mlx5e_tx_timeout_work() is queued with priv->wq, which is
> >> allocated with WQ_MEM_RECLAIM flags. see:
> >>
> >> mlx5e_netdev_init() -> create_singlethread_workqueue()
> >
> > There are two reasons for that, first mlx5 driver was written far before
> > WQ_MEM_RECLAIM usage was clarified, second mlx5 has bugs.
> >
> >>
> >>
> >> From the comment in kernel/workqueue.c, the work queued with
> >> wq with WQ_MEM_RECLAIM flag set seems to be executed without
> >> blocking under some rare case. I still not quite understand
> >> the comment, and I can not find any doc that point out the
> >> GFP_KERNEL allocations can not be done in wq with WQ_MEM_RECLAIM
> >> yet. Is there any doc that mentions that GFP_KERNEL allocations
> >> can not be done in wq with WQ_MEM_RECLAIM?
> >
> > It is whole purpose of WQ_MEM_RECLAIM flag - allow progress in case of
> > memory pressure. Allocation memory while we are under memory pressure
> > is an invitation for a disaster.
>
> Ok, make sense.
>
> >
> >>
> >>
> >> /**
> >> * rescuer_thread - the rescuer thread function
> >> * @__rescuer: self
> >> *
> >> * Workqueue rescuer thread function. There's one rescuer for each
> >> * workqueue which has WQ_MEM_RECLAIM set.
> >> *
> >> * Regular work processing on a pool may block trying to create a new
> >> * worker which uses GFP_KERNEL allocation which has slight chance of
> >> * developing into deadlock if some works currently on the same queue
> >> * need to be processed to satisfy the GFP_KERNEL allocation. This is
> >> * the problem rescuer solves.
> >> *
> >> * When such condition is possible, the pool summons rescuers of all
> >> * workqueues which have works queued on the pool and let them process
> >> * those works so that forward progress can be guaranteed.
> >> *
> >> * This should happen rarely.
> >> *
> >> * Return: 0
> >> */
> >>
> >>
> >> The below is the reason we add the sets "hclge_service_task" workqueue
> >> with WQ_MEM_RECLAIM through analysing why other ethernet drivers has
> >> allocated wq with WQ_MEM_RECLAIM flag, I may be wrong about that:
> >
> > Many drivers are developed using copy/paste technique, so it is wrong
> > to assume that "other ethernet drivers" did the right thing.
> >
> >>
> >> hns3 ethernet driver may be used as the low level transport of a
> >> network file system, memory reclaim data path may depend on the
> >> worker in hns3 driver to bring back the ethernet link so that it flush
> >> the some cache to network based disk.
> >
> > Unlikely that this "network file system" dependency on ethernet link is correct.
>
> 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@gmail.com>
> Acked-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.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 wouldn't truly count on what is written in commit messages of patch
series which globally replaced create_workqueue() interface.
>
>
> 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.
You are proposing to put WQ_MEM_RECLAIM in infiniband queue and not to
special queue inside of the driver.
>
> >
> > Thanks
> >
> >>
> >>>
> >>> Jason
> >>>
> >>>
> >>
> >
> > .
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC rdma-next] RDMA/core: Add attribute WQ_MEM_RECLAIM to workqueue "infiniband"
2020-02-19 11:07 ` Leon Romanovsky
@ 2020-02-20 1:16 ` Yunsheng Lin
0 siblings, 0 replies; 11+ messages in thread
From: Yunsheng Lin @ 2020-02-20 1:16 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jason Gunthorpe, Lang Cheng, dledford, davem, salil.mehta,
yisen.zhuang, linuxarm, netdev, linux-rdma, Saeed Mahameed,
bhaktipriya96, tj, Jeff Kirsher
On 2020/2/19 19:07, Leon Romanovsky wrote:
> On Wed, Feb 19, 2020 at 03:40:59PM +0800, Yunsheng Lin wrote:
>> +cc Bhaktipriya, Tejun and Jeff
>>
>> On 2020/2/19 14:45, Leon Romanovsky wrote:
>>> On Wed, Feb 19, 2020 at 09:13:23AM +0800, Yunsheng Lin wrote:
>>>> On 2020/2/18 23:31, Jason Gunthorpe wrote:
>>>>> On Tue, Feb 18, 2020 at 11:35:35AM +0800, Lang Cheng wrote:
>>>>>> The hns3 driver sets "hclge_service_task" workqueue with
>>>>>> WQ_MEM_RECLAIM flag in order to guarantee forward progress
>>>>>> under memory pressure.
>>>>>
>>>>> Don't do that. WQ_MEM_RECLAIM is only to be used by things interlinked
>>>>> with reclaimed processing.
>>>>>
>>>>> Work on queues marked with WQ_MEM_RECLAIM can't use GFP_KERNEL
>>>>> allocations, can't do certain kinds of sleeps, can't hold certain
>>>>> kinds of locks, etc.
>>
>> By the way, what kind of sleeps and locks can not be done in the work
>> queued to wq marked with WQ_MEM_RECLAIM?
>
> I didn't see this knowledge documented, but I would assume that
> everything that can block memory reclaim progress should not be
> in such workqueue.
>
>>
>>>>
>>>> From mlx5 driver, it seems that there is GFP_KERNEL allocations
>>>> on wq marked with WQ_MEM_RECLAIM too:
>>>>
>>>> mlx5e_tx_timeout_work() -> mlx5e_safe_reopen_channels() ->
>>>> mlx5e_safe_switch_channels() -> mlx5e_open_channels()
>>>>
>>>> kcalloc() is called with GFP_KERNEL in mlx5e_open_channels(),
>>>> and mlx5e_tx_timeout_work() is queued with priv->wq, which is
>>>> allocated with WQ_MEM_RECLAIM flags. see:
>>>>
>>>> mlx5e_netdev_init() -> create_singlethread_workqueue()
>>>
>>> There are two reasons for that, first mlx5 driver was written far before
>>> WQ_MEM_RECLAIM usage was clarified, second mlx5 has bugs.
>>>
>>>>
>>>>
>>>> From the comment in kernel/workqueue.c, the work queued with
>>>> wq with WQ_MEM_RECLAIM flag set seems to be executed without
>>>> blocking under some rare case. I still not quite understand
>>>> the comment, and I can not find any doc that point out the
>>>> GFP_KERNEL allocations can not be done in wq with WQ_MEM_RECLAIM
>>>> yet. Is there any doc that mentions that GFP_KERNEL allocations
>>>> can not be done in wq with WQ_MEM_RECLAIM?
>>>
>>> It is whole purpose of WQ_MEM_RECLAIM flag - allow progress in case of
>>> memory pressure. Allocation memory while we are under memory pressure
>>> is an invitation for a disaster.
>>
>> Ok, make sense.
>>
>>>
>>>>
>>>>
>>>> /**
>>>> * rescuer_thread - the rescuer thread function
>>>> * @__rescuer: self
>>>> *
>>>> * Workqueue rescuer thread function. There's one rescuer for each
>>>> * workqueue which has WQ_MEM_RECLAIM set.
>>>> *
>>>> * Regular work processing on a pool may block trying to create a new
>>>> * worker which uses GFP_KERNEL allocation which has slight chance of
>>>> * developing into deadlock if some works currently on the same queue
>>>> * need to be processed to satisfy the GFP_KERNEL allocation. This is
>>>> * the problem rescuer solves.
>>>> *
>>>> * When such condition is possible, the pool summons rescuers of all
>>>> * workqueues which have works queued on the pool and let them process
>>>> * those works so that forward progress can be guaranteed.
>>>> *
>>>> * This should happen rarely.
>>>> *
>>>> * Return: 0
>>>> */
>>>>
>>>>
>>>> The below is the reason we add the sets "hclge_service_task" workqueue
>>>> with WQ_MEM_RECLAIM through analysing why other ethernet drivers has
>>>> allocated wq with WQ_MEM_RECLAIM flag, I may be wrong about that:
>>>
>>> Many drivers are developed using copy/paste technique, so it is wrong
>>> to assume that "other ethernet drivers" did the right thing.
>>>
>>>>
>>>> hns3 ethernet driver may be used as the low level transport of a
>>>> network file system, memory reclaim data path may depend on the
>>>> worker in hns3 driver to bring back the ethernet link so that it flush
>>>> the some cache to network based disk.
>>>
>>> Unlikely that this "network file system" dependency on ethernet link is correct.
>>
>> 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@gmail.com>
>> Acked-by: Tejun Heo <tj@kernel.org>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.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 wouldn't truly count on what is written in commit messages of patch
> series which globally replaced create_workqueue() interface.
>
>>
>>
>> 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.
>
> You are proposing to put WQ_MEM_RECLAIM in infiniband queue and not to
> special queue inside of the driver.
In the commit log, we thought of three ways to avoid the warning.
which is:
1. Allocate the "hclge_service_task" workqueue without
WQ_MEM_RECLAIM flag, which may cause deadlock problem
when hns3 driver is used as the low level transport of
a network file system
2. Do not unregister ib_device during reset process, maybe
only disable accessing to the ib_device using disable_device()
as rdma_dev_change_netns() does.
3. Allocate the "infiniband" workqueue with WQ_MEM_RECLAIM flag.
Actually I prefer the second one, which need IB stack to provide
interface to disable and enable ib_device in the roce device driver
when there is hardware reset happening.
Is the above interface reasonable?
>
>>
>>>
>>> Thanks
>>>
>>>>
>>>>>
>>>>> Jason
>>>>>
>>>>>
>>>>
>>>
>>> .
>>>
>>
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC rdma-next] RDMA/core: Add attribute WQ_MEM_RECLAIM to workqueue "infiniband"
2020-02-19 7:40 ` Yunsheng Lin
2020-02-19 11:07 ` Leon Romanovsky
@ 2020-02-19 13:04 ` Jason Gunthorpe
2020-02-20 1:06 ` Yunsheng Lin
2020-02-20 17:46 ` Alexander Duyck
2 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2020-02-19 13:04 UTC (permalink / raw)
To: Yunsheng Lin
Cc: Leon Romanovsky, Lang Cheng, dledford, davem, salil.mehta,
yisen.zhuang, linuxarm, netdev, linux-rdma, Saeed Mahameed,
bhaktipriya96, tj, Jeff Kirsher
On Wed, Feb 19, 2020 at 03:40:59PM +0800, Yunsheng Lin wrote:
> +cc Bhaktipriya, Tejun and Jeff
>
> On 2020/2/19 14:45, Leon Romanovsky wrote:
> > On Wed, Feb 19, 2020 at 09:13:23AM +0800, Yunsheng Lin wrote:
> >> On 2020/2/18 23:31, Jason Gunthorpe wrote:
> >>> On Tue, Feb 18, 2020 at 11:35:35AM +0800, Lang Cheng wrote:
> >>>> The hns3 driver sets "hclge_service_task" workqueue with
> >>>> WQ_MEM_RECLAIM flag in order to guarantee forward progress
> >>>> under memory pressure.
> >>>
> >>> Don't do that. WQ_MEM_RECLAIM is only to be used by things interlinked
> >>> with reclaimed processing.
> >>>
> >>> Work on queues marked with WQ_MEM_RECLAIM can't use GFP_KERNEL
> >>> allocations, can't do certain kinds of sleeps, can't hold certain
> >>> kinds of locks, etc.
>
> By the way, what kind of sleeps and locks can not be done in the work
> queued to wq marked with WQ_MEM_RECLAIM?
Anything that recurses back into a blocking allocation function.
If we are freeing memory because an allocation failed (eg GFP_KERNEL)
then we cannot go back into a blockable allocation while trying to
progress the first failing allocation. That is a deadlock.
So a WQ cannot hold any locks that enclose GFP_KERNEL in any other
threads.
Unfortunately we don't have a lockdep test for this by default.
> >> hns3 ethernet driver may be used as the low level transport of a
> >> network file system, memory reclaim data path may depend on the
> >> worker in hns3 driver to bring back the ethernet link so that it flush
> >> the some cache to network based disk.
> >
> > Unlikely that this "network file system" dependency on ethernet link is correct.
>
> 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.
I don't really know how this works when the networking stacks
intersect with the block stack.
Forward progress on something like a NVMeOF requires a lot of stuff to
be working, and presumably under reclaim.
But, we can't make everything WQ_MEM_RECLAIM safe, because we could
never do a GFP_KERNEL allocation..
I have never seen specific guidance what to do here, I assume it is
broken.
Jason
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC rdma-next] RDMA/core: Add attribute WQ_MEM_RECLAIM to workqueue "infiniband"
2020-02-19 13:04 ` Jason Gunthorpe
@ 2020-02-20 1:06 ` Yunsheng Lin
0 siblings, 0 replies; 11+ messages in thread
From: Yunsheng Lin @ 2020-02-20 1:06 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Lang Cheng, dledford, davem, salil.mehta,
yisen.zhuang, linuxarm, netdev, linux-rdma, Saeed Mahameed,
bhaktipriya96, tj, Jeff Kirsher
On 2020/2/19 21:04, Jason Gunthorpe wrote:
> On Wed, Feb 19, 2020 at 03:40:59PM +0800, Yunsheng Lin wrote:
>> +cc Bhaktipriya, Tejun and Jeff
>>
>> On 2020/2/19 14:45, Leon Romanovsky wrote:
>>> On Wed, Feb 19, 2020 at 09:13:23AM +0800, Yunsheng Lin wrote:
>>>> On 2020/2/18 23:31, Jason Gunthorpe wrote:
>>>>> On Tue, Feb 18, 2020 at 11:35:35AM +0800, Lang Cheng wrote:
>>>>>> The hns3 driver sets "hclge_service_task" workqueue with WQ_MEM_RECLAIM flag in order to guarantee forward progress under memory pressure.
>>>>>
>>>>> Don't do that. WQ_MEM_RECLAIM is only to be used by things interlinked with reclaimed processing.
>>>>>
>>>>> Work on queues marked with WQ_MEM_RECLAIM can't use GFP_KERNEL allocations, can't do certain kinds of sleeps, can't hold certain kinds of locks, etc.
>>
>> By the way, what kind of sleeps and locks can not be done in the work queued to wq marked with WQ_MEM_RECLAIM?
>
> Anything that recurses back into a blocking allocation function.
>
> If we are freeing memory because an allocation failed (eg GFP_KERNEL) then we cannot go back into a blockable allocation while trying to progress the first failing allocation. That is a deadlock.
>
> So a WQ cannot hold any locks that enclose GFP_KERNEL in any other threads.
>
> Unfortunately we don't have a lockdep test for this by default.
>
>>>> hns3 ethernet driver may be used as the low level transport of a network file system, memory reclaim data path may depend on the worker in hns3 driver to bring back the ethernet link so that it flush the some cache to network based disk.
>>>
>>> Unlikely that this "network file system" dependency on ethernet link is correct.
>>
>> 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.
>
> I don't really know how this works when the networking stacks intersect with the block stack.
>
> Forward progress on something like a NVMeOF requires a lot of stuff to be working, and presumably under reclaim.
>
> But, we can't make everything WQ_MEM_RECLAIM safe, because we could never do a GFP_KERNEL allocation..
>
> I have never seen specific guidance what to do here, I assume it is broken.
I assume the forward progress guarantee of network device's wq is broken
too, at least for the case of hns3, fm10k and mlx5 driver.
So I suggest to remove WQ_MEM_RECLAIM for hns3' wq for now.
For now there are two known problems which defeat the forward progress
guarantee of WQ_MEM_RECLAIM when adding the WQ_MEM_RECLAIM for hns3'
wq:
1. GFP_KERNEL allocation in the hns3' work queued to WQ_MEM_RECLAIM wq.
2. hns3' WQ_MEM_RECLAIM wq flushing infiniband' !WQ_MEM_RECLAIM wq.
We can add the WQ_MEM_RECLAIM back when we have fixed the above problem and
find more specific guidance about handling progress guarantee in network
device's wq .
Thanks for the feedback.
>
> Jason
>
> .
>
f
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC rdma-next] RDMA/core: Add attribute WQ_MEM_RECLAIM to workqueue "infiniband"
2020-02-19 7:40 ` Yunsheng Lin
2020-02-19 11:07 ` Leon Romanovsky
2020-02-19 13:04 ` Jason Gunthorpe
@ 2020-02-20 17:46 ` Alexander Duyck
2020-02-21 1:44 ` Yunsheng Lin
2 siblings, 1 reply; 11+ messages in thread
From: Alexander Duyck @ 2020-02-20 17:46 UTC (permalink / raw)
To: Yunsheng Lin
Cc: Leon Romanovsky, Jason Gunthorpe, Lang Cheng, Doug Ledford,
David Miller, Salil Mehta, yisen.zhuang, LinuxArm, Netdev,
linux-rdma, Saeed Mahameed, bhaktipriya96, Tejun Heo,
Jeff Kirsher
On Tue, Feb 18, 2020 at 11:42 PM Yunsheng Lin <linyunsheng@huawei.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@gmail.com>
> Acked-by: Tejun Heo <tj@kernel.org>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC rdma-next] RDMA/core: Add attribute WQ_MEM_RECLAIM to workqueue "infiniband"
2020-02-20 17:46 ` Alexander Duyck
@ 2020-02-21 1:44 ` Yunsheng Lin
0 siblings, 0 replies; 11+ messages in thread
From: Yunsheng Lin @ 2020-02-21 1:44 UTC (permalink / raw)
To: Alexander Duyck
Cc: Leon Romanovsky, Jason Gunthorpe, Lang Cheng, Doug Ledford,
David Miller, Salil Mehta, yisen.zhuang, LinuxArm, Netdev,
linux-rdma, Saeed Mahameed, bhaktipriya96, Tejun Heo,
Jeff Kirsher
On 2020/2/21 1:46, Alexander Duyck wrote:
> On Tue, Feb 18, 2020 at 11:42 PM Yunsheng Lin <linyunsheng@huawei.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@gmail.com>
>> Acked-by: Tejun Heo <tj@kernel.org>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.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.
Yes, the hns3 driver does a few GFP_KERNEL calls too when resetting the
interface in hclge_reset_service_task(), which will run in the hns3 driver'
wq.
>
>> 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.
Ok, Will remove the WQ_MEM_RECLAIM first.
Thanks.
>
> - Alex
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread