From: Antonio Quartulli <antonio@meshcoding.com>
To: The list for a Better Approach To Mobile Ad-hoc Networking
<b.a.t.m.a.n@lists.open-mesh.org>
Cc: Simon Wunderlich <simon@open-mesh.com>
Subject: Re: [B.A.T.M.A.N.] [RFCv2 3/6] batman-adv: split out router from orig_node
Date: Sun, 22 Sep 2013 22:47:59 +0200 [thread overview]
Message-ID: <20130922204759.GE3911@neomailbox.net> (raw)
In-Reply-To: <1378312060-30922-4-git-send-email-siwu@hrz.tu-chemnitz.de>
[-- Attachment #1: Type: text/plain, Size: 8305 bytes --]
On Wed, Sep 04, 2013 at 06:27:37PM +0200, Simon Wunderlich wrote:
> From: Simon Wunderlich <simon@open-mesh.com>
>
> For the network wide multi interface optimization there are different
> routers for each outgoing interface (outgoing from the OGM perspective,
> incoming for payload traffic). To reflect this, change the router and
> associated data to a list of routers.
>
> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> ---
[...]
> +static void
> +batadv_iv_ogm_process_per_outif(const struct ethhdr *ethhdr,
> + struct batadv_orig_node *orig_node,
> + struct batadv_ogm_packet *batadv_ogm_packet,
> + const unsigned char *tt_buff,
> + struct batadv_hard_iface *if_incoming,
> + struct batadv_hard_iface *if_outgoing)
> {
> struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface);
> - struct batadv_hard_iface *hard_iface;
> - struct batadv_orig_node *orig_neigh_node, *orig_node, *orig_node_tmp;
> struct batadv_neigh_node *router = NULL, *router_router = NULL;
> struct batadv_neigh_node *orig_neigh_router = NULL;
> struct batadv_neigh_node_ifinfo *router_ifinfo = NULL;
> - int has_directlink_flag;
> - int is_my_addr = 0, is_my_orig = 0, is_my_oldorig = 0;
> - int is_bidirect;
> - bool is_single_hop_neigh = false;
> - bool is_from_best_next_hop = false;
> - int sameseq, similar_ttl;
> + struct batadv_orig_node *orig_neigh_node, *orig_node_tmp;
> + struct batadv_orig_node_ifinfo *orig_node_ifinfo;
> + int is_bidirect, sameseq, similar_ttl;
> enum batadv_dup_status dup_status;
> - uint32_t if_incoming_seqno;
> uint8_t *prev_sender;
> + struct batadv_ogm_packet ogm_packet_backup;
> + bool is_from_best_next_hop = false;
> + bool is_single_hop_neigh = false;
Since you have the opportunity, please sort these new declarations by line
length (like you have done 6 lines above).
[...]
> +
> + sameseq = orig_node_ifinfo->last_real_seqno ==
> + ntohl(batadv_ogm_packet->seqno));
> + similar_ttl = (orig_node_ifinfo->last_ttl - 3) <=
> + batadv_ogm_packet->header.ttl;
you know this is very ugly, right? :-)
you could try to remove sameseq and similar_ttl and use temporary variables for
last_real_seqno, seqno, last_ttl and header.ttl.....
> + if (is_bidirect && ((dup_status == BATADV_NO_DUP) ||
> + (sameseq && similar_ttl)))
...then you can try to make them fit this 'if' directly.
> + batadv_iv_ogm_orig_update(bat_priv, orig_node,
> + orig_node_ifinfo, ethhdr,
> + batadv_ogm_packet, if_incoming,
> + if_outgoing, tt_buff, dup_status);
> + rcu_read_unlock();
> +
[..]
> +static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,
> + struct batadv_ogm_packet *batadv_ogm_packet,
> + const unsigned char *tt_buff,
> + struct batadv_hard_iface *if_incoming)
> +{
> + struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface);
> + struct batadv_hard_iface *hard_iface;
> + struct batadv_orig_node *orig_neigh_node, *orig_node;
> + uint32_t if_incoming_seqno;
> + int has_directlink_flag;
> + int is_my_addr = 0, is_my_orig = 0, is_my_oldorig = 0;
Please sort the declarations by line length.
[...]
> diff --git a/network-coding.c b/network-coding.c
> index 173a96e..6d1b659 100644
> --- a/network-coding.c
> +++ b/network-coding.c
> @@ -1010,12 +1010,17 @@ static bool batadv_nc_code_packets(struct batadv_priv *bat_priv,
> int coded_size = sizeof(*coded_packet);
> int header_add = coded_size - unicast_size;
>
> - router_neigh = batadv_orig_node_get_router(neigh_node->orig_node);
> + /* TODO: do we need to consider the outgoing interface for
> + * coded packets?
> + */
> + router_neigh = batadv_orig_node_get_router(neigh_node->orig_node,
> + NULL);
Perhaps we can consider them like "locally-generated". I think we should ask
Martin to give us his feedback :-)
[...]
>
> /**
> + * batadv_orig_node_get_ifinfo - gets the ifinfo from an orig_node
> + * @orig_node: the orig node to be queried
> + * @if_received: the interface for which the ifinfo should be acquired
Put a blank line here.
> + * Returns: the requested orig_node_ifinfo or NULL if not found
we never use the ':' after Returns (and a . is missing at the end of the
sentence). Please make sure that all the kernel doc you added respect those
guidelines.
[...]
>
> +static void batadv_orig_node_ifinfo_free_rcu(struct rcu_head *rcu)
kernel doc?
> +{
> + struct batadv_orig_node_ifinfo *orig_node_ifinfo;
> +
> + orig_node_ifinfo = container_of(rcu, struct batadv_orig_node_ifinfo,
> + rcu);
> + if (orig_node_ifinfo->if_outgoing)
> + batadv_hardif_free_ref(orig_node_ifinfo->if_outgoing);
Please, be sure this is not creating any race condition during the cleanup phase.
We had problems int the past with a RCU callback scheduling another callback
too.
The cleanup code has an rcu_barrier() to ensure everything is done and then
safely free the various structures. What you do here may violate this assumption
and the scheduled callback may generate a General Protection Fault.
[...]
> @@ -291,6 +366,12 @@ static void batadv_orig_node_free_rcu(struct rcu_head *rcu)
> batadv_neigh_node_free_ref(neigh_node);
> }
>
> + hlist_for_each_entry_safe(orig_node_ifinfo, node_tmp,
> + &orig_node->ifinfo_list, list) {
> + hlist_del_rcu(&orig_node_ifinfo->list);
> + call_rcu(&orig_node_ifinfo->rcu,
> + batadv_orig_node_ifinfo_free_rcu);
Same here. Please ensure everything is ok when scheduling a callback from
another callback.
[...]
> /**
> + * batadv_purge_orig_ifinfo - purge ifinfo entries from originator
> + * @bat_priv: the bat priv with all the soft interface information
> + * @orig_node: orig node which is to be checked
> + * Returns: true if any ifinfo entry was purged, false otherwise
> + */
> +static bool
> +batadv_purge_orig_ifinfo(struct batadv_priv *bat_priv,
> + struct batadv_orig_node *orig_node)
> +{
> + struct hlist_node *node_tmp;
> + struct batadv_orig_node_ifinfo *orig_node_ifinfo;
> + bool ifinfo_purged = false;
> + struct batadv_hard_iface *if_outgoing;
> +
sort by line length please.
> + rcu_read_lock();
> + spin_lock_bh(&orig_node->neigh_list_lock);
why do you need both the rcu and the neigh_list_lock at the same time?
You acquire and release the two in the same moment (maybe I am overlooking
something?)
[...]
> +/**
> * batadv_purge_orig_neighbors - purges neighbors from originator
> * @bat_priv: the bat priv with all the soft interface information
> * @orig_node: orig node which is to be checked
> @@ -516,6 +643,8 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv,
> struct batadv_orig_node *orig_node)
> {
> struct batadv_neigh_node *best_neigh_node;
> + struct batadv_hard_iface *hard_iface;
> + bool changed = false;
this is useless and then.....
>
> if (batadv_has_timed_out(orig_node->last_seen,
> 2 * BATADV_PURGE_TIMEOUT)) {
> @@ -525,11 +654,32 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv,
> jiffies_to_msecs(orig_node->last_seen));
> return true;
> }
> - if (!batadv_purge_orig_neighbors(bat_priv, orig_node))
> + changed = changed || batadv_purge_orig_ifinfo(bat_priv, orig_node);
....just do:
changed = batadv_purge_orig_ifinfo(...);
[...]
> diff --git a/originator.h b/originator.h
> index 161c1e3..1dce5b4 100644
> --- a/originator.h
> +++ b/originator.h
> @@ -36,7 +36,16 @@ batadv_neigh_node_new(struct batadv_hard_iface *hard_iface,
> struct batadv_orig_node *orig_node);
> void batadv_neigh_node_free_ref(struct batadv_neigh_node *neigh_node);
> struct batadv_neigh_node *
> -batadv_orig_node_get_router(struct batadv_orig_node *orig_node);
> +batadv_orig_node_get_router(struct batadv_orig_node *orig_node,
> + const struct batadv_hard_iface *if_received);
> +void
why void on another line? It should fit in the next one.
> +batadv_orig_node_set_router(struct batadv_orig_node *orig_node,
> + struct batadv_hard_iface *if_received,
> + struct batadv_neigh_node *router);
Cheers,
--
Antonio Quartulli
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
next prev parent reply other threads:[~2013-09-22 20:47 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-04 16:27 [B.A.T.M.A.N.] [RFCv2 0/6] add network wide multi interface optimization Simon Wunderlich
2013-09-04 16:27 ` [B.A.T.M.A.N.] [RFCv2 1/6] batman-adv: remove bonding and interface alternating Simon Wunderlich
2013-09-04 16:27 ` [B.A.T.M.A.N.] [RFCv2 2/6] batman-adv: split tq information in neigh_node struct Simon Wunderlich
2013-09-13 13:23 ` Antonio Quartulli
2013-10-03 20:39 ` Simon Wunderlich
2013-09-04 16:27 ` [B.A.T.M.A.N.] [RFCv2 3/6] batman-adv: split out router from orig_node Simon Wunderlich
2013-09-22 20:47 ` Antonio Quartulli [this message]
2013-09-04 16:27 ` [B.A.T.M.A.N.] [RFCv2 4/6] batman-adv: add WiFi penalty Simon Wunderlich
2013-09-25 20:18 ` Antonio Quartulli
2013-09-04 16:27 ` [B.A.T.M.A.N.] [RFCv2 5/6] batman-adv: consider outgoing interface in OGM sending Simon Wunderlich
2013-10-01 9:00 ` Antonio Quartulli
2013-10-03 22:11 ` Simon Wunderlich
2013-09-04 16:27 ` [B.A.T.M.A.N.] [RFCv2 6/6] batman-adv: add bonding again Simon Wunderlich
2013-10-01 9:24 ` Antonio Quartulli
2013-10-03 22:53 ` Simon Wunderlich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130922204759.GE3911@neomailbox.net \
--to=antonio@meshcoding.com \
--cc=b.a.t.m.a.n@lists.open-mesh.org \
--cc=simon@open-mesh.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.