[RFC v2 0/4] mac802111: channel context reservation (was: multi-vif/multi-channel CSA implementation)

[RFC v2 0/4] mac802111: channel context reservation (was: multi-vif/multi-channel CSA implementation)

[Luca Coelho]

From: Luciano Coelho <luciano.coelho@intel.com>

Hi,

I have changed my strategy slightly.  Instead of trying to get all the
CSA stuff done at once, I'm sending this patchset to add the concept
of channel reservation.

The last patch "mac80211: add usage of CS channel reservation for STA"
should be looked at as a proof of concept for the channel context
reservation feature. ;)

This series is based on top of mac80211-next/master, without including
my combination check changes.  When the combination check patchset
gets applied, I can either send the "merge" of the two features as a
separate patchset or I can modify this patchset accordingly (though I
prefer the former, so I can do things in small steps ;).

Please review and let me know what you think.  The changes from my
first RFC are inlined in the commit message of the 2/4 patch.

--
Cheers,
Luca.

Luciano Coelho (4):
  mac80211: split ieee80211_vif_change_channel in two
  mac80211: implement chanctx reservation
  mac80211: allow reservation of a running chanctx
  mac80211: add usage of CS channel reservation for STA

 include/net/mac80211.h     |   7 ++
 net/mac80211/chan.c        | 228 +++++++++++++++++++++++++++++++++++++++++----
 net/mac80211/ieee80211_i.h |  18 +++-
 net/mac80211/mlme.c        |  70 ++++++--------
 4 files changed, 264 insertions(+), 59 deletions(-)

-- 
1.8.5.3

[RFC v2 1/4] mac80211: split ieee80211_vif_change_channel in two

[Luca Coelho]

From: Luciano Coelho <luciano.coelho@intel.com>

ieee80211_vif_change_channel() locks chanctx_mtx.  When implementing
channel reservation for CS, we will need to call the function to
change channel when the lock is already held, so split the part that
requires the lock out and leave the locking in the original function.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 net/mac80211/chan.c | 58 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 42c6592..a27c6ec 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -547,39 +547,20 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
 	return ret;
 }
 
-int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
-				 u32 *changed)
+static int __ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
+					  struct ieee80211_chanctx *ctx,
+					  u32 *changed)
 {
 	struct ieee80211_local *local = sdata->local;
-	struct ieee80211_chanctx_conf *conf;
-	struct ieee80211_chanctx *ctx;
 	const struct cfg80211_chan_def *chandef = &sdata->csa_chandef;
-	int ret;
 	u32 chanctx_changed = 0;
 
-	lockdep_assert_held(&local->mtx);
-
-	/* should never be called if not performing a channel switch. */
-	if (WARN_ON(!sdata->vif.csa_active))
-		return -EINVAL;
-
 	if (!cfg80211_chandef_usable(sdata->local->hw.wiphy, chandef,
 				     IEEE80211_CHAN_DISABLED))
 		return -EINVAL;
 
-	mutex_lock(&local->chanctx_mtx);
-	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
-					 lockdep_is_held(&local->chanctx_mtx));
-	if (!conf) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	ctx = container_of(conf, struct ieee80211_chanctx, conf);
-	if (ctx->refcount != 1) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (ctx->refcount != 1)
+		return -EINVAL;
 
 	if (sdata->vif.bss_conf.chandef.width != chandef->width) {
 		chanctx_changed = IEEE80211_CHANCTX_CHANGE_WIDTH;
@@ -597,7 +578,34 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
 	ieee80211_recalc_radar_chanctx(local, ctx);
 	ieee80211_recalc_chanctx_min_def(local, ctx);
 
-	ret = 0;
+	return 0;
+}
+
+int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
+				 u32 *changed)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_chanctx_conf *conf;
+	struct ieee80211_chanctx *ctx;
+	int ret;
+
+	lockdep_assert_held(&local->mtx);
+
+	/* should never be called if not performing a channel switch. */
+	if (WARN_ON(!sdata->vif.csa_active))
+		return -EINVAL;
+
+	mutex_lock(&local->chanctx_mtx);
+	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+					 lockdep_is_held(&local->chanctx_mtx));
+	if (!conf) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ctx = container_of(conf, struct ieee80211_chanctx, conf);
+
+	ret = __ieee80211_vif_change_channel(sdata, ctx, changed);
  out:
 	mutex_unlock(&local->chanctx_mtx);
 	return ret;
