diff mbox series

[v2,7/7] batman-adv: ELP - add throughput meter test duration attribute

Message ID 20180518014754.23644-8-mareklindner@neomailbox.ch (mailing list archive)
State Superseded, archived
Delegated to: Simon Wunderlich
Headers
Series B.A.T.M.A.N. V - fallback to tp meter estimation if throughput otherwise not available |

Commit Message

Marek Lindner May 18, 2018, 1:47 a.m. UTC 
  When the ELP throughput meter fallback kicks in to trigger
a throughput meter measurement the test duration can be
configured via this attribute.

Default tp test duration: 1000ms

Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
---
 Documentation/ABI/testing/sysfs-class-net-batman-adv | 7 +++++++
 net/batman-adv/bat_v.c                               | 1 +
 net/batman-adv/bat_v_elp.c                           | 5 ++++-
 net/batman-adv/sysfs.c                               | 3 +++
 net/batman-adv/types.h                               | 3 +++
 5 files changed, 18 insertions(+), 1 deletion(-)

Comments

Linus Lüssing May 21, 2018, 1:46 p.m. UTC | #1 
On Fri, May 18, 2018 at 09:47:54AM +0800, Marek Lindner wrote:
> When the ELP throughput meter fallback kicks in to trigger
> a throughput meter measurement the test duration can be
> configured via this attribute.
> 
> Default tp test duration: 1000ms

Would it make sense to note the adjusted default tp test duration
in the commit message, too? It is adjusted from 10ms to 1000ms
here, right?


I'm also wondering if it would make sense to make the test
interval adjustable instead of the duration:

With 1000ms on a 16MBit/s DSL line this would generate 864GB of
traffic per month and would be an issue for several existing
setups right now.

Is there some minimum test duration at which the tp meter is
supposed to not work realiably anymore, where an increased check
period would be more suitable?

Regards, Linus
Linus Lüssing May 21, 2018, 1:57 p.m. UTC | #2 
On Mon, May 21, 2018 at 03:46:57PM +0200, Linus Lüssing wrote:
> With 1000ms on a 16MBit/s DSL line this would generate 864GB of
> traffic per month and would be an issue for several existing
> setups right now.

Sorry, 86GB per month:

16 [MBit/s] / 8 [bits->bytes] * 60 [minutes] * 24 [hours] * 30
[days] / 1000 [MB->GB] = 86GB
Sven Eckelmann May 21, 2018, 2:34 p.m. UTC | #3 
On Freitag, 18. Mai 2018 03:47:54 CEST Marek Lindner wrote:
> When the ELP throughput meter fallback kicks in to trigger
> a throughput meter measurement the test duration can be
> configured via this attribute.
> 
> Default tp test duration: 1000ms
> 
> Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
> ---
>  Documentation/ABI/testing/sysfs-class-net-batman-adv | 7 +++++++

Please discuss this with Jiri [1].

Kind regards,
	Sven


[1] https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2018-May/017814.html
Antonio Quartulli Aug. 4, 2018, 8:41 a.m. UTC | #4 
Hi,

On 21/05/18 22:34, Sven Eckelmann wrote:
> On Freitag, 18. Mai 2018 03:47:54 CEST Marek Lindner wrote:
>> When the ELP throughput meter fallback kicks in to trigger
>> a throughput meter measurement the test duration can be
>> configured via this attribute.
>>
>> Default tp test duration: 1000ms
>>
>> Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
>> ---
>>  Documentation/ABI/testing/sysfs-class-net-batman-adv | 7 +++++++
> 
> Please discuss this with Jiri [1].
> 

After re-reading Jiri's points I can't really understand why we should
now switch to netlink. I think all our sysfs knobs are used to inject
settings *to* userspace, therefore his point 1) does not really apply to
us. Point 2) is a bit generic and does not really explain why we should
*switch*.

This said, I'd rather keep this patch as it is and possibly discuss the
matter when sending this code to netdev for merging.

I've discussed this with Marek too and he is fine with this approach.

Cheers,
> 
> [1] https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2018-May/017814.html
>
Sven Eckelmann Aug. 4, 2018, 9:02 a.m. UTC | #5 
On Samstag, 4. August 2018 10:41:09 CEST Antonio Quartulli wrote:
[...]
> >>  Documentation/ABI/testing/sysfs-class-net-batman-adv | 7 +++++++
> > 
> > Please discuss this with Jiri [1].
> > 
> 
> After re-reading Jiri's points I can't really understand why we should
> now switch to netlink. I think all our sysfs knobs are used to inject
> settings *to* userspace, therefore his point 1) does not really apply to
> us. Point 2) is a bit generic and does not really explain why we should
> *switch*.

