Skip to contentSkip to conversation

mac80211 / o11s mesh: Modularize Path Selection Protocol

PATCH New Florian Sesser 2 participants 10 messages Feb 8, 2009 – Feb 9, 2009

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 mac80211 / o11s mesh: Modularize Path Selection Protocol

Hi all!

Here is my second version of my abstraction modularizing the o80211s mesh path selection protocol.

Sorry for taking so long, thanks for your comments!

I once again welcome your suggestions. Please have a look at my
mesh_path_selection_ops: Do you think they will suffice? It seems to me that they do for mesh_hwmp, but I also might have overseen something trivial here?

Where are the o11s guys BTW?

Changes from v1:

* Incorporated suggestions from Rami and Johannes, especially:
* checkpatch.pl (sorry 'bout that)
* Moved two or so unnecessary comments to commit msgs
* removed two unnecessary EXPORT_SYMBOLs

     (mesh_..._algo_find and mesh_..._algo_set)

* removed mesh_..._find_by_name, use only find(u32 id) (that only breaks loading missing modules from disk, but that isn't really needed.)
* keep mesh_hwmp built into mac80211, not as seperate module. registers itself on load.
* wrote a Kconfig for my mesh_pptest skeleton module (although I don't think anybody wants this in vanilla?! I merely included it for reference...)

* Renamed mesh_path_sel_algo_(un)register to

    ieee80211_mesh_path_sel_proto_(un)register

* Moved mesh_path_timer/ieee80211_mesh_path_timer from mesh.c to mesh_pathtbl.c

* Removed the typedefs for the function pointers in mesh_path_sel_ops and renamed them to avoid 'hungarian notation'

What I didn't do:

* Change seven or so different calls via function pointers to static inlines. Johannes pointed me to rate_control, but the code there also isn't explicitly elegant IMHO - half a page of empty static inlines for the case of an algorithm being *not* used. Of course that gets compiled out, but the functions here aren't called very often, and in the case of HWMP, which heavily floods and by the draft standard is expected to work for mesh networks of up to 32 nodes, the performance benefit is negligible IMHO. [The block device IO scheduler, where I took most of my inspiration from, also does not do that - and the functions there do get called a lot.]

What I don't know if I should do:

* The names of the six extra EXPORT_SYMBOLS that are needed by mesh path selection algorithms (mainly form mesh_pathtbl.c and mesh.c) begin with mesh_ ... not ieee80211_... . Should I rename them?

Greetings!

Florian

#1 lore raw

PATCH 1/3 mac80211: Modularize Mesh path selection protocol

Mesh path selection protocols should be exchangeable. This patch includes the infrastructure needed to enable path selection protocols to register themselves to later be (at the moment:) manually configured for a specific mesh device.

* Moved enum mpath_frame_type and macro max_preq_retries(s) to mesh.h (from mesh_hwmp.c)

* Moved mesh_path_timer() and ieee80211_mesh_path_timer() to mesh_path.c (from mesh.c)

* New EXPORT_SYMBOLS:
ieee80211_mesh_path_sel_proto_register, ieee80211_mesh_path_sel_proto_unregister, mesh_path_timer,
mesh_path_tx_pending, mesh_path_assign_nexthop, mesh_path_discard_frame,
mesh_path_add,
mesh_path_lookup

---
 net/mac80211/ieee80211_i.h  |    5 ++
 net/mac80211/mesh.c         |  152 +++++++++++++++++++++++++++++++++++++++----
 net/mac80211/mesh.h         |   64 +++++++++++++++++-
 net/mac80211/mesh_hwmp.c    |   68 +++++++++----------
 net/mac80211/mesh_pathtbl.c |   55 ++++++++++++++--
 net/mac80211/sta_info.c     |    1 +
 net/mac80211/tx.c           |    5 +-
 net/mac80211/util.c         |    2 +
 8 files changed, 294 insertions(+), 58 deletions(-)
net/mac80211/ieee80211_i.h +5 -0
index 5a1f19a..8daf6a4 100644
@@ -365,6 +365,8 @@ struct ieee80211_if_mesh {
 	u8 mesh_pm_id[4];
 	/* Congestion Control Mode Identifier */
 	u8 mesh_cc_id[4];
+	/* Active Path Selection Protocol */
+	struct mesh_path_sel_proto *mesh_pp;
 	/* Local mesh Destination Sequence Number */
 	u32 dsn;
 	/* Last used PREQ ID */
@@ -534,6 +536,9 @@ struct ieee80211_sub_if_data {
 		struct dentry *dot11MeshHWMPmaxPREQretries;
 		struct dentry *path_refresh_time;
 		struct dentry *min_discovery_timeout;
+		struct dentry *mesh_path_selection_protocol_id;
+		struct dentry *mesh_path_selection_metric_id;
+		struct dentry *mesh_congestion_mode_control_id;
 	} mesh_config;
 #endif
 
net/mac80211/mesh.c +138 -14

219 lines not on this page · raw message

net/mac80211/mesh.h +61 -3

100 lines not on this page · raw message

net/mac80211/mesh_hwmp.c +33 -35

107 lines not on this page · raw message

net/mac80211/mesh_pathtbl.c +50 -5

105 lines not on this page · raw message

net/mac80211/sta_info.c +1 -0
index 10c5539..b10d754 100644
@@ -105,6 +105,7 @@ struct sta_info *sta_info_get(struct ieee80211_local *local, const u8 *addr)
 	}
 	return sta;
 }
+EXPORT_SYMBOL(sta_info_get);
 
 struct sta_info *sta_info_get_by_idx(struct ieee80211_local *local, int idx,
 				     struct net_device *dev)
net/mac80211/tx.c +4 -1
index f1c726d..97dde64 100644
@@ -1351,11 +1351,14 @@ int ieee80211_master_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	    ieee80211_is_data(hdr->frame_control)) {
 		if (is_multicast_ether_addr(hdr->addr3))
 			memcpy(hdr->addr1, hdr->addr3, ETH_ALEN);
+#ifdef CONFIG_MAC80211_MESH
 		else
-			if (mesh_nexthop_lookup(skb, osdata)) {
+			if (osdata->u.mesh.mesh_pp->ops->
+				nexthop_lookup(skb, osdata)) {
 				dev_put(odev);
 				return 0;
 			}
+#endif
 		if (memcmp(odev->dev_addr, hdr->addr4, ETH_ALEN) != 0)
 			IEEE80211_IFSTA_MESH_CTR_INC(&osdata->u.mesh,
 							    fwded_frames);
net/mac80211/util.c +2 -0
index 73c7d73..c579606 100644
@@ -674,6 +674,7 @@ void ieee802_11_parse_elems(u8 *start, size_t len,
 		pos += elen;
 	}
 }
+EXPORT_SYMBOL(ieee802_11_parse_elems);
 
 void ieee80211_set_wmm_default(struct ieee80211_sub_if_data *sdata)
 {
@@ -714,6 +715,7 @@ void ieee80211_tx_skb(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb,
 
 	dev_queue_xmit(skb);
 }
+EXPORT_SYMBOL(ieee80211_tx_skb);
 
 int ieee80211_set_freq(struct ieee80211_sub_if_data *sdata, int freqMHz)
 {
Signature · 1 line
1.5.6.5
#2 lore raw

PATCH 2/3 mac80211: Modul. mesh path prot: Comm w/ Userspace

Modularize mesh path selection protocol.

This patch enables communication with userspace through the "iw" tool. The corresponding patch to IW will be posted on this list.

---
 include/linux/nl80211.h |   10 ++++++++++
 include/net/cfg80211.h  |    4 ++++
 net/mac80211/cfg.c      |    9 +++++++++
 net/wireless/nl80211.c  |   18 ++++++++++++++++++
 4 files changed, 41 insertions(+), 0 deletions(-)
include/linux/nl80211.h +10 -0
index 4bc2704..1125e73 100644
@@ -776,6 +776,13 @@ enum nl80211_mntr_flags {
  * @NL80211_MESHCONF_HWMP_NET_DIAM_TRVS_TIME: The interval of time (in TUs)
  * that it takes for an HWMP information element to propagate across the mesh
  *
+ * @NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID: Path Selection Protocol
+ * ID, consisting of an OUI + arbitrary 8 bit
+ *
+ * @NL80211_MESHCONF_PATH_SELECTION_METRIC_ID: Path Selection Metric ID
+ *
+ * @NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID: Congestion Control Mode ID
+ *
  * @NL80211_MESHCONF_ATTR_MAX: highest possible mesh configuration attribute
  *
  * @__NL80211_MESHCONF_ATTR_AFTER_LAST: internal use
@@ -795,6 +802,9 @@ enum nl80211_meshconf_params {
 	NL80211_MESHCONF_HWMP_ACTIVE_PATH_TIMEOUT,
 	NL80211_MESHCONF_HWMP_PREQ_MIN_INTERVAL,
 	NL80211_MESHCONF_HWMP_NET_DIAM_TRVS_TIME,
+	NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID,
+	NL80211_MESHCONF_PATH_SELECTION_METRIC_ID,
+	NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID,
 
 	/* keep last */
 	__NL80211_MESHCONF_ATTR_AFTER_LAST,
include/net/cfg80211.h +4 -0
index dd1fd51..83eb2a1 100644
@@ -458,6 +458,10 @@ struct mesh_config {
 	u32 dot11MeshHWMPactivePathTimeout;
 	u16 dot11MeshHWMPpreqMinInterval;
 	u16 dot11MeshHWMPnetDiameterTraversalTime;
+	/* Mesh IDs */
+	u32 mesh_path_selection_protocol_id;
+	u32 mesh_path_selection_metric_id;
+	u32 mesh_congestion_control_mode_id;
 };
 
 /**
net/mac80211/cfg.c +9 -0
index a1a1344..c885086 100644
@@ -1085,6 +1085,15 @@ static int ieee80211_set_mesh_params(struct wiphy *wiphy,
 			   mask))
 		conf->dot11MeshHWMPnetDiameterTraversalTime =
 			nconf->dot11MeshHWMPnetDiameterTraversalTime;
+	if (_chg_mesh_attr(NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID, mask))
+		mesh_ids_set_pp(&sdata->u.mesh,
+		nconf->mesh_path_selection_protocol_id);
+	if (_chg_mesh_attr(NL80211_MESHCONF_PATH_SELECTION_METRIC_ID, mask))
+		conf->mesh_path_selection_metric_id =
+			nconf->mesh_path_selection_metric_id;
+	if (_chg_mesh_attr(NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID, mask))
+		conf->mesh_congestion_control_mode_id =
+			nconf->mesh_congestion_control_mode_id;
 	return 0;
 }
 
net/wireless/nl80211.c +18 -0
index d452396..6e62e33 100644
@@ -1979,6 +1979,12 @@ static int nl80211_get_mesh_params(struct sk_buff *skb,
 			cur_params.dot11MeshHWMPpreqMinInterval);
 	NLA_PUT_U16(msg, NL80211_MESHCONF_HWMP_NET_DIAM_TRVS_TIME,
 			cur_params.dot11MeshHWMPnetDiameterTraversalTime);
+	NLA_PUT_U32(msg, NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID,
+			cur_params.mesh_path_selection_protocol_id);
+	NLA_PUT_U32(msg, NL80211_MESHCONF_PATH_SELECTION_METRIC_ID,
+			cur_params.mesh_path_selection_metric_id);
+	NLA_PUT_U32(msg, NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID,
+			cur_params.mesh_congestion_control_mode_id);
 	nla_nest_end(msg, pinfoattr);
 	genlmsg_end(msg, hdr);
 	err = genlmsg_unicast(msg, info->snd_pid);
@@ -2018,6 +2024,9 @@ nl80211_meshconf_params_policy[NL80211_MESHCONF_ATTR_MAX+1] __read_mostly = {
 	[NL80211_MESHCONF_HWMP_ACTIVE_PATH_TIMEOUT] = { .type = NLA_U32 },
 	[NL80211_MESHCONF_HWMP_PREQ_MIN_INTERVAL] = { .type = NLA_U16 },
 	[NL80211_MESHCONF_HWMP_NET_DIAM_TRVS_TIME] = { .type = NLA_U16 },
+	[NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID] = { .type = NLA_U32 },
+	[NL80211_MESHCONF_PATH_SELECTION_METRIC_ID] = { .type = NLA_U32 },
+	[NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID] = { .type = NLA_U32 },
 };
 
 static int nl80211_set_mesh_params(struct sk_buff *skb, struct genl_info *info)
@@ -2079,6 +2088,15 @@ static int nl80211_set_mesh_params(struct sk_buff *skb, struct genl_info *info)
 			dot11MeshHWMPnetDiameterTraversalTime,
 			mask, NL80211_MESHCONF_HWMP_NET_DIAM_TRVS_TIME,
 			nla_get_u16);
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, mesh_path_selection_protocol_id,
+			mask, NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID,
+			nla_get_u32);
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, mesh_path_selection_metric_id,
+			mask, NL80211_MESHCONF_PATH_SELECTION_METRIC_ID,
+			nla_get_u32);
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, mesh_congestion_control_mode_id,
+			mask, NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID,
+			nla_get_u32);
 
 	/* Apply changes */
 	rtnl_lock();
Signature · 1 line
1.5.6.5
#3 lore raw

PATCH 3/3 mac80211: Modul. mesh path prot: Skeleton Module

Modularize mesh path selection protocol.

This patch provides a skeleton module, which registers itself upon being loaded into the kernel, but does not really do path selection.

Make a mesh device use this module by insmodding it and issuing:
iw dev <mesh-dev> set mesh_param mesh_path_selection_protocol_id 000F0F0F (of course after patching IW).

---
 net/mac80211/Kconfig       |   19 +++++++++--
 net/mac80211/Makefile      |    2 +
 net/mac80211/mesh_pptest.c |   81 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 3 deletions(-)
 create mode 100644 net/mac80211/mesh_pptest.c
net/mac80211/Kconfig +16 -3
index 60c1616..e635f15 100644
@@ -65,10 +65,23 @@ config MAC80211_MESH
 	bool "Enable mac80211 mesh networking (pre-802.11s) support"
 	depends on MAC80211 && EXPERIMENTAL
 	---help---
-	 This options enables support of Draft 802.11s mesh networking.
-	 The implementation is based on Draft 1.08 of the Mesh Networking
-	 amendment. For more information visit http://o11s.org/.
+	  This options enables support of Draft 802.11s mesh networking.
+	  The implementation is based on Draft 1.08 of the Mesh Networking
+	  amendment. For more information visit http://o11s.org/.
 
+	  Includes the HWMP Mesh path selection protocol, as that
+	  is a requirement of the (Draft) standard.
+
+config MAC80211_MESH_PP_TEST
+	tristate "80211s: Path selection protocol test module"
+	default n
+	depends on MAC80211_MESH && MAC80211 && EXPERIMENTAL && m
+	---help---
+	  Skeleton module for the path selection protocol modularization
+	  code. Does not do actual path selection, just registers
+	  itself and has the required methods.
+
+	  Do not select this option, rather look at the code in mesh_pptest.c.
 
 config MAC80211_LEDS
 	bool "Enable LED triggers"
net/mac80211/Makefile +2 -0
index 58c94bb..7b3b5bd 100644
@@ -38,6 +38,8 @@ mac80211-$(CONFIG_MAC80211_MESH) += \
 	mesh_plink.o \
 	mesh_hwmp.o
 
+obj-$(CONFIG_MAC80211_MESH_PP_TEST) += mesh_pptest.o
+
 mac80211-$(CONFIG_PM) += pm.o
 
 # objects for PID algorithm
net/mac80211/mesh_pptest.c new file +81 -0

81 lines not on this page · raw message

Signature · 1 line
1.5.6.5
maintainer
#4 lore raw

Re: [PATCH 3/3] mac80211: Modul. mesh path prot: Skeleton Module

Quoted 10 lines · Florian Sesser

Modularize mesh path selection protocol.

This patch provides a skeleton module, which registers itself upon being loaded into the kernel, but does not really do path selection.

Make a mesh device use this module by insmodding it and issuing:
iw dev <mesh-dev> set mesh_param mesh_path_selection_protocol_id 000F0F0F (of course after patching IW).

00-0F-0F   (hex)                Real ID Technology Co., Ltd.
000F0F     (base 16)            Real ID Technology Co., Ltd.
                                9F Hanmi B/D 192-19
                                Nonhyeon-Dong
                                Gangnam-Gu Seoul 135-010
                                KOREA, REPUBLIC OF

Have you asked them about using their OUI?

Quoted 20 lines
---
 net/mac80211/Kconfig       |   19 +++++++++--
 net/mac80211/Makefile      |    2 +
 net/mac80211/mesh_pptest.c |   81 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+), 3 deletions(-)
 create mode 100644 net/mac80211/mesh_pptest.c
net/mac80211/Kconfig +1 -3
index 60c1616..e635f15 100644
@@ -65,10 +65,23 @@ config MAC80211_MESH
 	bool "Enable mac80211 mesh networking (pre-802.11s) support"
 	depends on MAC80211 && EXPERIMENTAL
 	---help---
-	 This options enables support of Draft 802.11s mesh networking.
-	 The implementation is based on Draft 1.08 of the Mesh Networking
-	 amendment. For more information visit http://o11s.org/.
+	  This options enables support of Draft 802.11s mesh networking.

net/mac80211/Kconfig config MAC80211_MESH · line 78

Quoted 10 lines
net/mac80211/Kconfig +9 -0
+	  The implementation is based on Draft 1.08 of the Mesh Networking
+	  amendment. For more information visit http://o11s.org/.
 
+	  Includes the HWMP Mesh path selection protocol, as that
+	  is a requirement of the (Draft) standard.
+
+config MAC80211_MESH_PP_TEST
+	tristate "80211s: Path selection protocol test module"
+	default n
+	depends on MAC80211_MESH && MAC80211 && EXPERIMENTAL && m

&& m ???

johannes

#5 lore raw

Re: [PATCH 3/3] mac80211: Modul. mesh path prot: Skeleton Module

Hi!

Johannes, thanks for all of your comments! I will need some time to work through them, though, please be patient.

Quoted 8 lines · Johannes Berg
00-0F-0F   (hex)                Real ID Technology Co., Ltd.
000F0F     (base 16)            Real ID Technology Co., Ltd.
                                9F Hanmi B/D 192-19
                                Nonhyeon-Dong
                                Gangnam-Gu Seoul 135-010
                                KOREA, REPUBLIC OF

Have you asked them about using their OUI?

Nah, sorry about that - I didn't get 0x000FACFF meant 'null' in the first place, and since I don't have access to an OUI yet, I selected this value by random. I still hope nobody seriously wants my skeleton module in the kernel code though.

BTW... do you know which OUI in that case (at devel time, this is an university project, afaik the university does not have an OUI I can use) makes sense for stuff like that? I believe there is a reserved range, but I don't know for what reason it's reserved, and if I should use it to avoid misunderstandings.

Quoted 6 lines

net/mac80211/Kconfig config MAC80211_MESH · line 78

Quoted 4 lines
net/mac80211/Kconfig +4 -0
+config MAC80211_MESH_PP_TEST
+	tristate "80211s: Path selection protocol test module"
+	default n
+	depends on MAC80211_MESH && MAC80211 && EXPERIMENTAL && m

&& m ???

Limits the tristate to 'n' or 'm', because I didn't want to clutter the code with ifdefs for the 'compiled-in' case. This cannot be the final solution either. I need to investigate some more on what happens if I just compile it in.

[do you really think I should to copy the style of rate control? i don't like it at all. they have KBs of (IMHO) not very useful code instead of keeping it to the point? Even in the Kconfig. Please correct me if I got this wrong.]

Quoted 1 line

Generally, this looks like a decent idea,

Thanks :)

Quoted 1 line

but the execution definitely is lacking.

I know that, as I'm pretty new to kernel coding. Should I get me some mentor from the kernel mentors project, or are you willing to correct my stupid beginner mistakes all the time?

Also: As you still haven't said anything about my ops struct, I deem you think I have made a somewhat reasonable selection here?

Again, thanks a lot!

Florian

maintainer
#6 lore raw

Re: [PATCH 3/3] mac80211: Modul. mesh path prot: Skeleton Module

Hi,

Quoted 11 lines

Johannes, thanks for all of your comments! I will need some time to work through them, though, please be patient.

Quoted 7 lines · Johannes Berg
00-0F-0F   (hex)                Real ID Technology Co., Ltd.
000F0F     (base 16)            Real ID Technology Co., Ltd.
                                9F Hanmi B/D 192-19
                                Nonhyeon-Dong
                                Gangnam-Gu Seoul 135-010
                                KOREA, REPUBLIC OF
Quoted 4 lines
Quoted 1 line

Have you asked them about using their OUI?

Nah, sorry about that - I didn't get 0x000FACFF meant 'null' in the first place,

Actually I was looking at Draft 2.0 - 2.08/09 seems to have this updated. And no, I don't really have access to the documents.

Quoted 3 lines

and since I don't have access to an OUI yet, I selected this value by random. I still hope nobody seriously wants my skeleton module in the kernel code though.

Heh, right.

Quoted 5 lines

BTW... do you know which OUI in that case (at devel time, this is an university project, afaik the university does not have an OUI I can use) makes sense for stuff like that? I believe there is a reserved range, but I don't know for what reason it's reserved, and if I should use it to avoid misunderstandings.

I don't think there is a reserved OUI.

Quoted 4 lines

[do you really think I should to copy the style of rate control? i don't like it at all. they have KBs of (IMHO) not very useful code instead of keeping it to the point? Even in the Kconfig. Please correct me if I got this wrong.]

Depends what you mean by that? I think you should force one algorithm to be built in -- just like rate control. I also think you should take a look at abstracting the API like rate control to not expose so many internal data structures. Alternatively, just don't make them kernel modules but require building the algorithms into mac80211.ko anyway.

Quoted 3 lines

I know that, as I'm pretty new to kernel coding. Should I get me some mentor from the kernel mentors project, or are you willing to correct my stupid beginner mistakes all the time?

Seems fine at this point, I take it you've looked through
http://wireless.kernel.org/en/developers/Documentation/SubmittingPatches
and the links at the bottom of that page. Also Documentation/CodingStyle, and checkpatch etc. but that seemed fairly clean this time around, so I guess you did.

What I didn't like about your patches was (1) the cramped style with so few empty lines, and (2) the API issue I mentioned.

Quoted 2 lines

Also: As you still haven't said anything about my ops struct, I deem you think I have made a somewhat reasonable selection here?

Not sure what to say there -- you don't really have a choice which functions to make virtual. The timer thing seems a little odd, either the algorithm should do everything or nothing, imho.

I don't think it's necessary to split this into an ops and a registration struct though.

johannes

maintainer
#7 lore raw

Re: [PATCH 2/3] mac80211: Modul. mesh path prot: Comm w/ Userspace

In reply to Florian Sesser
Quoted 1 line · Florian Sesser

Modularize mesh path selection protocol.

that's the subject, you also need to mentioned nl80211/cfg80211 since that is the bulk of the change.

Quoted 3 lines

This patch enables communication with userspace through the "iw" tool. The corresponding patch to IW will be posted on this list.

NACK to this description. 'iw' is but one implementation of an nl80211 userspace client.

include/net/cfg80211.h struct mesh_config { · line 464

Quoted 9 lines

+++ b/include/net/cfg80211.h

include/net/cfg80211.h +4 -0
@@ -458,6 +458,10 @@ struct mesh_config {
 	u32 dot11MeshHWMPactivePathTimeout;
 	u16 dot11MeshHWMPpreqMinInterval;
 	u16 dot11MeshHWMPnetDiameterTraversalTime;
+	/* Mesh IDs */
+	u32 mesh_path_selection_protocol_id;
+	u32 mesh_path_selection_metric_id;
+	u32 mesh_congestion_control_mode_id;

That's not a useful comment.

net/mac80211/cfg.c static int ieee80211_set_mesh_params(struct wiphy *wiphy, · line 1096

Quoted 9 lines
net/mac80211/cfg.c +9 -0
+	if (_chg_mesh_attr(NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID, mask))
+		mesh_ids_set_pp(&sdata->u.mesh,
+		nconf->mesh_path_selection_protocol_id);
+	if (_chg_mesh_attr(NL80211_MESHCONF_PATH_SELECTION_METRIC_ID, mask))
+		conf->mesh_path_selection_metric_id =
+			nconf->mesh_path_selection_metric_id;
+	if (_chg_mesh_attr(NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID, mask))
+		conf->mesh_congestion_control_mode_id =
+			nconf->mesh_congestion_control_mode_id;

Shouldn't this actually effect the change too?

net/wireless/nl80211.c static int nl80211_set_mesh_params(struct sk_buff *skb, struct genl_info *info) · line 2099

Quoted 9 lines
net/wireless/nl80211.c +9 -0
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, mesh_path_selection_protocol_id,
+			mask, NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID,
+			nla_get_u32);
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, mesh_path_selection_metric_id,
+			mask, NL80211_MESHCONF_PATH_SELECTION_METRIC_ID,
+			nla_get_u32);
+	FILL_IN_MESH_PARAM_IF_SET(tb, cfg, mesh_congestion_control_mode_id,
+			mask, NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID,
+			nla_get_u32);

can that possibly be unset?

johannes

maintainer
#8 lore raw

Re: [PATCH 1/3] mac80211: Modularize Mesh path selection protocol

In reply to Florian Sesser

net/mac80211/mesh.c void ieee80211s_init(void) · line 37

Quoted 4 lines · Florian Sesser
net/mac80211/mesh.c +2 -0
 	rm_cache = kmem_cache_create("mesh_rmc", sizeof(struct rmc_entry),
 				     0, 0, NULL);
+	if (!(ieee80211_mesh_path_sel_proto_find(0x000FACFF)))
+		ieee80211_mesh_path_sel_proto_register(&mesh_path_sel_hwmp);

Explain please. Why the null protocol here?

net/mac80211/mesh.c void mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata) · line 128

Quoted 9 lines
net/mac80211/mesh.c +9 -0
+struct mesh_path_sel_proto *ieee80211_mesh_path_sel_proto_find(u32 id)
+{
+	struct mesh_path_sel_proto *a;
+	list_for_each_entry(a, &mesh_path_sel_proto_list, list) {
+		if (a->id == id)
+			return a;
+	}
+	return NULL;
+}

Explain why there's no need for any locking.

Quoted 8 lines
net/mac80211/mesh.c +8 -0
+void ieee80211_mesh_path_sel_proto_set(struct ieee80211_if_mesh *sta,
+				struct mesh_path_sel_proto *algo)
+{
+	/* first, check if algo has been properly registered */
+	if (ieee80211_mesh_path_sel_proto_find(algo->id)) {
+		if (!sta->mesh_pp) {
+			/* set all three locations: */
+			/* connects an interface to its mesh pp algo */

net/mac80211/mesh.c void mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata) · line 147

Quoted 10 lines
net/mac80211/mesh.c +10 -0
+			sta->mesh_pp = algo;
+			/* used to compare meshes (see mesh_matches_local) */
+			memcpy(sta->mesh_pp_id, &(algo->id), 4);
+			/* used by the nl80211 layer */
+			sta->mshcfg.mesh_path_selection_protocol_id = algo->id;
+
+			printk(KERN_INFO "o11s: Initial mesh path selection "
+				"algorithm %s / %08X\n", algo->name, algo->id);
+			return;
+		}

Please rewrite this entire function to have less deep nesting. And rename the "sta" variable here to something more customary. "sta" means struct sta_info. Also, see whether the printks are all necessary, should depend on debugging options and are user triggerable.

net/mac80211/mesh.c void mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata) · line 180

Quoted 10 lines
net/mac80211/mesh.c +10 -0
+void ieee80211_mesh_path_sel_proto_register(struct mesh_path_sel_proto *algo)
+{
+	/* Sanity checks */
+	BUG_ON(!algo->ops->path_start_discovery);
+	BUG_ON(!algo->ops->nexthop_lookup);
+	BUG_ON(!algo->ops->path_error_tx);
+	BUG_ON(!algo->ops->queue_preq);
+	BUG_ON(!algo->ops->rx_path_sel_frame);
+	BUG_ON(!algo->ops->path_sel_start);
+	BUG_ON(!algo->ops->path_sel_stop);

How about also BUG_ON(!algo->ops) :)

net/mac80211/mesh.c void mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata) · line 193

Quoted 12 lines
net/mac80211/mesh.c +12 -0
+	spin_lock(&path_sel_list_lock);
+	if (!ieee80211_mesh_path_sel_proto_find(algo->id)) {
+		list_add_tail(&algo->list, &mesh_path_sel_proto_list);
+		spin_unlock(&path_sel_list_lock);
+		printk(KERN_INFO "o11s: Mesh path selection algorithm %s / "
+			"%08X registered\n", algo->name, algo->id);
+	} else {
+		spin_unlock(&path_sel_list_lock);
+		printk(KERN_WARNING "o11s: Error: Path selection algorithm "
+			"%s / %08X already registered\n", algo->name,
+			algo->id);
+	}

Wow, that unlocking is awkward. Fix that please by moving it out of the conditionals.

net/mac80211/mesh.c void mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata) · line 207

Quoted 11 lines
net/mac80211/mesh.c +11 -0
+void ieee80211_mesh_path_sel_proto_unregister(struct mesh_path_sel_proto *algo)
+{
+	if (ieee80211_mesh_path_sel_proto_find(algo->id)) {
+		list_del(&algo->list);
+		printk(KERN_INFO "o11s: Mesh path selection algorithm %s / "
+			"%08X unregistered\n", algo->name, algo->id);
+	} else {
+		printk(KERN_WARNING "o11s: Error: Mesh path selection algorithm"
+			" %s / %08X not found\n", algo->name, algo->id);
+	}
+}

Explain the ability to not lock here.

Quoted 1 line

+/* Set path selection protocol ID: OUI followed by arbitrary 2 bytes */

two bytes??

net/mac80211/mesh.c void mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata) · line 231

Quoted 13 lines
net/mac80211/mesh.c +13 -0
+void mesh_ids_set_pp(struct ieee80211_if_mesh *sta, u32 pp)
+{
+	struct mesh_path_sel_proto *nalgo =
+		ieee80211_mesh_path_sel_proto_find(pp);
+
+	if (nalgo)
+		ieee80211_mesh_path_sel_proto_set(sta, nalgo);
+	else
+		printk(KERN_WARNING "o11s: Could not find mesh path selection "
+			"protocol with ID %08X\n", pp);
+}
+/* Set path selection metric ID: OUI followed by arbitrary 2 bytes */
+void mesh_ids_set_pm(struct ieee80211_if_mesh *sta, u32 pm)

Generally you need to be way more liberal with adding empty lines to aid the reader. Your _primary_ (!!) concern should be the _human_ consumer of the code, not the compiler.

net/mac80211/mesh.c void mesh_accept_plinks_update(struct ieee80211_sub_if_data *sdata) · line 239

Quoted 8 lines
net/mac80211/mesh.c +8 -0
+{
+	memcpy(sta->mesh_pm_id, &pm, 4);
+}
+/* Set congestion control mode ID: OUI followed by arbitrary 2 bytes */
+void mesh_ids_set_cc(struct ieee80211_if_mesh *sta, u32 cc)
+{
+	memcpy(sta->mesh_cc_id, &cc, 4);
+}

Both of these look like candidates for static inlines, but they're both not endian safe. Also observe the variable naming I mentioned above.

Quoted 18 lines
net/mac80211/mesh.c +0 -10
@@ -346,16 +463,6 @@ void mesh_table_free(struct mesh_table *tbl, bool free_leafs)
 	__mesh_table_free(tbl);
 }
 
-static void ieee80211_mesh_path_timer(unsigned long data)
-{
-	struct ieee80211_sub_if_data *sdata =
-		(struct ieee80211_sub_if_data *) data;
-	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
-	struct ieee80211_local *local = sdata->local;
-
-	queue_work(local->hw.workqueue, &ifmsh->work);
-}
-
 struct mesh_table *mesh_table_grow(struct mesh_table *tbl)
 {
 	struct mesh_table *newtbl;

net/mac80211/mesh.c void ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata) · line 555

Quoted 5 lines
net/mac80211/mesh.c +2 -0
@@ -444,6 +551,16 @@ void ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata)
 	queue_work(local->hw.workqueue, &ifmsh->work);
 	ieee80211_if_config(sdata, IEEE80211_IFCC_BEACON |
 				   IEEE80211_IFCC_BEACON_ENABLED);
+
+	/* If no PS protocol has manually been set, go with the default */

"PS" is overloaded, elaborate.

net/mac80211/mesh.c void ieee80211_start_mesh(struct ieee80211_sub_if_data *sdata) · line 564

Quoted 9 lines
net/mac80211/mesh.c +8 -0
+	if (!ifmsh->mesh_pp)
+		ifmsh->mesh_pp = ieee80211_mesh_path_sel_proto_find
+			(0x000FACFF);
+	/* If we still have no PS Proto, bail out */
+	if (!ifmsh->mesh_pp) {
+		printk(KERN_WARNING "o11s: Could not load HWMP, aborting\n");
+		ieee80211_stop_mesh(sdata);
+	}
 }