-- 
1.8.5.3

[RFC v2 2/4] mac80211: implement chanctx reservation

[Luca Coelho]

From: Luciano Coelho <luciano.coelho@intel.com>

In order to support channel switch with multiple vifs and multiple
contexts, we implement a concept of channel context reservation.  This
allows us to reserve a channel context to be used later.

The reservation functionality is not tied directly to channel switch
and may be used in other situations (eg. reserving a channel context
during IBSS join).

When reserving the context, the following algorithm is used:

1) try to find an existing context that matches our future chandef and
   reserve it if it exists;
2) otherwise, check if we're the only vif in the current context, in
   which case we can just change our current context.  To prevent
   other vifs from joining this context in the meantime, we mark it as
   exclusive.  This is an optimization to avoid using extra contexts
   when not necessary and requires the driver to support changing a
   context on-the-fly;
3) if we're not the only vif in the current context, create a new
   context and reserve it;

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
In RFC v2:

   * reformulated the patch to make channel reservation generic and
     not only for channel switch, as we may find other uses for it in
     the future;
   * added ieee80211_vif_unreserve_chanctx();
   * removed reservation of the current chanctx from this patch;
   * added a stop_queue_reason for chanctx reservation;
   * added a few TODOs according to Michal's comments to be handled in
     future patch series;
   * set sdata->csa_chandef to the received chandef in all cases (it
     was only happening in the alone-in-the-vif case;
---
 net/mac80211/chan.c        | 139 +++++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/ieee80211_i.h |  11 ++++
 2 files changed, 150 insertions(+)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index a27c6ec..bd42d17 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -419,6 +419,9 @@ static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
 
 	ctx = container_of(conf, struct ieee80211_chanctx, conf);
 
+	if (sdata->reserved_chanctx)
+		ieee80211_vif_unreserve_chanctx(sdata);
+
 	ieee80211_unassign_vif_chanctx(sdata, ctx);
 	if (ctx->refcount == 0)
 		ieee80211_free_chanctx(local, ctx);
@@ -611,6 +614,142 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
 	return ret;
 }
 
