[RFC,v4,00/19] batman-adv: netlink restructuring, part 2

  Hi,

Jiri Pirko called the batman-adv configuration interface a while back "a
huge mistake" [1] and suggested genl as alternative. The first
reimplementation [2] (based on the team/devlink design) was rejected. This
second implementation is now using a nl80211-like design. The objects which
can now be modified/queried are:

* mesh/soft-interface (BATADV_CMD_GET_MESH/BATADV_CMD_SET_MESH)
* slave/hard-interface (BATADV_CMD_GET_HARDIF/BATADV_CMD_SET_HARDIF)
* vlan (BATADV_CMD_GET_VLAN/BATADV_CMD_SET_VLAN)

Such a set messages automatically triggers a *_SET_* "reply" via the config
multicast group. The same behavior was also added to the sysfs interface.

I would like to ask the other developers to check whether their
configuration values are represented correctly. And please also check
whether the current implementation might collide with some other features
which you might have in your pipeline.

Unfortunately, Linus Luessing is the only batman-adv developer who provided
feedback.

There were also two topics which were not yet really discussed and thus
these requests (from Linus) were not yet implemented:

* convert BATADV_ATTR_MULTICAST_MODE_ENABLED to an u32 and let don't handle
  it like a boolean. Instead use it to select how multicast traffic has to
  be handled:
  
  - 0: ignore multicast optimization and just flood it like broadcast
    traffic
  - 1: enabled multicast optimization
  - 2: undefined but also some kind of multicast optimization
  - 3: undefined but also some kind of multicast of optimization
  - ...

* convert BATADV_ATTR_AGGREGATION_OGM_ENABLED to u32 and use it
  to mark which type of traffic should be aggregated:

  - bit 0: enable aggregation of OGM(2)s
  - bit 1: yet undefined packet type which allows some kind of aggregation
  - bit 2: yet undefined packet type which allows some kind of aggregation
  - ...

v4:
===

Incorporate various feedback from

* Linus Luessing:

  - Fix typos/spelling/grammar problems:

    + "already allow" -> "already allows"
    + "mesh mesh" -> "mesh"
    + "safed" -> "saved"

  - Let batadv_get_softif_from_info actually return net_device instead of
    net_device private data (batadv_priv)
  - store genl_info_net(info) in temporary variable to avoid line breaks
    in function parameters
  - document that batadv_get_*_from_info functions increase the reference
    counter of the returned object
  - directly drop reference counter to hard_dev net_device in
    batadv_get_hardif_from_info instead of doing that in batadv_post_doit
  - fix NULL pointer deref of primary_if when batadv device doesn't have
    any slave device
  - rewrite user_ptr[1] flags precheck in batadv_pre_doit to count the
    number of set bits (to only allow one of the flags which use
    user_ptr[1])

* Jiri Pirko:

  - Add _ENABLED Postfix for boolean attributes:
  
    + BATADV_ATTR_AGGREGATED_OGMS_ENABLED
    + BATADV_ATTR_AP_ISOLATION_ENABLED
    + BATADV_ATTR_BONDING_ENABLED
    + BATADV_ATTR_BRIDGE_LOOP_AVOIDANCE_ENABLED
    + BATADV_ATTR_DISTRIBUTED_ARP_TABLE_ENABLED
    + BATADV_ATTR_FRAGMENTATION_ENABLED
    + BATADV_ATTR_MULTICAST_MODE_ENABLED
    + BATADV_ATTR_NETWORK_CODING_ENABLED

v3:
===

* correctly assign kernel-doc for BATADV_GW_MODE_CLIENT
* fix validation check for gw.sel_class
* fix setting of BATADV_ATTR_ORIG_INTERVAL
* Cc the (most likely) responsible developers for each setting to more
  directly ask them for feedback

v2:
===

* rewrite everything in a more nl80211-like message/attribute format.

Kind regards,
        Sven

[1] https://www.open-mesh.org/issues/300
[2] https://patchwork.open-mesh.org/cover/17610/

