[B.A.T.M.A.N.] [PATCH v2 0/7] B.A.T.M.A.N. V - fallback to tp meter estimation if throughput otherwise not available
Under normal circumstances B.A.T.M.A.N. V retrieves the neighbor throughput values to populate its metric tables from the various drivers such as WiFi throughput tables and Ethernet throughput.. Whenever the interface drivers do not export link throughput information manual overrides become necessary. To further automate and thus better support these setups, ELP may call the batman-adv throughput meter to schedule a throughput estimation to be used to populate the metric table.
v2: * added sysfs attribute to configure tp meter test duration * fixed null pointer dereference in TP meter packet sending routine * fixed storing the measured throughput in the correct variable * checkpatch/kerneldoc/sparse/smatch cleanup
Antonio Quartulli (3): batman-adv: tp_meter - prevent concurrent tp_meter sessions by using workqueue batman-adv: tp_meter - don't check for existing session batman-adv: tp_meter - add option to perform one-hop test
Marek Lindner (4): batman-adv: tp_meter - allow up to 10 queued sessions batman-adv: tp_meter - add caller distinction batman-adv: ELP - use tp meter to estimate the throughput if otherwise not available batman-adv: ELP - add throughput meter test duration attribute
.../ABI/testing/sysfs-class-net-batman-adv | 7 + include/uapi/linux/batadv_packet.h | 2 + net/batman-adv/bat_v.c | 1 + net/batman-adv/bat_v_elp.c | 66 ++- net/batman-adv/bat_v_elp.h | 21 + net/batman-adv/main.c | 10 +- net/batman-adv/main.h | 7 +- net/batman-adv/netlink.c | 3 +- net/batman-adv/routing.c | 6 +- net/batman-adv/sysfs.c | 3 + net/batman-adv/tp_meter.c | 463 +++++++++++------- net/batman-adv/tp_meter.h | 11 +- net/batman-adv/types.h | 36 ++ 13 files changed, 438 insertions(+), 198 deletions(-)
From: Antonio Quartulli a@unstable.cc
To ensure that no more than one tp_meter session runs at the same time, use an ordered workqueue instead of spawning one kthread per session.
Signed-off-by: Antonio Quartulli a@unstable.cc --- net/batman-adv/main.c | 10 ++++- net/batman-adv/tp_meter.c | 77 +++++++++++++++++++-------------------- net/batman-adv/tp_meter.h | 3 +- net/batman-adv/types.h | 3 ++ 4 files changed, 50 insertions(+), 43 deletions(-)
diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c index 69c0d85b..cd3f1924 100644 --- a/net/batman-adv/main.c +++ b/net/batman-adv/main.c @@ -91,6 +91,10 @@ static int __init batadv_init(void) if (ret < 0) return ret;
+ ret = batadv_tp_meter_init(); + if (ret < 0) + goto err_tp_meter; + INIT_LIST_HEAD(&batadv_hardif_list); batadv_algo_init();
@@ -99,7 +103,6 @@ static int __init batadv_init(void) batadv_v_init(); batadv_iv_init(); batadv_nc_init(); - batadv_tp_meter_init();
batadv_event_workqueue = create_singlethread_workqueue("bat_events"); if (!batadv_event_workqueue) @@ -118,9 +121,11 @@ static int __init batadv_init(void) return 0;
err_create_wq: + batadv_tp_meter_destroy(); +err_tp_meter: batadv_tt_cache_destroy();
- return -ENOMEM; + return ret; }
static void __exit batadv_exit(void) @@ -138,6 +143,7 @@ static void __exit batadv_exit(void) rcu_barrier();
batadv_tt_cache_destroy(); + batadv_tp_meter_destroy(); }
/** diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c index 11520de9..c0d27f7d 100644 --- a/net/batman-adv/tp_meter.c +++ b/net/batman-adv/tp_meter.c @@ -32,7 +32,6 @@ #include <linux/jiffies.h> #include <linux/kernel.h> #include <linux/kref.h> -#include <linux/kthread.h> #include <linux/list.h> #include <linux/netdevice.h> #include <linux/param.h> @@ -97,6 +96,9 @@
static u8 batadv_tp_prerandom[4096] __read_mostly;
+/* ordered work queue */ +static struct workqueue_struct *batadv_tp_meter_queue; + /** * batadv_tp_session_cookie() - generate session cookie based on session ids * @session: TP session identifier @@ -808,19 +810,22 @@ static int batadv_tp_wait_available(struct batadv_tp_vars *tp_vars, size_t plen)
/** * batadv_tp_send() - main sending thread of a tp meter session - * @arg: address of the related tp_vars + * @work: delayed work reference of the related tp_vars * * Return: nothing, this function never returns */ -static int batadv_tp_send(void *arg) +static void batadv_tp_send(struct work_struct *work) { - struct batadv_tp_vars *tp_vars = arg; - struct batadv_priv *bat_priv = tp_vars->bat_priv; struct batadv_hard_iface *primary_if = NULL; struct batadv_orig_node *orig_node = NULL; + struct batadv_tp_vars *tp_vars; size_t payload_len, packet_len; + struct batadv_priv *bat_priv; int err = 0;
+ tp_vars = container_of(work, struct batadv_tp_vars, test_work); + bat_priv = tp_vars->bat_priv; + if (unlikely(tp_vars->role != BATADV_TP_SENDER)) { err = BATADV_TP_REASON_DST_UNREACHABLE; tp_vars->reason = err; @@ -901,40 +906,17 @@ static int batadv_tp_send(void *arg) batadv_tp_sender_cleanup(bat_priv, tp_vars);
batadv_tp_vars_put(tp_vars); - - do_exit(0); }
/** - * batadv_tp_start_kthread() - start new thread which manages the tp meter - * sender + * batadv_tp_start_work - start new thread which manages the tp meter sender * @tp_vars: the private data of the current TP meter session */ -static void batadv_tp_start_kthread(struct batadv_tp_vars *tp_vars) +static void batadv_tp_start_work(struct batadv_tp_vars *tp_vars) { - struct task_struct *kthread; - struct batadv_priv *bat_priv = tp_vars->bat_priv; - u32 session_cookie; - - kref_get(&tp_vars->refcount); - kthread = kthread_create(batadv_tp_send, tp_vars, "kbatadv_tp_meter"); - if (IS_ERR(kthread)) { - session_cookie = batadv_tp_session_cookie(tp_vars->session, - tp_vars->icmp_uid); - pr_err("batadv: cannot create tp meter kthread\n"); - batadv_tp_batctl_error_notify(BATADV_TP_REASON_MEMORY_ERROR, - tp_vars->other_end, - bat_priv, session_cookie); - - /* drop reserved reference for kthread */ - batadv_tp_vars_put(tp_vars); - - /* cleanup of failed tp meter variables */ - batadv_tp_sender_cleanup(bat_priv, tp_vars); - return; - } - - wake_up_process(kthread); + /* init work item that will actually execute the test and schedule it */ + INIT_WORK(&tp_vars->test_work, batadv_tp_send); + queue_work(batadv_tp_meter_queue, &tp_vars->test_work); }
/** @@ -1053,13 +1035,10 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, /* init work item for finished tp tests */ INIT_DELAYED_WORK(&tp_vars->finish_work, batadv_tp_sender_finish);
- /* start tp kthread. This way the write() call issued from userspace can - * happily return and avoid to block + /* schedule the tp worker. This way the write() call issued from + * userspace can happily return and avoid to block */ - batadv_tp_start_kthread(tp_vars); - - /* don't return reference to new tp_vars */ - batadv_tp_vars_put(tp_vars); + batadv_tp_start_work(tp_vars); }
/** @@ -1498,8 +1477,26 @@ void batadv_tp_meter_recv(struct batadv_priv *bat_priv, struct sk_buff *skb)
/** * batadv_tp_meter_init() - initialize global tp_meter structures + * + * Return: 0 on success, -ENOMEM if the tp_meter queue allocation fails */ -void __init batadv_tp_meter_init(void) +int __init batadv_tp_meter_init(void) { get_random_bytes(batadv_tp_prerandom, sizeof(batadv_tp_prerandom)); + + batadv_tp_meter_queue = alloc_ordered_workqueue("bat_tp_meter", 0); + if (!batadv_tp_meter_queue) + return -ENOMEM; + + return 0; +} + +/** + * batadv_tp_meter_destroy() - destroy tp meter memory allocations + */ +void batadv_tp_meter_destroy(void) +{ + flush_workqueue(batadv_tp_meter_queue); + destroy_workqueue(batadv_tp_meter_queue); + batadv_tp_meter_queue = NULL; } diff --git a/net/batman-adv/tp_meter.h b/net/batman-adv/tp_meter.h index 68e60097..ab0bde26 100644 --- a/net/batman-adv/tp_meter.h +++ b/net/batman-adv/tp_meter.h @@ -25,7 +25,8 @@
struct sk_buff;
-void batadv_tp_meter_init(void); +int batadv_tp_meter_init(void); +void batadv_tp_meter_destroy(void); void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, u32 test_length, u32 *cookie); void batadv_tp_stop(struct batadv_priv *bat_priv, const u8 *dst, diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index 360357f8..baae5206 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -1354,6 +1354,9 @@ struct batadv_tp_vars { /** @finish_work: work item for the finishing procedure */ struct delayed_work finish_work;
+ /** @test_work: work item for the test process */ + struct work_struct test_work; + /** @test_length: test length in milliseconds */ u32 test_length;
Hi,
looks like you have reference counting issues here:
On Freitag, 18. Mai 2018 09:47:48 CEST Marek Lindner wrote:
-static void batadv_tp_start_kthread(struct batadv_tp_vars *tp_vars) +static void batadv_tp_start_work(struct batadv_tp_vars *tp_vars) {
struct task_struct *kthread;struct batadv_priv *bat_priv = tp_vars->bat_priv;u32 session_cookie;kref_get(&tp_vars->refcount);kthread = kthread_create(batadv_tp_send, tp_vars, "kbatadv_tp_meter");if (IS_ERR(kthread)) {session_cookie = batadv_tp_session_cookie(tp_vars->session,tp_vars->icmp_uid);pr_err("batadv: cannot create tp meter kthread\n");batadv_tp_batctl_error_notify(BATADV_TP_REASON_MEMORY_ERROR,tp_vars->other_end,bat_priv, session_cookie);/* drop reserved reference for kthread */batadv_tp_vars_put(tp_vars);/* cleanup of failed tp meter variables */batadv_tp_sender_cleanup(bat_priv, tp_vars);return;}wake_up_process(kthread);
/* init work item that will actually execute the test and schedule it */INIT_WORK(&tp_vars->test_work, batadv_tp_send);queue_work(batadv_tp_meter_queue, &tp_vars->test_work);}
You completely ignore here that queue_work can fail. And then you have an item with a reference counter > 0 in memory and no one cares about it.
/** @@ -1053,13 +1035,10 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, /* init work item for finished tp tests */ INIT_DELAYED_WORK(&tp_vars->finish_work, batadv_tp_sender_finish);
/* start tp kthread. This way the write() call issued from userspace can* happily return and avoid to block
/* schedule the tp worker. This way the write() call issued from* userspace can happily return and avoid to block */
batadv_tp_start_kthread(tp_vars);/* don't return reference to new tp_vars */batadv_tp_vars_put(tp_vars);
batadv_tp_start_work(tp_vars);}
Just from the context of this function, it isn't clear what happened with the reference.
Kind regards, Sven
From: Antonio Quartulli a@unstable.cc
Since the conversion from kthread to queue worker it is not possible to run more than one "sender" session at a time. For this reason, checking if another session to the same destination is already scheduled is not useful anymore.
Remove such check and allow the user to enqueue a new session to a previously targeted node.
Signed-off-by: Antonio Quartulli a@unstable.cc --- net/batman-adv/tp_meter.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c index c0d27f7d..a416c36d 100644 --- a/net/batman-adv/tp_meter.c +++ b/net/batman-adv/tp_meter.c @@ -939,21 +939,7 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, session_cookie = batadv_tp_session_cookie(session_id, icmp_uid); *cookie = session_cookie;
- /* look for an already existing test towards this node */ - spin_lock_bh(&bat_priv->tp_list_lock); - tp_vars = batadv_tp_list_find(bat_priv, dst); - if (tp_vars) { - spin_unlock_bh(&bat_priv->tp_list_lock); - batadv_tp_vars_put(tp_vars); - batadv_dbg(BATADV_DBG_TP_METER, bat_priv, - "Meter: test to or from the same node already ongoing, aborting\n"); - batadv_tp_batctl_error_notify(BATADV_TP_REASON_ALREADY_ONGOING, - dst, bat_priv, session_cookie); - return; - } - if (!atomic_add_unless(&bat_priv->tp_num, 1, BATADV_TP_MAX_NUM)) { - spin_unlock_bh(&bat_priv->tp_list_lock); batadv_dbg(BATADV_DBG_TP_METER, bat_priv, "Meter: too many ongoing sessions, aborting (SEND)\n"); batadv_tp_batctl_error_notify(BATADV_TP_REASON_TOO_MANY, dst, @@ -963,10 +949,6 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst,
tp_vars = kmalloc(sizeof(*tp_vars), GFP_ATOMIC); if (!tp_vars) { - spin_unlock_bh(&bat_priv->tp_list_lock); - batadv_dbg(BATADV_DBG_TP_METER, bat_priv, - "Meter: %s cannot allocate list elements\n", - __func__); batadv_tp_batctl_error_notify(BATADV_TP_REASON_MEMORY_ERROR, dst, bat_priv, session_cookie); return; @@ -1021,6 +1003,7 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, spin_lock_init(&tp_vars->prerandom_lock);
kref_get(&tp_vars->refcount); + spin_lock_bh(&bat_priv->tp_list_lock); hlist_add_head_rcu(&tp_vars->list, &bat_priv->tp_list); spin_unlock_bh(&bat_priv->tp_list_lock);
Signed-off-by: Marek Lindner mareklindner@neomailbox.ch --- net/batman-adv/main.h | 6 ++++-- net/batman-adv/tp_meter.c | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h index 8da3c933..89dfaf87 100644 --- a/net/batman-adv/main.h +++ b/net/batman-adv/main.h @@ -137,9 +137,11 @@ #define BATADV_NC_NODE_TIMEOUT 10000 /* Milliseconds */
/** - * BATADV_TP_MAX_NUM - maximum number of simultaneously active tp sessions + * BATADV_TP_MAX_NUM_QUEUE - maximum number of queued (outgoing) tp sessions + * BATADV_TP_MAX_NUM_RECV - maximum number of simultaneous receiving streams */ -#define BATADV_TP_MAX_NUM 5 +#define BATADV_TP_MAX_NUM_QUEUE 10 +#define BATADV_TP_MAX_NUM_RECV 1
/** * enum batadv_mesh_state - State of a soft interface diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c index a416c36d..e89d3942 100644 --- a/net/batman-adv/tp_meter.c +++ b/net/batman-adv/tp_meter.c @@ -939,7 +939,7 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, session_cookie = batadv_tp_session_cookie(session_id, icmp_uid); *cookie = session_cookie;
- if (!atomic_add_unless(&bat_priv->tp_num, 1, BATADV_TP_MAX_NUM)) { + if (!atomic_add_unless(&bat_priv->tp_num, 1, BATADV_TP_MAX_NUM_QUEUE)) { batadv_dbg(BATADV_DBG_TP_METER, bat_priv, "Meter: too many ongoing sessions, aborting (SEND)\n"); batadv_tp_batctl_error_notify(BATADV_TP_REASON_TOO_MANY, dst, @@ -1313,7 +1313,7 @@ batadv_tp_init_recv(struct batadv_priv *bat_priv, if (tp_vars) goto out_unlock;
- if (!atomic_add_unless(&bat_priv->tp_num, 1, BATADV_TP_MAX_NUM)) { + if (!atomic_add_unless(&bat_priv->tp_num, 1, BATADV_TP_MAX_NUM_RECV)) { batadv_dbg(BATADV_DBG_TP_METER, bat_priv, "Meter: too many ongoing sessions, aborting (RECV)\n"); goto out_unlock;
The throughput meter can be called from user space as well as from the batman-adv kernel module itself. Add infrastructure to handle the different callers.
Signed-off-by: Marek Lindner mareklindner@neomailbox.ch --- net/batman-adv/netlink.c | 3 +- net/batman-adv/tp_meter.c | 108 ++++++++++++++++++++++---------------- net/batman-adv/tp_meter.h | 3 +- net/batman-adv/types.h | 13 +++++ 4 files changed, 79 insertions(+), 48 deletions(-)
diff --git a/net/batman-adv/netlink.c b/net/batman-adv/netlink.c index 0d9459b6..b0e1b73c 100644 --- a/net/batman-adv/netlink.c +++ b/net/batman-adv/netlink.c @@ -378,7 +378,8 @@ batadv_netlink_tp_meter_start(struct sk_buff *skb, struct genl_info *info) }
bat_priv = netdev_priv(soft_iface); - batadv_tp_start(bat_priv, dst, test_length, &cookie); + batadv_tp_start(bat_priv, dst, test_length, &cookie, + BATADV_TP_USERSPACE);
ret = batadv_netlink_tp_meter_put(msg, cookie);
diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c index e89d3942..50a0e4fa 100644 --- a/net/batman-adv/tp_meter.c +++ b/net/batman-adv/tp_meter.c @@ -215,50 +215,71 @@ static void batadv_tp_update_rto(struct batadv_tp_vars *tp_vars, }
/** - * batadv_tp_batctl_notify() - send client status result to client - * @reason: reason for tp meter session stop - * @dst: destination of tp_meter session + * batadv_tp_caller_notify() - send tp meter status result to caller * @bat_priv: the bat priv with all the soft interface information - * @start_time: start of transmission in jiffies - * @total_sent: bytes acked to the receiver - * @cookie: cookie of tp_meter session + * @tp_vars: the private data of the current TP meter session + * @reason: reason for tp meter session stop */ -static void batadv_tp_batctl_notify(enum batadv_tp_meter_reason reason, - const u8 *dst, struct batadv_priv *bat_priv, - unsigned long start_time, u64 total_sent, - u32 cookie) +static void batadv_tp_caller_notify(struct batadv_priv *bat_priv, + struct batadv_tp_vars *tp_vars, + enum batadv_tp_meter_reason reason) { - u32 test_time; - u8 result; u32 total_bytes; + u32 test_time; + u32 cookie; + bool reason_is_error;
- if (!batadv_tp_is_error(reason)) { - result = BATADV_TP_REASON_COMPLETE; - test_time = jiffies_to_msecs(jiffies - start_time); - total_bytes = total_sent; - } else { - result = reason; - test_time = 0; - total_bytes = 0; - } + reason_is_error = batadv_tp_is_error(reason); + + switch (tp_vars->caller) { + case BATADV_TP_USERSPACE: + cookie = batadv_tp_session_cookie(tp_vars->session, + tp_vars->icmp_uid);
- batadv_netlink_tpmeter_notify(bat_priv, dst, result, test_time, - total_bytes, cookie); + if (reason_is_error) { + batadv_netlink_tpmeter_notify(bat_priv, + tp_vars->other_end, + reason, 0, 0, cookie); + return; + } + + test_time = jiffies_to_msecs(jiffies - tp_vars->start_time); + total_bytes = atomic64_read(&tp_vars->tot_sent); + batadv_netlink_tpmeter_notify(bat_priv, tp_vars->other_end, + BATADV_TP_REASON_COMPLETE, + test_time, total_bytes, cookie); + + break; + case BATADV_TP_ELP: + break; + default: + break; + } }
/** - * batadv_tp_batctl_error_notify() - send client error result to client + * batadv_tp_caller_init_error() - report early tp meter errors to caller + * @bat_priv: the bat priv with all the soft interface information + * @caller: caller of tp meter session (user space or ELP) * @reason: reason for tp meter session stop * @dst: destination of tp_meter session - * @bat_priv: the bat priv with all the soft interface information * @cookie: cookie of tp_meter session */ -static void batadv_tp_batctl_error_notify(enum batadv_tp_meter_reason reason, - const u8 *dst, - struct batadv_priv *bat_priv, - u32 cookie) +static void batadv_tp_caller_init_error(struct batadv_priv *bat_priv, + enum batadv_tp_meter_caller caller, + enum batadv_tp_meter_reason reason, + const u8 *dst, u32 cookie) { - batadv_tp_batctl_notify(reason, dst, bat_priv, 0, 0, cookie); + switch (caller) { + case BATADV_TP_USERSPACE: + batadv_netlink_tpmeter_notify(bat_priv, dst, reason, 0, 0, + cookie); + break; + case BATADV_TP_ELP: + break; + default: + break; + } }
/** @@ -411,8 +432,6 @@ static void batadv_tp_sender_cleanup(struct batadv_priv *bat_priv, static void batadv_tp_sender_end(struct batadv_priv *bat_priv, struct batadv_tp_vars *tp_vars) { - u32 session_cookie; - batadv_dbg(BATADV_DBG_TP_METER, bat_priv, "Test towards %pM finished..shutting down (reason=%d)\n", tp_vars->other_end, tp_vars->reason); @@ -425,15 +444,7 @@ static void batadv_tp_sender_end(struct batadv_priv *bat_priv, "Final values: cwnd=%u ss_threshold=%u\n", tp_vars->cwnd, tp_vars->ss_threshold);
- session_cookie = batadv_tp_session_cookie(tp_vars->session, - tp_vars->icmp_uid); - - batadv_tp_batctl_notify(tp_vars->reason, - tp_vars->other_end, - bat_priv, - tp_vars->start_time, - atomic64_read(&tp_vars->tot_sent), - session_cookie); + batadv_tp_caller_notify(bat_priv, tp_vars, tp_vars->reason); }
/** @@ -925,9 +936,11 @@ static void batadv_tp_start_work(struct batadv_tp_vars *tp_vars) * @dst: the receiver MAC address * @test_length: test length in milliseconds * @cookie: session cookie + * @caller: caller of tp meter session (user space or ELP) */ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, - u32 test_length, u32 *cookie) + u32 test_length, u32 *cookie, + enum batadv_tp_meter_caller caller) { struct batadv_tp_vars *tp_vars; u8 session_id[2]; @@ -942,15 +955,17 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, if (!atomic_add_unless(&bat_priv->tp_num, 1, BATADV_TP_MAX_NUM_QUEUE)) { batadv_dbg(BATADV_DBG_TP_METER, bat_priv, "Meter: too many ongoing sessions, aborting (SEND)\n"); - batadv_tp_batctl_error_notify(BATADV_TP_REASON_TOO_MANY, dst, - bat_priv, session_cookie); + batadv_tp_caller_init_error(bat_priv, caller, + BATADV_TP_REASON_TOO_MANY, dst, + session_cookie); return; }
tp_vars = kmalloc(sizeof(*tp_vars), GFP_ATOMIC); if (!tp_vars) { - batadv_tp_batctl_error_notify(BATADV_TP_REASON_MEMORY_ERROR, - dst, bat_priv, session_cookie); + batadv_tp_caller_init_error(bat_priv, caller, + BATADV_TP_REASON_MEMORY_ERROR, dst, + session_cookie); return; }
@@ -958,6 +973,7 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, ether_addr_copy(tp_vars->other_end, dst); kref_init(&tp_vars->refcount); tp_vars->role = BATADV_TP_SENDER; + tp_vars->caller = caller; atomic_set(&tp_vars->sending, 1); memcpy(tp_vars->session, session_id, sizeof(session_id)); tp_vars->icmp_uid = icmp_uid; diff --git a/net/batman-adv/tp_meter.h b/net/batman-adv/tp_meter.h index ab0bde26..3b11a3e9 100644 --- a/net/batman-adv/tp_meter.h +++ b/net/batman-adv/tp_meter.h @@ -28,7 +28,8 @@ struct sk_buff; int batadv_tp_meter_init(void); void batadv_tp_meter_destroy(void); void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, - u32 test_length, u32 *cookie); + u32 test_length, u32 *cookie, + enum batadv_tp_meter_caller caller); void batadv_tp_stop(struct batadv_priv *bat_priv, const u8 *dst, u8 return_value); void batadv_tp_meter_recv(struct batadv_priv *bat_priv, struct sk_buff *skb); diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index baae5206..b38ca166 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -1323,6 +1323,16 @@ enum batadv_tp_meter_role { BATADV_TP_SENDER };
+/** + * enum batadv_tp_meter_caller - initiator of the tp meter session + * @BATADV_TP_USERSPACE: initiated by user space + * @BATADV_TP_ELP: initiated by ELP + */ +enum batadv_tp_meter_caller { + BATADV_TP_USERSPACE, + BATADV_TP_ELP +}; + /** * struct batadv_tp_vars - tp meter private variables per session */ @@ -1345,6 +1355,9 @@ struct batadv_tp_vars { /** @role: receiver/sender modi */ enum batadv_tp_meter_role role;
+ /** @caller: caller of tp meter session (user space or ELP) */ + enum batadv_tp_meter_caller caller; + /** @sending: sending binary semaphore: 1 if sending, 0 is not */ atomic_t sending;
From: Antonio Quartulli a@unstable.cc
A link test is a TP session ran over a specific one-hop link, rather than towards an originator in the mesh.
Signed-off-by: Antonio Quartulli a@unstable.cc --- include/uapi/linux/batadv_packet.h | 2 + net/batman-adv/netlink.c | 2 +- net/batman-adv/routing.c | 6 +- net/batman-adv/tp_meter.c | 232 +++++++++++++++++++---------- net/batman-adv/tp_meter.h | 5 +- net/batman-adv/types.h | 3 + 6 files changed, 167 insertions(+), 83 deletions(-)
diff --git a/include/uapi/linux/batadv_packet.h b/include/uapi/linux/batadv_packet.h index 894d8d2f..4e6e075b 100644 --- a/include/uapi/linux/batadv_packet.h +++ b/include/uapi/linux/batadv_packet.h @@ -351,10 +351,12 @@ struct batadv_icmp_tp_packet { * enum batadv_icmp_tp_subtype - ICMP TP Meter packet subtypes * @BATADV_TP_MSG: Msg from sender to receiver * @BATADV_TP_ACK: acknowledgment from receiver to sender + * @BATADV_TP_MSG_LINK: Msg from sender to receiver used for link test (one-hop) */ enum batadv_icmp_tp_subtype { BATADV_TP_MSG = 0, BATADV_TP_ACK, + BATADV_TP_MSG_LINK, };
#define BATADV_RR_LEN 16 diff --git a/net/batman-adv/netlink.c b/net/batman-adv/netlink.c index b0e1b73c..064020cc 100644 --- a/net/batman-adv/netlink.c +++ b/net/batman-adv/netlink.c @@ -378,7 +378,7 @@ batadv_netlink_tp_meter_start(struct sk_buff *skb, struct genl_info *info) }
bat_priv = netdev_priv(soft_iface); - batadv_tp_start(bat_priv, dst, test_length, &cookie, + batadv_tp_start(bat_priv, dst, NULL, test_length, &cookie, BATADV_TP_USERSPACE);
ret = batadv_netlink_tp_meter_put(msg, cookie); diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c index cc3ed93a..dbf2d556 100644 --- a/net/batman-adv/routing.c +++ b/net/batman-adv/routing.c @@ -223,12 +223,14 @@ bool batadv_check_management_packet(struct sk_buff *skb, /** * batadv_recv_my_icmp_packet() - receive an icmp packet locally * @bat_priv: the bat priv with all the soft interface information + * @recv_if: interface that the skb is received on * @skb: icmp packet to process * * Return: NET_RX_SUCCESS if the packet has been consumed or NET_RX_DROP * otherwise. */ static int batadv_recv_my_icmp_packet(struct batadv_priv *bat_priv, + struct batadv_hard_iface *recv_if, struct sk_buff *skb) { struct batadv_hard_iface *primary_if = NULL; @@ -281,7 +283,7 @@ static int batadv_recv_my_icmp_packet(struct batadv_priv *bat_priv, if (!pskb_may_pull(skb, sizeof(struct batadv_icmp_tp_packet))) goto out;
- batadv_tp_meter_recv(bat_priv, skb); + batadv_tp_meter_recv(bat_priv, recv_if, skb); ret = NET_RX_SUCCESS; /* skb was consumed */ skb = NULL; @@ -418,7 +420,7 @@ int batadv_recv_icmp_packet(struct sk_buff *skb,
/* packet for me */ if (batadv_is_my_mac(bat_priv, icmph->dst)) - return batadv_recv_my_icmp_packet(bat_priv, skb); + return batadv_recv_my_icmp_packet(bat_priv, recv_if, skb);
/* TTL exceeded */ if (icmph->ttl < 2) diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c index 50a0e4fa..87aaeb1d 100644 --- a/net/batman-adv/tp_meter.c +++ b/net/batman-adv/tp_meter.c @@ -381,6 +381,9 @@ static void batadv_tp_vars_release(struct kref *ref) } spin_unlock_bh(&tp_vars->unacked_lock);
+ if (tp_vars->hardif_neigh) + batadv_hardif_neigh_put(tp_vars->hardif_neigh); + kfree_rcu(tp_vars, rcu); }
@@ -579,9 +582,8 @@ static void batadv_tp_fill_prerandom(struct batadv_tp_vars *tp_vars,
/** * batadv_tp_send_msg() - send a single message + * @bat_priv: the bat priv with all the soft interface information * @tp_vars: the private TP meter data for this session - * @src: source mac address - * @orig_node: the originator of the destination * @seqno: sequence number of this packet * @len: length of the entire packet * @session: session identifier @@ -594,26 +596,56 @@ static void batadv_tp_fill_prerandom(struct batadv_tp_vars *tp_vars, * not reachable, BATADV_TP_REASON_MEMORY_ERROR if the packet couldn't be * allocated */ -static int batadv_tp_send_msg(struct batadv_tp_vars *tp_vars, const u8 *src, - struct batadv_orig_node *orig_node, - u32 seqno, size_t len, const u8 *session, - int uid, u32 timestamp) +static int batadv_tp_send_msg(struct batadv_priv *bat_priv, + struct batadv_tp_vars *tp_vars, u32 seqno, + size_t len, const u8 *session, int uid, + u32 timestamp) { + struct batadv_hard_iface *primary_if = NULL; + struct batadv_orig_node *orig_node = NULL; struct batadv_icmp_tp_packet *icmp; struct sk_buff *skb; - int r; - u8 *data; + int r, ret = 0; + u8 *data, *src, *dst, subtype; + struct net_device *netdev; size_t data_len;
- skb = netdev_alloc_skb_ip_align(NULL, len + ETH_HLEN); - if (unlikely(!skb)) - return BATADV_TP_REASON_MEMORY_ERROR; + /* link test */ + if (tp_vars->hardif_neigh) { + dst = tp_vars->hardif_neigh->addr; + src = tp_vars->hardif_neigh->if_incoming->net_dev->dev_addr; + subtype = BATADV_TP_MSG_LINK; + netdev = tp_vars->hardif_neigh->if_incoming->net_dev; + } else { + orig_node = batadv_orig_hash_find(bat_priv, tp_vars->other_end); + if (unlikely(!orig_node)) { + ret = BATADV_TP_REASON_DST_UNREACHABLE; + goto out; + } + + primary_if = batadv_primary_if_get_selected(bat_priv); + if (unlikely(!primary_if)) { + ret = BATADV_TP_REASON_DST_UNREACHABLE; + goto out; + } + + dst = orig_node->orig; + src = primary_if->net_dev->dev_addr; + subtype = BATADV_TP_MSG; + netdev = NULL; + } + + skb = netdev_alloc_skb_ip_align(netdev, len + ETH_HLEN); + if (unlikely(!skb)) { + ret = BATADV_TP_REASON_MEMORY_ERROR; + goto out; + }
skb_reserve(skb, ETH_HLEN); icmp = skb_put(skb, sizeof(*icmp));
/* fill the icmp header */ - ether_addr_copy(icmp->dst, orig_node->orig); + ether_addr_copy(icmp->dst, dst); ether_addr_copy(icmp->orig, src); icmp->version = BATADV_COMPAT_VERSION; icmp->packet_type = BATADV_ICMP; @@ -621,7 +653,7 @@ static int batadv_tp_send_msg(struct batadv_tp_vars *tp_vars, const u8 *src, icmp->msg_type = BATADV_TP; icmp->uid = uid;
- icmp->subtype = BATADV_TP_MSG; + icmp->subtype = subtype; memcpy(icmp->session, session, sizeof(icmp->session)); icmp->seqno = htonl(seqno); icmp->timestamp = htonl(timestamp); @@ -630,11 +662,23 @@ static int batadv_tp_send_msg(struct batadv_tp_vars *tp_vars, const u8 *src, data = skb_put(skb, data_len); batadv_tp_fill_prerandom(tp_vars, data, data_len);
- r = batadv_send_skb_to_orig(skb, orig_node, NULL); - if (r == NET_XMIT_SUCCESS) - return 0; + if (tp_vars->hardif_neigh) + r = batadv_send_skb_packet(skb, + tp_vars->hardif_neigh->if_incoming, + dst); + else + r = batadv_send_skb_to_orig(skb, orig_node, NULL);
- return BATADV_TP_REASON_CANT_SEND; + if (unlikely(r != NET_XMIT_SUCCESS)) + ret = BATADV_TP_REASON_CANT_SEND; + +out: + if (likely(primary_if)) + batadv_hardif_put(primary_if); + if (likely(orig_node)) + batadv_orig_node_put(orig_node); + + return ret; }
/** @@ -653,7 +697,6 @@ static void batadv_tp_recv_ack(struct batadv_priv *bat_priv, struct batadv_tp_vars *tp_vars; size_t packet_len, mss; u32 rtt, recv_ack, cwnd; - unsigned char *dev_addr;
packet_len = BATADV_TP_PLEN; mss = BATADV_TP_PLEN; @@ -675,13 +718,11 @@ static void batadv_tp_recv_ack(struct batadv_priv *bat_priv, (u32)atomic_read(&tp_vars->last_acked))) goto out;
- primary_if = batadv_primary_if_get_selected(bat_priv); - if (unlikely(!primary_if)) - goto out; - - orig_node = batadv_orig_hash_find(bat_priv, icmp->orig); - if (unlikely(!orig_node)) - goto out; + if (!tp_vars->hardif_neigh) { + primary_if = batadv_primary_if_get_selected(bat_priv); + if (unlikely(!primary_if)) + goto out; + }
/* update RTO with the new sampled RTT, if any */ rtt = jiffies_to_msecs(jiffies) - ntohl(icmp->timestamp); @@ -703,8 +744,11 @@ static void batadv_tp_recv_ack(struct batadv_priv *bat_priv, goto out;
/* if this is the third duplicate ACK do Fast Retransmit */ - batadv_tp_send_msg(tp_vars, primary_if->net_dev->dev_addr, - orig_node, recv_ack, packet_len, + + /* if we have a hardif_neigh, it means that this is a LINK test, + * therefore use the according function + */ + batadv_tp_send_msg(bat_priv, tp_vars, recv_ack, packet_len, icmp->session, icmp->uid, jiffies_to_msecs(jiffies));
@@ -741,9 +785,7 @@ static void batadv_tp_recv_ack(struct batadv_priv *bat_priv, * immediately as specified by NewReno (see * Section 3.2 of RFC6582 for details) */ - dev_addr = primary_if->net_dev->dev_addr; - batadv_tp_send_msg(tp_vars, dev_addr, - orig_node, recv_ack, + batadv_tp_send_msg(bat_priv, tp_vars, recv_ack, packet_len, icmp->session, icmp->uid, jiffies_to_msecs(jiffies)); @@ -827,8 +869,6 @@ static int batadv_tp_wait_available(struct batadv_tp_vars *tp_vars, size_t plen) */ static void batadv_tp_send(struct work_struct *work) { - struct batadv_hard_iface *primary_if = NULL; - struct batadv_orig_node *orig_node = NULL; struct batadv_tp_vars *tp_vars; size_t payload_len, packet_len; struct batadv_priv *bat_priv; @@ -843,20 +883,6 @@ static void batadv_tp_send(struct work_struct *work) goto out; }
- orig_node = batadv_orig_hash_find(bat_priv, tp_vars->other_end); - if (unlikely(!orig_node)) { - err = BATADV_TP_REASON_DST_UNREACHABLE; - tp_vars->reason = err; - goto out; - } - - primary_if = batadv_primary_if_get_selected(bat_priv); - if (unlikely(!primary_if)) { - err = BATADV_TP_REASON_DST_UNREACHABLE; - tp_vars->reason = err; - goto out; - } - /* assume that all the hard_interfaces have a correctly * configured MTU, so use the soft_iface MTU as MSS. * This might not be true and in that case the fragmentation @@ -883,10 +909,9 @@ static void batadv_tp_send(struct work_struct *work) */ packet_len = payload_len + sizeof(struct batadv_unicast_packet);
- err = batadv_tp_send_msg(tp_vars, primary_if->net_dev->dev_addr, - orig_node, tp_vars->last_sent, - packet_len, - tp_vars->session, tp_vars->icmp_uid, + err = batadv_tp_send_msg(bat_priv, tp_vars, tp_vars->last_sent, + packet_len, tp_vars->session, + tp_vars->icmp_uid, jiffies_to_msecs(jiffies));
/* something went wrong during the preparation/transmission */ @@ -908,11 +933,6 @@ static void batadv_tp_send(struct work_struct *work) }
out: - if (likely(primary_if)) - batadv_hardif_put(primary_if); - if (likely(orig_node)) - batadv_orig_node_put(orig_node); - batadv_tp_sender_end(bat_priv, tp_vars); batadv_tp_sender_cleanup(bat_priv, tp_vars);
@@ -934,23 +954,27 @@ static void batadv_tp_start_work(struct batadv_tp_vars *tp_vars) * batadv_tp_start() - start a new tp meter session * @bat_priv: the bat priv with all the soft interface information * @dst: the receiver MAC address + * @neigh: neighbour towars which we have to run the test (one-hop test) * @test_length: test length in milliseconds * @cookie: session cookie * @caller: caller of tp meter session (user space or ELP) */ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, + struct batadv_hardif_neigh_node *neigh, u32 test_length, u32 *cookie, enum batadv_tp_meter_caller caller) { struct batadv_tp_vars *tp_vars; u8 session_id[2]; u8 icmp_uid; - u32 session_cookie; + u32 session_cookie = 0;
get_random_bytes(session_id, sizeof(session_id)); get_random_bytes(&icmp_uid, 1); - session_cookie = batadv_tp_session_cookie(session_id, icmp_uid); - *cookie = session_cookie; + if (cookie) { + session_cookie = batadv_tp_session_cookie(session_id, icmp_uid); + *cookie = session_cookie; + }
if (!atomic_add_unless(&bat_priv->tp_num, 1, BATADV_TP_MAX_NUM_QUEUE)) { batadv_dbg(BATADV_DBG_TP_METER, bat_priv, @@ -971,6 +995,10 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst,
/* initialize tp_vars */ ether_addr_copy(tp_vars->other_end, dst); + if (neigh) { + kref_get(&neigh->refcount); + tp_vars->hardif_neigh = neigh; + } kref_init(&tp_vars->refcount); tp_vars->role = BATADV_TP_SENDER; tp_vars->caller = caller; @@ -1132,7 +1160,7 @@ static void batadv_tp_receiver_shutdown(struct timer_list *t) /** * batadv_tp_send_ack() - send an ACK packet * @bat_priv: the bat priv with all the soft interface information - * @dst: the mac address of the destination originator + * @tp_vars: the private data of the current TP meter session * @seq: the sequence number to ACK * @timestamp: the timestamp to echo back in the ACK * @session: session identifier @@ -1141,29 +1169,42 @@ static void batadv_tp_receiver_shutdown(struct timer_list *t) * Return: 0 on success, a positive integer representing the reason of the * failure otherwise */ -static int batadv_tp_send_ack(struct batadv_priv *bat_priv, const u8 *dst, +static int batadv_tp_send_ack(struct batadv_priv *bat_priv, + struct batadv_tp_vars *tp_vars, u32 seq, __be32 timestamp, const u8 *session, int socket_index) { struct batadv_hard_iface *primary_if = NULL; - struct batadv_orig_node *orig_node; + struct batadv_orig_node *orig_node = NULL; struct batadv_icmp_tp_packet *icmp; + struct net_device *netdev = NULL; struct sk_buff *skb; + u8 *src, *dst; int r, ret;
- orig_node = batadv_orig_hash_find(bat_priv, dst); - if (unlikely(!orig_node)) { - ret = BATADV_TP_REASON_DST_UNREACHABLE; - goto out; - } + if (tp_vars->hardif_neigh) { + dst = tp_vars->hardif_neigh->addr; + src = tp_vars->hardif_neigh->if_incoming->net_dev->dev_addr; + netdev = tp_vars->hardif_neigh->if_incoming->net_dev; + } else { + orig_node = batadv_orig_hash_find(bat_priv, tp_vars->other_end); + if (unlikely(!orig_node)) { + ret = BATADV_TP_REASON_DST_UNREACHABLE; + goto out; + }
- primary_if = batadv_primary_if_get_selected(bat_priv); - if (unlikely(!primary_if)) { - ret = BATADV_TP_REASON_DST_UNREACHABLE; - goto out; + primary_if = batadv_primary_if_get_selected(bat_priv); + if (unlikely(!primary_if)) { + ret = BATADV_TP_REASON_DST_UNREACHABLE; + goto out; + } + + dst = orig_node->orig; + src = primary_if->net_dev->dev_addr; + netdev = NULL; }
- skb = netdev_alloc_skb_ip_align(NULL, sizeof(*icmp) + ETH_HLEN); + skb = netdev_alloc_skb_ip_align(netdev, sizeof(*icmp) + ETH_HLEN); if (unlikely(!skb)) { ret = BATADV_TP_REASON_MEMORY_ERROR; goto out; @@ -1175,8 +1216,8 @@ static int batadv_tp_send_ack(struct batadv_priv *bat_priv, const u8 *dst, icmp->version = BATADV_COMPAT_VERSION; icmp->ttl = BATADV_TTL; icmp->msg_type = BATADV_TP; - ether_addr_copy(icmp->dst, orig_node->orig); - ether_addr_copy(icmp->orig, primary_if->net_dev->dev_addr); + ether_addr_copy(icmp->dst, dst); + ether_addr_copy(icmp->orig, src); icmp->uid = socket_index;
icmp->subtype = BATADV_TP_ACK; @@ -1185,7 +1226,13 @@ static int batadv_tp_send_ack(struct batadv_priv *bat_priv, const u8 *dst, icmp->timestamp = timestamp;
/* send the ack */ - r = batadv_send_skb_to_orig(skb, orig_node, NULL); + if (tp_vars->hardif_neigh) + r = batadv_send_skb_packet(skb, + tp_vars->hardif_neigh->if_incoming, + dst); + else + r = batadv_send_skb_to_orig(skb, orig_node, NULL); + if (unlikely(r < 0) || r == NET_XMIT_DROP) { ret = BATADV_TP_REASON_DST_UNREACHABLE; goto out; @@ -1313,14 +1360,17 @@ static void batadv_tp_ack_unordered(struct batadv_tp_vars *tp_vars) /** * batadv_tp_init_recv() - return matching or create new receiver tp_vars * @bat_priv: the bat priv with all the soft interface information + * @recv_if: interface that the skb is received on * @icmp: received icmp tp msg * * Return: corresponding tp_vars or NULL on errors */ static struct batadv_tp_vars * batadv_tp_init_recv(struct batadv_priv *bat_priv, + struct batadv_hard_iface *recv_if, const struct batadv_icmp_tp_packet *icmp) { + struct batadv_hardif_neigh_node *neigh = NULL; struct batadv_tp_vars *tp_vars;
spin_lock_bh(&bat_priv->tp_list_lock); @@ -1335,15 +1385,30 @@ batadv_tp_init_recv(struct batadv_priv *bat_priv, goto out_unlock; }
+ /* the sender is starting a LINK test, therefore we have retrieve its + * corresponding hardif_neigh_node that we'll use later to send ACKs + * back + */ + if (icmp->subtype == BATADV_TP_MSG_LINK) { + neigh = batadv_hardif_neigh_get(recv_if, icmp->orig); + if (!neigh) { + batadv_dbg(BATADV_DBG_TP_METER, bat_priv, + "Meter: %s() can't retrieve sender neigh object for %pM\n", + __func__, icmp->orig); + goto out_unlock; + } + } + tp_vars = kmalloc(sizeof(*tp_vars), GFP_ATOMIC); if (!tp_vars) - goto out_unlock; + goto err_neigh_put;
ether_addr_copy(tp_vars->other_end, icmp->orig); tp_vars->role = BATADV_TP_RECEIVER; memcpy(tp_vars->session, icmp->session, sizeof(tp_vars->session)); tp_vars->last_recv = BATADV_TP_FIRST_SEQ; tp_vars->bat_priv = bat_priv; + tp_vars->hardif_neigh = neigh; kref_init(&tp_vars->refcount);
spin_lock_init(&tp_vars->unacked_lock); @@ -1356,7 +1421,10 @@ batadv_tp_init_recv(struct batadv_priv *bat_priv, timer_setup(&tp_vars->timer, batadv_tp_receiver_shutdown, 0);
batadv_tp_reset_receiver_timer(tp_vars); + goto out_unlock;
+err_neigh_put: + batadv_hardif_neigh_put(neigh); out_unlock: spin_unlock_bh(&bat_priv->tp_list_lock);
@@ -1366,11 +1434,13 @@ batadv_tp_init_recv(struct batadv_priv *bat_priv, /** * batadv_tp_recv_msg() - process a single data message * @bat_priv: the bat priv with all the soft interface information + * @recv_if: interface that the skb is received on * @skb: the buffer containing the received packet * * Process a received TP MSG packet */ static void batadv_tp_recv_msg(struct batadv_priv *bat_priv, + struct batadv_hard_iface *recv_if, const struct sk_buff *skb) { const struct batadv_icmp_tp_packet *icmp; @@ -1385,7 +1455,7 @@ static void batadv_tp_recv_msg(struct batadv_priv *bat_priv, * first packet is lost, the tp meter does not work anymore! */ if (seqno == BATADV_TP_FIRST_SEQ) { - tp_vars = batadv_tp_init_recv(bat_priv, icmp); + tp_vars = batadv_tp_init_recv(bat_priv, recv_if, icmp); if (!tp_vars) { batadv_dbg(BATADV_DBG_TP_METER, bat_priv, "Meter: seqno != BATADV_TP_FIRST_SEQ cannot initiate connection\n"); @@ -1441,7 +1511,7 @@ static void batadv_tp_recv_msg(struct batadv_priv *bat_priv, * is going to be sent is a duplicate (the sender will count them and * possibly enter Fast Retransmit as soon as it has reached 3) */ - batadv_tp_send_ack(bat_priv, icmp->orig, tp_vars->last_recv, + batadv_tp_send_ack(bat_priv, tp_vars, tp_vars->last_recv, icmp->timestamp, icmp->session, icmp->uid); out: if (likely(tp_vars)) @@ -1451,9 +1521,12 @@ static void batadv_tp_recv_msg(struct batadv_priv *bat_priv, /** * batadv_tp_meter_recv() - main TP Meter receiving function * @bat_priv: the bat priv with all the soft interface information + * @recv_if: interface that the skb is received on * @skb: the buffer containing the received packet */ -void batadv_tp_meter_recv(struct batadv_priv *bat_priv, struct sk_buff *skb) +void batadv_tp_meter_recv(struct batadv_priv *bat_priv, + struct batadv_hard_iface *recv_if, + struct sk_buff *skb) { struct batadv_icmp_tp_packet *icmp;
@@ -1461,7 +1534,8 @@ void batadv_tp_meter_recv(struct batadv_priv *bat_priv, struct sk_buff *skb)
switch (icmp->subtype) { case BATADV_TP_MSG: - batadv_tp_recv_msg(bat_priv, skb); + case BATADV_TP_MSG_LINK: + batadv_tp_recv_msg(bat_priv, recv_if, skb); break; case BATADV_TP_ACK: batadv_tp_recv_ack(bat_priv, skb); diff --git a/net/batman-adv/tp_meter.h b/net/batman-adv/tp_meter.h index 3b11a3e9..3a1be483 100644 --- a/net/batman-adv/tp_meter.h +++ b/net/batman-adv/tp_meter.h @@ -28,10 +28,13 @@ struct sk_buff; int batadv_tp_meter_init(void); void batadv_tp_meter_destroy(void); void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, + struct batadv_hardif_neigh_node *neigh, u32 test_length, u32 *cookie, enum batadv_tp_meter_caller caller); void batadv_tp_stop(struct batadv_priv *bat_priv, const u8 *dst, u8 return_value); -void batadv_tp_meter_recv(struct batadv_priv *bat_priv, struct sk_buff *skb); +void batadv_tp_meter_recv(struct batadv_priv *bat_priv, + struct batadv_hard_iface *recv_if, + struct sk_buff *skb);
#endif /* _NET_BATMAN_ADV_TP_METER_H_ */ diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index b38ca166..0a9bee88 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -1454,6 +1454,9 @@ struct batadv_tp_vars {
/** @rcu: struct used for freeing in an RCU-safe manner */ struct rcu_head rcu; + + /** @hardif_neigh: in case of LINK test, represents the other-end */ + struct batadv_hardif_neigh_node *hardif_neigh; };
/**
Signed-off-by: Marek Lindner mareklindner@neomailbox.ch --- net/batman-adv/bat_v_elp.c | 63 ++++++++++++++++++++++++++++++++++++-- net/batman-adv/bat_v_elp.h | 21 +++++++++++++ net/batman-adv/main.h | 1 + net/batman-adv/tp_meter.c | 37 +++++++++++++++++----- net/batman-adv/types.h | 14 +++++++++ 5 files changed, 126 insertions(+), 10 deletions(-)
diff --git a/net/batman-adv/bat_v_elp.c b/net/batman-adv/bat_v_elp.c index 71c20c1d..3f0d7816 100644 --- a/net/batman-adv/bat_v_elp.c +++ b/net/batman-adv/bat_v_elp.c @@ -51,6 +51,7 @@ #include "originator.h" #include "routing.h" #include "send.h" +#include "tp_meter.h"
/** * batadv_v_elp_start_timer() - restart timer for ELP periodic work @@ -67,6 +68,41 @@ static void batadv_v_elp_start_timer(struct batadv_hard_iface *hard_iface) msecs_to_jiffies(msecs)); }
+/** + * batadv_v_elp_tp_start() - start a tp meter session for a neighbor + * @neigh: neighbor to run tp meter on + */ +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); + + neigh->bat_v.tp_meter_running = true; + batadv_tp_start(bat_priv, neigh->addr, neigh, 10, NULL, BATADV_TP_ELP); +} + +/** + * batadv_v_elp_tp_fail() - handle tp meter session failure + * @neigh: neighbor to run tp meter on + */ +void batadv_v_elp_tp_fail(struct batadv_hardif_neigh_node *neigh) +{ + neigh->bat_v.tp_meter_running = false; +} + +/** + * batadv_v_elp_tp_finish() - post-process tp meter results + * @neigh: neighbor tp meter on + * @throughput: tp meter throughput result + */ +void batadv_v_elp_tp_finish(struct batadv_hardif_neigh_node *neigh, + u32 throughput) +{ + neigh->bat_v.tp_meter_throughput = throughput; + neigh->bat_v.last_tp_meter_run = jiffies; + neigh->bat_v.tp_meter_running = false; +} + /** * batadv_v_elp_get_throughput() - get the throughput towards a neighbour * @neigh: the neighbour for which the throughput has to be obtained @@ -78,6 +114,7 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) { struct batadv_hard_iface *hard_iface = neigh->if_incoming; struct ethtool_link_ksettings link_settings; + u32 last_tp_run_msecs, last_tp_run_jiffies; struct net_device *real_netdev; struct station_info sinfo; u32 throughput; @@ -112,10 +149,13 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) */ return 0; } + + /* unsupported WiFi driver */ if (ret) - goto default_throughput; + goto fallback_throughput; + if (!(sinfo.filled & BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT))) - goto default_throughput; + goto fallback_throughput;
return sinfo.expected_throughput / 100; } @@ -152,6 +192,25 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) return throughput * 10; }
+fallback_throughput: + last_tp_run_jiffies = jiffies - neigh->bat_v.last_tp_meter_run; + last_tp_run_msecs = jiffies_to_msecs(last_tp_run_jiffies); + + /* check the tp_meter_running flag before checking the timestamp to + * avoid a race condition where a new tp meter session is scheduled + * right after the previous tp meter session has completed + */ + if (!neigh->bat_v.tp_meter_running && + last_tp_run_msecs > BATADV_ELP_TP_RUN_INTERVAL) + batadv_v_elp_tp_start(neigh); + + /* discard too old tp test results */ + if (last_tp_run_msecs > 2 * BATADV_ELP_TP_RUN_INTERVAL) + neigh->bat_v.tp_meter_throughput = 0; + + if (neigh->bat_v.tp_meter_throughput) + return neigh->bat_v.tp_meter_throughput; + default_throughput: if (!(hard_iface->bat_v.flags & BATADV_WARNING_DEFAULT)) { batadv_info(hard_iface->soft_iface, diff --git a/net/batman-adv/bat_v_elp.h b/net/batman-adv/bat_v_elp.h index e8c7b7fd..4ffb1fde 100644 --- a/net/batman-adv/bat_v_elp.h +++ b/net/batman-adv/bat_v_elp.h @@ -21,6 +21,8 @@
#include "main.h"
+#include <linux/types.h> + struct sk_buff; struct work_struct;
@@ -33,4 +35,23 @@ int batadv_v_elp_packet_recv(struct sk_buff *skb, struct batadv_hard_iface *if_incoming); void batadv_v_elp_throughput_metric_update(struct work_struct *work);
+#ifdef CONFIG_BATMAN_ADV_BATMAN_V + +void batadv_v_elp_tp_fail(struct batadv_hardif_neigh_node *neigh); +void batadv_v_elp_tp_finish(struct batadv_hardif_neigh_node *neigh, + u32 throughput); + +#else + +static inline void batadv_v_elp_tp_fail(struct batadv_hardif_neigh_node *neigh) +{ +} + +static inline void +batadv_v_elp_tp_finish(struct batadv_hardif_neigh_node *neigh, u32 throughput) +{ +} + +#endif /* CONFIG_BATMAN_ADV_BATMAN_V */ + #endif /* _NET_BATMAN_ADV_BAT_V_ELP_H_ */ diff --git a/net/batman-adv/main.h b/net/batman-adv/main.h index 89dfaf87..ed4ae913 100644 --- a/net/batman-adv/main.h +++ b/net/batman-adv/main.h @@ -69,6 +69,7 @@ #define BATADV_ELP_MIN_PROBE_SIZE 200 /* bytes */ #define BATADV_ELP_PROBE_MAX_TX_DIFF 100 /* milliseconds */ #define BATADV_ELP_MAX_AGE 64 +#define BATADV_ELP_TP_RUN_INTERVAL 60000 /* milliseconds */ #define BATADV_OGM_MAX_ORIGDIFF 5 #define BATADV_OGM_MAX_AGE 64
diff --git a/net/batman-adv/tp_meter.c b/net/batman-adv/tp_meter.c index 87aaeb1d..ec47ca79 100644 --- a/net/batman-adv/tp_meter.c +++ b/net/batman-adv/tp_meter.c @@ -24,7 +24,7 @@ #include <linux/byteorder/generic.h> #include <linux/cache.h> #include <linux/compiler.h> -#include <linux/err.h> +#include <linux/errno.h> #include <linux/etherdevice.h> #include <linux/gfp.h> #include <linux/if_ether.h> @@ -33,9 +33,9 @@ #include <linux/kernel.h> #include <linux/kref.h> #include <linux/list.h> +#include <linux/log2.h> #include <linux/netdevice.h> #include <linux/param.h> -#include <linux/printk.h> #include <linux/random.h> #include <linux/rculist.h> #include <linux/rcupdate.h> @@ -51,6 +51,7 @@ #include <uapi/linux/batadv_packet.h> #include <uapi/linux/batman_adv.h>
+#include "bat_v_elp.h" #include "hard-interface.h" #include "log.h" #include "netlink.h" @@ -225,6 +226,7 @@ static void batadv_tp_caller_notify(struct batadv_priv *bat_priv, enum batadv_tp_meter_reason reason) { u32 total_bytes; + u64 throughput; u32 test_time; u32 cookie; bool reason_is_error; @@ -251,6 +253,21 @@ static void batadv_tp_caller_notify(struct batadv_priv *bat_priv,
break; case BATADV_TP_ELP: + if (reason_is_error) { + batadv_v_elp_tp_fail(tp_vars->hardif_neigh); + return; + } + + test_time = jiffies_to_msecs(jiffies - tp_vars->start_time); + total_bytes = atomic64_read(&tp_vars->tot_sent); + + /* The following calculation includes these steps: + * - convert bytes to bits + * - divide bits by the test length (msecs) + * - convert result from bits/ms to 0.1Mb/s (* 1024 * 10 / 1000) + */ + throughput = total_bytes * 8 >> ilog2(test_time) / 10; + batadv_v_elp_tp_finish(tp_vars->hardif_neigh, throughput); break; default: break; @@ -264,11 +281,14 @@ static void batadv_tp_caller_notify(struct batadv_priv *bat_priv, * @reason: reason for tp meter session stop * @dst: destination of tp_meter session * @cookie: cookie of tp_meter session + * @hardif_neigh: neighbor towards which the test was ran (for one-hop test) */ -static void batadv_tp_caller_init_error(struct batadv_priv *bat_priv, - enum batadv_tp_meter_caller caller, - enum batadv_tp_meter_reason reason, - const u8 *dst, u32 cookie) +static void +batadv_tp_caller_init_error(struct batadv_priv *bat_priv, + enum batadv_tp_meter_caller caller, + enum batadv_tp_meter_reason reason, const u8 *dst, + u32 cookie, + struct batadv_hardif_neigh_node *hardif_neigh) { switch (caller) { case BATADV_TP_USERSPACE: @@ -276,6 +296,7 @@ static void batadv_tp_caller_init_error(struct batadv_priv *bat_priv, cookie); break; case BATADV_TP_ELP: + batadv_v_elp_tp_fail(hardif_neigh); break; default: break; @@ -981,7 +1002,7 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, "Meter: too many ongoing sessions, aborting (SEND)\n"); batadv_tp_caller_init_error(bat_priv, caller, BATADV_TP_REASON_TOO_MANY, dst, - session_cookie); + session_cookie, neigh); return; }
@@ -989,7 +1010,7 @@ void batadv_tp_start(struct batadv_priv *bat_priv, const u8 *dst, if (!tp_vars) { batadv_tp_caller_init_error(bat_priv, caller, BATADV_TP_REASON_MEMORY_ERROR, dst, - session_cookie); + session_cookie, neigh); return; }
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index 0a9bee88..148f23ad 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -583,6 +583,20 @@ struct batadv_hardif_neigh_node_bat_v {
/** @metric_work: work queue callback item for metric update */ struct work_struct metric_work; + + /** + * @tp_meter_running: tp meter measurements towards this neighbor in + * progress + */ + unsigned char tp_meter_running:1; + + /** + * @last_tp_meter_run: timestamp of last tp meter measurement completion + */ + unsigned long last_tp_meter_run; + + /** @tp_meter_throughput: throughput information measured by tp meter */ + unsigned long tp_meter_throughput; };
/**
On Fri, May 18, 2018 at 09:47:53AM +0800, Marek Lindner wrote:
@@ -251,6 +253,21 @@ static void batadv_tp_caller_notify(struct batadv_priv *bat_priv,
break;case BATADV_TP_ELP:
if (reason_is_error) {batadv_v_elp_tp_fail(tp_vars->hardif_neigh);return;}test_time = jiffies_to_msecs(jiffies - tp_vars->start_time);total_bytes = atomic64_read(&tp_vars->tot_sent);/* The following calculation includes these steps:* - convert bytes to bits* - divide bits by the test length (msecs)* - convert result from bits/ms to 0.1Mb/s (* 1024 * 10 / 1000)*/throughput = total_bytes * 8 >> ilog2(test_time) / 10;batadv_v_elp_tp_finish(tp_vars->hardif_neigh, throughput);
I find the throughput calculation quite hard to read here, would it be possible to put this into an extra (inline?) function?
Also the comment for the "convert result..." seems wrong, "[bits/ms]*1024*10/1000" would be 0.01Mb/s, not 0.1Mb/s?
What is the advantage of using the ilog2 and shift operator here compared to plain multiplications and divisions?
Also, when trying this in a small C program I get weird results:
----- #include <stdio.h> #include <math.h>
int main() { unsigned long test_time = 10000; // 10s unsigned long total_bytes = 20000000; // 16MBit/s unsigned long throughput, throughput2; unsigned long log_test_time = log(test_time) / log(2);
throughput = total_bytes * 8 >> log_test_time / 10;
// Straightforward approach? throughput2 = total_bytes * 8 / test_time * 1000 / 1024 / 100;
printf("Result: %lu (log_test_time: %lu)\n", throughput, log_test_time); printf("Result2: %lu\n", throughput2);
return 0; } ----- $ ./test Result: 80000000 (log_test_time: 13) Result2: 156 $ file ./test ./test: ELF 32-bit LSB pie executable ARM, EABI5 version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-armhf.so.3, for GNU/Linux 3.2.0, BuildID[sha1]=d18f32829cdd2bc42cf744cdcafde7cdbd315cb0, not stripped -----
Regards, Linus
On Montag, 21. Mai 2018 15:17:11 CEST Linus Lüssing wrote:
What is the advantage of using the ilog2 and shift operator here compared to plain multiplications and divisions?
They most likely ran into the problem that they must do a 64 bit division (which cannot be done on 32 bit machines in the kernel directly with C's native "x / y" statement ). But in this case, it is most likely a lot better to use do_div (or similar function) to avoid a lot of rounding problems which this shift approach has.
Kind regards, Sven
On Montag, 21. Mai 2018 15:17:11 CEST Linus Lüssing wrote:
throughput = total_bytes * 8 >> ilog2(test_time) / 10;
[...]
[...]
throughput = total_bytes * 8 >> log_test_time / 10; // Straightforward approach? throughput2 = total_bytes * 8 / test_time * 1000 / 1024 / 100;
[...]
$ ./test Result: 80000000 (log_test_time: 13) Result2: 156 $ file ./test ./test: ELF 32-bit LSB pie executable ARM, EABI5 version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-armhf.so.3, for GNU/Linux 3.2.0, BuildID[sha1]=d18f32829cdd2bc42cf744cdcafde7cdbd315cb0, not stripped
Thanks for this small example program. Yes, there are parenthesis missing in the calculation. Right now, following is calculated:
(total_bytes * 8) >> (ilog2(test_time) / 10);
But the author most likely wanted following precedence:
((total_bytes * 8) >> ilog2(test_time)) / 10;
And together with the fixed unit, you would get:
(total_bytes * 8 >> ilog2(test_time) / 100;
Your example program would then show following result because the shifting stuff is still the wrong approach:
Result: 195 (log_test_time: 13) Result2: 156
The calculation still has to be changed to something like this to get
// when 0.1 Mbit/s == 100 kbit/s throughput = total_bytes * 5; do_div(throughput, test_time * 64);
// when 0.1 Mbit/s == 102.4 kbit/s throughput = total_bytes * 625; do_div(throughput, test_time * 8192);
// when 0.1 Mbit/s == 100 kbit/s, and 1kbit/s == 1000 bit (instead of 1024 bit): throughput = total_bytes; do_div(throughput, test_time * 125);
Please keep in mind that we must do a check of the divisor (for 0) before doing this do_div.
Kind regards, Sven
Hi,
On 22/05/18 03:06, Sven Eckelmann wrote:
On Montag, 21. Mai 2018 15:17:11 CEST Linus Lüssing wrote:
throughput = total_bytes * 8 >> ilog2(test_time) / 10;[...]
[...]
throughput = total_bytes * 8 >> log_test_time / 10; // Straightforward approach? throughput2 = total_bytes * 8 / test_time * 1000 / 1024 / 100;[...]
$ ./test Result: 80000000 (log_test_time: 13) Result2: 156 $ file ./test ./test: ELF 32-bit LSB pie executable ARM, EABI5 version 1 (SYSV), dynamically linked, interpreter /lib/ld-linux-armhf.so.3, for GNU/Linux 3.2.0, BuildID[sha1]=d18f32829cdd2bc42cf744cdcafde7cdbd315cb0, not stripped
Thanks for this small example program. Yes, there are parenthesis missing in the calculation. Right now, following is calculated:
(total_bytes * 8) >> (ilog2(test_time) / 10);But the author most likely wanted following precedence:
((total_bytes * 8) >> ilog2(test_time)) / 10;And together with the fixed unit, you would get:
(total_bytes * 8 >> ilog2(test_time) / 100;Your example program would then show following result because the shifting stuff is still the wrong approach:
Result: 195 (log_test_time: 13) Result2: 156The calculation still has to be changed to something like this to get
// when 0.1 Mbit/s == 100 kbit/s throughput = total_bytes * 5; do_div(throughput, test_time * 64); // when 0.1 Mbit/s == 102.4 kbit/s throughput = total_bytes * 625; do_div(throughput, test_time * 8192); // when 0.1 Mbit/s == 100 kbit/s, and 1kbit/s == 1000 bit (instead of 1024 bit): throughput = total_bytes; do_div(throughput, test_time * 125);Please keep in mind that we must do a check of the divisor (for 0) before doing this do_div.
Thanks for the code and the correction :-)
Yes, we wanted to avoid 64 bits explicit divisions and we assumed test_time would have been a power of 2 (otherwise we'd accept a small error).
Anyway, the last case brought by Sven depicts what we want to implement. The patch will be changed accordingly.
Cheers,
On Fri, May 18, 2018 at 09:47:53AM +0800, Marek Lindner wrote:
+fallback_throughput:
- last_tp_run_jiffies = jiffies - neigh->bat_v.last_tp_meter_run;
- last_tp_run_msecs = jiffies_to_msecs(last_tp_run_jiffies);
- /* check the tp_meter_running flag before checking the timestamp to
* avoid a race condition where a new tp meter session is scheduled* right after the previous tp meter session has completed*/- if (!neigh->bat_v.tp_meter_running &&
last_tp_run_msecs > BATADV_ELP_TP_RUN_INTERVAL)batadv_v_elp_tp_start(neigh);- /* discard too old tp test results */
- if (last_tp_run_msecs > 2 * BATADV_ELP_TP_RUN_INTERVAL)
neigh->bat_v.tp_meter_throughput = 0;
So far we were either using time_before(), time_after_eq() or our own wrapper batadv_has_timed_out(). Would it make sense to use some of these here, too?
On Monday, 21 May 2018 22:43:04 HKT Linus Lüssing wrote:
On Fri, May 18, 2018 at 09:47:53AM +0800, Marek Lindner wrote:
+fallback_throughput:
last_tp_run_jiffies = jiffies - neigh->bat_v.last_tp_meter_run;last_tp_run_msecs = jiffies_to_msecs(last_tp_run_jiffies);/* check the tp_meter_running flag before checking the timestampto
* avoid a race condition where a new tp meter session isscheduled
* right after the previous tp meter session has completed*/if (!neigh->bat_v.tp_meter_running &&last_tp_run_msecs > BATADV_ELP_TP_RUN_INTERVAL)batadv_v_elp_tp_start(neigh);/* discard too old tp test results */if (last_tp_run_msecs > 2 * BATADV_ELP_TP_RUN_INTERVAL)neigh->bat_v.tp_meter_throughput = 0;So far we were either using time_before(), time_after_eq() or our own wrapper batadv_has_timed_out(). Would it make sense to use some of these here, too?
Good point. That changes has been staged for v3.
Cheers, Marek
On Fri, May 18, 2018 at 09:47:53AM +0800, Marek Lindner wrote:
+/**
- batadv_v_elp_tp_start() - start a tp meter session for a neighbor
- @neigh: neighbor to run tp meter on
- */
+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);
- neigh->bat_v.tp_meter_running = true;
- batadv_tp_start(bat_priv, neigh->addr, neigh, 10, NULL, BATADV_TP_ELP);
+}
This seems racy here? We have no ordering constraints between the assigment and batadv_tp_start() which might lead to...
@@ -152,6 +192,25 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) return throughput * 10; }
+fallback_throughput:
- last_tp_run_jiffies = jiffies - neigh->bat_v.last_tp_meter_run;
- last_tp_run_msecs = jiffies_to_msecs(last_tp_run_jiffies);
- /* check the tp_meter_running flag before checking the timestamp to
* avoid a race condition where a new tp meter session is scheduled* right after the previous tp meter session has completed*/- if (!neigh->bat_v.tp_meter_running &&
last_tp_run_msecs > BATADV_ELP_TP_RUN_INTERVAL)batadv_v_elp_tp_start(neigh);
...multiple instances potentially being started at the same time here?
Hi Lunus,
On 21/05/18 22:48, Linus Lüssing wrote:
On Fri, May 18, 2018 at 09:47:53AM +0800, Marek Lindner wrote:
+/**
- batadv_v_elp_tp_start() - start a tp meter session for a neighbor
- @neigh: neighbor to run tp meter on
- */
+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);
- neigh->bat_v.tp_meter_running = true;
- batadv_tp_start(bat_priv, neigh->addr, neigh, 10, NULL, BATADV_TP_ELP);
+}
This seems racy here? We have no ordering constraints between the assigment and batadv_tp_start() which might lead to...
@@ -152,6 +192,25 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) return throughput * 10; }
+fallback_throughput:
- last_tp_run_jiffies = jiffies - neigh->bat_v.last_tp_meter_run;
- last_tp_run_msecs = jiffies_to_msecs(last_tp_run_jiffies);
- /* check the tp_meter_running flag before checking the timestamp to
* avoid a race condition where a new tp meter session is scheduled* right after the previous tp meter session has completed*/- if (!neigh->bat_v.tp_meter_running &&
last_tp_run_msecs > BATADV_ELP_TP_RUN_INTERVAL)batadv_v_elp_tp_start(neigh);...multiple instances potentially being started at the same time here?
- batadv_tp_start() is only invoked by batadv_v_elp_get_throughput(); - batadv_v_elp_get_throughput() is only invoked by batadv_v_elp_throughput_metric_update(); - batadv_v_elp_throughput_metric_update() is a periodic function executed by a timer and we can't have concurrent executions (unless we use crazy interval values).
Maybe we should add a comment to make this more clear?
On Fri, May 18, 2018 at 09:47:53AM +0800, Marek Lindner wrote:
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index 0a9bee88..148f23ad 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -583,6 +583,20 @@ struct batadv_hardif_neigh_node_bat_v {
/** @metric_work: work queue callback item for metric update */ struct work_struct metric_work;
- /**
* @tp_meter_running: tp meter measurements towards this neighbor in* progress*/- unsigned char tp_meter_running:1;
I'm currently wondering: Would it make sense to prevent parallel tp meter execution not only to a specific neighbor but parallel executions on a hard-interface in general?
Hi,
On 21/05/18 23:01, Linus Lüssing wrote:
On Fri, May 18, 2018 at 09:47:53AM +0800, Marek Lindner wrote:
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h index 0a9bee88..148f23ad 100644 --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -583,6 +583,20 @@ struct batadv_hardif_neigh_node_bat_v {
/** @metric_work: work queue callback item for metric update */ struct work_struct metric_work;
- /**
* @tp_meter_running: tp meter measurements towards this neighbor in* progress*/- unsigned char tp_meter_running:1;
I'm currently wondering: Would it make sense to prevent parallel tp meter execution not only to a specific neighbor but parallel executions on a hard-interface in general?
This flag prevents scheduling new measurements to this neighbour if one has already been scheduled and has not returned any result yet.
What you are looking for is implemented in patch 1/7: a system-wide queue prevents concurrent measurements at all. All the measurements are serialized and performed one after the other.
Cheers,
On Freitag, 18. Mai 2018 03:47:53 CEST Marek Lindner wrote:
Signed-off-by: Marek Lindner mareklindner@neomailbox.ch
net/batman-adv/bat_v_elp.c | 63 ++++++++++++++++++++++++++++++++++++-- net/batman-adv/bat_v_elp.h | 21 +++++++++++++ net/batman-adv/main.h | 1 + net/batman-adv/tp_meter.c | 37 +++++++++++++++++----- net/batman-adv/types.h | 14 +++++++++ 5 files changed, 126 insertions(+), 10 deletions(-)
This seems to create weird (as in incorrect) looking results. Here the ones from Matthias Fritzsche:
[2018-05-21 18:19:41] <txt-file> ordexm, marec, ecsv_: Now I have one router and the its neighbour with batman_V with tp-meter. https://paste.debian.net/1025691/ [2018-05-21 18:24:28] <txt-file> ah … now "batctl n" shows a tp of 16064.1 [2018-05-21 18:29:07] <ecsv_> uhm, isn't this rather... high [2018-05-21 18:29:53] <txt-file> ecsv_: I think so. This node has an upload of 4 MBit/s [2018-05-21 18:30:34] <txt-file> now (6 minutes later) tq=2.2 [2018-05-21 18:30:48] <txt-file> which sounds much more realistic.
The setup is using an gluon based router (TP-Link TL-WDR3600 v1) [1] which uses a vpn connection (fastd) to a server (descartes.routers.chemnitz.freifunk.net) in the internet. The automatic throughput test runs between these two devices.
Kind regards, Sven
[1] https://meshviewer.chemnitz.freifunk.net/#!v:m;n:c04a00e44be0
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(-)
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;
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 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
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
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
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Linus Lüssing -
Marek Lindner -
Sven Eckelmann