Add a new work-queue for destructing stations?

Add a new work-queue for destructing stations?

[Ben Greear]

As previously posted, there can be cases where the RTNL is held for a
very long time when trying to do the:  flush_workqueue(local->workqueue);
in mac80211_do_stop because there are lots of 'slow' work-items queued up.

I'd like to work on making this faster...

My first idea is to add a second work-queue to the 'local' for high-priority
items that can be executed independently from the current local->workqueue,
and put the free_sta_rcu work() in that queue.

I'm guessing that to be safe, the do_stop() code would need to selectively
purge any work items in the local->workqueue that relate to the sdata
being destroyed, as well.  I'm not sure how possible that would be...

Any comments on this, or suggestions for a better way to do this?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Add a new work-queue for destructing stations?

[Johannes Berg]

On Thu, 2012-12-13 at 09:46 -0800, Ben Greear wrote:
> As previously posted, there can be cases where the RTNL is held for a
> very long time when trying to do the:  flush_workqueue(local->workqueue);
> in mac80211_do_stop because there are lots of 'slow' work-items queued up.
> 
> I'd like to work on making this faster...
> 
> My first idea is to add a second work-queue to the 'local' for high-priority
> items that can be executed independently from the current local->workqueue,
> and put the free_sta_rcu work() in that queue.
> 
> I'm guessing that to be safe, the do_stop() code would need to selectively
> purge any work items in the local->workqueue that relate to the sdata
> being destroyed, as well.  I'm not sure how possible that would be...

I don't think that's easy, but you're welcome to try. The
free_sta_work() function references the sdata so it absolutely must run
at this point.

The only/easiest thing that seems vaguely safe would be a per-vif
workqueue for this? But that's really annoying too, since it needs an
extra thread etc.

johannes

Re: Add a new work-queue for destructing stations?

[Ben Greear]

On 12/13/2012 09:59 AM, Johannes Berg wrote:
> On Thu, 2012-12-13 at 09:46 -0800, Ben Greear wrote:
>> As previously posted, there can be cases where the RTNL is held for a
>> very long time when trying to do the:  flush_workqueue(local->workqueue);
>> in mac80211_do_stop because there are lots of 'slow' work-items queued up.
>>
>> I'd like to work on making this faster...
>>
>> My first idea is to add a second work-queue to the 'local' for high-priority
>> items that can be executed independently from the current local->workqueue,
>> and put the free_sta_rcu work() in that queue.
>>
>> I'm guessing that to be safe, the do_stop() code would need to selectively
>> purge any work items in the local->workqueue that relate to the sdata
>> being destroyed, as well.  I'm not sure how possible that would be...
>
> I don't think that's easy, but you're welcome to try. The
> free_sta_work() function references the sdata so it absolutely must run
> at this point.
>
> The only/easiest thing that seems vaguely safe would be a per-vif
> workqueue for this? But that's really annoying too, since it needs an
> extra thread etc.