Sven Eckelmann (19):
  batman-adv: Move common genl doit code pre/post hooks
  batman-adv: Prepare framework for mesh genl config
  batman-adv: Prepare framework for hardif genl config
  batman-adv: Prepare framework for vlan genl config
  batman-adv: Add aggregated_ogms mesh genl configuration
  batman-adv: Add ap_isolation mesh/vlan genl configuration
  batman-adv: Add bonding mesh genl configuration
  batman-adv: Add bridge_loop_avoidance mesh genl configuration
  batman-adv: Add distributed_arp_table mesh genl configuration
  batman-adv: Add fragmentation mesh genl configuration
  batman-adv: Add gateway mesh genl configuration
  batman-adv: Add hop_penalty mesh genl configuration
  batman-adv: Add log_level mesh genl configuration
  batman-adv: Add multicast_mode mesh genl configuration
  batman-adv: Add network_coding mesh genl configuration
  batman-adv: Add orig_interval mesh genl configuration
  batman-adv: Add elp_interval hardif genl configuration
  batman-adv: Add throughput_override hardif genl configuration
  batman-adv: Trigger genl notification on sysfs config change

 include/uapi/linux/batman_adv.h |  189 +++++-
 net/batman-adv/gateway_client.c |    1 -
 net/batman-adv/gateway_common.c |    1 +
 net/batman-adv/gateway_common.h |    6 -
 net/batman-adv/netlink.c        | 1080 +++++++++++++++++++++++++++----
 net/batman-adv/netlink.h        |    6 +
 net/batman-adv/soft-interface.c |    2 +-
 net/batman-adv/sysfs.c          |   64 +-
 8 files changed, 1202 insertions(+), 147 deletions(-)
  

Sat, Jan 19, 2019 at 04:56:07PM CET, sven@narfation.org wrote:
>Hi,
>
>Jiri Pirko called the batman-adv configuration interface a while back "a
>huge mistake" [1] and suggested genl as alternative. The first
>reimplementation [2] (based on the team/devlink design) was rejected. This
>second implementation is now using a nl80211-like design. The objects which
>can now be modified/queried are:
>
>* mesh/soft-interface (BATADV_CMD_GET_MESH/BATADV_CMD_SET_MESH)
>* slave/hard-interface (BATADV_CMD_GET_HARDIF/BATADV_CMD_SET_HARDIF)
>* vlan (BATADV_CMD_GET_VLAN/BATADV_CMD_SET_VLAN)
>
>Such a set messages automatically triggers a *_SET_* "reply" via the config
>multicast group. The same behavior was also added to the sysfs interface.

In general aside of couple of nits I described, this iteration of
the patchset looks very nice to me. Thanks!
  

On Saturday, 19 January 2019 16.56.07 CET Sven Eckelmann wrote:
[...]
> There were also two topics which were not yet really discussed and thus
> these requests (from Linus) were not yet implemented:

@Jiri, @Linus maybe you can discuss these topics further and select the 
correct solution.

> * convert BATADV_ATTR_MULTICAST_MODE_ENABLED to an u32 and let don't handle
>   it like a boolean. Instead use it to select how multicast traffic has to
>   be handled:
>   
>   - 0: ignore multicast optimization and just flood it like broadcast
>     traffic
>   - 1: enabled multicast optimization
>   - 2: undefined but also some kind of multicast optimization
>   - 3: undefined but also some kind of multicast of optimization
>   - ...

Multicast mode is currently defined.

* according to batctl manpage:

    multicast_mode|mm [0|1]
           If no parameter is given the current multicast mode set‐
           ting is displayed. Otherwise the parameter is used to en‐
           able or disable multicast optimizations (i.e. disabling
           means always sending own multicast frames via classic
           flooding).

* according to sysfs ABI:

   What:           /sys/class/net/<mesh_iface>/mesh/multicast_mode
   Date:           Feb 2014
   Contact:        Linus Lüssing <linus.luessing@web.de>
   Description:
                   Indicates whether multicast optimizations are enabled
                   or disabled. If set to zero then all nodes in the
                   mesh are going to use classic flooding for any
                   multicast packet with no optimizations.