+int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata)
+{
+
+	lockdep_assert_held(&sdata->local->chanctx_mtx);
+
+	if (WARN_ON(!sdata->reserved_chanctx))
+		return -EINVAL;
+
+	if (--sdata->reserved_chanctx->refcount == 0)
+		ieee80211_free_chanctx(sdata->local, sdata->reserved_chanctx);
+
+	sdata->reserved_chanctx = NULL;
+
+	return 0;
+}
+
+int ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
+				  const struct cfg80211_chan_def *chandef)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_chanctx_conf *conf;
+	struct ieee80211_chanctx *new_ctx, *curr_ctx;
+	int ret = 0;
+
+	mutex_lock(&local->chanctx_mtx);
+
+	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+					 lockdep_is_held(&local->chanctx_mtx));
+	if (!conf) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	curr_ctx = container_of(conf, struct ieee80211_chanctx, conf);
+
+	/* 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 */
+		new_ctx = ieee80211_new_chanctx(local, chandef,
+						IEEE80211_CHANCTX_SHARED);
+		if (IS_ERR(new_ctx)) {
+			ret = PTR_ERR(new_ctx);
+			goto out;
+		}
+	}
+
+	/* reserve the new or existing context */
+	sdata->reserved_chanctx = new_ctx;
+	new_ctx->refcount++;
+
+	sdata->csa_chandef = *chandef;
+out:
+	mutex_unlock(&local->chanctx_mtx);
+	return ret;
+}
+
+int ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
+				       u32 *changed)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_chanctx *ctx;
+	struct ieee80211_chanctx *old_ctx;
+	struct ieee80211_chanctx_conf *conf;
+	int ret, local_changed = *changed;
+
+	/* TODO: need to recheck if the chandef is usable etc.? */
+
+	lockdep_assert_held(&local->mtx);
+
+	mutex_lock(&local->chanctx_mtx);
+
+	ctx = sdata->reserved_chanctx;
+	if (WARN_ON(!ctx)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	conf = rcu_dereference_protected(sdata->vif.chanctx_conf,
+					 lockdep_is_held(&local->chanctx_mtx));
+	if (!conf) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	old_ctx = container_of(conf, struct ieee80211_chanctx, conf);
+
+	ieee80211_stop_queues_by_reason(&local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CHANCTX);
+
+	ieee80211_unassign_vif_chanctx(sdata, old_ctx);
+	if (old_ctx->refcount == 0)
+		ieee80211_free_chanctx(local, old_ctx);
+
+	/* TODO: we're assuming that the bandwidth of the context
+	 * changes here, but in fact, it will only change if the
+	 * combination of the channels used in this context change.
+	 * We should set this flag according to what happens when
+	 * ieee80211_recalc_chanctx_chantype() is called.  Maybe the
+	 * nicest thing to do would be to change that function so that
+	 * it returns changed flags (which will be either 0 or
+	 * BSS_CHANGED_BANDWIDTH).
+	 */
+
+	if (sdata->vif.bss_conf.chandef.width != ctx->conf.def.width)
+		local_changed |= BSS_CHANGED_BANDWIDTH;
+
+	sdata->vif.bss_conf.chandef = ctx->conf.def;
+
+	/* unref our reservation before assigning */
+	ctx->refcount--;
+	sdata->reserved_chanctx = NULL;
+	ret = ieee80211_assign_vif_chanctx(sdata, ctx);
+	if (ret) {
+		/* if assign fails refcount stays the same */
+		if (ctx->refcount == 0)
+			ieee80211_free_chanctx(local, ctx);
+		goto out;
+	}
+
+	ieee80211_wake_queues_by_reason(&sdata->local->hw,
+					IEEE80211_MAX_QUEUE_MAP,
+					IEEE80211_QUEUE_STOP_REASON_CHANCTX);
+
+	*changed = local_changed;
+
+	ieee80211_recalc_chanctx_chantype(local, ctx);
+	ieee80211_recalc_smps_chanctx(local, ctx);
+	ieee80211_recalc_radar_chanctx(local, ctx);
+out:
+	mutex_unlock(&local->chanctx_mtx);
+	return ret;
+}
+
 int ieee80211_vif_change_bandwidth(struct ieee80211_sub_if_data *sdata,
 				   const struct cfg80211_chan_def *chandef,
 				   u32 *changed)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 8603dfb..998fbbb 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -756,6 +756,8 @@ struct ieee80211_sub_if_data {
 	bool csa_radar_required;
 	struct cfg80211_chan_def csa_chandef;
 
+	struct ieee80211_chanctx *reserved_chanctx;
+
 	/* used to reconfigure hardware SM PS */
 	struct work_struct recalc_smps;
 
@@ -899,6 +901,7 @@ enum queue_stop_reason {
 	IEEE80211_QUEUE_STOP_REASON_SKB_ADD,
 	IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL,
 	IEEE80211_QUEUE_STOP_REASON_FLUSH,
+	IEEE80211_QUEUE_STOP_REASON_CHANCTX,
 };
 
 #ifdef CONFIG_MAC80211_LEDS
@@ -1776,6 +1779,14 @@ ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
 			  const struct cfg80211_chan_def *chandef,
 			  enum ieee80211_chanctx_mode mode);
 int __must_check
+ieee80211_vif_reserve_chanctx(struct ieee80211_sub_if_data *sdata,
+			      const struct cfg80211_chan_def *chandef);
+int __must_check
+ieee80211_vif_use_reserved_context(struct ieee80211_sub_if_data *sdata,
+				   u32 *changed);
+int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata);
+
+int __must_check
 ieee80211_vif_change_bandwidth(struct ieee80211_sub_if_data *sdata,
 			       const struct cfg80211_chan_def *chandef,
 			       u32 *changed);
