mac80211: sta info locking

mac80211: sta info locking

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

Hi,

Ron, I noticed you added spinlocks to ampdu_mlme for both the and TX
processing. Generally, the locking in sta_info is lacking, for example
the flags member is simply updated all over the place without regard for
locking.

I have boiled down the locking requirements to various things:

 * static sta info members, set on allocation (fine now after a patch
   that splits allocation and hash table insertion, except in one
   place where an AP entry is added, need to review)
 * semi-static information, to review, but most likely updated only
   on external events like cfg80211 which are synchronized
 * flags, needs locking badly
 * powersave frame queues (have internal locking)
 * sta info members updated only within the TX status path
   -> fine per my previous mail
 * sta info members updated only within the RX path
   -> also fine per my previous mail
 * debug counters we don't really care about (channel use)
 * AMPDU stuff (own locking)

Hence, I think we can actually get away without more locking if we
protect the flags better. Should we use a spinlock or the atomic
set_bit()/clear_bit()/etc. operations?

If we were to use a spinlock, could the AMPDU stuff use that too? Does
it really need one spinlock for TX and one for RX?

I don't think the spinlock would be contended a lot, but it is possible
that, at the same time,
 * the TX path is trying to clear the PSPOLL flag
 * hostapd is trying to clear the WLAN_STA_AUTHORIZED flag
 * the RX path is trying to set the PS flag

....

I hope you're all (not just Ron, I just addressed you because of the
ampdu stuff) reading this and trying to wrap your head around the
synchronisation issues in mac80211, I don't want to be the only one with
then somehow magic knowledge about it... Once I understand it I'll try
to document it too.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: mac80211: sta info locking

[John W. Linville]

On Fri, Feb 22, 2008 at 11:44:29AM +0100, Johannes Berg wrote:

> Hence, I think we can actually get away without more locking if we
> protect the flags better. Should we use a spinlock or the atomic
> set_bit()/clear_bit()/etc. operations?

Using the atomic operations seems appropriate to me.

-- 
John W. Linville
linville@tuxdriver.com

Re: mac80211: sta info locking

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]


> > Hence, I think we can actually get away without more locking if we
> > protect the flags better. Should we use a spinlock or the atomic
> > set_bit()/clear_bit()/etc. operations?
> 
> Using the atomic operations seems appropriate to me.

Right, but I figured if we could get rid of the AMPDU spinlocks and just
use a single one in total (for flags as well) then that'd be of benefit
too; even with the dynamic allocation strategy (see other mail) we'd not
need to allocate two more spinlocks for ampdu.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: mac80211: sta info locking

[John W. Linville]

On Fri, Feb 22, 2008 at 04:16:49PM +0100, Johannes Berg wrote:
> 
> > > Hence, I think we can actually get away without more locking if we
> > > protect the flags better. Should we use a spinlock or the atomic
> > > set_bit()/clear_bit()/etc. operations?
> > 
> > Using the atomic operations seems appropriate to me.
> 
> Right, but I figured if we could get rid of the AMPDU spinlocks and just
> use a single one in total (for flags as well) then that'd be of benefit
> too; even with the dynamic allocation strategy (see other mail) we'd not
> need to allocate two more spinlocks for ampdu.

Yes, I thought that was behind your question.  I'll let Ron comment
on the AMPDU spinlock usage.

John
-- 
John W. Linville
linville@tuxdriver.com

Re: mac80211: sta info locking

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]


On Fri, 2008-02-22 at 10:57 -0500, John W. Linville wrote:
> On Fri, Feb 22, 2008 at 04:16:49PM +0100, Johannes Berg wrote:
> > 
> > > > Hence, I think we can actually get away without more locking if we
> > > > protect the flags better. Should we use a spinlock or the atomic
> > > > set_bit()/clear_bit()/etc. operations?
> > > 
> > > Using the atomic operations seems appropriate to me.
> > 
> > Right, but I figured if we could get rid of the AMPDU spinlocks and just
> > use a single one in total (for flags as well) then that'd be of benefit
> > too; even with the dynamic allocation strategy (see other mail) we'd not
> > need to allocate two more spinlocks for ampdu.
> 
> Yes, I thought that was behind your question.  I'll let Ron comment
> on the AMPDU spinlock usage.

Ok so the mesh code came with a spinlock too, the AMPDU code has two.