Both define it as boolean value and therefore it was converted to a boolean 
value (via u8) in netlink.

But Linus now suggested that it is actually an u32. Most likely 0 == to 
something like BATADV_MULTICAST_MODE_FLOODING. But I have no idea what 1 is or 
what 2, 3, 4, .. would be. So I need some input here.

And Jiri said that it should be renamed to BATADV_ATTR_MULTICAST_ENABLED -
which seems to suggest that he doesn't like the idea of a u32 for some reason
and prefers to use a boolean value.

And now Linus even said that it should be a bit field - which makes it even 
more vague to me and I have now absolutely no idea what should be implemented.

* BIT 0 for flooding vs ?
* BIT 1 for ?
* ...

> * convert BATADV_ATTR_AGGREGATION_OGM_ENABLED to u32 and use it
>   to mark which type of traffic should be aggregated:
> 
>   - bit 0: enable aggregation of OGM(2)s
>   - bit 1: yet undefined packet type which allows some kind of aggregation
>   - bit 2: yet undefined packet type which allows some kind of aggregation
>   - ...

Aggregated OGM is currently defined as:


* according to batctl manpage:

    aggregation|ag [0|1]
           If no parameter is given the current aggregation setting
           is displayed. Otherwise the parameter is used to enable or
           disable OGM packet aggregation.

* according to sysfs ABI:

    What:           /sys/class/net/<mesh_iface>/mesh/aggregated_ogms
    Date:           May 2010
    Contact:        Marek Lindner <mareklindner@neomailbox.ch>
    Description:
                    Indicates whether the batman protocol messages of the
                    mesh <mesh_iface> shall be aggregated or not.

So sysfs is only one possible backend for the batctl command. There is 
currently nothing which I would assume to be aggregatable beside OGMs but let 
us assume for now that there is now something and some way to aggregate things 
beside OGMs in a save and backward compatible way. Let's call this FOO - so we 
have BATADV_ATTR_AGGREGATION_OGM_ENABLED and 
BATADV_ATTR_AGGREGATION_FOO_ENABLED. Or we have BATADV_ATTR_AGGREGATION as an 
u32 and just use the second bit as marker for FOO (and of course the first bit 
as marker for OGM).

Would it now be more preferable to use BATADV_ATTR_AGGREGATION_OGM_ENABLED as 
u8 (boolean) or to to switch to BATADV_ATTR_AGGREGATION (u32) & assign single 
bits to packet types.

Kind regards,
	Sven
  

Sat, Jan 26, 2019 at 11:47:20AM CET, sven@narfation.org wrote:
>On Saturday, 19 January 2019 16.56.07 CET Sven Eckelmann wrote:
>[...]
>> There were also two topics which were not yet really discussed and thus
>> these requests (from Linus) were not yet implemented:
>
>@Jiri, @Linus maybe you can discuss these topics further and select the 
>correct solution.
>
>> * convert BATADV_ATTR_MULTICAST_MODE_ENABLED to an u32 and let don't handle
>>   it like a boolean. Instead use it to select how multicast traffic has to
>>   be handled:
>>   
>>   - 0: ignore multicast optimization and just flood it like broadcast
>>     traffic
>>   - 1: enabled multicast optimization
>>   - 2: undefined but also some kind of multicast optimization
>>   - 3: undefined but also some kind of multicast of optimization
>>   - ...
>
>Multicast mode is currently defined.
>
>* according to batctl manpage:
>
>    multicast_mode|mm [0|1]
>           If no parameter is given the current multicast mode set‐
>           ting is displayed. Otherwise the parameter is used to en‐
>           able or disable multicast optimizations (i.e. disabling
>           means always sending own multicast frames via classic
>           flooding).
>
>* according to sysfs ABI:
>
>   What:           /sys/class/net/<mesh_iface>/mesh/multicast_mode
>   Date:           Feb 2014
>   Contact:        Linus Lüssing <linus.luessing@web.de>
>   Description:
>                   Indicates whether multicast optimizations are enabled
>                   or disabled. If set to zero then all nodes in the
>                   mesh are going to use classic flooding for any
>                   multicast packet with no optimizations.
>
>Both define it as boolean value and therefore it was converted to a boolean 
>value (via u8) in netlink.
>
>But Linus now suggested that it is actually an u32. Most likely 0 == to 
>something like BATADV_MULTICAST_MODE_FLOODING. But I have no idea what 1 is or 
>what 2, 3, 4, .. would be. So I need some input here.
>
>And Jiri said that it should be renamed to BATADV_ATTR_MULTICAST_ENABLED -
>which seems to suggest that he doesn't like the idea of a u32 for some reason
>and prefers to use a boolean value.
>
>And now Linus even said that it should be a bit field - which makes it even 
>more vague to me and I have now absolutely no idea what should be implemented.

