Add a new work-queue for destructing stations?
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
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 6 lines · 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); inmac80211_do_stopbecause there are lots of 'slow' work-items queued up.I'd like to work on making this faster...
Quoted 7 lines
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 thefree_sta_rcuwork()in that queue.I'm guessing that to be safe, the
do_stop()code would need to selectively purge any work items in thelocal->workqueuethat 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
Quoted 15 lines · Johannes Berg
Quoted 6 lines · 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); inmac80211_do_stopbecause there are lots of 'slow' work-items queued up.I'd like to work on making this faster...
Quoted 7 lines
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 thefree_sta_rcuwork()in that queue.I'm guessing that to be safe, the
do_stop()code would need to selectively purge any work items in thelocal->workqueuethat relate to the sdata being destroyed, as well. I'm not sure how possible that would be...
Quoted 7 lines
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
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 10 lines · Ben Greear
Quoted 3 lines
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 pendingsdata->workitems 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 thefree_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
Quoted 13 lines · Johannes Berg
Quoted 10 lines · Ben Greear
Quoted 3 lines
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 pendingsdata->workitems 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 thefree_sta_work()on a different work-queue safely, I think....
Quoted 3 lines
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 withfree_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
Quoted 14 lines · Johannes Berg
Quoted 13 lines · Johannes Berg
Quoted 10 lines · Ben Greear
Quoted 3 lines
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 pendingsdata->workitems 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 thefree_sta_work()on a different work-queue safely, I think....
Quoted 8 lines
Quoted 3 lines
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 withfree_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
Quoted 13 lines · Johannes Berg
Quoted 10 lines · Ben Greear
Quoted 3 lines
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 pendingsdata->workitems 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 thefree_sta_work()on a different work-queue safely, I think....
Quoted 3 lines
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 withfree_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
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 31 lines · Ben Greear
Quoted 13 lines · Johannes Berg
Quoted 10 lines · Ben Greear
Quoted 3 lines
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 pendingsdata->workitems 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 thefree_sta_work()on a different work-queue safely, I think....Quoted 3 lines
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 withfree_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 afterfree_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 thesta->ampdu_mlme.work, and probably others as well.
Quoted 5 lines
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
Quoted 32 lines · Johannes Berg
Quoted 31 lines · Ben Greear
Quoted 13 lines · Johannes Berg
Quoted 10 lines · Ben Greear
Quoted 3 lines
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 pendingsdata->workitems 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 thefree_sta_work()on a different work-queue safely, I think....Quoted 3 lines
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 withfree_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 afterfree_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 thesta->ampdu_mlme.work, and probably others as well.
Quoted 8 lines
Quoted 5 lines
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
Quoted 3 lines
johannes
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 8 lines · Ben Greear
Quoted 8 lines
Quoted 8 lines
So, near where we currently call
cancel_work_sync(&sdata->work), I think we'd also need to cancel things like thesta->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).
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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 thesdata->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
Quoted 19 lines · Johannes Berg
Quoted 8 lines · Ben Greear
Quoted 8 lines
Quoted 8 lines
So, near where we currently call
cancel_work_sync(&sdata->work), I think we'd also need to cancel things like thesta->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).Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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 thesdata->work....
Quoted 7 lines
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
Quoted 3 lines
johannes
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 28 lines · Ben Greear
Quoted 19 lines · Johannes Berg
Quoted 8 lines · Ben Greear
Quoted 8 lines
Quoted 8 lines
So, near where we currently call
cancel_work_sync(&sdata->work), I think we'd also need to cancel things like thesta->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).Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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 thesdata->work....Quoted 7 lines
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.
Quoted 5 lines
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 fullflush_workqueue()inmac80211_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 theflush_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
Quoted 3 lines · Johannes Berg
Quoted 3 lines
Quoted 3 lines
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
Quoted 11 lines · Johannes Berg
Quoted 3 lines · Johannes Berg
Quoted 3 lines
Quoted 3 lines
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 commitb22cfcfca, 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 tosta_free_wkwe 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...
Quoted 3 lines
johannes
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com
Quoted 7 lines · Ben Greear
Quoted 6 lines · Johannes Berg
Quoted 3 lines · Johannes Berg
Quoted 3 lines
Quoted 3 lines
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?
Quoted 7 lines
Quoted 5 lines
Wait actually, that isn't a concern. The
flush_workqueue()was new in commitb22cfcfca, 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 tosta_free_wkwe 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.
Quoted 3 lines
I didn't understand your proposal for the
sta_free_wkchanges, 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
Quoted 16 lines · Johannes Berg
Quoted 7 lines · Ben Greear
Quoted 6 lines · Johannes Berg
Quoted 3 lines · Johannes Berg
Quoted 3 lines
Quoted 3 lines
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?Quoted 7 lines
Quoted 5 lines
Wait actually, that isn't a concern. The
flush_workqueue()was new in commitb22cfcfca, 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 tosta_free_wkwe should be fine.We still need to cancel the other mac80211 work items though, right?
Quoted 8 lines
Well, no, because we didn't add any work items since we added the
flush_workqueue()there.Quoted 3 lines
I didn't understand your proposal for the
sta_free_wkchanges, 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
Quoted 3 lines
johannes
Signature · 2 lines
Ben Greear <[email protected]> Candela Technologies Inc http://www.candelatech.com