* [B.A.T.M.A.N.] [PATCHv3] batman-adv: postpone OGM preparation to sending time
@ 2012-12-15 11:14 Antonio Quartulli
2012-12-26 10:08 ` Marek Lindner
0 siblings, 1 reply; 5+ messages in thread
From: Antonio Quartulli @ 2012-12-15 11:14 UTC (permalink / raw)
To: b.a.t.m.a.n
OGMs are currently prepared 1 originator interval in advance
then the time they are used to be sent. This means that once
in the air they carry old information (like TT announcements
and possibly other flags).
To fix this, postpone the OGM creation to the same time of
sending, in this way the OGM is first created and then
immediately sent.
This patch also removes a "magic" -2 operation that was
introduced in the past to address the fact that batman-adv
was sending "delayed OGM" plus the fact that the OGM seqno
was increased right after having sent a new packet. Since
this change is removing both the behaviours, the "magic"
operation is not needed anymore.
Signed-off-by: Antonio Quartulli <ordex@autistici.org>
---
v3:
* add more kernel doc
v2:
* added kernel doc
* fixed sending time computation: previously msecs were summed to jiffies and
max_aggregation_time was wrongly summed twice
bat_iv_ogm.c | 139 +++++++++++++++++++++++++++++++++++++----------------------
types.h | 1 +
2 files changed, 89 insertions(+), 51 deletions(-)
diff --git a/bat_iv_ogm.c b/bat_iv_ogm.c
index ff56fd5..244751c 100644
--- a/bat_iv_ogm.c
+++ b/bat_iv_ogm.c
@@ -54,39 +54,6 @@ out:
return neigh_node;
}
-static int batadv_iv_ogm_iface_enable(struct batadv_hard_iface *hard_iface)
-{
- struct batadv_ogm_packet *batadv_ogm_packet;
- unsigned char *ogm_buff;
- uint32_t random_seqno;
- int res = -ENOMEM;
-
- /* randomize initial seqno to avoid collision */
- get_random_bytes(&random_seqno, sizeof(random_seqno));
- atomic_set(&hard_iface->bat_iv.ogm_seqno, random_seqno);
-
- hard_iface->bat_iv.ogm_buff_len = BATADV_OGM_HLEN;
- ogm_buff = kmalloc(hard_iface->bat_iv.ogm_buff_len, GFP_ATOMIC);
- if (!ogm_buff)
- goto out;
-
- hard_iface->bat_iv.ogm_buff = ogm_buff;
-
- batadv_ogm_packet = (struct batadv_ogm_packet *)ogm_buff;
- batadv_ogm_packet->header.packet_type = BATADV_IV_OGM;
- batadv_ogm_packet->header.version = BATADV_COMPAT_VERSION;
- batadv_ogm_packet->header.ttl = 2;
- batadv_ogm_packet->flags = BATADV_NO_FLAGS;
- batadv_ogm_packet->tq = BATADV_TQ_MAX_VALUE;
- batadv_ogm_packet->tt_num_changes = 0;
- batadv_ogm_packet->ttvn = 0;
-
- res = 0;
-
-out:
- return res;
-}
-
static void batadv_iv_ogm_iface_disable(struct batadv_hard_iface *hard_iface)
{
kfree(hard_iface->bat_iv.ogm_buff);
@@ -116,16 +83,22 @@ batadv_iv_ogm_primary_iface_set(struct batadv_hard_iface *hard_iface)
batadv_ogm_packet->header.ttl = BATADV_TTL;
}
-/* when do we schedule our own ogm to be sent */
+/**
+ * batadv_iv_ogm_emit_wait_time - compute the OGM preparation waiting time
+ * @bat_priv: the bat priv with all the soft interface information
+ *
+ * Returns the amount of jiffies to wait before preparing and sending the next
+ * own OGM
+ */
static unsigned long
-batadv_iv_ogm_emit_send_time(const struct batadv_priv *bat_priv)
+batadv_iv_ogm_emit_wait_time(const struct batadv_priv *bat_priv)
{
unsigned int msecs;
msecs = atomic_read(&bat_priv->orig_interval) - BATADV_JITTER;
msecs += (random32() % 2 * BATADV_JITTER);
- return jiffies + msecs_to_jiffies(msecs);
+ return msecs_to_jiffies(msecs);
}
/* when do we schedule a ogm packet to be sent */
@@ -459,7 +432,8 @@ out:
/* aggregate a new packet into the existing ogm packet */
static void batadv_iv_ogm_aggregate(struct batadv_forw_packet *forw_packet_aggr,
const unsigned char *packet_buff,
- int packet_len, bool direct_link)
+ int packet_len, bool direct_link,
+ int own_packet)
{
unsigned char *skb_buff;
unsigned long new_direct_link_flag;
@@ -468,6 +442,7 @@ static void batadv_iv_ogm_aggregate(struct batadv_forw_packet *forw_packet_aggr,
memcpy(skb_buff, packet_buff, packet_len);
forw_packet_aggr->packet_len += packet_len;
forw_packet_aggr->num_packets++;
+ forw_packet_aggr->own |= own_packet;
/* save packet direct link flag status */
if (direct_link) {
@@ -498,8 +473,7 @@ static void batadv_iv_ogm_queue_add(struct batadv_priv *bat_priv,
/* find position for the packet in the forward queue */
spin_lock_bh(&bat_priv->forw_bat_list_lock);
- /* own packets are not to be aggregated */
- if ((atomic_read(&bat_priv->aggregated_ogms)) && (!own_packet)) {
+ if ((atomic_read(&bat_priv->aggregated_ogms))) {
hlist_for_each_entry(forw_packet_pos, tmp_node,
&bat_priv->forw_bat_list, list) {
if (batadv_iv_ogm_can_aggregate(batadv_ogm_packet,
@@ -532,7 +506,7 @@ static void batadv_iv_ogm_queue_add(struct batadv_priv *bat_priv,
if_incoming, own_packet);
} else {
batadv_iv_ogm_aggregate(forw_packet_aggr, packet_buff,
- packet_len, direct_link);
+ packet_len, direct_link, own_packet);
spin_unlock_bh(&bat_priv->forw_bat_list_lock);
}
}
@@ -593,14 +567,39 @@ static void batadv_iv_ogm_forward(struct batadv_orig_node *orig_node,
static void batadv_iv_ogm_schedule(struct batadv_hard_iface *hard_iface)
{
struct batadv_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
- unsigned char **ogm_buff = &hard_iface->bat_iv.ogm_buff;
+
+ queue_delayed_work(batadv_event_workqueue, &hard_iface->bat_iv.work,
+ batadv_iv_ogm_emit_wait_time(bat_priv));
+}
+
+/**
+ * batadv_iv_ogm_send - prepare an send an own OGM
+ * @work: kernel work struct
+ *
+ * Prepare the OGM and immediately enqueue it for sending
+ */
+static void batadv_iv_ogm_send(struct work_struct *work)
+{
+ struct delayed_work *delayed_work;
+ struct batadv_hard_iface_bat_iv *bat_iv;
+ struct batadv_hard_iface *primary_if, *hard_iface;
+ struct batadv_priv *bat_priv;
+ unsigned char **ogm_buff;
struct batadv_ogm_packet *batadv_ogm_packet;
- struct batadv_hard_iface *primary_if;
- int *ogm_buff_len = &hard_iface->bat_iv.ogm_buff_len;
+ int *ogm_buff_len;
int vis_server, tt_num_changes = 0;
uint32_t seqno;
uint8_t bandwidth;
+ delayed_work = container_of(work, struct delayed_work, work);
+ bat_iv = container_of(delayed_work, struct batadv_hard_iface_bat_iv,
+ work);
+ hard_iface = container_of(bat_iv, struct batadv_hard_iface, bat_iv);
+
+ bat_priv = netdev_priv(hard_iface->soft_iface);
+ ogm_buff = &hard_iface->bat_iv.ogm_buff;
+ ogm_buff_len = &hard_iface->bat_iv.ogm_buff_len;
+
vis_server = atomic_read(&bat_priv->vis_mode);
primary_if = batadv_primary_if_get_selected(bat_priv);
@@ -611,10 +610,9 @@ static void batadv_iv_ogm_schedule(struct batadv_hard_iface *hard_iface)
batadv_ogm_packet = (struct batadv_ogm_packet *)(*ogm_buff);
+ seqno = (uint32_t)atomic_add_return(1, &hard_iface->bat_iv.ogm_seqno);
/* change sequence number to network order */
- seqno = (uint32_t)atomic_read(&hard_iface->bat_iv.ogm_seqno);
batadv_ogm_packet->seqno = htonl(seqno);
- atomic_inc(&hard_iface->bat_iv.ogm_seqno);
batadv_ogm_packet->ttvn = atomic_read(&bat_priv->tt.vn);
batadv_ogm_packet->tt_crc = htons(bat_priv->tt.local_crc);
@@ -637,12 +635,55 @@ static void batadv_iv_ogm_schedule(struct batadv_hard_iface *hard_iface)
batadv_slide_own_bcast_window(hard_iface);
batadv_iv_ogm_queue_add(bat_priv, hard_iface->bat_iv.ogm_buff,
hard_iface->bat_iv.ogm_buff_len, hard_iface, 1,
- batadv_iv_ogm_emit_send_time(bat_priv));
+ batadv_iv_ogm_fwd_send_time());
if (primary_if)
batadv_hardif_free_ref(primary_if);
}
+/**
+ * batadv_iv_ogm_iface_enable - initialize protocol specific hard_iface members
+ * @hard_iface: the interface getting enabled
+ *
+ * Returns 0 on success and a negative value representing the error in case of
+ * failure
+ *
+ */
+static int batadv_iv_ogm_iface_enable(struct batadv_hard_iface *hard_iface)
+{
+ struct batadv_ogm_packet *batadv_ogm_packet;
+ unsigned char *ogm_buff;
+ uint32_t random_seqno;
+ int res = -ENOMEM;
+
+ /* randomize initial seqno to avoid collision */
+ get_random_bytes(&random_seqno, sizeof(random_seqno));
+ atomic_set(&hard_iface->bat_iv.ogm_seqno, random_seqno);
+
+ hard_iface->bat_iv.ogm_buff_len = BATADV_OGM_HLEN;
+ ogm_buff = kmalloc(hard_iface->bat_iv.ogm_buff_len, GFP_ATOMIC);
+ if (!ogm_buff)
+ goto out;
+
+ hard_iface->bat_iv.ogm_buff = ogm_buff;
+
+ batadv_ogm_packet = (struct batadv_ogm_packet *)ogm_buff;
+ batadv_ogm_packet->header.packet_type = BATADV_IV_OGM;
+ batadv_ogm_packet->header.version = BATADV_COMPAT_VERSION;
+ batadv_ogm_packet->header.ttl = 2;
+ batadv_ogm_packet->flags = BATADV_NO_FLAGS;
+ batadv_ogm_packet->tq = BATADV_TQ_MAX_VALUE;
+ batadv_ogm_packet->tt_num_changes = 0;
+ batadv_ogm_packet->ttvn = 0;
+
+ INIT_DELAYED_WORK(&hard_iface->bat_iv.work, batadv_iv_ogm_send);
+
+ res = 0;
+
+out:
+ return res;
+}
+
static void
batadv_iv_ogm_orig_update(struct batadv_priv *bat_priv,
struct batadv_orig_node *orig_node,
@@ -998,7 +1039,6 @@ static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,
bool is_single_hop_neigh = false;
bool is_from_best_next_hop = false;
int is_duplicate, sameseq, simlar_ttl;
- uint32_t if_incoming_seqno;
uint8_t *prev_sender;
/* Silently drop when the batman packet is actually not a
@@ -1016,9 +1056,6 @@ static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,
if (batadv_ogm_packet->header.packet_type != BATADV_IV_OGM)
return;
- /* could be changed by schedule_own_packet() */
- if_incoming_seqno = atomic_read(&if_incoming->bat_iv.ogm_seqno);
-
if (batadv_ogm_packet->flags & BATADV_DIRECTLINK)
has_directlink_flag = 1;
else
@@ -1108,7 +1145,7 @@ static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,
spin_lock_bh(&orig_neigh_node->ogm_cnt_lock);
word = &(orig_neigh_node->bcast_own[offset]);
- bit_pos = if_incoming_seqno - 2;
+ bit_pos = atomic_read(&if_incoming->bat_iv.ogm_seqno);
bit_pos -= ntohl(batadv_ogm_packet->seqno);
batadv_set_bit(word, bit_pos);
weight = &orig_neigh_node->bcast_own_sum[if_num];
diff --git a/types.h b/types.h
index 030ce41..978381a 100644
--- a/types.h
+++ b/types.h
@@ -49,6 +49,7 @@ struct batadv_hard_iface_bat_iv {
unsigned char *ogm_buff;
int ogm_buff_len;
atomic_t ogm_seqno;
+ struct delayed_work work;
};
struct batadv_hard_iface {
--
1.8.0.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv3] batman-adv: postpone OGM preparation to sending time
2012-12-15 11:14 [B.A.T.M.A.N.] [PATCHv3] batman-adv: postpone OGM preparation to sending time Antonio Quartulli
@ 2012-12-26 10:08 ` Marek Lindner
2013-01-02 7:48 ` Antonio Quartulli
0 siblings, 1 reply; 5+ messages in thread
From: Marek Lindner @ 2012-12-26 10:08 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Saturday, December 15, 2012 19:14:42 Antonio Quartulli wrote:
> OGMs are currently prepared 1 originator interval in advance
> then the time they are used to be sent.
> This means that once in the air they carry old information (like TT
> announcements and possibly other flags). To fix this, postpone the OGM
> creation to the same time of sending, in this way the OGM is first created
> and then immediately sent.
How about:
OGMs are currently assembled and enqueued one originator interval before they
are sent. This means they might carry information which was outdated while the
OGM waited in the outgoing packet queue (like TT announcements and possibly
other flags).
The OGM assembly has to be postponed to the latest possible moment before
sending in order to minimize the gap between gathering the data and flooding
it to the network.
> -/* when do we schedule our own ogm to be sent */
> +/**
> + * batadv_iv_ogm_emit_wait_time - compute the OGM preparation waiting time
> + * @bat_priv: the bat priv with all the soft interface information
> + *
> + * Returns the amount of jiffies to wait before preparing and sending the
> next + * own OGM
> + */
I'd say "Returns the number of jiffies [..]".
> @@ -468,6 +442,7 @@ static void batadv_iv_ogm_aggregate(struct
> batadv_forw_packet *forw_packet_aggr, memcpy(skb_buff, packet_buff,
> packet_len);
> forw_packet_aggr->packet_len += packet_len;
> forw_packet_aggr->num_packets++;
> + forw_packet_aggr->own |= own_packet;
>
> /* save packet direct link flag status */
> if (direct_link) {
Using "|= own_packet" isn't strictly necessary because "forw_packet_aggr->own"
isn't a bit field.
Did you vigorously test this code ? Especially, multi-node with multiple
interface setups are of interest. Also use different orig intervals to ensure
it still works everywhere.
The thing is: Throughout the code you can find the implicite assumption of the
first aggregated packet being an "own packet" (if forw_packet_aggr->own is
set). Therefore, you have to be very careful changing that logic. One function
you definitely overlooked is batadv_iv_ogm_send_to_if() but there might be
others.
> @@ -498,8 +473,7 @@ static void batadv_iv_ogm_queue_add(struct batadv_priv
> *bat_priv,
>
> /* find position for the packet in the forward queue */
> spin_lock_bh(&bat_priv->forw_bat_list_lock);
> - /* own packets are not to be aggregated */
> - if ((atomic_read(&bat_priv->aggregated_ogms)) && (!own_packet)) {
> + if ((atomic_read(&bat_priv->aggregated_ogms))) {
> hlist_for_each_entry(forw_packet_pos, tmp_node,
> &bat_priv->forw_bat_list, list) {
> if (batadv_iv_ogm_can_aggregate(batadv_ogm_packet,
Now it gets interesting! Did you review the impact of
batadv_iv_ogm_can_aggregate() before you bravely removed the exclusion of own
packets ? That is quite a beast ...
> +/**
> + * batadv_iv_ogm_send - prepare an send an own OGM
> + * @work: kernel work struct
> + *
> + * Prepare the OGM and immediately enqueue it for sending
> + */
prepare and send an own OGM
^^^
> +static void batadv_iv_ogm_send(struct work_struct *work)
The naming is less than optimal. bat_iv_ogm.c now has:
* batadv_iv_ogm_send
* batadv_iv_ogm_emit
> --- a/types.h
> +++ b/types.h
> @@ -49,6 +49,7 @@ struct batadv_hard_iface_bat_iv {
> unsigned char *ogm_buff;
> int ogm_buff_len;
> atomic_t ogm_seqno;
> + struct delayed_work work;
> };
Guess we need some kernel doc for this change too. Since types.h now is fully
documented ...
Cheers,
Marek
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv3] batman-adv: postpone OGM preparation to sending time
2012-12-26 10:08 ` Marek Lindner
@ 2013-01-02 7:48 ` Antonio Quartulli
2013-01-02 11:44 ` Marek Lindner
0 siblings, 1 reply; 5+ messages in thread
From: Antonio Quartulli @ 2013-01-02 7:48 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
[-- Attachment #1: Type: text/plain, Size: 4728 bytes --]
On Wed, Dec 26, 2012 at 06:08:50 +0800, Marek Lindner wrote:
> On Saturday, December 15, 2012 19:14:42 Antonio Quartulli wrote:
> > OGMs are currently prepared 1 originator interval in advance
> > then the time they are used to be sent.
> > This means that once in the air they carry old information (like TT
> > announcements and possibly other flags). To fix this, postpone the OGM
> > creation to the same time of sending, in this way the OGM is first created
> > and then immediately sent.
>
> How about:
> OGMs are currently assembled and enqueued one originator interval before they
> are sent. This means they might carry information which was outdated while the
> OGM waited in the outgoing packet queue (like TT announcements and possibly
> other flags).
> The OGM assembly has to be postponed to the latest possible moment before
> sending in order to minimize the gap between gathering the data and flooding
> it to the network.
Sounds much better. Thanks!
>
>
> > -/* when do we schedule our own ogm to be sent */
> > +/**
> > + * batadv_iv_ogm_emit_wait_time - compute the OGM preparation waiting time
> > + * @bat_priv: the bat priv with all the soft interface information
> > + *
> > + * Returns the amount of jiffies to wait before preparing and sending the
> > next + * own OGM
> > + */
>
> I'd say "Returns the number of jiffies [..]".
>
Yeah
>
> > @@ -468,6 +442,7 @@ static void batadv_iv_ogm_aggregate(struct
> > batadv_forw_packet *forw_packet_aggr, memcpy(skb_buff, packet_buff,
> > packet_len);
> > forw_packet_aggr->packet_len += packet_len;
> > forw_packet_aggr->num_packets++;
> > + forw_packet_aggr->own |= own_packet;
> >
> > /* save packet direct link flag status */
> > if (direct_link) {
>
> Using "|= own_packet" isn't strictly necessary because "forw_packet_aggr->own"
> isn't a bit field.
Well the point is that here own_packet could be false, but forw_packet_aggr->own
might already be true, so I didn't want to destroy the original value.
> Did you vigorously test this code ? Especially, multi-node with multiple
> interface setups are of interest. Also use different orig intervals to ensure
> it still works everywhere.
>
I will try more topologies and in particular different orig intervals as soon as
I have the possibility
> The thing is: Throughout the code you can find the implicite assumption of the
> first aggregated packet being an "own packet" (if forw_packet_aggr->own is
> set). Therefore, you have to be very careful changing that logic. One function
> you definitely overlooked is batadv_iv_ogm_send_to_if() but there might be
> others.
>
>
> > @@ -498,8 +473,7 @@ static void batadv_iv_ogm_queue_add(struct batadv_priv
> > *bat_priv,
> >
> > /* find position for the packet in the forward queue */
> > spin_lock_bh(&bat_priv->forw_bat_list_lock);
> > - /* own packets are not to be aggregated */
> > - if ((atomic_read(&bat_priv->aggregated_ogms)) && (!own_packet)) {
> > + if ((atomic_read(&bat_priv->aggregated_ogms))) {
> > hlist_for_each_entry(forw_packet_pos, tmp_node,
> > &bat_priv->forw_bat_list, list) {
> > if (batadv_iv_ogm_can_aggregate(batadv_ogm_packet,
>
> Now it gets interesting! Did you review the impact of
> batadv_iv_ogm_can_aggregate() before you bravely removed the exclusion of own
> packets ? That is quite a beast ...
well, the poor existing doc didn't point out all the possible threats :-P I'll
investigate more. Thanks :)
>
>
> > +/**
> > + * batadv_iv_ogm_send - prepare an send an own OGM
> > + * @work: kernel work struct
> > + *
> > + * Prepare the OGM and immediately enqueue it for sending
> > + */
>
> prepare and send an own OGM
> ^^^
>
well, technically it is enqueued, not sent..
>
> > +static void batadv_iv_ogm_send(struct work_struct *work)
>
> The naming is less than optimal. bat_iv_ogm.c now has:
> * batadv_iv_ogm_send
> * batadv_iv_ogm_emit
mh..ok, I will propose something better :)
>
>
> > --- a/types.h
> > +++ b/types.h
> > @@ -49,6 +49,7 @@ struct batadv_hard_iface_bat_iv {
> > unsigned char *ogm_buff;
> > int ogm_buff_len;
> > atomic_t ogm_seqno;
> > + struct delayed_work work;
> > };
>
> Guess we need some kernel doc for this change too. Since types.h now is fully
> documented ...
>
only because you are commenting this patch after having merged your full-kernel-doc-patch :-D
Ok, I will fix it too.
Thank you very much for your precious feedback.
Greetings from down under!
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv3] batman-adv: postpone OGM preparation to sending time
2013-01-02 7:48 ` Antonio Quartulli
@ 2013-01-02 11:44 ` Marek Lindner
2013-01-03 9:54 ` Antonio Quartulli
0 siblings, 1 reply; 5+ messages in thread
From: Marek Lindner @ 2013-01-02 11:44 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Wednesday, January 02, 2013 15:48:13 Antonio Quartulli wrote:
> > > @@ -468,6 +442,7 @@ static void batadv_iv_ogm_aggregate(struct
> > > batadv_forw_packet *forw_packet_aggr, memcpy(skb_buff, packet_buff,
> > > packet_len);
> > >
> > > forw_packet_aggr->packet_len += packet_len;
> > > forw_packet_aggr->num_packets++;
> > >
> > > + forw_packet_aggr->own |= own_packet;
> > >
> > > /* save packet direct link flag status */
> > > if (direct_link) {
> >
> > Using "|= own_packet" isn't strictly necessary because
> > "forw_packet_aggr->own" isn't a bit field.
>
> Well the point is that here own_packet could be false, but
> forw_packet_aggr->own might already be true, so I didn't want to destroy
> the original value.
Good point. However, your "solution" is far from obvious. Either you make it
more obvious or you should add a comment.
> > Did you vigorously test this code ? Especially, multi-node with multiple
> > interface setups are of interest. Also use different orig intervals to
> > ensure it still works everywhere.
>
> I will try more topologies and in particular different orig intervals as
> soon as I have the possibility
Ok.
> > The thing is: Throughout the code you can find the implicite assumption
> > of the first aggregated packet being an "own packet" (if
> > forw_packet_aggr->own is set). Therefore, you have to be very careful
> > changing that logic. One function you definitely overlooked is
> > batadv_iv_ogm_send_to_if() but there might be others.
You did not comment this section. Hopefully it wasn't overlooked ?
> > > +/**
> > > + * batadv_iv_ogm_send - prepare an send an own OGM
> > > + * @work: kernel work struct
> > > + *
> > > + * Prepare the OGM and immediately enqueue it for sending
> > > + */
> >
> > prepare and send an own OGM
> >
> > ^^^
>
> well, technically it is enqueued, not sent..
Your kernel doc states:
batadv_iv_ogm_send - prepare an send an own OGM
Therefore I proposed a fix:
batadv_iv_ogm_send - prepare and send an own OGM
If you wish to reword it altogether that's also ok for me.
Cheers,
Marek
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv3] batman-adv: postpone OGM preparation to sending time
2013-01-02 11:44 ` Marek Lindner
@ 2013-01-03 9:54 ` Antonio Quartulli
0 siblings, 0 replies; 5+ messages in thread
From: Antonio Quartulli @ 2013-01-03 9:54 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
[-- Attachment #1: Type: text/plain, Size: 2673 bytes --]
On Wed, Jan 02, 2013 at 07:44:21PM +0800, Marek Lindner wrote:
> On Wednesday, January 02, 2013 15:48:13 Antonio Quartulli wrote:
> > > > @@ -468,6 +442,7 @@ static void batadv_iv_ogm_aggregate(struct
> > > > batadv_forw_packet *forw_packet_aggr, memcpy(skb_buff, packet_buff,
> > > > packet_len);
> > > >
> > > > forw_packet_aggr->packet_len += packet_len;
> > > > forw_packet_aggr->num_packets++;
> > > >
> > > > + forw_packet_aggr->own |= own_packet;
> > > >
> > > > /* save packet direct link flag status */
> > > > if (direct_link) {
> > >
> > > Using "|= own_packet" isn't strictly necessary because
> > > "forw_packet_aggr->own" isn't a bit field.
> >
> > Well the point is that here own_packet could be false, but
> > forw_packet_aggr->own might already be true, so I didn't want to destroy
> > the original value.
>
> Good point. However, your "solution" is far from obvious. Either you make it
> more obvious or you should add a comment.
>
Oky. Will try to make it more clear and avoid obfuscated code :)
>
> > > Did you vigorously test this code ? Especially, multi-node with multiple
> > > interface setups are of interest. Also use different orig intervals to
> > > ensure it still works everywhere.
> >
> > I will try more topologies and in particular different orig intervals as
> > soon as I have the possibility
>
> Ok.
>
>
> > > The thing is: Throughout the code you can find the implicite assumption
> > > of the first aggregated packet being an "own packet" (if
> > > forw_packet_aggr->own is set). Therefore, you have to be very careful
> > > changing that logic. One function you definitely overlooked is
> > > batadv_iv_ogm_send_to_if() but there might be others.
>
> You did not comment this section. Hopefully it wasn't overlooked ?
no, it wasn't. I simply silently acknowledged your comment :)
>
>
> > > > +/**
> > > > + * batadv_iv_ogm_send - prepare an send an own OGM
> > > > + * @work: kernel work struct
> > > > + *
> > > > + * Prepare the OGM and immediately enqueue it for sending
> > > > + */
> > >
> > > prepare and send an own OGM
> > >
> > > ^^^
> >
> > well, technically it is enqueued, not sent..
>
> Your kernel doc states:
> batadv_iv_ogm_send - prepare an send an own OGM
>
> Therefore I proposed a fix:
> batadv_iv_ogm_send - prepare and send an own OGM
oh, right! I just read the line above your comment, not the rest.
>
> If you wish to reword it altogether that's also ok for me.
>
mh, maybe. Thanks²
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-01-03 9:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-15 11:14 [B.A.T.M.A.N.] [PATCHv3] batman-adv: postpone OGM preparation to sending time Antonio Quartulli
2012-12-26 10:08 ` Marek Lindner
2013-01-02 7:48 ` Antonio Quartulli
2013-01-02 11:44 ` Marek Lindner
2013-01-03 9:54 ` Antonio Quartulli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).