-- 
1.8.5.3

[RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

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.

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.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 include/net/mac80211.h     |  7 ++++++
 net/mac80211/chan.c        | 61 ++++++++++++++++++++++++++++++++++++++++------
 net/mac80211/ieee80211_i.h |  7 +++++-
 3 files changed, 67 insertions(+), 8 deletions(-)

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;
-- 
1.8.5.3

[RFC v2 4/4] mac80211: add usage of CS channel reservation for STA

[Luca Coelho]

From: Luciano Coelho <luciano.coelho@intel.com>

For simplicity, and as a proof of concept, only the STA case is
covered for now.  But the P2P GO and AP cases should also work with
this new concept.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
In RFC v2:

   * fixed some locking issues;
---
 net/mac80211/mlme.c | 70 ++++++++++++++++++++++-------------------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 46b62bb..44902bc 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -927,18 +927,20 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 	if (!ifmgd->associated)
 		goto out;
 
+	/* TODO: maybe this if block can be moved to use_reserved()? */
 	mutex_lock(&local->mtx);
-	ret = ieee80211_vif_change_channel(sdata, &changed);
-	mutex_unlock(&local->mtx);
-	if (ret) {
-		sdata_info(sdata,
-			   "vif channel switch failed, disconnecting\n");
-		ieee80211_queue_work(&sdata->local->hw,
-				     &ifmgd->csa_connection_drop_work);
-		goto out;
-	}
 
 	if (!local->use_chanctx) {
+		ret = ieee80211_vif_change_channel(sdata, &changed);
+		if (ret) {
+			sdata_info(sdata,
+				   "vif channel switch failed, disconnecting\n");
+			ieee80211_queue_work(&sdata->local->hw,
+					     &ifmgd->csa_connection_drop_work);
+			mutex_unlock(&local->mtx);
+			goto out;
+		}
+
 		local->_oper_chandef = sdata->csa_chandef;
 		/* Call "hw_config" only if doing sw channel switch.
 		 * Otherwise update the channel directly
@@ -947,7 +949,18 @@ static void ieee80211_chswitch_work(struct work_struct *work)
 			ieee80211_hw_config(local, 0);
 		else
 			local->hw.conf.chandef = local->_oper_chandef;
+	} else {
+		ret = ieee80211_vif_use_reserved_context(sdata, &changed);
+		if (ret) {
+			sdata_info(sdata,
+				   "using reserved context during channel switch failed, disconnecting\n");
+			ieee80211_queue_work(&sdata->local->hw,
+					     &ifmgd->csa_connection_drop_work);
+			mutex_unlock(&local->mtx);
+			goto out;
+		}
 	}
+	mutex_unlock(&local->mtx);
 
 	/* XXX: shouldn't really modify cfg80211-owned data! */
 	ifmgd->associated->channel = sdata->csa_chandef.chan;
@@ -998,7 +1011,6 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
 	struct cfg80211_bss *cbss = ifmgd->associated;
-	struct ieee80211_chanctx *chanctx;
 	enum ieee80211_band current_band;
 	struct ieee80211_csa_ie csa_ie;
 	int res;
@@ -1039,44 +1051,20 @@ ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 		return;
 	}
 
-	ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
-
-	mutex_lock(&local->chanctx_mtx);
-	if (local->use_chanctx) {
-		u32 num_chanctx = 0;
-		list_for_each_entry(chanctx, &local->chanctx_list, list)
-		       num_chanctx++;
-
-		if (num_chanctx > 1 ||
-		    !(local->hw.flags & IEEE80211_HW_CHANCTX_STA_CSA)) {
-			sdata_info(sdata,
-				   "not handling chan-switch with channel contexts\n");
-			ieee80211_queue_work(&local->hw,
-					     &ifmgd->csa_connection_drop_work);
-			mutex_unlock(&local->chanctx_mtx);
-			return;
-		}
-	}
+	/* TODO: handle !local->chanctx) here */
 
-	if (WARN_ON(!rcu_access_pointer(sdata->vif.chanctx_conf))) {
-		ieee80211_queue_work(&local->hw,
-				     &ifmgd->csa_connection_drop_work);
-		mutex_unlock(&local->chanctx_mtx);
-		return;
-	}
-	chanctx = container_of(rcu_access_pointer(sdata->vif.chanctx_conf),
-			       struct ieee80211_chanctx, conf);
-	if (chanctx->refcount > 1) {
+	mutex_lock(&local->mtx);
+	res = ieee80211_vif_reserve_chanctx(sdata, &csa_ie.chandef);
+	mutex_unlock(&local->mtx);
+	if (res) {
 		sdata_info(sdata,
-			   "channel switch with multiple interfaces on the same channel, disconnecting\n");
+			   "reserving context for channel switch failed, disconnecting\n");
 		ieee80211_queue_work(&local->hw,
 				     &ifmgd->csa_connection_drop_work);
-		mutex_unlock(&local->chanctx_mtx);
 		return;
 	}
-	mutex_unlock(&local->chanctx_mtx);
 
-	sdata->csa_chandef = csa_ie.chandef;
+	ifmgd->flags |= IEEE80211_STA_CSA_RECEIVED;
 	sdata->vif.csa_active = true;
 
 	if (csa_ie.mode)
-- 
1.8.5.3

Re: [RFC v2 2/4] mac80211: implement chanctx reservation

[Michal Kazior]

On 27 February 2014 15:41, Luca Coelho <luca@coelho.fi> wrote:
> From: Luciano Coelho <luciano.coelho@intel.com>
>
> In order to support channel switch with multiple vifs and multiple
> contexts, we implement a concept of channel context reservation.  This
> allows us to reserve a channel context to be used later.
>
> The reservation functionality is not tied directly to channel switch
> and may be used in other situations (eg. reserving a channel context
> during IBSS join).
>
> When reserving the context, the following algorithm is used:
>
> 1) try to find an existing context that matches our future chandef and
>    reserve it if it exists;

