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. >> > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