If it is bool, it should be bool. If it is a bitfield (future more bits
than one needed), it should be a bitfield.


>
>* BIT 0 for flooding vs ?
>* BIT 1 for ?
>* ...
>
>> * convert BATADV_ATTR_AGGREGATION_OGM_ENABLED to u32 and use it
>>   to mark which type of traffic should be aggregated:
>> 
>>   - bit 0: enable aggregation of OGM(2)s
>>   - bit 1: yet undefined packet type which allows some kind of aggregation
>>   - bit 2: yet undefined packet type which allows some kind of aggregation
>>   - ...
>
>Aggregated OGM is currently defined as:
>
>
>* according to batctl manpage:
>
>    aggregation|ag [0|1]
>           If no parameter is given the current aggregation setting
>           is displayed. Otherwise the parameter is used to enable or
>           disable OGM packet aggregation.
>
>* according to sysfs ABI:
>
>    What:           /sys/class/net/<mesh_iface>/mesh/aggregated_ogms
>    Date:           May 2010
>    Contact:        Marek Lindner <mareklindner@neomailbox.ch>
>    Description:
>                    Indicates whether the batman protocol messages of the
>                    mesh <mesh_iface> shall be aggregated or not.
>
>So sysfs is only one possible backend for the batctl command. There is 
>currently nothing which I would assume to be aggregatable beside OGMs but let 
>us assume for now that there is now something and some way to aggregate things 
>beside OGMs in a save and backward compatible way. Let's call this FOO - so we 
>have BATADV_ATTR_AGGREGATION_OGM_ENABLED and 
>BATADV_ATTR_AGGREGATION_FOO_ENABLED. Or we have BATADV_ATTR_AGGREGATION as an 
>u32 and just use the second bit as marker for FOO (and of course the first bit 
>as marker for OGM).
>
>Would it now be more preferable to use BATADV_ATTR_AGGREGATION_OGM_ENABLED as 
>u8 (boolean) or to to switch to BATADV_ATTR_AGGREGATION (u32) & assign single 
>bits to packet types.
>
>Kind regards,
>	Sven
  

On Saturday, January 26, 2019 11:47:20 AM CET Sven Eckelmann wrote:
> Aggregated OGM is currently defined as:
> 
> 
> * according to batctl manpage:
> 
>     aggregation|ag [0|1]
>            If no parameter is given the current aggregation setting
>            is displayed. Otherwise the parameter is used to enable or
>            disable OGM packet aggregation.
> 
> * according to sysfs ABI:
> 
>     What:           /sys/class/net/<mesh_iface>/mesh/aggregated_ogms
>     Date:           May 2010
>     Contact:        Marek Lindner <mareklindner@neomailbox.ch>
>     Description:
>                     Indicates whether the batman protocol messages of the
>                     mesh <mesh_iface> shall be aggregated or not.
> 
> So sysfs is only one possible backend for the batctl command. There is 
> currently nothing which I would assume to be aggregatable beside OGMs but
> let  us assume for now that there is now something and some way to
> aggregate things beside OGMs in a save and backward compatible way. Let's
> call this FOO - so we have BATADV_ATTR_AGGREGATION_OGM_ENABLED and
> BATADV_ATTR_AGGREGATION_FOO_ENABLED. Or we have BATADV_ATTR_AGGREGATION as
> an  u32 and just use the second bit as marker for FOO (and of course the
> first bit as marker for OGM).
> 
> Would it now be more preferable to use BATADV_ATTR_AGGREGATION_OGM_ENABLED
> as  u8 (boolean) or to to switch to BATADV_ATTR_AGGREGATION (u32) & assign
> single bits to packet types.

