Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM

[Benjamin Beichler]

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: benjamin.beich...@uni-rostock.de
www: http://www.imd.uni-rostock.de/




smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM

[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.

Thanks.

-- 
tejun

Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM

[Johannes Berg]

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

Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM

[Tejun Heo]

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

Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM

[Johannes Berg]

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

Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM

[Arend van Spriel]

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" >:
 >
 > 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

Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM

[Johannes Berg]

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

Re: [PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM

[Benjamin Beichler]

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 
>
> 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.
>

-- 
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: benjamin.beich...@uni-rostock.de
www: http://www.imd.uni-rostock.de/




smime.p7s
Description: S/MIME Cryptographic Signature

[PATCH v2] mac80211_hwsim: don't use WQ_MEM_RECLAIM

[Johannes Berg]

From: Johannes Berg 

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.

Reported-by: syzbot+41cdaf4232c50e658...@syzkaller.appspotmail.com
Signed-off-by: Johannes Berg 
---
 drivers/net/wireless/mac80211_hwsim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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(_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(_radios_rht, _rht_params);
-- 
2.15.1