Ron/Tomas, does the ampdu MLME really need two spinlocks?

Does anybody have any objections to just having a *single* spinlock per
sta_info and using that everywhere and protect the flags with it too?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: mac80211: sta info locking

[Tomas Winkler]

On Mon, Feb 25, 2008 at 10:46 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>  On Fri, 2008-02-22 at 10:57 -0500, John W. Linville wrote:
>  > On Fri, Feb 22, 2008 at 04:16:49PM +0100, Johannes Berg wrote:
>  > >
>  > > > > Hence, I think we can actually get away without more locking if we
>  > > > > protect the flags better. Should we use a spinlock or the atomic
>  > > > > set_bit()/clear_bit()/etc. operations?
>  > > >
>  > > > Using the atomic operations seems appropriate to me.
>  > >
>  > > Right, but I figured if we could get rid of the AMPDU spinlocks and just
>  > > use a single one in total (for flags as well) then that'd be of benefit
>  > > too; even with the dynamic allocation strategy (see other mail) we'd not
>  > > need to allocate two more spinlocks for ampdu.
>  >
>  > Yes, I thought that was behind your question.  I'll let Ron comment
>  > on the AMPDU spinlock usage.
>
>  Ok so the mesh code came with a spinlock too, the AMPDU code has two.
>
>  Ron/Tomas, does the ampdu MLME really need two spinlocks?

The problem is that RX a TX BA establishments usually happens
concurrently for single STA, those are independent state machines.  We
have to review this carefully before spinlokcs are removed.

Thanks
Tomas

Re: mac80211: sta info locking

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]


On Tue, 2008-02-26 at 00:50 +0200, Tomas Winkler wrote:
> On Mon, Feb 25, 2008 at 10:46 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> >
> >  On Fri, 2008-02-22 at 10:57 -0500, John W. Linville wrote:
> >  > On Fri, Feb 22, 2008 at 04:16:49PM +0100, Johannes Berg wrote:
> >  > >
> >  > > > > Hence, I think we can actually get away without more locking if we
> >  > > > > protect the flags better. Should we use a spinlock or the atomic
> >  > > > > set_bit()/clear_bit()/etc. operations?
> >  > > >
> >  > > > Using the atomic operations seems appropriate to me.
> >  > >
> >  > > Right, but I figured if we could get rid of the AMPDU spinlocks and just
> >  > > use a single one in total (for flags as well) then that'd be of benefit
> >  > > too; even with the dynamic allocation strategy (see other mail) we'd not
> >  > > need to allocate two more spinlocks for ampdu.
> >  >
> >  > Yes, I thought that was behind your question.  I'll let Ron comment
> >  > on the AMPDU spinlock usage.
> >
> >  Ok so the mesh code came with a spinlock too, the AMPDU code has two.
> >
> >  Ron/Tomas, does the ampdu MLME really need two spinlocks?
> 
> The problem is that RX a TX BA establishments usually happens
> concurrently for single STA, those are independent state machines.  We
> have to review this carefully before spinlokcs are removed.

I wouldn't want to remove them, but it seems overkill two have two
spinlocks. What I'd like to do is remove the ampdu tx, ampdu rx and mesh
spinlocks and instead use just a single one for all three uses, and then
use that one to protect the flags as well.

It seems to me that even if the RX/TX BA establishment is happening
concurrently then it's still serialized by the frames on the air so
ultimately can't actually happen on different CPUs so the lock won't be
contended.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: mac80211: sta info locking

[Tomas Winkler]

