Message ID | 20180124074051.19939-1-johannes@sipsolutions.net (mailing list archive) |
---|---|
State | Accepted |
Delegated to: | Johannes Berg |
Headers | show |
Hi Johannes, sorry for introducing that error, but I'm a bit confused by the workqueue documentation. My assumption was, that deleting hwsim radios is reclaiming memory, and since this queue does nothing else it would save/necessary to set this flag. Maybe a hint in the documentation, that a work item on a WQ_MEM_RECLAIM queue must not call flush of an !WQ_MEM_RECLAIM queue would be nice. Maybe it's kind of obvious, but there is also a reminder not to forget that flag, if a queue may have work items that reclaim memory. Again sorry for not seeing the warning on my test system ... kind regards Benjamin Am 24.01.2018 um 08:40 schrieb Johannes Berg: > From: Johannes Berg <johannes.berg@intel.com> > > We're obviously not part of a memory reclaim path, so don't set the flag. > > This also causes a warning in check_flush_dependency() since we end up > in a code path that flushes a non-reclaim workqueue, and we shouldn't do > that if we were really part of reclaim. >
On Wed, 2018-01-24 at 10:39 +0100, Benjamin Beichler wrote: > sorry for introducing that error, but I'm a bit confused by the > workqueue documentation. > My assumption was, that deleting hwsim radios is reclaiming memory, and > since this queue does nothing else it would save/necessary to set this flag. > > Maybe a hint in the documentation, that a work item on a WQ_MEM_RECLAIM > queue must not call flush of an !WQ_MEM_RECLAIM queue would be nice. > Maybe it's kind of obvious, but there is also a reminder not to forget > that flag, if a queue may have work items that reclaim memory Yeah, honestly, I'm not really sure either. Clearly we can't set it, but other drivers also set it... I don't think it was *intended* for when you're freeing memory, since I think reclaiming is what happens when you write out dirty buffers to disk etc. johannes
resending as it included html and got blocked from the list. On 1/25/2018 7:21 PM, Arend Van Spriel wrote: > Op 24 jan. 2018 11:46 schreef "Johannes Berg" <johannes@sipsolutions.net > <mailto:johannes@sipsolutions.net>>: > > > > On Wed, 2018-01-24 at 10:39 +0100, Benjamin Beichler wrote: > > > > > sorry for introducing that error, but I'm a bit confused by the > > > workqueue documentation. > > > My assumption was, that deleting hwsim radios is reclaiming memory, and > > > since this queue does nothing else it would save/necessary to set > this flag. > > > > > > Maybe a hint in the documentation, that a work item on a WQ_MEM_RECLAIM > > > queue must not call flush of an !WQ_MEM_RECLAIM queue would be nice. > > > Maybe it's kind of obvious, but there is also a reminder not to forget > > > that flag, if a queue may have work items that reclaim memory > > > > Yeah, honestly, I'm not really sure either. Clearly we can't set it, > > but other drivers also set it... > > That triggered something in my memory. So indeed we use it in brcmfmac > as well. We used create_singlethread_workqueue(), but I wanted to avoid > snprintf and specify the name format so switched to using > alloc_ordered_workqueue() keeping WQ_MEM_RECLAIM as per the macro > definition. #define create_singlethread_workqueue(name) \ alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name) > Don't recall why I dropped the __WQ_LEGACY flag though. > > Regards, > Arend > > > I don't think it was *intended* for when you're freeing memory, since I > > think reclaiming is what happens when you write out dirty buffers to > > disk etc. > > > > johannes >
I guess we should just ask Tejun :-) Tejun, the problem was a report that a WQ_MEM_RECLAIM workqueue is flushing another that isn't, and it turns out that lots of wireless drivers are using WQ_MEM_RECLAIM for some reason. Arend said: > > > > Maybe a hint in the documentation, that a work item on a WQ_MEM_RECLAIM > > > > queue must not call flush of an !WQ_MEM_RECLAIM queue would be nice. > > > > Maybe it's kind of obvious, but there is also a reminder not to forget > > > > that flag, if a queue may have work items that reclaim memory > > > > > > Yeah, honestly, I'm not really sure either. Clearly we can't set it, > > > but other drivers also set it... > > > > That triggered something in my memory. So indeed we use it in brcmfmac > > as well. We used create_singlethread_workqueue(), but I wanted to avoid > > snprintf and specify the name format so switched to using > > alloc_ordered_workqueue() keeping WQ_MEM_RECLAIM as per the macro > > definition. > > #define create_singlethread_workqueue(name) \ > alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name) > > > Don't recall why I dropped the __WQ_LEGACY flag though. johannes
Hello, On Thu, Jan 25, 2018 at 10:01:26PM +0100, Johannes Berg wrote: > I guess we should just ask Tejun :-) > > Tejun, the problem was a report that a WQ_MEM_RECLAIM workqueue is > flushing another that isn't, and it turns out that lots of wireless > drivers are using WQ_MEM_RECLAIM for some reason. Yeah, that came up a couple years ago. IIRC, there wasn't a definite answer but the sentiment seemed that things like nfs over wireless should probably considered. No idea how serious that concern is. > Arend said: > > > > > Maybe a hint in the documentation, that a work item on a WQ_MEM_RECLAIM > > > > > queue must not call flush of an !WQ_MEM_RECLAIM queue would be nice. > > > > > Maybe it's kind of obvious, but there is also a reminder not to forget > > > > > that flag, if a queue may have work items that reclaim memory > > > > > > > > Yeah, honestly, I'm not really sure either. Clearly we can't set it, > > > > but other drivers also set it... So, anything which can sit in memory reclaim path needs to have that flag set and having that flag set automatically means that it can't depend on anything which isn't protected the same way as that'd break that protection. > > > That triggered something in my memory. So indeed we use it in brcmfmac > > > as well. We used create_singlethread_workqueue(), but I wanted to avoid > > > snprintf and specify the name format so switched to using > > > alloc_ordered_workqueue() keeping WQ_MEM_RECLAIM as per the macro > > > definition. > > > > #define create_singlethread_workqueue(name) \ > > alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name) > > > > > Don't recall why I dropped the __WQ_LEGACY flag though. The only thing that flag does is disabling the flush dependency check which is necessary because in the old implementation, all workqueues were basically WQ_MEM_RECLAIM workqueues leading to spurious triggering of the warnings. Thanks.
On Thu, 2018-01-25 at 14:26 -0800, Tejun Heo wrote: > Yeah, that came up a couple years ago. IIRC, there wasn't a definite > answer but the sentiment seemed that things like nfs over wireless > should probably considered. No idea how serious that concern is. Not that you mention it ... Honestly though, wifi connections break all the time, so not sure you'd really want that. But anyway, that's what we have. > So, anything which can sit in memory reclaim path needs to have that > flag set and having that flag set automatically means that it can't > depend on anything which isn't protected the same way as that'd break > that protection. Right. I actually misinterpreted this though - the warning comes from: workqueue: WQ_MEM_RECLAIM hwsim_wq:destroy_radio is flushing !WQ_MEM_RECLAIM events_highpri:flush_backlog and it's flushing something in flush_all_backlogs(). Our workqueue here is only at teardown time (when you remove a netdev) so it's not sitting in any critical path for reclaim anyway. So I think this is the right patch, I'll queue it up. Thanks for the reminder on NFS :-) johannes
Hello, On Fri, Jan 26, 2018 at 09:08:02AM +0100, Johannes Berg wrote: > Not that you mention it ... Honestly though, wifi connections break all > the time, so not sure you'd really want that. But anyway, that's what > we have. I'd be a lot happier to remove WQ_MEM_RECLAIM from wireless drivers too, and glancing the code, it looked like there already are so many holes that whether having that flag or not shouldn't matter all that much. It was just difficult for me to make a case for removing it preemptively. If you're up for it, please be my guest. Thanks.
Hi, Am 26.01.2018 um 16:05 schrieb Tejun Heo: > Hello, > > On Fri, Jan 26, 2018 at 09:08:02AM +0100, Johannes Berg wrote: >> Not that you mention it ... Honestly though, wifi connections break all >> the time, so not sure you'd really want that. But anyway, that's what >> we have. > I'd be a lot happier to remove WQ_MEM_RECLAIM from wireless drivers > too, and glancing the code, it looked like there already are so many > holes that whether having that flag or not shouldn't matter all that > much. It was just difficult for me to make a case for removing it > preemptively. If you're up for it, please be my guest. I think, it is quite clear, why this error is produced, since in memory reclaim, we should not synchronously flush unimportant work queues. I think there could be valid use cases to have work queues with that flag in wireless drivers, if the are used carefully. Therefore I suggest a hint in the work queue documentation like: "Check whether work_items on WQ_MEM_RECLAIM wq may actively delay memory reclaim with network timeouts and check whether other work queues without WQ_MEM_RECLAIM may be flushed within work_items of the queue, as this will prioritize non-reclaiming work_items on the flushed queue the same level as reclaiming work_items". At least this can help for future usage, as it could have avoided my error :-D > Thanks. >
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index 1cf22e62e3dd..6e0af815f25e 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -3516,7 +3516,7 @@ static int __init init_mac80211_hwsim(void) spin_lock_init(&hwsim_radio_lock); - hwsim_wq = alloc_workqueue("hwsim_wq",WQ_MEM_RECLAIM,0); + hwsim_wq = alloc_workqueue("hwsim_wq", 0, 0); if (!hwsim_wq) return -ENOMEM; rhashtable_init(&hwsim_radios_rht, &hwsim_rht_params);