> 2) otherwise, check if we're the only vif in the current context, in
>    which case we can just change our current context.  To prevent
>    other vifs from joining this context in the meantime, we mark it as
>    exclusive.  This is an optimization to avoid using extra contexts
>    when not necessary and requires the driver to support changing a
>    context on-the-fly;

This commit part is out of date (you've moved this into a separate patch).


> @@ -611,6 +614,142 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
>         return ret;
>  }
>
> +int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata)
> +{
> +
> +       lockdep_assert_held(&sdata->local->chanctx_mtx);

Empty line :-)


> +       if (sdata->vif.bss_conf.chandef.width != ctx->conf.def.width)
> +               local_changed |= BSS_CHANGED_BANDWIDTH;
> +
> +       sdata->vif.bss_conf.chandef = ctx->conf.def;

I think you should be simply using sdata->csa_chandef here. What's the
point of setting the csa_chandef during reservation otherwise?

csa_chandef should probably be renamed to reserved_chandef for consistency.


> +
> +       /* unref our reservation before assigning */
> +       ctx->refcount--;
> +       sdata->reserved_chanctx = NULL;
> +       ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> +       if (ret) {
> +               /* if assign fails refcount stays the same */
> +               if (ctx->refcount == 0)
> +                       ieee80211_free_chanctx(local, ctx);
> +               goto out;
> +       }
> +
> +       ieee80211_wake_queues_by_reason(&sdata->local->hw,
> +                                       IEEE80211_MAX_QUEUE_MAP,
> +                                       IEEE80211_QUEUE_STOP_REASON_CHANCTX);

If you fail to assign chanctx you don't wake queues back. Is this intended?


> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index 8603dfb..998fbbb 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -756,6 +756,8 @@ struct ieee80211_sub_if_data {
>         bool csa_radar_required;
>         struct cfg80211_chan_def csa_chandef;
>
> +       struct ieee80211_chanctx *reserved_chanctx;
> +

It's a good idea to document this is protected by chanctx_mtx (and not
wdev.mtx).


Michał

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Michal Kazior]

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ł

Re: [RFC v2 2/4] mac80211: implement chanctx reservation