On Tue, Feb 26, 2008 at 12:55 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>
>  On Tue, 2008-02-26 at 00:50 +0200, Tomas Winkler wrote:
>  > On Mon, Feb 25, 2008 at 10:46 PM, Johannes Berg
>  > <johannes@sipsolutions.net> wrote:
>  > >
>  > >  On Fri, 2008-02-22 at 10:57 -0500, John W. Linville wrote:
>  > >  > On Fri, Feb 22, 2008 at 04:16:49PM +0100, Johannes Berg wrote:
>  > >  > >
>  > >  > > > > Hence, I think we can actually get away without more locking if we
>  > >  > > > > protect the flags better. Should we use a spinlock or the atomic
>  > >  > > > > set_bit()/clear_bit()/etc. operations?
>  > >  > > >
>  > >  > > > Using the atomic operations seems appropriate to me.
>  > >  > >
>  > >  > > Right, but I figured if we could get rid of the AMPDU spinlocks and just
>  > >  > > use a single one in total (for flags as well) then that'd be of benefit
>  > >  > > too; even with the dynamic allocation strategy (see other mail) we'd not
>  > >  > > need to allocate two more spinlocks for ampdu.
>  > >  >
>  > >  > Yes, I thought that was behind your question.  I'll let Ron comment
>  > >  > on the AMPDU spinlock usage.
>  > >
>  > >  Ok so the mesh code came with a spinlock too, the AMPDU code has two.
>  > >
>  > >  Ron/Tomas, does the ampdu MLME really need two spinlocks?
>  >
>  > The problem is that RX a TX BA establishments usually happens
>  > concurrently for single STA, those are independent state machines.  We
>  > have to review this carefully before spinlokcs are removed.
>
>  I wouldn't want to remove them, but it seems overkill two have two
>  spinlocks. What I'd like to do is remove the ampdu tx, ampdu rx and mesh
>  spinlocks and instead use just a single one for all three uses, and then
>  use that one to protect the flags as well.
>
>  It seems to me that even if the RX/TX BA establishment is happening
>  concurrently then it's still serialized by the frames on the air so
>  ultimately can't actually happen on different CPUs so the lock won't be
>  contended.

I'm not sure about this, this is quite gentle and requires heavy
testing.  I'll try to review this with Ron tomorrow and all other
changes you've suggested.

Do you have any setup to check your these changes?

Currently there is known possible deadlock when unloading driver while
heavy HT traffic. This is happening while treering down BA session. I
need to solve this as well.


Thanks
Tomas

Re: mac80211: sta info locking

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]


> >  > The problem is that RX a TX BA establishments usually happens
> >  > concurrently for single STA, those are independent state machines.  We
> >  > have to review this carefully before spinlokcs are removed.
> >
> >  I wouldn't want to remove them, but it seems overkill two have two
> >  spinlocks. What I'd like to do is remove the ampdu tx, ampdu rx and mesh
> >  spinlocks and instead use just a single one for all three uses, and then
> >  use that one to protect the flags as well.
> >
> >  It seems to me that even if the RX/TX BA establishment is happening
> >  concurrently then it's still serialized by the frames on the air so
> >  ultimately can't actually happen on different CPUs so the lock won't be
> >  contended.
> 
> I'm not sure about this, this is quite gentle and requires heavy
> testing.  I'll try to review this with Ron tomorrow and all other
> changes you've suggested.

Thanks. Yes, I do realize that this is a bit intrusive. On the other
hand, I can't see the code holding the spinlock for longer code/periods
of time (nor should it). Or do you think the driver is allowed to call
into the AMPDU TX setup path while being called from the AMPDU RX setup
path? That would be a bit weird.

> Do you have any setup to check your these changes?

Unfortunately not quite yet. I have an 11n AP now though, but broke of
one of the antennas, I'll need to see how well it works.

> Currently there is known possible deadlock when unloading driver while
> heavy HT traffic. This is happening while treering down BA session. I
> need to solve this as well.

Interesting, where does it deadlock?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: mac80211: sta info locking

[Tomas Winkler]

On Tue, Feb 26, 2008 at 1:23 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>  > >  > The problem is that RX a TX BA establishments usually happens
>  > >  > concurrently for single STA, those are independent state machines.  We
>  > >  > have to review this carefully before spinlokcs are removed.
>  > >
>  > >  I wouldn't want to remove them, but it seems overkill two have two
>  > >  spinlocks. What I'd like to do is remove the ampdu tx, ampdu rx and mesh
>  > >  spinlocks and instead use just a single one for all three uses, and then
>  > >  use that one to protect the flags as well.
>  > >
>  > >  It seems to me that even if the RX/TX BA establishment is happening
>  > >  concurrently then it's still serialized by the frames on the air so
>  > >  ultimately can't actually happen on different CPUs so the lock won't be
>  > >  contended.
>  >
>  > I'm not sure about this, this is quite gentle and requires heavy
>  > testing.  I'll try to review this with Ron tomorrow and all other
>  > changes you've suggested.
>
>  Thanks. Yes, I do realize that this is a bit intrusive. On the other
>  hand, I can't see the code holding the spinlock for longer code/periods
>  of time (nor should it). Or do you think the driver is allowed to call
>  into the AMPDU TX setup path while being called from the AMPDU RX setup
>  path? That would be a bit weird.
>

