[v2,7/7] batman-adv: ELP - add throughput meter test duration attribute
Commit Message
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
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
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
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
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
>
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
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
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
@@ -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>
@@ -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);
}
/**
@@ -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);
}
/**
@@ -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,
@@ -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;