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" <[email protected]
> <mailto:[email protected]>>:
> >
> > 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
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 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.
--
tejun
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.
--
tejun
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.
>
--
M.Sc. Benjamin Beichler
Universität Rostock, Fakultät für Informatik und Elektrotechnik
Institut für Angewandte Mikroelektronik und Datentechnik
University of Rostock, Department of CS and EE
Institute of Applied Microelectronics and CE
Richard-Wagner-Straße 31
18119 Rostock
Deutschland/Germany
phone: +49 (0) 381 498 - 7278
email: [email protected]
www: http://www.imd.uni-rostock.de/