Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

From: Luca Coelho <hidden>
Date: 2014-02-28 15:32:14

On February 28, 2014 4:55:16 PM EET, Michal Kazior [off-list ref] wrote:

On 28 February 2014 15:32, Luca Coelho [off-list ref] wrote:

quoted

On Fri, 2014-02-28 at 15:07 +0100, Michal Kazior wrote:

quoted

On 28 February 2014 14:41, Luca Coelho [off-list ref] wrote:

quoted

On Fri, 2014-02-28 at 13:56 +0100, Michal Kazior wrote:

quoted

The check is simply I have in mind is simply:

bool ieee80211_chanctx_needs_channel_change(struct

ieee80211_local

quoted

*local, struct ieee80211_chanctx *ctx) {
 lockdep_assert_held(&local->chanctx_mtx);
 rcu_read_lock();
 list_for_each_entry_rcu(sdata, &local->interfaces, list) {
  if (sdata->reserved_chanctx != ctx)
   continue;
  if (get_current_chanctx(sdata) == sdata->reserved_chanctx)
   return true;
 }
 rcu_read_unlock();
 return false;
}

IOW if there's a least one vif bound to given chanctx and the vif

has

quoted

both current and future chanctx the same, then the chanctx

requires

quoted

in-place channel change (and this matches your original condition
(mode == RESERVED)).

This should be future proof for multi-interface/channel.

Okay, I get your point, it's not strictly necessary.  But this

would be

quoted

needed in other places too, for example in the combinations check.

We

quoted

don't want to allow a new interface to join a chanctx that is

going to

quoted

change.  In my merge between the combination check series and this

one,

quoted

I have this: http://pastebin.coelho.fi/65603f9d06b28cb2.txt

Hmm.. Good point, but the snippet doesn't prevent new vifs from
joining a chanctx that's going to change channel.

No, the prevention actually happens in ieee80211_find_chanctx() and

it's

quoted

already part of this series.  I just wanted to point out that this is
needed in several different places.

If you consider multi-vif I don't think doing that in find_chanctx()
is the best thing. If you skip reserved chanctx in find_chanctx() then
you can't use this function when you reserve chanctx that needs
channel change with more than 1 vif.

We would obviously have to change this function for the multi-vif-changing-together case. But it would just be a matter of checking if the desired future changef is compatible with the reserved one.

quoted

I'm also not quite sure if you need it in the combo check at all.
Can't you just throw EBUSY when you try to assign a new vif to

chanctx

quoted

that's going to change channel?

A reserved channel context is taking up space, so new interfaces

can't

quoted

rely on being able to use it.  Let's say a HW supports 1 chanctx and
there is one vif.  Now the vif wants to change channel and reserves

its

quoted

chanctx to be changed later.  If a new vif needs a chanctx, it can't

use

quoted

the one that is reserved, because it will be changed in the future.

So,

quoted

during the combination checks, we need to calculate the number of

needed

quoted

chanctxs for the new vif to be added is 2, so it should fail.

..but returning EBUSY in assign_vif clearly fulfils this requirement.
And I'm still not convinced you need to take "reserved" chanctx into
any consideration during combination checks.

There are many places where we could fail. I'd rather keep all the checks for chanctx availability in one place (ie. in the combination checks). But this discussion is not strictly related to the RESERVED stuff.

quoted

For multi-channel hw you could allow creating new chanctx (if

there's

quoted

enough channels in current combination) and make 2 chanctx that will
be compatible in the future (and worry about merging them later), or
you could deny that until reservation is finalized.

Yes, if you have enough chanctxs to use, it's not a problem.  But

during

quoted

the count we need to consider the ones that will be changed (and are
thus marked as RESERVED) almost as an EXCLUSIVE chanctx, so they are
counted separately.  The difference between EXCLUSIVE and RESERVED,

is

quoted

only that a RESERVED chanctx can have more than one vif (as long as

all

quoted

of them have compatible future chandefs).

I think it's just an obfuscation. Either way "reserved" is an
orthogonal state to the original mode. You just want to keep it in the
"old_mode" and have some [mode, old_mode] combinations invalid leaving
just 4 states that can be actually set.

This is a very good point. It would have been better to add a separate boolean.

quoted

If I'd use the iteration function there would be a lot of

iterations

quoted

going on.  Not sure that's a problem though.

The advantages of your approach is that we need less moving parts

(ie.

quoted

less stuff to save in sdata).  The advantage of using a new mode

is that

quoted

it would require less code to run.

I'd rather not have to worry about memoizing variables and
recalculating them when it's not strictly necessary (this isn't tx
path). In both cases you have to worry about locking which I think

is

quoted

enough.

As I said, I don't have a preference.  Except that, being lazy, I'd
prefer not to change what I already did. :P

For what I care I can just make follow up patches that rework some of
the bits I'm complaining about now that I think are necessary for
multi-vif channel switching as long as Johannes is okay with that
(i.e. merge your current code and then accept my re-work).

Good to see that you're not lazy. :P

I'm not in
any place to force you to do my bidding ;-)

That's true, but it's far from meaning I won't hear you. ;)

But seriously, I'm almost fully convinced your approach is better. I'll try to spin without the RESERVED mode.

But that probably will only happened Monday, because I'm already spinning down for the weekend.

--
Luca.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help