That looks horrible. No space between the function name and opening parenthesis please, more blank lines. Avoid all the error cases by forcing the user to build at least one algorithm into the kernel.

net/mac80211/mesh.c static void ieee80211_mesh_rx_mgmt_action(struct ieee80211_sub_if_data *sdata, · line 644

Quoted 8 lines
net/mac80211/mesh.c +2 -1
 void ieee80211_stop_mesh(struct ieee80211_sub_if_data *sdata)
@@ -523,7 +640,8 @@ static void ieee80211_mesh_rx_mgmt_action(struct ieee80211_sub_if_data *sdata,
 		mesh_rx_plink_frame(sdata, mgmt, len, rx_status);
 		break;
 	case MESH_PATH_SEL_CATEGORY:
-		mesh_rx_path_sel_frame(sdata, mgmt, len);
+		sdata->u.mesh.mesh_pp->ops->
+			rx_path_sel_frame(sdata, mgmt, len);

Add static inline wrappers for the function ops calls, and if you call those like the original functions your patch could be a lot clearer.

net/mac80211/mesh.c static void ieee80211_mesh_work(struct work_struct *work) · line 697

Quoted 2 lines
net/mac80211/mesh.c +1 -1
-		mesh_path_start_discovery(sdata);
+		ifmsh->mesh_pp->ops->path_start_discovery(sdata);

ditto

net/mac80211/mesh.h struct mesh_rmc { · line 176

Quoted 9 lines
net/mac80211/mesh.h +5 -0
@@ -169,6 +169,57 @@ struct mesh_rmc {
  */
 #define MESH_PREQ_MIN_INT	10
 #define MESH_DIAM_TRAVERSAL_TIME 50
+
+/* Default Path Selection, Path Metric, Congestion Control Mode */
+#define MESH_PATH_SELECTION_PROTOCOL_ID 0x000FACFF
+#define MESH_PATH_SELECTION_METRIC_ID 0x000FACFF
+#define MESH_CONGESTION_CONTROL_MODE_ID 0x000FACFF

These are "null", and you should probably define them in a proper header (linux/ieee80211.h), not this internal one.

net/mac80211/mesh.h struct mesh_rmc { · line 185

Quoted 2 lines
net/mac80211/mesh.h +2 -0
+/**
+  * struct mesh_path_sel_ops - Mesh Path Selection Protocol function pointers

Is it conceivable that somebody wants a MPSP _outside_ net/mac80211/? Say an external test module? In that case all this needs to go into include/net/mac80211.h. Mind you, in that case you cannot pass around internal structs like "sub_if_data" or "mesh_path".

However, if you don't want to clean it up to allow that, I oppose adding all the EXPORT_SYMBOLs and will ask you to not modularise the algorithms into kernel modules but rather just make them selectable at mac80211 build time and build them all into mac80211.ko.

net/mac80211/mesh.h struct mesh_rmc { · line 196

Quoted 2 lines
net/mac80211/mesh.h +2 -0
+  * This resembles the main part of the mesh path selection protocol
+  * abstracion layer. Every new PS protocol has to define every member.

typo "abstraction", see above for "PS" or just remove the "PS" here.

net/mac80211/mesh.h struct mesh_rmc { · line 209

Quoted 12 lines
net/mac80211/mesh.h +12 -0
+struct mesh_path_sel_ops {
+	void (*path_start_discovery) (struct ieee80211_sub_if_data *sdata);
+	int (*nexthop_lookup) (struct sk_buff *skb,
+		struct ieee80211_sub_if_data *sdata);
+	void (*queue_preq) (struct mesh_path *mpath, u8 flags);
+	int (*path_error_tx) (u8 *dst, __le32 dst_dsn, u8 *ra,
+		struct ieee80211_sub_if_data *sdata);
+	void (*rx_path_sel_frame) (struct ieee80211_sub_if_data *sdata,
+		struct ieee80211_mgmt *mgmt, size_t len);
+	void (*path_sel_start) (struct ieee80211_if_mesh *sta);
+	void (*path_sel_stop) (struct ieee80211_if_mesh *sta);
+};

no space between ) (, please indent to the right depth of the parameters.

net/mac80211/mesh.h struct mesh_rmc { · line 218

Quoted 8 lines
net/mac80211/mesh.h +8 -0
+#define MESH_ALGO_NAME_MAX    (16)
+struct mesh_path_sel_proto {
+	struct list_head list;
+	char name[MESH_ALGO_NAME_MAX];
+	u32 id;
+	struct mesh_path_sel_ops *ops;
+	struct module *owner;
+};

Why bother with two structs? You also need to make it perfectly clear that the "list" member is not for public consumption but internal.

net/mac80211/mesh.h void mesh_path_discard_frame(struct sk_buff *skb, · line 329

Quoted 3 lines
net/mac80211/mesh.h +1 -0
 #ifdef CONFIG_MAC80211_MESH
 extern int mesh_allocated;
+extern struct mesh_path_sel_proto mesh_path_sel_hwmp;

Why is that not a pointer?

net/mac80211/mesh_hwmp.c line 19

Quoted 1 line
net/mac80211/mesh_hwmp.c +1 -0
+struct mesh_path_sel_proto mesh_path_sel_hwmp;

??? You just pulled that declaration from mesh.h

net/mac80211/mesh_hwmp.c enddiscovery: · line 771

Quoted 3 lines
net/mac80211/mesh_hwmp.c +1 -1
-int mesh_nexthop_lookup(struct sk_buff *skb,
+static int mesh_nexthop_lookup(struct sk_buff *skb,
 			struct ieee80211_sub_if_data *sdata)

please adjust the parameter indent

net/mac80211/mesh_pathtbl.c line 29

Quoted 10 lines
net/mac80211/mesh_pathtbl.c +1 -1
index 3c72557..f8a795f 100644
@@ -26,7 +26,7 @@
 				time_after(jiffies, mpath->exp_time) && \
 				!(mpath->flags & MESH_PATH_FIXED))
 
-struct mpath_node {
+		struct mpath_node {

???

Quoted 24 lines
net/mac80211/mesh_pathtbl.c +20 -0
 	struct hlist_node list;
 	struct rcu_head rcu;
 	/* This indirection allows two different tables to point to the same
 
+void mesh_path_timer(unsigned long data)
+{
+	struct ieee80211_sub_if_data *sdata;
+	struct mesh_path *mpath;
+
+	rcu_read_lock();
+	mpath = (struct mesh_path *) data;
+	mpath = rcu_dereference(mpath);
+	if (!mpath)
+		goto endmpathtimer;
+	spin_lock_bh(&mpath->state_lock);
+	sdata = mpath->sdata;
+	if (mpath->flags & MESH_PATH_RESOLVED ||
+			(!(mpath->flags & MESH_PATH_RESOLVING)))
+		mpath->flags &= ~(MESH_PATH_RESOLVING | MESH_PATH_RESOLVED);
+	else if (mpath->discovery_retries < max_preq_retries(sdata)) {
+		++mpath->discovery_retries;
+		mpath->discovery_timeout *= 2;
+		sdata->u.mesh.mesh_pp->ops->queue_preq(mpath, 0);
+	} else {

net/mac80211/mesh_pathtbl.c void mesh_pathtbl_unregister(void) · line 684

Quoted 10 lines
net/mac80211/mesh_pathtbl.c +10 -0
+		mpath->flags = 0;
+		mpath->exp_time = jiffies;
+		mesh_path_flush_pending(mpath);
+	}
+
+	spin_unlock_bh(&mpath->state_lock);
+endmpathtimer:
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL(mesh_path_timer);

This export doesn't look right at all, explain.

net/mac80211/sta_info.c struct sta_info *sta_info_get(struct ieee80211_local *local, const u8 *addr) · line 108

Quoted 7 lines
net/mac80211/sta_info.c +1 -0
@@ -105,6 +105,7 @@ struct sta_info *sta_info_get(struct ieee80211_local *local, const u8 *addr)
 	}
 	return sta;
 }
+EXPORT_SYMBOL(sta_info_get);

absolutely not.

net/mac80211/sta_info.c struct sta_info *sta_info_get(struct ieee80211_local *local, const u8 *addr)

Quoted 9 lines
net/mac80211/sta_info.c +4 -1
+#ifdef CONFIG_MAC80211_MESH
 		else
-			if (mesh_nexthop_lookup(skb, osdata)) {
+			if (osdata->u.mesh.mesh_pp->ops->
+				nexthop_lookup(skb, osdata)) {
 				dev_put(odev);
 				return 0;
 			}
+#endif

see above - static inlines, and those can get ifdefs.

Generally, this looks like a decent idea, but the execution definitely is lacking. You need to put more thought into proper boundaries/APIs between the various parts of things instead of blindly exporting all symbols that might be necessary.

Also, it is good etiquette to CC the maintainer and people who have previously worked on the code or commented on your patches.

johannes

#9 lore raw

PATCH 1/1 iw: Added capability to set individual mesh IDs

In reply to Florian Sesser

This allows the setting of
* Path selection protocol ID
* Path selection metric ID
* Congestion control mode ID

---
 mesh.c    |   26 ++++++++++++++++++++++++++
 nl80211.h |    3 +++
 2 files changed, 29 insertions(+), 0 deletions(-)
mesh.c +26 -0
index 932196e..5dabe19 100644
@@ -93,6 +93,18 @@ static uint32_t _parse_u32(const char *str, _any *ret)
 	return 0;
 }
 
+static uint32_t _parse_hex_u32(const char *str, _any *ret)
+{
+	char *endptr = NULL;
+	long long int v = strtoll(str, &endptr, 16);
+	if (*endptr != '\0')
+		return 0xffffffff;
+	if ((v < 0) || (v > 0xffffffff))
+		return 0xffffffff;
+	ret->u.as_32 = (uint32_t)v;
+	return 0;
+}
+
 static void _print_u8(struct nlattr *a)
 {
 	printf("%d", nla_get_u8(a));
@@ -123,6 +135,11 @@ static void _print_u32_in_TUs(struct nlattr *a)
 	printf("%d TUs", nla_get_u32(a));
 }
 
+void _print_u32_as_hex(struct nlattr *a)
+{
+	printf("%08X", nla_get_u32(a));
+}
+
 /* The current mesh parameters */
 const static struct mesh_param_descr _mesh_param_descrs[] =
 {
@@ -165,6 +182,15 @@ const static struct mesh_param_descr _mesh_param_descrs[] =
 	{"mesh_hwmp_net_diameter_traversal_time",
 	NL80211_MESHCONF_HWMP_NET_DIAM_TRVS_TIME,
 	_my_nla_put_u16, _parse_u16, _print_u16_in_TUs},
+	{"mesh_path_selection_protocol_id",
+	NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID,
+	_my_nla_put_u32, _parse_hex_u32, _print_u32_as_hex},
+	{"mesh_path_selection_metric_id",
+	NL80211_MESHCONF_PATH_SELECTION_METRIC_ID,
+	_my_nla_put_u32, _parse_hex_u32, _print_u32_as_hex},
+	{"mesh_congestion_control_mode_id",
+	NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID,
+	_my_nla_put_u32, _parse_hex_u32, _print_u32_as_hex},
 };
 
 static void print_all_mesh_param_descr(void)
nl80211.h +3 -0
index 4bc2704..150029c 100644
@@ -795,6 +795,9 @@ enum nl80211_meshconf_params {
 	NL80211_MESHCONF_HWMP_ACTIVE_PATH_TIMEOUT,
 	NL80211_MESHCONF_HWMP_PREQ_MIN_INTERVAL,
 	NL80211_MESHCONF_HWMP_NET_DIAM_TRVS_TIME,
+	NL80211_MESHCONF_PATH_SELECTION_PROTOCOL_ID,
+	NL80211_MESHCONF_PATH_SELECTION_METRIC_ID,
+	NL80211_MESHCONF_CONGESTION_CONTROL_MODE_ID,
 
 	/* keep last */
 	__NL80211_MESHCONF_ATTR_AFTER_LAST,
Signature · 1 line
1.5.6.5