[03/17] batman-adv: Add network_coding and mcast sysfs files to README

  From: Sven Eckelmann <sven@narfation.org>

Signed-off-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
---
 Documentation/networking/batman-adv.txt | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
  

Thu, Oct 27, 2016 at 09:01:36PM CEST, sw@simonwunderlich.de wrote:
>From: Sven Eckelmann <sven@narfation.org>
>
>Signed-off-by: Sven Eckelmann <sven@narfation.org>
>Signed-off-by: Simon Wunderlich <sw@simonwunderlich.de>
>---
> Documentation/networking/batman-adv.txt | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
>diff --git a/Documentation/networking/batman-adv.txt b/Documentation/networking/batman-adv.txt
>index d414e60..8afa991 100644
>--- a/Documentation/networking/batman-adv.txt
>+++ b/Documentation/networking/batman-adv.txt
>@@ -71,10 +71,11 @@ All  mesh  wide  settings  can be found in batman's own interface
> folder:
> 
> # ls /sys/class/net/bat0/mesh/
>-#aggregated_ogms        distributed_arp_table  gw_sel_class    orig_interval
>-#ap_isolation           fragmentation          hop_penalty     routing_algo
>-#bonding                gw_bandwidth           isolation_mark  vlan0
>-#bridge_loop_avoidance  gw_mode                log_level
>+# aggregated_ogms        fragmentation  isolation_mark  routing_algo
>+# ap_isolation           gw_bandwidth   log_level       vlan0
>+# bonding                gw_mode        multicast_mode
>+# bridge_loop_avoidance  gw_sel_class   network_coding
>+# distributed_arp_table  hop_penalty    orig_interval

I strongly believe it is a huge mistake to use sysfs for things like
this. This should be done via generic netlink api.
>
  

On Samstag, 29. Oktober 2016 12:33:01 CEST Jiri Pirko wrote:
[...]
> >--- a/Documentation/networking/batman-adv.txt
> >+++ b/Documentation/networking/batman-adv.txt
> >@@ -71,10 +71,11 @@ All  mesh  wide  settings  can be found in batman's own interface
> > folder:
> > 
> > # ls /sys/class/net/bat0/mesh/
> >-#aggregated_ogms        distributed_arp_table  gw_sel_class    orig_interval
> >-#ap_isolation           fragmentation          hop_penalty     routing_algo
> >-#bonding                gw_bandwidth           isolation_mark  vlan0
> >-#bridge_loop_avoidance  gw_mode                log_level
> >+# aggregated_ogms        fragmentation  isolation_mark  routing_algo
> >+# ap_isolation           gw_bandwidth   log_level       vlan0
> >+# bonding                gw_mode        multicast_mode
> >+# bridge_loop_avoidance  gw_sel_class   network_coding
> >+# distributed_arp_table  hop_penalty    orig_interval
> 
> I strongly believe it is a huge mistake to use sysfs for things like
> this. This should be done via generic netlink api.

This doesn't change the problem that it is already that way. This patch
only adds the list of available files to the README.

Kind regards,
	Sven
  

Sat, Oct 29, 2016 at 12:37:07PM CEST, sven@narfation.org wrote:
>On Samstag, 29. Oktober 2016 12:33:01 CEST Jiri Pirko wrote:
>[...]
>> >--- a/Documentation/networking/batman-adv.txt
>> >+++ b/Documentation/networking/batman-adv.txt
>> >@@ -71,10 +71,11 @@ All  mesh  wide  settings  can be found in batman's own interface
>> > folder:
>> > 
>> > # ls /sys/class/net/bat0/mesh/
>> >-#aggregated_ogms        distributed_arp_table  gw_sel_class    orig_interval
>> >-#ap_isolation           fragmentation          hop_penalty     routing_algo
>> >-#bonding                gw_bandwidth           isolation_mark  vlan0
>> >-#bridge_loop_avoidance  gw_mode                log_level
>> >+# aggregated_ogms        fragmentation  isolation_mark  routing_algo
>> >+# ap_isolation           gw_bandwidth   log_level       vlan0
>> >+# bonding                gw_mode        multicast_mode
>> >+# bridge_loop_avoidance  gw_sel_class   network_coding
>> >+# distributed_arp_table  hop_penalty    orig_interval
>> 
>> I strongly believe it is a huge mistake to use sysfs for things like
>> this. This should be done via generic netlink api.
>
>This doesn't change the problem that it is already that way. This patch
>only adds the list of available files to the README.

Sure. Just found out you did it like that. Therefore I commented. I
suggest to rework the api to use genl entirely.

>
>Kind regards,
>	Sven
  

