On Fri, 2014-02-28 at 13:56 +0100, Michal Kazior wrote:
> On 28 February 2014 13:17, Luca Coelho <luca@xxxxxxxxx> wrote:
> > On Thu, 2014-02-27 at 16:29 +0100, Michal Kazior wrote:
> >> On 27 February 2014 15:41, Luca Coelho <luca@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