Wouldn't it have been better when Jiri would also see your reply? Now he isn't 
even aware of your criticism.

> This said, I'd rather keep this patch as it is and possibly discuss the
> matter when sending this code to netdev for merging.
> 
> I've discussed this with Marek too and he is fine with this approach.

Interesting, I don't see it this way. This patch [0] is for netdev and we have 
the statement that "usage of sysfs in netdev subsystem is frowned upon". Now 
you want that we ignore that and than maybe Simon have to deal with the 
fallout when he forwards it to David?

Kind regards,
	Sven

[0] https://patchwork.open-mesh.org/patch/17372/

> > [1] https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2018-May/017814.html
Marek Lindner Aug. 4, 2018, 9:05 a.m. UTC | #6 
On Monday, 21 May 2018 21:46:57 HKT Linus Lüssing wrote:
> On Fri, May 18, 2018 at 09:47:54AM +0800, Marek Lindner wrote:
> > When the ELP throughput meter fallback kicks in to trigger
> > a throughput meter measurement the test duration can be
> > configured via this attribute.
> > 
> > Default tp test duration: 1000ms
> 
> Would it make sense to note the adjusted default tp test duration
> in the commit message, too? It is adjusted from 10ms to 1000ms
> here, right?

I am having a hard time following your thoughts here. The default duration is 
part of the commit message. The user space tp meter test is not affected by 
this change. The tp meter ELP duration of 10ms default was introduced in the 
previous patch only. 
Anyway, will change the previous patch to use 1000ms.


> I'm also wondering if it would make sense to make the test
> interval adjustable instead of the duration:
> 
> With 1000ms on a 16MBit/s DSL line this would generate 864GB of
> traffic per month and would be an issue for several existing
> setups right now.

Assuming you are talking about the batman-adv-over-VPN-over-internet use-case:
Simply set the interface throughput to 16MBit/s to disable the ELP throughput 
meter measuring altogether (see throughput_override).

ELP throughput measuring is not built to improve that use-case.


> Is there some minimum test duration at which the tp meter is
> supposed to not work realiably anymore, where an increased check
> period would be more suitable?

The ELP throughput meter test is designed to handle interface / neighbors with 
fluctuating link throughput. For those setups, having up-to-date link 
throughput information is what makes this worthwhile.

Static DSL uplinks don't fall into that category. Every regular throughput 
check (no matter how rare) is worse than no test at all. To handle those case, 
batman-adv already provides a knob.

Cheers,
Marek
Antonio Quartulli Aug. 4, 2018, 9:08 a.m. UTC | #7 
Hi,

On 04/08/18 17:02, Sven Eckelmann wrote:
> On Samstag, 4. August 2018 10:41:09 CEST Antonio Quartulli wrote:
> [...]
>>>>  Documentation/ABI/testing/sysfs-class-net-batman-adv | 7 +++++++
>>>
>>> Please discuss this with Jiri [1].
>>>
>>
>> After re-reading Jiri's points I can't really understand why we should
>> now switch to netlink. I think all our sysfs knobs are used to inject
>> settings *to* userspace, therefore his point 1) does not really apply to
>> us. Point 2) is a bit generic and does not really explain why we should
>> *switch*.
> 
> Wouldn't it have been better when Jiri would also see your reply? Now he isn't 
> even aware of your criticism.
> 
>> This said, I'd rather keep this patch as it is and possibly discuss the
>> matter when sending this code to netdev for merging.
>>
>> I've discussed this with Marek too and he is fine with this approach.
> 
> Interesting, I don't see it this way. This patch [0] is for netdev and we have 
> the statement that "usage of sysfs in netdev subsystem is frowned upon". Now 
> you want that we ignore that and than maybe Simon have to deal with the 
> fallout when he forwards it to David?

Nope, I'd rather step in myself, should David or anybody else complain
about the patch once sent to netdev. But I totally understand your point.

Personally I did not see Jiri's statements as representative of netdev
as a whole, but it looked like his personal opinion to me (I might be
wrong).