On Samstag, 29. Oktober 2016 12:56:28 CEST Jiri Pirko wrote:
[...]
> >> I strongly believe it is a huge mistake to use sysfs for things like
> >> this. This should be done via generic netlink api.
> >
> >This doesn't change the problem that it is already that way. This patch
> >only adds the list of available files to the README.
> 
> Sure. Just found out you did it like that. Therefore I commented. I
> suggest to rework the api to use genl entirely.

Fair enough, I have added it to the issue tracker [1].

It seems there is no easy way to drop support for modifying batman-adv
attributes of the interface or its ports via sysfs in the near
future. But disallowing sysfs for new attributes might be a viable
policy.

Kind regards,
	Sven

[1] https://www.open-mesh.org/issues/300
  

Sat, Oct 29, 2016 at 01:46:59PM CEST, sven@narfation.org wrote:
>On Samstag, 29. Oktober 2016 12:56:28 CEST Jiri Pirko wrote:
>[...]
>> >> I strongly believe it is a huge mistake to use sysfs for things like
>> >> this. This should be done via generic netlink api.
>> >
>> >This doesn't change the problem that it is already that way. This patch
>> >only adds the list of available files to the README.
>> 
>> Sure. Just found out you did it like that. Therefore I commented. I
>> suggest to rework the api to use genl entirely.
>
>Fair enough, I have added it to the issue tracker [1].
>
>It seems there is no easy way to drop support for modifying batman-adv
>attributes of the interface or its ports via sysfs in the near
>future. But disallowing sysfs for new attributes might be a viable
>policy.

Cool. Thanks!


>
>Kind regards,
>	Sven
>
>[1] https://www.open-mesh.org/issues/300
  

On Sat, Oct 29, 2016 at 12:56:28PM +0200, Jiri Pirko wrote:
> >> I strongly believe it is a huge mistake to use sysfs for things like
> >> this. This should be done via generic netlink api.
> >
> >This doesn't change the problem that it is already that way. This patch
> >only adds the list of available files to the README.
> 
> Sure. Just found out you did it like that. Therefore I commented. I
> suggest to rework the api to use genl entirely.

Hi Jiri,

Thanks for sharing your thoughts!

Could you explain a bit more on which disadvantages you see in
the usage of sysfs here?

Regards, Linus
  

On Dienstag, 27. März 2018 17:43:08 CEST Linus Lüssing wrote:
> On Sat, Oct 29, 2016 at 12:56:28PM +0200, Jiri Pirko wrote:
> > >> I strongly believe it is a huge mistake to use sysfs for things like
> > >> this. This should be done via generic netlink api.
> > >
> > >This doesn't change the problem that it is already that way. This patch
> > >only adds the list of available files to the README.
> > 
> > Sure. Just found out you did it like that. Therefore I commented. I
> > suggest to rework the api to use genl entirely.
> 
> Hi Jiri,
> 
> Thanks for sharing your thoughts!
> 
> Could you explain a bit more on which disadvantages you see in
> the usage of sysfs here?

Linus is asking because of following patch: 
https://patchwork.open-mesh.org/patch/17340/

Kind regards,
	Sven
  

Hi Jiri,

seems like you still haven't answered Linus' question.

On Montag, 7. Mai 2018 08:34:16 CEST Sven Eckelmann wrote:
> On Dienstag, 27. März 2018 17:43:08 CEST Linus Lüssing wrote:
> > On Sat, Oct 29, 2016 at 12:56:28PM +0200, Jiri Pirko wrote:
> > > >> I strongly believe it is a huge mistake to use sysfs for things like
> > > >> this. This should be done via generic netlink api.
> > > >
> > > >This doesn't change the problem that it is already that way. This patch
> > > >only adds the list of available files to the README.
> > > 
> > > Sure. Just found out you did it like that. Therefore I commented. I
> > > suggest to rework the api to use genl entirely.
> > 
> > Hi Jiri,
> > 
> > Thanks for sharing your thoughts!
> > 
> > Could you explain a bit more on which disadvantages you see in
> > the usage of sysfs here?
> 
> Linus is asking because of following patch: 
> https://patchwork.open-mesh.org/patch/17340/

The next patch with a similar problem would be 
https://patchwork.open-mesh.org/patch/17372/. It is rather important that you 
are discussing this with Linus Luessing and Marek Lindner.

Kind regards,
	Sven
  

Tue, Mar 27, 2018 at 05:43:08PM CEST, linus.luessing@c0d3.blue wrote:
>On Sat, Oct 29, 2016 at 12:56:28PM +0200, Jiri Pirko wrote:
>> >> I strongly believe it is a huge mistake to use sysfs for things like
>> >> this. This should be done via generic netlink api.
>> >
>> >This doesn't change the problem that it is already that way. This patch
>> >only adds the list of available files to the README.
>> 
>> Sure. Just found out you did it like that. Therefore I commented. I
>> suggest to rework the api to use genl entirely.
>
>Hi Jiri,
>
>Thanks for sharing your thoughts!
>
>Could you explain a bit more on which disadvantages you see in
>the usage of sysfs here?

