| Message ID | 1393512081-31453-4-git-send-email-luca@coelho.fi (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 27 February 2014 15:41, Luca Coelho <luca@coelho.fi> wrote: > From: Luciano Coelho <luciano.coelho@intel.com> > > With single-channel drivers, we need to be able to change a running > chanctx if we want to use chanctx reservation. Not all drivers may be > able to do this, so add a flag that indicates support for it. > > Changing a running chanctx can also be used as an optimization in > multi-channel drivers when the context needs to be reserved for future > usage. I think this can be generalized (not necessarily in this very patch). Since you've moved combination checks into mac80211 you can easily check how many channels you can have with current iftype setup. This means you can know beforehand if you can create a new chanctx or have to attempt a chanctx channel switch. > 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. > @@ -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. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-02-27 at 16:29 +0100, Michal Kazior wrote: > On 27 February 2014 15:41, Luca Coelho <luca@coelho.fi> wrote: > > From: Luciano Coelho <luciano.coelho@intel.com> > > > > With single-channel drivers, we need to be able to change a running > > chanctx if we want to use chanctx reservation. Not all drivers may be > > able to do this, so add a flag that indicates support for it. > > > > Changing a running chanctx can also be used as an optimization in > > multi-channel drivers when the context needs to be reserved for future > > usage. > > I think this can be generalized (not necessarily in this very patch). > Since you've moved combination checks into mac80211 you can easily > check how many channels you can have with current iftype setup. This > means you can know beforehand if you can create a new chanctx or have > to attempt a chanctx channel switch. That's the idea. I'm keeping both series separate, but when they get applied, I will use the combinations check for chanctx reservation. > > 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. > > @@ -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. Thanks a lot for your reviews and comments! -- Cheers, Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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. >> > @@ -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"? 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. 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? Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 February 2014 14:41, Luca Coelho <luca@coelho.fi> wrote: > On Fri, 2014-02-28 at 13:56 +0100, Michal Kazior wrote: >> 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 Hmm.. Good point, but the snippet doesn't prevent new vifs from joining a chanctx that's going to change channel. 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 that's going to change channel? For multi-channel hw you could allow creating new chanctx (if there's 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. > 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. 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 enough. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2014-02-28 at 15:07 +0100, Michal Kazior wrote: > On 28 February 2014 14:41, Luca Coelho <luca@coelho.fi> wrote: > > On Fri, 2014-02-28 at 13:56 +0100, Michal Kazior wrote: > >> 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 > > 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 already part of this series. I just wanted to point out that this is needed in several different places. > 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 > that's going to change channel? A reserved channel context is taking up space, so new interfaces can't 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 chanctx to be changed later. If a new vif needs a chanctx, it can't use the one that is reserved, because it will be changed in the future. So, during the combination checks, we need to calculate the number of needed chanctxs for the new vif to be added is 2, so it should fail. > For multi-channel hw you could allow creating new chanctx (if there's > 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 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 only that a RESERVED chanctx can have more than one vif (as long as all of them have compatible future chandefs). > > 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. > > 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 > 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 -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28 February 2014 15:32, Luca Coelho <luca@coelho.fi> wrote: > On Fri, 2014-02-28 at 15:07 +0100, Michal Kazior wrote: >> On 28 February 2014 14:41, Luca Coelho <luca@coelho.fi> wrote: >> > On Fri, 2014-02-28 at 13:56 +0100, Michal Kazior wrote: >> >> 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 >> >> 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 > 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. >> 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 >> that's going to change channel? > > A reserved channel context is taking up space, so new interfaces can't > 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 > chanctx to be changed later. If a new vif needs a chanctx, it can't use > the one that is reserved, because it will be changed in the future. So, > during the combination checks, we need to calculate the number of needed > 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. >> For multi-channel hw you could allow creating new chanctx (if there's >> 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 > 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 > only that a RESERVED chanctx can have more than one vif (as long as all > 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. >> > 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. >> >> 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 >> 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). I'm not in any place to force you to do my bidding ;-) Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On February 28, 2014 4:55:16 PM EET, Michal Kazior <michal.kazior@tieto.com> wrote: >On 28 February 2014 15:32, Luca Coelho <luca@coelho.fi> wrote: >> On Fri, 2014-02-28 at 15:07 +0100, Michal Kazior wrote: >>> On 28 February 2014 14:41, Luca Coelho <luca@coelho.fi> wrote: >>> > On Fri, 2014-02-28 at 13:56 +0100, Michal Kazior wrote: >>> >> 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 >>> >>> 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 >> 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. >>> 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 >>> that's going to change channel? >> >> A reserved channel context is taking up space, so new interfaces >can't >> 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 >> chanctx to be changed later. If a new vif needs a chanctx, it can't >use >> the one that is reserved, because it will be changed in the future. >So, >> during the combination checks, we need to calculate the number of >needed >> 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. > >>> For multi-channel hw you could allow creating new chanctx (if >there's >>> 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 >> 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 >> only that a RESERVED chanctx can have more than one vif (as long as >all >> 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. >>> > 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. >>> >>> 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 >>> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Micha?, I had a new idea. On Fri, 2014-02-28 at 17:31 +0200, Luca Coelho wrote: > 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. What about this: when we are reserving the chanctx, even if we're the only ones in it (and thus will change it on-the-fly), we increase the refcount. This means that we would have refcount == 2, even though we're the only users, but that's aligned with what happens when we reserve a different chanctx. When we use the reservation, we do exactly the same thing as if we were moving to a new chanctx, but add a intermediate step where we check if the old ctx has refcount 0, in which case we change its channel: Reserving our own chanctx for future change: 1. new_ctx = old_ctx; 2. increase new_ctx.refcount (new.refcount == 2, old.refcount == 2); Using the reservation: 1. unassign the vif from the old chanctx (old.refcount == 1, new.refcount == 1); 2. we decrease the refcount of the new chanctx (new.refcount == 0, old.refcount == 0); 3. if (old.refcount == 0) means we were the only user, change channel; 4. we assign ourselves to the new chanctx (new.refcount == 1 again); This would make this whole thing pretty generic with only one extra if for the on-the-fly chanctx change case. If more vifs came and are changing the chanctx at the same time, it will be fine too because the channel will only change when the last reserver uses the reservation. Does this make sense? -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-03-03 at 11:57 +0200, Luca Coelho wrote: > Hi Micha?, > > I had a new idea. > > > On Fri, 2014-02-28 at 17:31 +0200, Luca Coelho wrote: > > 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. > > What about this: when we are reserving the chanctx, even if we're the > only ones in it (and thus will change it on-the-fly), we increase the > refcount. This means that we would have refcount == 2, even though > we're the only users, but that's aligned with what happens when we > reserve a different chanctx. > > When we use the reservation, we do exactly the same thing as if we were > moving to a new chanctx, but add a intermediate step where we check if > the old ctx has refcount 0, in which case we change its channel: > > Reserving our own chanctx for future change: > > 1. new_ctx = old_ctx; > 2. increase new_ctx.refcount (new.refcount == 2, old.refcount == 2); > > > Using the reservation: > > 1. unassign the vif from the old chanctx (old.refcount == 1, > new.refcount == 1); > 2. we decrease the refcount of the new chanctx (new.refcount == 0, > old.refcount == 0); > 3. if (old.refcount == 0) means we were the only user, change channel; > 4. we assign ourselves to the new chanctx (new.refcount == 1 again); > > > This would make this whole thing pretty generic with only one extra if > for the on-the-fly chanctx change case. > > If more vifs came and are changing the chanctx at the same time, it will > be fine too because the channel will only change when the last reserver > uses the reservation. > > Does this make sense? Oh wait, this last part won't work with multiple vifs changing at the same time, because in that case the refcount will never be 0. I guess instead of checking if old.refcount == 0, we should check if the old_ctx and new_ctx are identical. The first vif to get here will change the channel and the other ones will NOP because the channel they want is already the current channel of the chanctx. Let me try to do this. -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 March 2014 10:57, Luca Coelho <luca@coelho.fi> wrote: > Hi Micha?, > > I had a new idea. > > > On Fri, 2014-02-28 at 17:31 +0200, Luca Coelho wrote: >> 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. > > What about this: when we are reserving the chanctx, even if we're the > only ones in it (and thus will change it on-the-fly), we increase the > refcount. This means that we would have refcount == 2, even though > we're the only users, but that's aligned with what happens when we > reserve a different chanctx. > > When we use the reservation, we do exactly the same thing as if we were > moving to a new chanctx, but add a intermediate step where we check if > the old ctx has refcount 0, in which case we change its channel: > > Reserving our own chanctx for future change: > > 1. new_ctx = old_ctx; > 2. increase new_ctx.refcount (new.refcount == 2, old.refcount == 2); I was thinking of doing it like this. > Using the reservation: > > 1. unassign the vif from the old chanctx (old.refcount == 1, > new.refcount == 1); > 2. we decrease the refcount of the new chanctx (new.refcount == 0, > old.refcount == 0); > 3. if (old.refcount == 0) means we were the only user, change channel; > 4. we assign ourselves to the new chanctx (new.refcount == 1 again); I didn't really consider unassigning vif from chanctx like that (since the original single-channel channel non-chanctx hw doesn't do that). If you assume it is safe to unassign the vif then the logic seems plausible. I like it. > This would make this whole thing pretty generic with only one extra if > for the on-the-fly chanctx change case. I'd rather call it "in place" chanctx change case :) > If more vifs came and are changing the chanctx at the same time, it will > be fine too because the channel will only change when the last reserver > uses the reservation. > > Does this make sense? Does the following match your idea of multi-vif reservation with the refcount idea? [2 vifs on chanctx->refcount==2] * vif1 reserve (refcount==3) * vif2 reserve (refcount==4) * vif1 use reservation: (refcount==3) * stop queues * unassign vif (refcount==2) * since refcount!=0 do nothing more * vif3 use reservation: (refcount==1) * unassign vif (refcount==0) * since refcount==0 do: * chanctx channel change * vif1 assign (refcount==1) * vif2 assign (refcount==2) * wake queues Additionally you'd have to check after each "use reservation": if all vifs with matching reserved_chanctx have no assigned chanctx but the reserved_chanctx->refcount > 0, then disconnect all vifs with the matching reserved_chanctx. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-03-03 at 11:38 +0100, Michal Kazior wrote: > On 3 March 2014 10:57, Luca Coelho <luca@coelho.fi> wrote: > > Hi Micha?, > > > > I had a new idea. > > > > > > On Fri, 2014-02-28 at 17:31 +0200, Luca Coelho wrote: > >> 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. > > > > What about this: when we are reserving the chanctx, even if we're the > > only ones in it (and thus will change it on-the-fly), we increase the > > refcount. This means that we would have refcount == 2, even though > > we're the only users, but that's aligned with what happens when we > > reserve a different chanctx. > > > > When we use the reservation, we do exactly the same thing as if we were > > moving to a new chanctx, but add a intermediate step where we check if > > the old ctx has refcount 0, in which case we change its channel: > > > > Reserving our own chanctx for future change: > > > > 1. new_ctx = old_ctx; > > 2. increase new_ctx.refcount (new.refcount == 2, old.refcount == 2); > > I was thinking of doing it like this. Your previous comments have definitely influenced this idea. :) > > Using the reservation: > > > > 1. unassign the vif from the old chanctx (old.refcount == 1, > > new.refcount == 1); > > 2. we decrease the refcount of the new chanctx (new.refcount == 0, > > old.refcount == 0); > > 3. if (old.refcount == 0) means we were the only user, change channel; > > 4. we assign ourselves to the new chanctx (new.refcount == 1 again); > > I didn't really consider unassigning vif from chanctx like that (since > the original single-channel channel non-chanctx hw doesn't do that). > If you assume it is safe to unassign the vif then the logic seems > plausible. I like it. I think in theory it should be okay. I don't know if any driver does something funny with it, though. For drivers that don't implement the unassign_vif_chanctx op (like non-chanctx drivers), it should not be a problem. I think I could probably get rid of the IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag I was going to introduce as well. > > This would make this whole thing pretty generic with only one extra if > > for the on-the-fly chanctx change case. > > I'd rather call it "in place" chanctx change case :) Heheh, I didn't think about the term much and just assumed you'd understand what I meant. ;) > > If more vifs came and are changing the chanctx at the same time, it will > > be fine too because the channel will only change when the last reserver > > uses the reservation. > > > > Does this make sense? > > Does the following match your idea of multi-vif reservation with the > refcount idea? > > [2 vifs on chanctx->refcount==2] > * vif1 reserve (refcount==3) > * vif2 reserve (refcount==4) > * vif1 use reservation: (refcount==3) > * stop queues > * unassign vif (refcount==2) > * since refcount!=0 do nothing more > * vif3 use reservation: (refcount==1) > * unassign vif (refcount==0) > * since refcount==0 do: > * chanctx channel change > * vif1 assign (refcount==1) > * vif2 assign (refcount==2) > * wake queues Right, this is a good idea (better than what I wrote in my previous reply). I just need to iterate all the vifs and assign the ones with matching reserved_chanctx (and no assigned chanctx) to this chanctx when refcount reaches 0. > Additionally you'd have to check after each "use reservation": if all > vifs with matching reserved_chanctx have no assigned chanctx but the > reserved_chanctx->refcount > 0, then disconnect all vifs with the > matching reserved_chanctx. I'm not sure I understood this part. I think that when refcount reaches 0 I should disconnect the ones that are still using this chanctx, right? All the ones that wanted to switch together, will have already done so (since the refcount reached 0). If there's any remaining vif in the chanctx we want to change, disconnect them. -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 March 2014 13:37, Luca Coelho <luca@coelho.fi> wrote: > On Mon, 2014-03-03 at 11:38 +0100, Michal Kazior wrote: >> On 3 March 2014 10:57, Luca Coelho <luca@coelho.fi> wrote: >> > Using the reservation: >> > >> > 1. unassign the vif from the old chanctx (old.refcount == 1, >> > new.refcount == 1); >> > 2. we decrease the refcount of the new chanctx (new.refcount == 0, >> > old.refcount == 0); >> > 3. if (old.refcount == 0) means we were the only user, change channel; >> > 4. we assign ourselves to the new chanctx (new.refcount == 1 again); >> >> I didn't really consider unassigning vif from chanctx like that (since >> the original single-channel channel non-chanctx hw doesn't do that). >> If you assume it is safe to unassign the vif then the logic seems >> plausible. I like it. > > I think in theory it should be okay. I don't know if any driver does > something funny with it, though. > > For drivers that don't implement the unassign_vif_chanctx op (like > non-chanctx drivers), it should not be a problem. > > I think I could probably get rid of the > IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag I was going to introduce as > well. We could actually get rid of the CHANNEL_CHANGED for chanctx altogether. All it takes is "wait until refcount drops to 0, free chanctx, create new chanctx, start assigning vifs that had chanctx_reserved set". This would probably be seamless for non-chanctx drivers too. >> > If more vifs came and are changing the chanctx at the same time, it will >> > be fine too because the channel will only change when the last reserver >> > uses the reservation. >> > >> > Does this make sense? >> >> Does the following match your idea of multi-vif reservation with the >> refcount idea? >> >> [2 vifs on chanctx->refcount==2] >> * vif1 reserve (refcount==3) >> * vif2 reserve (refcount==4) >> * vif1 use reservation: (refcount==3) >> * stop queues >> * unassign vif (refcount==2) >> * since refcount!=0 do nothing more >> * vif3 use reservation: (refcount==1) >> * unassign vif (refcount==0) >> * since refcount==0 do: >> * chanctx channel change >> * vif1 assign (refcount==1) >> * vif2 assign (refcount==2) >> * wake queues > > Right, this is a good idea (better than what I wrote in my previous > reply). I just need to iterate all the vifs and assign the ones with > matching reserved_chanctx (and no assigned chanctx) to this chanctx when > refcount reaches 0. Yes. In theory you can even handle cases where ctx->refcount is still >0 if you have multi-channel hw. Consider the following: hw: 2 channels max ctx1 (chan 1): vif1 ctx2 (chan 2): vif2 vif3 * vif2 wants to switch to chan 3, but can't create new chanctx so use current one for "in place" hoping vif3 will also switch soon enough * vif1 decides it wants to follow vif2 to chan 3 * vif3 doesn't want to do anything * vif1 and vif2 both call use_reserved (order doesn't matter if you perform checks properly) * ctx1 should be freed now since vif1 has been unassigned * ctx2->refcount > 0, but num_chanctx < max_num_chanctx, so create new ctx1 for chan3 * assign vif1 and vif2 to ctx1 on chan 3 (this would depend on my ieee80211_max_num_channels() patch) >> Additionally you'd have to check after each "use reservation": if all >> vifs with matching reserved_chanctx have no assigned chanctx but the >> reserved_chanctx->refcount > 0, then disconnect all vifs with the >> matching reserved_chanctx. > > I'm not sure I understood this part. I think that when refcount reaches > 0 I should disconnect the ones that are still using this chanctx, right? > All the ones that wanted to switch together, will have already done so > (since the refcount reached 0). If there's any remaining vif in the > chanctx we want to change, disconnect them. There are two approaches here: a) first use_reservation implies actual channel change b) last use_reservation implies actual channel change You probably keep model (a) in mind, while I use (b). I prefer (b) for a couple of reasons: * it also allows to retain the current behavior easily: if you have many vifs use a chanctx and some of them want to CSA, then disconnect those who want CSA * it should be more resilient to timing because you wait on the channel until last vif is "done" * you can re-create chanctx and remove chanctx "channel change" Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-03-03 at 14:26 +0100, Michal Kazior wrote: > On 3 March 2014 13:37, Luca Coelho <luca@coelho.fi> wrote: > > On Mon, 2014-03-03 at 11:38 +0100, Michal Kazior wrote: > >> On 3 March 2014 10:57, Luca Coelho <luca@coelho.fi> wrote: > >> > Using the reservation: > >> > > >> > 1. unassign the vif from the old chanctx (old.refcount == 1, > >> > new.refcount == 1); > >> > 2. we decrease the refcount of the new chanctx (new.refcount == 0, > >> > old.refcount == 0); > >> > 3. if (old.refcount == 0) means we were the only user, change channel; > >> > 4. we assign ourselves to the new chanctx (new.refcount == 1 again); > >> > >> I didn't really consider unassigning vif from chanctx like that (since > >> the original single-channel channel non-chanctx hw doesn't do that). > >> If you assume it is safe to unassign the vif then the logic seems > >> plausible. I like it. > > > > I think in theory it should be okay. I don't know if any driver does > > something funny with it, though. > > > > For drivers that don't implement the unassign_vif_chanctx op (like > > non-chanctx drivers), it should not be a problem. > > > > I think I could probably get rid of the > > IEEE80211_HW_CHANGE_RUNNING_CHANCTX flag I was going to introduce as > > well. > > We could actually get rid of the CHANNEL_CHANGED for chanctx > altogether. All it takes is "wait until refcount drops to 0, free > chanctx, create new chanctx, start assigning vifs that had > chanctx_reserved set". This would probably be seamless for non-chanctx > drivers too. Yes. > >> > If more vifs came and are changing the chanctx at the same time, it will > >> > be fine too because the channel will only change when the last reserver > >> > uses the reservation. > >> > > >> > Does this make sense? > >> > >> Does the following match your idea of multi-vif reservation with the > >> refcount idea? > >> > >> [2 vifs on chanctx->refcount==2] > >> * vif1 reserve (refcount==3) > >> * vif2 reserve (refcount==4) > >> * vif1 use reservation: (refcount==3) > >> * stop queues > >> * unassign vif (refcount==2) > >> * since refcount!=0 do nothing more > >> * vif3 use reservation: (refcount==1) > >> * unassign vif (refcount==0) > >> * since refcount==0 do: > >> * chanctx channel change > >> * vif1 assign (refcount==1) > >> * vif2 assign (refcount==2) > >> * wake queues > > > > Right, this is a good idea (better than what I wrote in my previous > > reply). I just need to iterate all the vifs and assign the ones with > > matching reserved_chanctx (and no assigned chanctx) to this chanctx when > > refcount reaches 0. > > Yes. In theory you can even handle cases where ctx->refcount is still > >0 if you have multi-channel hw. Consider the following: > > hw: 2 channels max > ctx1 (chan 1): vif1 > ctx2 (chan 2): vif2 vif3 > > * vif2 wants to switch to chan 3, but can't create new chanctx so use > current one for "in place" hoping vif3 will also switch soon enough > * vif1 decides it wants to follow vif2 to chan 3 > * vif3 doesn't want to do anything > * vif1 and vif2 both call use_reserved (order doesn't matter if you > perform checks properly) > * ctx1 should be freed now since vif1 has been unassigned > * ctx2->refcount > 0, but num_chanctx < max_num_chanctx, so create new > ctx1 for chan3 > * assign vif1 and vif2 to ctx1 on chan 3 > > (this would depend on my ieee80211_max_num_channels() patch) Sounds good! But where do the channel combinations come in here? I think instead of just checking for max_num_channels we should do a full combination check. > >> Additionally you'd have to check after each "use reservation": if all > >> vifs with matching reserved_chanctx have no assigned chanctx but the > >> reserved_chanctx->refcount > 0, then disconnect all vifs with the > >> matching reserved_chanctx. > > > > I'm not sure I understood this part. I think that when refcount reaches > > 0 I should disconnect the ones that are still using this chanctx, right? > > All the ones that wanted to switch together, will have already done so > > (since the refcount reached 0). If there's any remaining vif in the > > chanctx we want to change, disconnect them. > > There are two approaches here: > a) first use_reservation implies actual channel change > b) last use_reservation implies actual channel change > > You probably keep model (a) in mind, while I use (b). Actually I was thinking about (b) as well. My question here is how do we discern vifs that are still in the channel but didn't want to change (and thus we should disconnect)? We can't rely on refcount. > I prefer (b) for a couple of reasons: > * it also allows to retain the current behavior easily: if you have > many vifs use a chanctx and some of them want to CSA, then disconnect > those who want CSA > * it should be more resilient to timing because you wait on the > channel until last vif is "done" > * you can re-create chanctx and remove chanctx "channel change" I agree, as long as we make sure they are in sync (ie. want to switch at the same time) when handling the channel switch request. -- Luca. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 March 2014 14:42, Luca Coelho <luca@coelho.fi> wrote: > On Mon, 2014-03-03 at 14:26 +0100, Michal Kazior wrote: >> On 3 March 2014 13:37, Luca Coelho <luca@coelho.fi> wrote: >> > On Mon, 2014-03-03 at 11:38 +0100, Michal Kazior wrote: >> >> On 3 March 2014 10:57, Luca Coelho <luca@coelho.fi> wrote: [...] >> >> > If more vifs came and are changing the chanctx at the same time, it will >> >> > be fine too because the channel will only change when the last reserver >> >> > uses the reservation. >> >> > >> >> > Does this make sense? >> >> >> >> Does the following match your idea of multi-vif reservation with the >> >> refcount idea? >> >> >> >> [2 vifs on chanctx->refcount==2] >> >> * vif1 reserve (refcount==3) >> >> * vif2 reserve (refcount==4) >> >> * vif1 use reservation: (refcount==3) >> >> * stop queues >> >> * unassign vif (refcount==2) >> >> * since refcount!=0 do nothing more >> >> * vif3 use reservation: (refcount==1) >> >> * unassign vif (refcount==0) >> >> * since refcount==0 do: >> >> * chanctx channel change >> >> * vif1 assign (refcount==1) >> >> * vif2 assign (refcount==2) >> >> * wake queues >> > >> > Right, this is a good idea (better than what I wrote in my previous >> > reply). I just need to iterate all the vifs and assign the ones with >> > matching reserved_chanctx (and no assigned chanctx) to this chanctx when >> > refcount reaches 0. >> >> Yes. In theory you can even handle cases where ctx->refcount is still >> >0 if you have multi-channel hw. Consider the following: >> >> hw: 2 channels max >> ctx1 (chan 1): vif1 >> ctx2 (chan 2): vif2 vif3 >> >> * vif2 wants to switch to chan 3, but can't create new chanctx so use >> current one for "in place" hoping vif3 will also switch soon enough >> * vif1 decides it wants to follow vif2 to chan 3 >> * vif3 doesn't want to do anything >> * vif1 and vif2 both call use_reserved (order doesn't matter if you >> perform checks properly) >> * ctx1 should be freed now since vif1 has been unassigned >> * ctx2->refcount > 0, but num_chanctx < max_num_chanctx, so create new >> ctx1 for chan3 >> * assign vif1 and vif2 to ctx1 on chan 3 >> >> (this would depend on my ieee80211_max_num_channels() patch) > > Sounds good! But where do the channel combinations come in here? I think > instead of just checking for max_num_channels we should do a full > combination check. The channel counting uses combination checks. It simply iterates over all *matching* ifcombs and picks the greatest max_num_channels. CSA doesn't change iftype and regular ifcomb checks (with your patch) is protected by chanctx_mtx, so it should be safe enough. >> >> Additionally you'd have to check after each "use reservation": if all >> >> vifs with matching reserved_chanctx have no assigned chanctx but the >> >> reserved_chanctx->refcount > 0, then disconnect all vifs with the >> >> matching reserved_chanctx. >> > >> > I'm not sure I understood this part. I think that when refcount reaches >> > 0 I should disconnect the ones that are still using this chanctx, right? >> > All the ones that wanted to switch together, will have already done so >> > (since the refcount reached 0). If there's any remaining vif in the >> > chanctx we want to change, disconnect them. >> >> There are two approaches here: >> a) first use_reservation implies actual channel change >> b) last use_reservation implies actual channel change >> >> You probably keep model (a) in mind, while I use (b). > > Actually I was thinking about (b) as well. > > My question here is how do we discern vifs that are still in the channel > but didn't want to change (and thus we should disconnect)? We can't rely > on refcount. They simply will have a NULL reserved_chanctx. The only thing missing (while including your patches) is a boolean per-sdata that will indicate whether use_reserved() was already called or not so you can check if all vifs with the same reserved_chanctx have "synchronized" and if that chanctx is suitable to use (i.e. refcount==0 when switching in-place, or it's possible to create a new chanctx which wasn't possible when original reservation was done). Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 86faa41..54c9ce7 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1553,6 +1553,12 @@ struct ieee80211_tx_control { * for a single active channel while using channel contexts. When support * is not enabled the default action is to disconnect when getting the * CSA frame. + * + * @IEEE80211_HW_CHANGE_RUNNING_CHANCTX: The hardware can change a + * channel context on-the-fly. This is needed for channel switch + * on single-channel hardware. It can also be used as an + * optimization in certain channel switch cases with + * multi-channel. */ enum ieee80211_hw_flags { IEEE80211_HW_HAS_RATE_CONTROL = 1<<0, @@ -1584,6 +1590,7 @@ enum ieee80211_hw_flags { IEEE80211_HW_TIMING_BEACON_ONLY = 1<<26, IEEE80211_HW_SUPPORTS_HT_CCK_RATES = 1<<27, IEEE80211_HW_CHANCTX_STA_CSA = 1<<28, + IEEE80211_HW_CHANGE_RUNNING_CHANCTX = 1<<29, }; /** diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c index bd42d17..564a495 100644 --- a/net/mac80211/chan.c +++ b/net/mac80211/chan.c @@ -177,6 +177,13 @@ ieee80211_find_chanctx(struct ieee80211_local *local, list_for_each_entry(ctx, &local->chanctx_list, list) { const struct cfg80211_chan_def *compat; + /* We don't support chanctx reservation for multiple + * vifs yet, so don't allow reserved chanctxs to be + * reused. + */ + if (ctx->mode == IEEE80211_CHANCTX_RESERVED) + continue; + if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE) continue; @@ -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. + */ + sdata->reserved_mode = curr_ctx->mode; + + /* We're the only user of the current context, mark it + * as reserved, so nobody tries to use it until we + * finish the channel switch. This is an optimization + * to prevent waste of contexts when the number is + * limited. + */ + curr_ctx->mode = IEEE80211_CHANCTX_RESERVED; + sdata->reserved_chanctx = curr_ctx; + } else { + /* create a new context and reserve it */ new_ctx = ieee80211_new_chanctx(local, chandef, IEEE80211_CHANCTX_SHARED); if (IS_ERR(new_ctx)) { ret = PTR_ERR(new_ctx); goto out; } - } + sdata->reserved_chanctx = new_ctx; - /* reserve the new or existing context */ - sdata->reserved_chanctx = new_ctx; - new_ctx->refcount++; + new_ctx->refcount++; + } sdata->csa_chandef = *chandef; out: @@ -702,6 +734,21 @@ int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata, old_ctx = container_of(conf, struct ieee80211_chanctx, conf); + if (old_ctx == ctx) { + WARN_ON(!ctx->mode != IEEE80211_CHANCTX_RESERVED); + + /* This is our own context, just change it */ + ret = __ieee80211_vif_change_channel(sdata, old_ctx, + &local_changed); + if (ret) + goto out; + + ctx->mode = sdata->reserved_mode; + + *changed = local_changed; + goto out; + } + ieee80211_stop_queues_by_reason(&local->hw, IEEE80211_MAX_QUEUE_MAP, IEEE80211_QUEUE_STOP_REASON_CHANCTX); diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 998fbbb..5015bc1 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -681,10 +681,14 @@ enum ieee80211_sdata_state_bits { * @IEEE80211_CHANCTX_EXCLUSIVE: channel context can be used * only by a single interface. This can be used for example for * non-fixed channel IBSS. + * @IEEE80211_CHANCTX_RESERVED: the channel context is reserved for + * future change and cannot be used by other interfaces until the + * reservation is taken into use. */ enum ieee80211_chanctx_mode { IEEE80211_CHANCTX_SHARED, - IEEE80211_CHANCTX_EXCLUSIVE + IEEE80211_CHANCTX_EXCLUSIVE, + IEEE80211_CHANCTX_RESERVED }; struct ieee80211_chanctx { @@ -757,6 +761,7 @@ struct ieee80211_sub_if_data { struct cfg80211_chan_def csa_chandef; struct ieee80211_chanctx *reserved_chanctx; + enum ieee80211_chanctx_mode reserved_mode; /* used to reconfigure hardware SM PS */ struct work_struct recalc_smps;