mac80211 / o11s mesh: Modularize Path Selection Protocol
Subsystems (2)
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 mymesh_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
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
@@ -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
@@ -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
@@ -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
@@ -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
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
@@ -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
@@ -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
@@ -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
@@ -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
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
@@ -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
@@ -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
Johannes Berg
maintainerRe: [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> setmesh_parammesh_path_selection_protocol_id000F0F0F (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 OFHave you asked them about using their OUI?
Quoted 20 lines
Signed-off-by Florian Sesser <[email protected]>--- 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.cnet/mac80211/Kconfig +1
-3@@ -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
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 OFHave 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
Johannes Berg
maintainerRe: [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
0x000FACFFmeant '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
Johannes Berg
maintainerRe: [PATCH 2/3] mac80211: Modul. mesh path prot: Comm w/ Userspace
In reply to Florian SesserQuoted 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.hinclude/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
Johannes Berg
maintainerRe: [PATCH 1/3] mac80211: Modularize Mesh path selection protocol
In reply to Florian Sessernet/mac80211/mesh.c void ieee80211s_init(void) · line 37
Quoted 4 lines · Florian Sesser
net/mac80211/mesh.c +2
-0rm_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
-1void 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@@ -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
-0struct 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
PATCH 1/1 iw: Added capability to set individual mesh IDs
In reply to Florian SesserThis 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
@@ -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
@@ -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