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

[Luca Coelho <luca@coelho.fi>]

On Fri, 2014-02-28 at 13:56 +0100, Michal Kazior wrote:
> On 28 February 2014 13:17, Luca Coelho <luca@coelho.fi> wrote:
> > On Thu, 2014-02-27 at 16:29 +0100, Michal Kazior wrote:
> >> On 27 February 2014 15:41, Luca Coelho <luca@coelho.fi> wrote:
> >> > Introduce IEEE80211_CHANCTX_RESERVED chanctx mode to mark a channel as
> >> > reserved so nobody else can use it (since we know it's going to
> >> > change).  In the future, we may allow several vifs to use the same
> >> > reservation as long as they plan to use the chanctx on the same
> >> > future channel.
> >>
> >> I don't really think you need a separate mode for that.
> >>
> >> Since reserved_chanctx is protected by chanctx_mtx you can safely
> >> iterate over interfaces and check if any vif is reserving the same
> >> chanctx it is assigned to.
> >
> > I think it's much simpler to keep this new mode.  Reserved channel
> > contexts are almost like exclusive contexts (as I was doing in my first
> > RFC), but not exactly the same, since they can be used for other
> > reservations.
> 
> I still argue the new mode is unnecessary. The nature of chanctx is
> not going to change (it's either shared or not) due to chanctx
> reservation. Also the name "reserved" is ambiguous because you have a
> ieee80211_vif_reserve_chanctx() which doesn't necessarily end up with
> chanctx mode being changed to RESERVED.

Right, I agree that the name "reserved" is not very good.


> The check is simply I have in mind is simply:
> 
> bool ieee80211_chanctx_needs_channel_change(struct ieee80211_local
> *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
> both current and future chanctx the same, then the chanctx requires
> 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
needed in other places too, for example in the combinations check.  We
don't want to allow a new interface to join a chanctx that is going to
change.  In my merge between the combination check series and this one,
I have this: http://pastebin.coelho.fi/65603f9d06b28cb2.txt

If I'd use the iteration function there would be a lot of iterations
going on.  Not sure that's a problem though.

The advantages of your approach is that we need less moving parts (ie.
less stuff to save in sdata).  The advantage of using a new mode is that
it would require less code to run.


> >> > @@ -622,7 +629,9 @@ int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata)
> >> >         if (WARN_ON(!sdata->reserved_chanctx))
> >> >                 return -EINVAL;
> >> >
> >> > -       if (--sdata->reserved_chanctx->refcount == 0)
> >> > +       if (sdata->reserved_chanctx->mode == IEEE80211_CHANCTX_RESERVED)
> >> > +               sdata->reserved_chanctx->mode = sdata->reserved_mode;
> >> > +       else if (--sdata->reserved_chanctx->refcount == 0)
> >> >                 ieee80211_free_chanctx(sdata->local, sdata->reserved_chanctx);
> >> >
> >> >         sdata->reserved_chanctx = NULL;
> >> > @@ -652,19 +661,42 @@ int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
> >> >         /* try to find another context with the chandef we want */
> >> >         new_ctx = ieee80211_find_chanctx(local, chandef,
> >> >                                          IEEE80211_CHANCTX_SHARED);
> >> > -       if (!new_ctx) {
> >> > -               /* create a new context */
> >> > +       if (new_ctx) {
> >> > +               /* reserve the existing compatible context */
> >> > +               sdata->reserved_chanctx = new_ctx;
> >> > +               new_ctx->refcount++;
> >> > +       } else if (curr_ctx->refcount == 1 &&
> >> > +                  (local->hw.flags & IEEE80211_HW_CHANGE_RUNNING_CHANCTX)) {
> >> > +               /* TODO: when implementing support for multiple
> >> > +                * interfaces switching at the same time, we may want
> >> > +                * other vifs to reserve it as well, as long as
> >> > +                * they're planning to switch to the same channel.  In
> >> > +                * that case, we probably have to save the future
> >> > +                * chandef and the reserved_mode in the context
> >> > +                * itself.
> >> > +                */
> >>
> >> We already save the future chandef (csa_chandef). reserved_mode is not
> >> necessary as per my comment above. Again, if you guarantee csa_chandef
> >> to be set under chanctx_mtx you can safely iterate over interfaces and
> >> calculate compat chandef.
> >
> > But the calculated "compat chandef" is not exactly what was required in
> > the first place.  In sdata->u.bss_conf.chandef we need to have the
> > chandef we want for *this* vif.  We need this to recalculate the
> > combined chandef if, for instance, another vif leaves our chanctx.
> >
> > I think we should keep saving the reserved_chandef in sdata (the one
> > that was requested when making the reservation) and also save the future
> > chandef as a compat combination of all the reservations for that
> > chanctx.
> >
> > You're right that we already have the future chandef.  I just added it
> > as "reserved_chandef" in the previous patch. ;) I'll reword this.
> 
> I'm confused now.
> 
> Where did you introduce "reserved_chandef"? Didn't you introduce
> "reserved_chanCTX"?

See v3. :) It was my wrong choice of words, I should have said "I will
add reserved_chandef in the next version of 2/4".


> To make this clear: the future chanctx chandef can be computed as follows:
> 
> get_compat_future_chanctx_chandef(local, ctx) {
>   list_for_each(sdata, local) {
>     if (sdata->reserved_chanctx != ctx)
>       continue;
>     compat = get_compat_chandef(compat, sdata->csa_chandef);
>     if (!compat) break;
>   }
>   return compat;
> }
> 
> IOW there's no need for chanctx->future_chandef.

I see.  Again, it's a trade-off between calculating or saving it.


>  This is actually
> safer because if you cancel a reservation (e.g. iface is brought down)
> you need to downgrade the future chanctx chandef to the minimum, don't
> you?

Right, whenever we add or remove a reservation for the context, we need
to recalculate.  But we can still do it if we save the future_chandef,
because we have the "reserved_chandef" per sdata (that I introduced in
my v3).

I don't know, I actually don't mind which approach we use.  Saving or
iterating?

Preferences anyone? Johannes?

--
Luca.