TX and RX BA sessions are triggered from different threads of
execution namely rate scaling and RX path. The protected sections are
quite long IMHO including call outs to underlying driver. It is very
likely that both sides decides for BA session at the same times.

>  > Do you have any setup to check your these changes?
>
>  Unfortunately not quite yet. I have an 11n AP now though, but broke of
>  one of the antennas, I'll need to see how well it works.

That's bad 11n is too sensitive on antennas setup.

>
>  > Currently there is known possible deadlock when unloading driver while
>  > heavy HT traffic. This is happening while trearing down BA session. I
>  > need to solve this as well.
>
>  Interesting, where does it deadlock?

IIRC there is nice loop including queue lock need to dig the
description tomorrow to satisfy your curiosity.


Tomas

Re: mac80211: sta info locking

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1760 bytes --]


> >  Thanks. Yes, I do realize that this is a bit intrusive. On the other
> >  hand, I can't see the code holding the spinlock for longer code/periods
> >  of time (nor should it). Or do you think the driver is allowed to call
> >  into the AMPDU TX setup path while being called from the AMPDU RX setup
> >  path? That would be a bit weird.
> >
> 
> TX and RX BA sessions are triggered from different threads of
> execution namely rate scaling and RX path. The protected sections are
> quite long IMHO including call outs to underlying driver. It is very
> likely that both sides decides for BA session at the same times.

Yes, but I still can't see how the driver would be calling both RX and
TX aggregation functions /within each other/. That's the only thing that
really matters since all the frame TX/RX is synchronized and thus the
spinlock can't really be contended. Anyway, I need to look deeper into
the code or just try or something.

> >  > Do you have any setup to check your these changes?
> >
> >  Unfortunately not quite yet. I have an 11n AP now though, but broke of
> >  one of the antennas, I'll need to see how well it works.
> 
> That's bad 11n is too sensitive on antennas setup.

Yeah, I know. I don't know why my router has three antennas though
knowing that the Broadcom HW only has two chains.

> >  > Currently there is known possible deadlock when unloading driver while
> >  > heavy HT traffic. This is happening while trearing down BA session. I
> >  > need to solve this as well.
> >
> >  Interesting, where does it deadlock?
> 
> IIRC there is nice loop including queue lock need to dig the
> description tomorrow to satisfy your curiosity.

Not too important so don't bother.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: mac80211: sta info locking

[Tomas Winkler]

On Tue, Feb 26, 2008 at 1:48 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>  > >  Thanks. Yes, I do realize that this is a bit intrusive. On the other
>  > >  hand, I can't see the code holding the spinlock for longer code/periods
>  > >  of time (nor should it). Or do you think the driver is allowed to call
>  > >  into the AMPDU TX setup path while being called from the AMPDU RX setup
>  > >  path? That would be a bit weird.
>  > >
>  >
>  > TX and RX BA sessions are triggered from different threads of
>  > execution namely rate scaling and RX path. The protected sections are
>  > quite long IMHO including call outs to underlying driver. It is very
>  > likely that both sides decides for BA session at the same times.
>
>  Yes, but I still can't see how the driver would be calling both RX and
>  TX aggregation functions /within each other/. That's the only thing that
>  really matters since all the frame TX/RX is synchronized and thus the
>  spinlock can't really be contended. Anyway, I need to look deeper into
>  the code or just try or something.
>
>
>  > >  > Do you have any setup to check your these changes?
>  > >
>  > >  Unfortunately not quite yet. I have an 11n AP now though, but broke of
>  > >  one of the antennas, I'll need to see how well it works.
>  >
>  > That's bad 11n is too sensitive on antennas setup.
>
>  Yeah, I know. I don't know why my router has three antennas though
>  knowing that the Broadcom HW only has two chains.

Number of antennas is not the same as number of chains.  Also 4965 has
3 antennas but only 2 chains.   We have some fixes in that context
coming this week.
Tomas

Re: mac80211: sta info locking

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 314 bytes --]


> Number of antennas is not the same as number of chains.  Also 4965 has
> 3 antennas but only 2 chains.   We have some fixes in that context
> coming this week.

Hah, ok, I thought it was the same. Does it do diversity then? I guess I
need to catch up and finally actually read the 11n draft.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]