I'd prefer BATADV_ATTR_AGGREGATION_OGM_ENABLED (as we have your patchset now). 
Although it may be technically possible to aggregate other things (e.g. 
broadcasts), I don't think this will be implemented anytime soon, if at all. 
And if we do, we can just make another BATADV_ATTR_AGGREGATION_FOO_ENABLED 
flag.

Cheers,
       Simon
  

On Sat, Jan 26, 2019 at 11:47:20AM +0100, Sven Eckelmann wrote:
> On Saturday, 19 January 2019 16.56.07 CET Sven Eckelmann wrote:
> [...]
> > There were also two topics which were not yet really discussed and thus
> > these requests (from Linus) were not yet implemented:
> 
> @Jiri, @Linus maybe you can discuss these topics further and select the 
> correct solution.
> 
> > * convert BATADV_ATTR_MULTICAST_MODE_ENABLED to an u32 and let don't handle
> >   it like a boolean. Instead use it to select how multicast traffic has to
> >   be handled:
> >   
> >   - 0: ignore multicast optimization and just flood it like broadcast
> >     traffic
> >   - 1: enabled multicast optimization
> >   - 2: undefined but also some kind of multicast optimization
> >   - 3: undefined but also some kind of multicast of optimization
> >   - ...
> 
> Multicast mode is currently defined.
> 
> * according to batctl manpage:
> 
>     multicast_mode|mm [0|1]
>            If no parameter is given the current multicast mode set‐
>            ting is displayed. Otherwise the parameter is used to en‐
>            able or disable multicast optimizations (i.e. disabling
>            means always sending own multicast frames via classic
>            flooding).
> 
> * according to sysfs ABI:
> 
>    What:           /sys/class/net/<mesh_iface>/mesh/multicast_mode
>    Date:           Feb 2014
>    Contact:        Linus Lüssing <linus.luessing@web.de>
>    Description:
>                    Indicates whether multicast optimizations are enabled
>                    or disabled. If set to zero then all nodes in the
>                    mesh are going to use classic flooding for any
>                    multicast packet with no optimizations.
> 
> Both define it as boolean value and therefore it was converted to a boolean 
> value (via u8) in netlink.
> 
> But Linus now suggested that it is actually an u32. Most likely 0 == to 
> something like BATADV_MULTICAST_MODE_FLOODING. But I have no idea what 1 is or 
> what 2, 3, 4, .. would be. So I need some input here.

Right, 0 would be flooding. 1 is the current multicast-aware multi-to-single-unicast
conversion mechanism, which would be extended to a multi-to-multi-unicast mechanism
with the last multicast related patch to the batman mailing list.

In this particular case there is no need to assign a new
multicast_mode == 2, because the old approach can be achieved
again by setting multicast_fanout to 1.


After the multi-to-(multi-)unicast conversion mechanism the next
simple step I would have in mind would be adding a "struct
batadv_mcast_packet". Which would basically behave similar to a
normal batadv_unicast_packet, but would be able to hold a flexible
amount of destination addresses.


And then there was the old tracker packet idea which could come
after that.

And then, recently a source-specific-multicast, reverse tracker
packet approach was discussed. Which would have yet another magnitude
of complexity. But would have very compelling scalability properties.