I will reply to [1] directly so we can take the discussion from there then.

Cheers,

> 
> Kind regards,
> 	Sven
> 
> [0] https://patchwork.open-mesh.org/patch/17372/
> 
>>> [1] https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2018-May/017814.html
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-class-net-batman-adv b/Documentation/ABI/testing/sysfs-class-net-batman-adv
index 89810684..7b3974a5 100644
--- a/Documentation/ABI/testing/sysfs-class-net-batman-adv
+++ b/Documentation/ABI/testing/sysfs-class-net-batman-adv
@@ -6,6 +6,13 @@  Description:
                 Defines the interval in milliseconds in which batman
                 emits probing packets for neighbor sensing (ELP).
 
+What:           /sys/class/net/<iface>/batman-adv/elp_tp_duration
+Date:           May 2018
+Contact:        Marek Lindner <mareklindner@neomailbox.ch>
+Description:
+                Defines measurement duration in milliseconds upon
+                ELP fallback throughput meter measurements.
+
 What:           /sys/class/net/<iface>/batman-adv/iface_status
 Date:           May 2010
 Contact:        Marek Lindner <mareklindner@neomailbox.ch>
diff --git a/net/batman-adv/bat_v.c b/net/batman-adv/bat_v.c
index ec93337e..be068516 100644
--- a/net/batman-adv/bat_v.c
+++ b/net/batman-adv/bat_v.c
@@ -1084,6 +1084,7 @@  void batadv_v_hardif_init(struct batadv_hard_iface *hard_iface)
 	 */
 	atomic_set(&hard_iface->bat_v.throughput_override, 0);
 	atomic_set(&hard_iface->bat_v.elp_interval, 500);
+	atomic_set(&hard_iface->bat_v.elp_tp_duration, 1000);
 }
 
 /**
diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c
index 3f0d7816..818a9a6a 100644
--- a/net/batman-adv/bat_v_elp.c
+++ b/net/batman-adv/bat_v_elp.c
@@ -76,9 +76,12 @@  static void batadv_v_elp_tp_start(struct batadv_hardif_neigh_node *neigh)
 {
 	struct batadv_hard_iface *hard_iface = neigh->if_incoming;
 	struct batadv_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
+	u32 elp_tp_duration;
 
 	neigh->bat_v.tp_meter_running = true;
-	batadv_tp_start(bat_priv, neigh->addr, neigh, 10, NULL, BATADV_TP_ELP);
+	elp_tp_duration = atomic_read(&hard_iface->bat_v.elp_tp_duration);
+	batadv_tp_start(bat_priv, neigh->addr, neigh, elp_tp_duration,
+			NULL, BATADV_TP_ELP);
 }
 
 /**
diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c
index f2eef43b..d8d08bfa 100644
--- a/net/batman-adv/sysfs.c
+++ b/net/batman-adv/sysfs.c
@@ -1128,6 +1128,8 @@  static BATADV_ATTR(iface_status, 0444, batadv_show_iface_status, NULL);
 #ifdef CONFIG_BATMAN_ADV_BATMAN_V
 BATADV_ATTR_HIF_UINT(elp_interval, bat_v.elp_interval, 0644,
 		     2 * BATADV_JITTER, INT_MAX, NULL);
+BATADV_ATTR_HIF_UINT(elp_tp_duration, bat_v.elp_tp_duration, 0644,
+		     1, INT_MAX, NULL);
 static BATADV_ATTR(throughput_override, 0644, batadv_show_throughput_override,
 		   batadv_store_throughput_override);
 #endif
@@ -1137,6 +1139,7 @@  static struct batadv_attribute *batadv_batman_attrs[] = {
 	&batadv_attr_iface_status,
 #ifdef CONFIG_BATMAN_ADV_BATMAN_V
 	&batadv_attr_elp_interval,
+	&batadv_attr_elp_tp_duration,
 	&batadv_attr_throughput_override,
 #endif
 	NULL,
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 148f23ad..8efe295a 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -117,6 +117,9 @@  struct batadv_hard_iface_bat_v {
 	/** @elp_interval: time interval between two ELP transmissions */
 	atomic_t elp_interval;
 
+	/** @elp_tp_duration: throughput meter fallback test duration */
+	atomic_t elp_tp_duration;
+
 	/** @elp_seqno: current ELP sequence number */
 	atomic_t elp_seqno;