Hi Colleen,
The approach seems sane to me. A few nits on the patch
itself:
On Thu, Apr 03, 2014 at 09:57:53PM -0700, Colleen Twitty wrote:
> metric = 0;
> - if (time_after(jiffies, ifmsh->last_sn_update +
> - net_traversal_jiffies(sdata)) ||
> - time_before(jiffies, ifmsh->last_sn_update)) {
> - target_sn = ++ifmsh->sn;
> - ifmsh->last_sn_update = jiffies;
> - }
> + if (chk_old_sn && SN_LT(old_sn, orig_sn))
> + ifmsh->sn = max(ifmsh->sn, target_sn)+1;
> + target_sn = ifmsh->sn;
whitespace is quite broken here...
Can you rename old_sn to old_orig_sn?
Also, I think 'max' is insufficient in the case of
sequence number rollover -- e.g. I think these are unsigned,
but 12 should be considered greater than 0xffffffff. It
would be a good test to artificially set target_sn to
numbers near 0x80000000 and 0xffffffff to make sure
rollover works properly.
Perhaps it makes sense to add:
#define SN_GE(a,b) SN_LT((b),(a))
or so. Using SN_LT(old_sn, orig_sn) here feels like negative
logic to me, compared to:
if (chk_old_sn && SN_GE(orig_sn, old_sn)) {
> + orig_addr = PREQ_IE_ORIG_ADDR(elems.preq);
> +
> + /* Sec. 13.10.8.3 HWMP sequence numbering calls for the target
> + * HWMP seq number to be incremented after each processed PREQ,
> + * but that would result in all but the last PREP metric info
> + * to be ignored. Instead, we only increment the target SN
> + * when we detect that the originator SN has changed. This
> + * results in all the PREPs generated in response to a single
> + * PREQ to have the same seq num, and, therefore, for the
> + * metrics for the different paths to be taken into account.
> + */
I'd rather have this comment near the increment of target SN
than here (but I like the comment!). Oh, please also add
the version of the std, i.e.
/* 802.11-2012 Sec. 13.10.8.3 ...
since the section numbers can change with every revision.
> + rcu_read_lock();
> + mpath = mesh_path_lookup(sdata, orig_addr);
I believe we do this lookup a few lines later in hwmp_route_info_get.
It'd probably be a bit of a mess to change things so that we only
do it once, but it's kind of unfortunate to have to do it twice.
> + u32 old_sn;
[...]
> if (last_hop_metric)
> hwmp_preq_frame_process(sdata, mgmt, elems.preq,
> - last_hop_metric);
> + last_hop_metric, chk_old_sn,
> + old_sn);
I know we don't use it -- but does compiler warn that old_sn
is passed uninitialized here if !chk_old_sn? I wouldn't oppose
initializing it to some dummy value.
--
Bob Copeland %% www.bobcopeland.com
_______________________________________________
Devel mailing list
[email protected]
http://lists.open80211s.org/cgi-bin/mailman/listinfo/devel