From a user perspective, once someone relies on some degree of
multicast capabilities and if a new mechanism somehow fails for
the user (for instance a bug or special topology properties), it
might not be feasible to go back to flooding. For instance, let's
say with multicast-to-multi-unicast a Freifunk-like network were now
able to scale to 1500 nodes. And now we introduce some new
enhancements/mechanisms for multicast and things break. Then
flooding would likely leave a broken/unusable network for them
too, due to congestion.

Maybe seeing "multicast_mode" as a gear shift would be an analogy?


> And Jiri said that it should be renamed to BATADV_ATTR_MULTICAST_ENABLED -
> which seems to suggest that he doesn't like the idea of a u32 for some reason
> and prefers to use a boolean value.
> 
> And now Linus even said that it should be a bit field - which makes it even 
> more vague to me and I have now absolutely no idea what should be implemented.
> 
> * BIT 0 for flooding vs ?

Flooding would always be available.

BIT 0: Enable multicast-to-(multi-)unicast
BIT 1: Enable multicast-packet-type
BIT 2: Enable multicast-tracker-packets
BIT 3: Enable ssm-multicast-tracker-packets
...

The thing is, these mechanisms do not necessarilly have to be
exclusive. For instance, the tracker packet mechanism needs to
propagate the tracker packets for a second or so first before it
can be safely used for multicast data packets. In the mean time we
could either flood or buffer the packet. Or we could send them via
multicast-to-multi-unicast.

Hm, but maybe this is getting too flexible. If multicast_mode == n
would mean that all features 0 <= n are available would be fine,
too, I think.


Another thought, if all this is too vague for now... what about
ommiting the BATADV_ATTR_MULTICAST_(MODE)_ENABLED for now and use
a reverse logic instead? Like
BATADV_ATTR_MULTICAST_FORCEFLOOD_ENABLED, defaulting to false.
That still leaves the opportunity to add a BATADV_ATTR_MULTICAST_MODE
option or BATADV_ATTR_MULTICAST_FEATURES bitset later when we have
actual code needing it.

(btw., is there a general preference in netlink towards either
grouping things in bitfields or individual _ENABLED attributes?)

Regards, Linus
  

On Tuesday, 5 February 2019 20.24.10 CET Linus Lüssing wrote:
[...]
> Another thought, if all this is too vague for now... 

Yes, it is to vague for the integration in netlink :D

> what about
> ommiting the BATADV_ATTR_MULTICAST_(MODE)_ENABLED for now and use
> a reverse logic instead? Like
> BATADV_ATTR_MULTICAST_FORCEFLOOD_ENABLED, defaulting to false.

Would be fine for me. But it is rather odd to map this to the batctl 
multicast_mode. But the name is explaining quite well what it does
and you can later rewrite the batctl part.

Kind regards,
	Sven
  

On Wed, Feb 06, 2019 at 07:20:25PM +0100, Sven Eckelmann wrote:
> On Tuesday, 5 February 2019 20.24.10 CET Linus Lüssing wrote:
> [...]
> > Another thought, if all this is too vague for now... 
> 
> Yes, it is to vague for the integration in netlink :D
> 
> > what about
> > ommiting the BATADV_ATTR_MULTICAST_(MODE)_ENABLED for now and use
> > a reverse logic instead? Like
> > BATADV_ATTR_MULTICAST_FORCEFLOOD_ENABLED, defaulting to false.
> 
> Would be fine for me. But it is rather odd to map this to the batctl 
> multicast_mode. But the name is explaining quite well what it does
> and you can later rewrite the batctl part.

Ok, will rewrite the batctl part later then. And probably keep "batctl
mm" as a (hidden?) option for compatibility reasons?

Regards, Linus
  

On Wednesday, 6 February 2019 20:08:14 CET Linus Lüssing wrote:
> Ok, will rewrite the batctl part later then. And probably keep "batctl
> mm" as a (hidden?) option for compatibility reasons?

Yes, sounds good.

Kind regards,
	Sven