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

On 28 February 2014 13:17, Luca Coelho [off-list ref] wrote:

quoted

On Thu, 2014-02-27 at 16:29 +0100, Michal Kazior wrote:

quoted

On 27 February 2014 15:41, Luca Coelho [off-list ref] wrote:

quoted

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.

quoted

quoted

quoted

@@ -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.