[Luca Coelho]

On Thu, 2014-02-27 at 16:16 +0100, Michal Kazior wrote:
> On 27 February 2014 15:41, Luca Coelho <luca@coelho.fi> wrote:
> > From: Luciano Coelho <luciano.coelho@intel.com>
> >
> > In order to support channel switch with multiple vifs and multiple
> > contexts, we implement a concept of channel context reservation.  This
> > allows us to reserve a channel context to be used later.
> >
> > The reservation functionality is not tied directly to channel switch
> > and may be used in other situations (eg. reserving a channel context
> > during IBSS join).
> >
> > When reserving the context, the following algorithm is used:
> >
> > 1) try to find an existing context that matches our future chandef and
> >    reserve it if it exists;
> 
> > 2) otherwise, check if we're the only vif in the current context, in
> >    which case we can just change our current context.  To prevent
> >    other vifs from joining this context in the meantime, we mark it as
> >    exclusive.  This is an optimization to avoid using extra contexts
> >    when not necessary and requires the driver to support changing a
> >    context on-the-fly;
> 
> This commit part is out of date (you've moved this into a separate patch).

Good point, I'll fix this part.


> 
> > @@ -611,6 +614,142 @@ int ieee80211_vif_change_channel(struct ieee80211_sub_if_data *sdata,
> >         return ret;
> >  }
> >
> > +int ieee80211_vif_unreserve_chanctx(struct ieee80211_sub_if_data *sdata)
> > +{
> > +
> > +       lockdep_assert_held(&sdata->local->chanctx_mtx);
> 
> Empty line :-)

Oops!


> > +       if (sdata->vif.bss_conf.chandef.width != ctx->conf.def.width)
> > +               local_changed |= BSS_CHANGED_BANDWIDTH;
> > +
> > +       sdata->vif.bss_conf.chandef = ctx->conf.def;
> 
> I think you should be simply using sdata->csa_chandef here. What's the
> point of setting the csa_chandef during reservation otherwise?

Yes, this is wrong.


> csa_chandef should probably be renamed to reserved_chandef for consistency.

I don't want to mess with CSA in this patch.  I'll leave the csa_chandef
as it is.  But you're right, I should use the "future" chandef, which
I'll add as reserved_chandef.



> > +       /* unref our reservation before assigning */
> > +       ctx->refcount--;
> > +       sdata->reserved_chanctx = NULL;
> > +       ret = ieee80211_assign_vif_chanctx(sdata, ctx);
> > +       if (ret) {
> > +               /* if assign fails refcount stays the same */
> > +               if (ctx->refcount == 0)
> > +                       ieee80211_free_chanctx(local, ctx);
> > +               goto out;
> > +       }
> > +
> > +       ieee80211_wake_queues_by_reason(&sdata->local->hw,
> > +                                       IEEE80211_MAX_QUEUE_MAP,
> > +                                       IEEE80211_QUEUE_STOP_REASON_CHANCTX);
> 
> If you fail to assign chanctx you don't wake queues back. Is this intended?

This was not intended, it was a mistake. :) I'll fix it.


> > diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> > index 8603dfb..998fbbb 100644
> > --- a/net/mac80211/ieee80211_i.h
> > +++ b/net/mac80211/ieee80211_i.h
> > @@ -756,6 +756,8 @@ struct ieee80211_sub_if_data {
> >         bool csa_radar_required;
> >         struct cfg80211_chan_def csa_chandef;
> >
> > +       struct ieee80211_chanctx *reserved_chanctx;
> > +
> 
> It's a good idea to document this is protected by chanctx_mtx (and not
> wdev.mtx).

Good idea.

--
Luca.

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

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.

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Michal Kazior]

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ł

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

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.

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Michal Kazior]

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ł

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

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.

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Michal Kazior]

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ł

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

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.

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

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.

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

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.

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Michal Kazior]

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ł

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

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.

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Michal Kazior]

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ł

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Luca Coelho]

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.

Re: [RFC v2 3/4] mac80211: allow reservation of a running chanctx

[Michal Kazior]

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ł