There are 2 major disadvantages.
1) You don't have any events on a change. An app has to poll in order to
   know what changed in kernel. Netlink handles this by sending
   multicast messages on a specific socket while whoever is interested
   gets the messages.
2) In sysfs, everything is string. There are even mixed values like
   "1 (means something)". There are no well defined values. Every driver
   can expose same things differently. In Netlink, you have well-defined
   attributes, with typed values. You can pass multiple attributes for
   the same value if needed.

In general, usage of sysfs in netdev subsystem is frowned upon. I would
suggest to convert your iface to Generic Netlink API and let the
existing sysfs API to rot.


>
>Regards, Linus
  

Hi Jiri,

On 20/05/18 14:19, Jiri Pirko wrote:
> Tue, Mar 27, 2018 at 05:43:08PM CEST, linus.luessing@c0d3.blue wrote:
>> On Sat, Oct 29, 2016 at 12:56:28PM +0200, Jiri Pirko wrote:
>>>>> I strongly believe it is a huge mistake to use sysfs for things like
>>>>> this. This should be done via generic netlink api.
>>>>
>>>> This doesn't change the problem that it is already that way. This patch
>>>> only adds the list of available files to the README.
>>>
>>> Sure. Just found out you did it like that. Therefore I commented. I
>>> suggest to rework the api to use genl entirely.
>>
>> Hi Jiri,
>>
>> Thanks for sharing your thoughts!
>>
>> Could you explain a bit more on which disadvantages you see in
>> the usage of sysfs here?
> 
> There are 2 major disadvantages.
> 1) You don't have any events on a change. An app has to poll in order to
>    know what changed in kernel. Netlink handles this by sending
>    multicast messages on a specific socket while whoever is interested
>    gets the messages.
> 2) In sysfs, everything is string. There are even mixed values like
>    "1 (means something)". There are no well defined values. Every driver
>    can expose same things differently. In Netlink, you have well-defined
>    attributes, with typed values. You can pass multiple attributes for
>    the same value if needed.
> 
> In general, usage of sysfs in netdev subsystem is frowned upon. I would
> suggest to convert your iface to Generic Netlink API and let the
> existing sysfs API to rot.

Do you have any pointer about where this discussion took place? I
imagine it happened in conjunction with some patches intended to other
drivers/netdev changes.

Reading that could give us a sense of how strict/important/severe this
decision was and how to prioritize future work.

I am asking because we have been working on a new feature since several
months and this feature introduces a new sysfs knob.

Now, although I understand the recommendation of switching to netlink, I
find it a bit impractical to delay a new (and fairly big) feature,
simply because it uses a potentially obsolete, but current, API.

Any opinion about this?


Thanks a lot


Regards,
  

Sat, Aug 04, 2018 at 11:24:11AM CEST, a@unstable.cc wrote:
>Hi Jiri,
>
>On 20/05/18 14:19, Jiri Pirko wrote:
>> Tue, Mar 27, 2018 at 05:43:08PM CEST, linus.luessing@c0d3.blue wrote:
>>> On Sat, Oct 29, 2016 at 12:56:28PM +0200, Jiri Pirko wrote:
>>>>>> I strongly believe it is a huge mistake to use sysfs for things like
>>>>>> this. This should be done via generic netlink api.
>>>>>
>>>>> This doesn't change the problem that it is already that way. This patch
>>>>> only adds the list of available files to the README.
>>>>
>>>> Sure. Just found out you did it like that. Therefore I commented. I
>>>> suggest to rework the api to use genl entirely.
>>>
>>> Hi Jiri,
>>>
>>> Thanks for sharing your thoughts!
>>>
>>> Could you explain a bit more on which disadvantages you see in
>>> the usage of sysfs here?
>> 
>> There are 2 major disadvantages.
>> 1) You don't have any events on a change. An app has to poll in order to
>>    know what changed in kernel. Netlink handles this by sending
>>    multicast messages on a specific socket while whoever is interested
>>    gets the messages.
>> 2) In sysfs, everything is string. There are even mixed values like
>>    "1 (means something)". There are no well defined values. Every driver
>>    can expose same things differently. In Netlink, you have well-defined
>>    attributes, with typed values. You can pass multiple attributes for
>>    the same value if needed.
>> 
>> In general, usage of sysfs in netdev subsystem is frowned upon. I would
>> suggest to convert your iface to Generic Netlink API and let the
>> existing sysfs API to rot.
>
>Do you have any pointer about where this discussion took place? I
>imagine it happened in conjunction with some patches intended to other
>drivers/netdev changes.
>
>Reading that could give us a sense of how strict/important/severe this
>decision was and how to prioritize future work.
>
>I am asking because we have been working on a new feature since several
>months and this feature introduces a new sysfs knob.
>
>Now, although I understand the recommendation of switching to netlink, I
>find it a bit impractical to delay a new (and fairly big) feature,
>simply because it uses a potentially obsolete, but current, API.
>
>Any opinion about this?

