mac80211 locking: tasklet vs. non-tasklet

mac80211 locking: tasklet vs. non-tasklet

[Johannes Berg]

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

Hi,

Looking through the locking in sta_info I noticed it was completely
broken especially wrt STA flag updates. However, most other things are
fine iff the drivers only use EITHER ieee80211_rx_irqsafe and
ieee80211_tx_status_irqsafe OR __ieee80211_rx and ieee80211_tx_status.
Also, in the latter case, drivers have to make sure that only one of
each of those calls is active at the same time, even on an SMP system.

The only drivers that currently use the non-irqsafe versions are ath5k
and iwlwifi, where iwlwifi even mixes between the two groups. ath5k
seems fine, it defers both to tasklets so only one call can be done at a
time.

With iwlwifi, however, there is a possibility that it invokes
ieee80211_tx_status() on one CPU while mac80211's tasklet is processing
another TX status that was submitted with ieee80211_tx_status_irqsafe().
Tomas, I think you mentioned that the TX status processing can't
actually ever call the non-irqsafe version, can we remove that call to
be sure? :)

Alternatively, short of imposing these requirements, we can add new
locking in mac80211. I don't think that would be good though.

johannes

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

Re: mac80211 locking: tasklet vs. non-tasklet

[Tomas Winkler]

On Fri, Feb 22, 2008 at 12:29 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Hi,
>
>  Looking through the locking in sta_info I noticed it was completely
>  broken especially wrt STA flag updates. However, most other things are
>  fine iff the drivers only use EITHER ieee80211_rx_irqsafe and
>  ieee80211_tx_status_irqsafe OR __ieee80211_rx and ieee80211_tx_status.
>  Also, in the latter case, drivers have to make sure that only one of
>  each of those calls is active at the same time, even on an SMP system.
>
>  The only drivers that currently use the non-irqsafe versions are ath5k
>  and iwlwifi, where iwlwifi even mixes between the two groups. ath5k
>  seems fine, it defers both to tasklets so only one call can be done at a
>  time.
>
>  With iwlwifi, however, there is a possibility that it invokes
>  ieee80211_tx_status() on one CPU while mac80211's tasklet is processing
>  another TX status that was submitted with ieee80211_tx_status_irqsafe().
>  Tomas, I think you mentioned that the TX status processing can't
>  actually ever call the non-irqsafe version, can we remove that call to
>  be sure? :)

Actually alike athk also iwlwifi driver calls tx_status from a tasklet
therfore the irqsafe can be removed. I've tested that it worked so far
However we added start_tx_ba_cb(_irqsafe) callback to get around AMPDU
queues reordering
In this case we have to use both handlers so I wonder where we have
problem here as the same mechanism as tx_status is used.

In general I'm missing some asynchronous mechanism that driver can
notify mac80211 about it's state.
Except BA states there  is  for example  netif_carrier_off/on
functionality in the driver level. It would be very useful for early
notification of disconnection.  A disconnection can happen due to
device resume or internal recoverable error. In this cases I would
expect mac to try associate again upon such trigger.

Thanks
Tomas



Thanks


>  Alternatively, short of imposing these requirements, we can add new
>  locking in mac80211. I don't think that would be good though.
>
>  johannes
>

Re: mac80211 locking: tasklet vs. non-tasklet

[Johannes Berg]

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


> >  With iwlwifi, however, there is a possibility that it invokes
> >  ieee80211_tx_status() on one CPU while mac80211's tasklet is processing
> >  another TX status that was submitted with ieee80211_tx_status_irqsafe().
> >  Tomas, I think you mentioned that the TX status processing can't
> >  actually ever call the non-irqsafe version, can we remove that call to
> >  be sure? :)
> 
> Actually alike athk also iwlwifi driver calls tx_status from a tasklet
> therfore the irqsafe can be removed. I've tested that it worked so far

Ok, fine too. Just the mixing is actually bad because of locking.

> However we added start_tx_ba_cb(_irqsafe) callback to get around AMPDU
> queues reordering
> In this case we have to use both handlers so I wonder where we have
> problem here as the same mechanism as tx_status is used.

I think the AMPDU stuff has its own spinlock around the fields it
accesses in those things. The thing with the tx status etc. is that it
uses no locking to update a few sta_info fields. If we added locking
around those accesses, mixing the two irqsafe/non-irqsafe versions would
be acceptable, but I don't see much point in that.

> In general I'm missing some asynchronous mechanism that driver can
> notify mac80211 about it's state.
> Except BA states there  is  for example  netif_carrier_off/on
> functionality in the driver level. It would be very useful for early
> notification of disconnection.  A disconnection can happen due to
> device resume or internal recoverable error. In this cases I would
> expect mac to try associate again upon such trigger.

I guess that's just missing. You can stop the queues but you can't tell
mac80211 that you reset the hw.

johannes

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

Re: mac80211 locking: tasklet vs. non-tasklet

[Tomas Winkler]

