On Mon, 2014-03-03 at 14:26 +0100, Michal Kazior wrote: > On 3 March 2014 13:37, Luca Coelho <luca@xxxxxxxxx> wrote: > > On Mon, 2014-03-03 at 11:38 +0100, Michal Kazior wrote: > >> On 3 March 2014 10:57, Luca Coelho <luca@xxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