I agree, that does not make sense.

I just wanted you to consider introducing netlink iface and migrate to
it as it is generally the preffered way to comunicate with userspace in
networking area (I don't have pointer any specific discussion though -
it is just a common knowledge :)). I will be more then happy to help you
with that. You should look at net/core/devlink.c and net/wireless/nl80211.c
to get some inspiration.


>
>
>Thanks a lot
>
>
>Regards,
>
>
>-- 
>Antonio Quartulli
>
  

On 04/08/18 17:36, Jiri Pirko wrote:
>>
>> Do you have any pointer about where this discussion took place? I
>> imagine it happened in conjunction with some patches intended to other
>> drivers/netdev changes.
>>
>> Reading that could give us a sense of how strict/important/severe this
>> decision was and how to prioritize future work.
>>
>> I am asking because we have been working on a new feature since several
>> months and this feature introduces a new sysfs knob.
>>
>> Now, although I understand the recommendation of switching to netlink, I
>> find it a bit impractical to delay a new (and fairly big) feature,
>> simply because it uses a potentially obsolete, but current, API.
>>
>> Any opinion about this?
> 
> I agree, that does not make sense.

Thanks for your reply, Jiri.

> 
> I just wanted you to consider introducing netlink iface and migrate to
> it as it is generally the preffered way to comunicate with userspace in
> networking area (I don't have pointer any specific discussion though -
> it is just a common knowledge :)).

That's ok. I was asking because in the past batman-adv was using debugfs
for dealing with settings and we were (properly) redirected to sysfs by
David.
Now it seems we need to migrate to the next thing :-) So I just wanted
to be sure we have to do it, but this seems to be the case..

> I will be more then happy to help you
> with that. You should look at net/core/devlink.c and net/wireless/nl80211.c
> to get some inspiration.

Actually we have already implemented a basic netlink API to be used when
sending information to userspace (i.e. routing tables, neighbour tables,
etc..), therefore I think we might be able to leverage on that.
But of course, any help will be appreciated :)

Thanks!

Cheers,
  

Sat, Aug 04, 2018 at 01:19:39PM CEST, a@unstable.cc wrote:
>On 04/08/18 17:36, Jiri Pirko wrote:
>>>
>>> Do you have any pointer about where this discussion took place? I
>>> imagine it happened in conjunction with some patches intended to other
>>> drivers/netdev changes.
>>>
>>> Reading that could give us a sense of how strict/important/severe this
>>> decision was and how to prioritize future work.
>>>
>>> I am asking because we have been working on a new feature since several
>>> months and this feature introduces a new sysfs knob.
>>>
>>> Now, although I understand the recommendation of switching to netlink, I
>>> find it a bit impractical to delay a new (and fairly big) feature,
>>> simply because it uses a potentially obsolete, but current, API.
>>>
>>> Any opinion about this?
>> 
>> I agree, that does not make sense.
>
>Thanks for your reply, Jiri.
>
>> 
>> I just wanted you to consider introducing netlink iface and migrate to
>> it as it is generally the preffered way to comunicate with userspace in
>> networking area (I don't have pointer any specific discussion though -
>> it is just a common knowledge :)).
>
>That's ok. I was asking because in the past batman-adv was using debugfs
>for dealing with settings and we were (properly) redirected to sysfs by
>David.
>Now it seems we need to migrate to the next thing :-) So I just wanted
>to be sure we have to do it, but this seems to be the case..
>
>> I will be more then happy to help you
>> with that. You should look at net/core/devlink.c and net/wireless/nl80211.c
>> to get some inspiration.
>
>Actually we have already implemented a basic netlink API to be used when
>sending information to userspace (i.e. routing tables, neighbour tables,
>etc..), therefore I think we might be able to leverage on that.
>But of course, any help will be appreciated :)

Yes, I see net/batman-adv/netlink.c. That looks fine. Shouldn't be hard
to migrate the existing sysfs things there and add new features.
Please don't forget to echo the configured options via netlink
notifications from the very beginning. Feel free to send me patches to
look at.

Thanks!


>
>Thanks!
>
>Cheers,
>
>
>
>-- 
>Antonio Quartulli
>