On Mon, Feb 25, 2008 at 11:51 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>  > >  With iwlwifi, however, there is a possibility that it invokes
>  > >  ieee80211_tx_status() on one CPU while mac80211's tasklet is processing
>  > >  another TX status that was submitted with ieee80211_tx_status_irqsafe().
>  > >  Tomas, I think you mentioned that the TX status processing can't
>  > >  actually ever call the non-irqsafe version, can we remove that call to
>  > >  be sure? :)
>  >
>  > Actually alike athk also iwlwifi driver calls tx_status from a tasklet
>  > therfore the irqsafe can be removed. I've tested that it worked so far
>
>  Ok, fine too. Just the mixing is actually bad because of locking.

There'll be a patch for it.

>
>  > However we added start_tx_ba_cb(_irqsafe) callback to get around AMPDU
>  > queues reordering
>  > In this case we have to use both handlers so I wonder where we have
>  > problem here as the same mechanism as tx_status is used.
>
>  I think the AMPDU stuff has its own spinlock around the fields it
>  accesses in those things. The thing with the tx status etc. is that it
>  uses no locking to update a few sta_info fields. If we added locking
>  around those accesses, mixing the two irqsafe/non-irqsafe versions would
>  be acceptable, but I don't see much point in that.

Great.

>
>  > In general I'm missing some asynchronous mechanism that driver can
>  > notify mac80211 about it's state.
>  > Except BA states there  is  for example  netif_carrier_off/on
>  > functionality in the driver level. It would be very useful for early
>  > notification of disconnection.  A disconnection can happen due to
>  > device resume or internal recoverable error. In this cases I would
>  > expect mac to try associate again upon such trigger.
>
>  I guess that's just missing. You can stop the queues but you can't tell
>  mac80211 that you reset the hw.

If there is no objection we will introduce some notification
mechanism...suggestions are welcome as well.

Thanks
Tomas

>  johannes
>

Re: mac80211 locking: tasklet vs. non-tasklet

[Johannes Berg]

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


> >  > Actually alike athk also iwlwifi driver calls tx_status from a tasklet
> >  > therfore the irqsafe can be removed. I've tested that it worked so far
> >
> >  Ok, fine too. Just the mixing is actually bad because of locking.
> 
> There'll be a patch for it.

Great, thanks. I'll be reposting my sta info rework today and that will
then add documentation about that too.

> >  > In general I'm missing some asynchronous mechanism that driver can
> >  > notify mac80211 about it's state.
> >  > Except BA states there  is  for example  netif_carrier_off/on
> >  > functionality in the driver level. It would be very useful for early
> >  > notification of disconnection.  A disconnection can happen due to
> >  > device resume or internal recoverable error. In this cases I would
> >  > expect mac to try associate again upon such trigger.
> >
> >  I guess that's just missing. You can stop the queues but you can't tell
> >  mac80211 that you reset the hw.
> 
> If there is no objection we will introduce some notification
> mechanism...suggestions are welcome as well.

I have no objection, though I'm unsure what precisely you want. What,
for example, if you're running hostapd and your hardware resets? Can you
keep station tables etc.?

johannes

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

Re: mac80211 locking: tasklet vs. non-tasklet

[Johannes Berg]

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

> > >  > A disconnection can happen due to
> > >  > device resume or internal recoverable error. In this cases I would
> > >  > expect mac to try associate again upon such trigger.
> > >
> > >  I guess that's just missing. You can stop the queues but you can't tell
> > >  mac80211 that you reset the hw.
> > 
> > If there is no objection we will introduce some notification
> > mechanism...suggestions are welcome as well.
> 
> I have no objection, though I'm unsure what precisely you want. What,
> for example, if you're running hostapd and your hardware resets? Can you
> keep station tables etc.?

I mean, if the hw simply resets quickly then I would expect the
association to withstand that, no?

johannes

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

Re: mac80211 locking: tasklet vs. non-tasklet

[Tomas Winkler]

On Mon, Feb 25, 2008 at 1:00 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> > > >  > A disconnection can happen due to
>  > > >  > device resume or internal recoverable error. In this cases I would
>  > > >  > expect mac to try associate again upon such trigger.
>  > > >
>  > > >  I guess that's just missing. You can stop the queues but you can't tell
>  > > >  mac80211 that you reset the hw.
>  > >
>  > > If there is no objection we will introduce some notification
>  > > mechanism...suggestions are welcome as well.
>  >
>  > I have no objection, though I'm unsure what precisely you want. What,
>  > for example, if you're running hostapd and your hardware resets? Can you
>  > keep station tables etc.?
>
>  I mean, if the hw simply resets quickly then I would expect the
>  association to withstand that, no?

I would notify mac only if driver cannot deal with reset and also
mac80211 configuration have to be reapplied such as start association
process again in STA mode.   It would be probably security problem if
we put just station into associated mode without proper association
process.
If in AP mode  stations have only soft state that it would be probably
not necessary to reset station table.

Tomas

>  johannes
>

Re: mac80211 locking: tasklet vs. non-tasklet

[Johannes Berg]

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


> I would notify mac only if driver cannot deal with reset and also
> mac80211 configuration have to be reapplied such as start association
> process again in STA mode.   It would be probably security problem if
> we put just station into associated mode without proper association
> process.

Hm, guess it could be. I think this will require quite some work to
completely put the mac80211 state into the hardware again, but this
would also be great for suspend/hibernate since that really is a similar
case.

johannes

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