So, cancel_work_sync(&sdata->work) would appear to remove
all pending sdata->work items from the work-queue.  As long as
there are no other different work items that reference
sdata (and maybe there are..I haven't looked at all of them),
then we should be safe to execute the free_sta_work()
on a different work-queue safely, I think....

I'll go look at the other work items and see what I can see.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Add a new work-queue for destructing stations?

[Johannes Berg]

On Thu, 2012-12-13 at 10:19 -0800, Ben Greear wrote:

> > I don't think that's easy, but you're welcome to try. The
> > free_sta_work() function references the sdata so it absolutely must run
> > at this point.

> So, cancel_work_sync(&sdata->work) would appear to remove
> all pending sdata->work items from the work-queue.  As long as
> there are no other different work items that reference
> sdata (and maybe there are..I haven't looked at all of them),
> then we should be safe to execute the free_sta_work()
> on a different work-queue safely, I think....

Sorry, I don't get it. free_sta_work() *itself* has to be executed
before the sdata is destroyed. cancel_work_sync(&sdata->work) has
nothing to do with free_sta_work.

johannes

Re: Add a new work-queue for destructing stations?

[Johannes Berg]

On Thu, 2012-12-13 at 19:24 +0100, Johannes Berg wrote:
> On Thu, 2012-12-13 at 10:19 -0800, Ben Greear wrote:
> 
> > > I don't think that's easy, but you're welcome to try. The
> > > free_sta_work() function references the sdata so it absolutely must run
> > > at this point.
> 
> > So, cancel_work_sync(&sdata->work) would appear to remove
> > all pending sdata->work items from the work-queue.  As long as
> > there are no other different work items that reference
> > sdata (and maybe there are..I haven't looked at all of them),
> > then we should be safe to execute the free_sta_work()
> > on a different work-queue safely, I think....
> 
> Sorry, I don't get it. free_sta_work() *itself* has to be executed
> before the sdata is destroyed. cancel_work_sync(&sdata->work) has
> nothing to do with free_sta_work.

In fact, this is already buggy now, and I should probably revert
b22cfcfca, because the work item might run after the AP is stopped and
then we call into the driver anyway, and on teardown things are probably
messed up. I'll poke at it tomorrow.

johannes

Re: Add a new work-queue for destructing stations?

[Ben Greear]

On 12/13/2012 10:24 AM, Johannes Berg wrote:
> On Thu, 2012-12-13 at 10:19 -0800, Ben Greear wrote:
>
>>> I don't think that's easy, but you're welcome to try. The
>>> free_sta_work() function references the sdata so it absolutely must run
>>> at this point.
>
>> So, cancel_work_sync(&sdata->work) would appear to remove
>> all pending sdata->work items from the work-queue.  As long as
>> there are no other different work items that reference
>> sdata (and maybe there are..I haven't looked at all of them),
>> then we should be safe to execute the free_sta_work()
>> on a different work-queue safely, I think....
>
> Sorry, I don't get it. free_sta_work() *itself* has to be executed
> before the sdata is destroyed. cancel_work_sync(&sdata->work) has
> nothing to do with free_sta_work.

My concern is this:

If I add a different work queue (called destructor-workqueue, perhaps)
for free_sta_work(), then it only helps
if I can not block on flushing the current 'big' work-queue.  But, if
there are items on the big work-queue that reference sdata, then
that could easily execute after free_sta_work(), and I'm guessing
that would be very bad.

So, near where we currently call cancel_work_sync(&sdata->work), I
think we'd also need to cancel things like the sta->ampdu_mlme.work,
and probably others as well.

Then, when we're sure there are no more references to sdata on the
main-work-queue, it would be safe to flush the destructor-workqueue
(which would have the fre_sta_work() on it).

Maybe?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Add a new work-queue for destructing stations?

[Johannes Berg]

On Thu, 2012-12-13 at 19:27 +0100, Johannes Berg wrote:
> On Thu, 2012-12-13 at 19:24 +0100, Johannes Berg wrote:
> > On Thu, 2012-12-13 at 10:19 -0800, Ben Greear wrote:
> > 
> > > > I don't think that's easy, but you're welcome to try. The
> > > > free_sta_work() function references the sdata so it absolutely must run
> > > > at this point.
> > 
> > > So, cancel_work_sync(&sdata->work) would appear to remove
> > > all pending sdata->work items from the work-queue.  As long as
> > > there are no other different work items that reference
> > > sdata (and maybe there are..I haven't looked at all of them),
> > > then we should be safe to execute the free_sta_work()
> > > on a different work-queue safely, I think....
> > 
> > Sorry, I don't get it. free_sta_work() *itself* has to be executed
> > before the sdata is destroyed. cancel_work_sync(&sdata->work) has
> > nothing to do with free_sta_work.
> 
> In fact, this is already buggy now, and I should probably revert
> b22cfcfca, because the work item might run after the AP is stopped and
> then we call into the driver anyway, and on teardown things are probably
> messed up. I'll poke at it tomorrow.

Oh I know an easy way to fix this: we just add the stations to a
(per-sdata?) "cleanup" list in the rcu callback and run a single work
item that cleans up this list. Then we can clean this list in the right
places (stop_ap, etc.)

johannes

Re: Add a new work-queue for destructing stations?

[Johannes Berg]

On Thu, 2012-12-13 at 10:30 -0800, Ben Greear wrote:
> On 12/13/2012 10:24 AM, Johannes Berg wrote:
> > On Thu, 2012-12-13 at 10:19 -0800, Ben Greear wrote:
> >
> >>> I don't think that's easy, but you're welcome to try. The
> >>> free_sta_work() function references the sdata so it absolutely must run
> >>> at this point.
> >
> >> So, cancel_work_sync(&sdata->work) would appear to remove
> >> all pending sdata->work items from the work-queue.  As long as
> >> there are no other different work items that reference
> >> sdata (and maybe there are..I haven't looked at all of them),
> >> then we should be safe to execute the free_sta_work()
> >> on a different work-queue safely, I think....
> >
> > Sorry, I don't get it. free_sta_work() *itself* has to be executed
> > before the sdata is destroyed. cancel_work_sync(&sdata->work) has
> > nothing to do with free_sta_work.
> 
> My concern is this:
> 
> If I add a different work queue (called destructor-workqueue, perhaps)
> for free_sta_work(), then it only helps
> if I can not block on flushing the current 'big' work-queue.  But, if
> there are items on the big work-queue that reference sdata, then
> that could easily execute after free_sta_work(), and I'm guessing
> that would be very bad.
> 
> So, near where we currently call cancel_work_sync(&sdata->work), I
> think we'd also need to cancel things like the sta->ampdu_mlme.work,
> and probably others as well.
> 
> Then, when we're sure there are no more references to sdata on the
> main-work-queue, it would be safe to flush the destructor-workqueue
> (which would have the fre_sta_work() on it).
> 
> Maybe?

Huh, ok, but I don't think you'll find much that doesn't reference an
sdata?

johannes

Re: Add a new work-queue for destructing stations?

[Ben Greear]

On 12/13/2012 10:37 AM, Johannes Berg wrote:
> On Thu, 2012-12-13 at 10:30 -0800, Ben Greear wrote:
>> On 12/13/2012 10:24 AM, Johannes Berg wrote:
>>> On Thu, 2012-12-13 at 10:19 -0800, Ben Greear wrote:
>>>
>>>>> I don't think that's easy, but you're welcome to try. The
>>>>> free_sta_work() function references the sdata so it absolutely must run
>>>>> at this point.
>>>
>>>> So, cancel_work_sync(&sdata->work) would appear to remove
>>>> all pending sdata->work items from the work-queue.  As long as
>>>> there are no other different work items that reference
>>>> sdata (and maybe there are..I haven't looked at all of them),
>>>> then we should be safe to execute the free_sta_work()
>>>> on a different work-queue safely, I think....
>>>
>>> Sorry, I don't get it. free_sta_work() *itself* has to be executed
>>> before the sdata is destroyed. cancel_work_sync(&sdata->work) has
>>> nothing to do with free_sta_work.
>>
>> My concern is this:
>>
>> If I add a different work queue (called destructor-workqueue, perhaps)
>> for free_sta_work(), then it only helps
>> if I can not block on flushing the current 'big' work-queue.  But, if
>> there are items on the big work-queue that reference sdata, then
>> that could easily execute after free_sta_work(), and I'm guessing
>> that would be very bad.
>>
>> So, near where we currently call cancel_work_sync(&sdata->work), I
>> think we'd also need to cancel things like the sta->ampdu_mlme.work,
>> and probably others as well.
>>
>> Then, when we're sure there are no more references to sdata on the
>> main-work-queue, it would be safe to flush the destructor-workqueue
>> (which would have the fre_sta_work() on it).
>>
>> Maybe?
>
> Huh, ok, but I don't think you'll find much that doesn't reference an
> sdata?

I think you are right...so just need to find all of those work items
and call cancel_work_sync() on them, just like we are currently cancelling
the sdata->work....

Thanks,
Ben


>
> johannes
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Add a new work-queue for destructing stations?

[Johannes Berg]

On Thu, 2012-12-13 at 10:39 -0800, Ben Greear wrote:

> >> So, near where we currently call cancel_work_sync(&sdata->work), I
> >> think we'd also need to cancel things like the sta->ampdu_mlme.work,
> >> and probably others as well.
> >>
> >> Then, when we're sure there are no more references to sdata on the
> >> main-work-queue, it would be safe to flush the destructor-workqueue
> >> (which would have the fre_sta_work() on it).
> >>
> >> Maybe?
> >
> > Huh, ok, but I don't think you'll find much that doesn't reference an
> > sdata?
> 
> I think you are right...so just need to find all of those work items
> and call cancel_work_sync() on them, just like we are currently cancelling
> the sdata->work....

Well the concern there is that we expose the workqueue to the driver,
and it might even put sdata-referencing work items on it and expect them
to be flushed out before stop_interface() is called?

You'd be welcome to change the API so that isn't true, or things are
different, or whatever, but it'd be a lot of careful auditing (and
probably fixing) of drivers.

johannes

Re: Add a new work-queue for destructing stations?

[Ben Greear]

On 12/13/2012 10:41 AM, Johannes Berg wrote:
> On Thu, 2012-12-13 at 10:39 -0800, Ben Greear wrote:
>
>>>> So, near where we currently call cancel_work_sync(&sdata->work), I
>>>> think we'd also need to cancel things like the sta->ampdu_mlme.work,
>>>> and probably others as well.
>>>>
>>>> Then, when we're sure there are no more references to sdata on the
>>>> main-work-queue, it would be safe to flush the destructor-workqueue
>>>> (which would have the fre_sta_work() on it).
>>>>
>>>> Maybe?
>>>
>>> Huh, ok, but I don't think you'll find much that doesn't reference an
>>> sdata?
>>
>> I think you are right...so just need to find all of those work items
>> and call cancel_work_sync() on them, just like we are currently cancelling
>> the sdata->work....
>
> Well the concern there is that we expose the workqueue to the driver,
> and it might even put sdata-referencing work items on it and expect them
> to be flushed out before stop_interface() is called?
>
> You'd be welcome to change the API so that isn't true, or things are
> different, or whatever, but it'd be a lot of careful auditing (and
> probably fixing) of drivers.

So, maybe a new driver api call to 'cancel_all_work_items(sdata)',
and if the driver does not implement this, then we stick with the
full flush_workqueue() in mac80211_do_stop().  But, if the driver
does implement it, and we add code there to flush any mac80211 related
work-items, then we could skip the flush_workqueue() call....

I'll go looking in ath9k to see how much it uses the work-queue...that
driver is my only real concern at the moment.

Thanks,
Ben

>
> johannes
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Add a new work-queue for destructing stations?

[Johannes Berg]

On Thu, 2012-12-13 at 10:47 -0800, Ben Greear wrote:
> On 12/13/2012 10:41 AM, Johannes Berg wrote:
> > On Thu, 2012-12-13 at 10:39 -0800, Ben Greear wrote:
> >
> >>>> So, near where we currently call cancel_work_sync(&sdata->work), I
> >>>> think we'd also need to cancel things like the sta->ampdu_mlme.work,
> >>>> and probably others as well.
> >>>>
> >>>> Then, when we're sure there are no more references to sdata on the
> >>>> main-work-queue, it would be safe to flush the destructor-workqueue
> >>>> (which would have the fre_sta_work() on it).
> >>>>
> >>>> Maybe?
> >>>
> >>> Huh, ok, but I don't think you'll find much that doesn't reference an
> >>> sdata?
> >>
> >> I think you are right...so just need to find all of those work items
> >> and call cancel_work_sync() on them, just like we are currently cancelling
> >> the sdata->work....
> >
> > Well the concern there is that we expose the workqueue to the driver,
> > and it might even put sdata-referencing work items on it and expect them
> > to be flushed out before stop_interface() is called?
> >
> > You'd be welcome to change the API so that isn't true, or things are
> > different, or whatever, but it'd be a lot of careful auditing (and
> > probably fixing) of drivers.
> 
> So, maybe a new driver api call to 'cancel_all_work_items(sdata)',
> and if the driver does not implement this, then we stick with the
> full flush_workqueue() in mac80211_do_stop().  But, if the driver
> does implement it, and we add code there to flush any mac80211 related
> work-items, then we could skip the flush_workqueue() call....

You don't even need the API, it can do it in remove_interface(). It's
just not ... guaranteed right now. Also, if it wants them to run,
locking may get tricky?

johannes

Re: Add a new work-queue for destructing stations?

[Johannes Berg]

On Thu, 2012-12-13 at 19:49 +0100, Johannes Berg wrote:

> > > Well the concern there is that we expose the workqueue to the driver,
> > > and it might even put sdata-referencing work items on it and expect them
> > > to be flushed out before stop_interface() is called?

Wait actually, that isn't a concern. The flush_workqueue() was new in
commit b22cfcfca, before that drivers couldn't rely on it. It seems very
unlikely that in the meantime drivers have come to rely on it and we
didn't document this, so ... with the proposed change to sta_free_wk we
should be fine.

johannes

Re: Add a new work-queue for destructing stations?

[Ben Greear]

On 12/13/2012 10:53 AM, Johannes Berg wrote:
> On Thu, 2012-12-13 at 19:49 +0100, Johannes Berg wrote:
>
>>>> Well the concern there is that we expose the workqueue to the driver,
>>>> and it might even put sdata-referencing work items on it and expect them
>>>> to be flushed out before stop_interface() is called?
>
> Wait actually, that isn't a concern. The flush_workqueue() was new in
> commit b22cfcfca, before that drivers couldn't rely on it. It seems very
> unlikely that in the meantime drivers have come to rely on it and we
> didn't document this, so ... with the proposed change to sta_free_wk we
> should be fine.

We still need to cancel the other mac80211 work items though, right?

I didn't understand your proposal for the sta_free_wk changes,
but I can at least work on tracking down the mac80211 work items
if that helps...

>
> johannes
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Add a new work-queue for destructing stations?

[Johannes Berg]

On Thu, 2012-12-13 at 11:00 -0800, Ben Greear wrote:
> On 12/13/2012 10:53 AM, Johannes Berg wrote:
> > On Thu, 2012-12-13 at 19:49 +0100, Johannes Berg wrote:
> >
> >>>> Well the concern there is that we expose the workqueue to the driver,
> >>>> and it might even put sdata-referencing work items on it and expect them
> >>>> to be flushed out before stop_interface() is called?
> >
> > Wait actually, that isn't a concern. The flush_workqueue() was new in
> > commit b22cfcfca, before that drivers couldn't rely on it. It seems very
> > unlikely that in the meantime drivers have come to rely on it and we
> > didn't document this, so ... with the proposed change to sta_free_wk we
> > should be fine.
> 
> We still need to cancel the other mac80211 work items though, right?

Well, no, because we didn't add any work items since we added the
flush_workqueue() there.

> I didn't understand your proposal for the sta_free_wk changes,
> but I can at least work on tracking down the mac80211 work items
> if that helps...

I'll get a patch out later or tomorrow.

johannes

Re: Add a new work-queue for destructing stations?

[Ben Greear]

On 12/13/2012 11:11 AM, Johannes Berg wrote:
> On Thu, 2012-12-13 at 11:00 -0800, Ben Greear wrote:
>> On 12/13/2012 10:53 AM, Johannes Berg wrote:
>>> On Thu, 2012-12-13 at 19:49 +0100, Johannes Berg wrote:
>>>
>>>>>> Well the concern there is that we expose the workqueue to the driver,
>>>>>> and it might even put sdata-referencing work items on it and expect them
>>>>>> to be flushed out before stop_interface() is called?
>>>
>>> Wait actually, that isn't a concern. The flush_workqueue() was new in
>>> commit b22cfcfca, before that drivers couldn't rely on it. It seems very
>>> unlikely that in the meantime drivers have come to rely on it and we
>>> didn't document this, so ... with the proposed change to sta_free_wk we
>>> should be fine.
>>
>> We still need to cancel the other mac80211 work items though, right?
>
> Well, no, because we didn't add any work items since we added the
> flush_workqueue() there.
>
>> I didn't understand your proposal for the sta_free_wk changes,
>> but I can at least work on tracking down the mac80211 work items
>> if that helps...
>
> I'll get a patch out later or tomorrow.

Ok, I'll be sure to test it, and will hold off on any of my own
efforts in this area in the meantime.

Thanks,
Ben

>
> johannes
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com