[aodvv2-discuss] Re: Fwd: [manet] A few more AODVv2 comments
- From: Victoria Mercieca <vmercieca0@xxxxxxxxx>
- To: "aodvv2-discuss@xxxxxxxxxxxxx" <aodvv2-discuss@xxxxxxxxxxxxx>
- Date: Wed, 25 Mar 2015 09:18:37 +0000
Hi Charlie,
Couple of minor comments below:
On Tue, Mar 24, 2015 at 7:29 PM, Charlie Perkins <
charles.perkins@xxxxxxxxxxxxx> wrote:
>
> Hello Vicky,
>
> I was a little nervous about leaving out the check for the
> default metric type.
>
> Here's an example of the new pseudocode:
>
> /* Build Metric Address Block TLV */
> metricAddrBlkTlv.value := outRREQ.origAddrMetric
> if (outRREQ.metricType != DEFAULT_METRIC_TYPE)
> { /* include Metric AddrBlkTlv extension byte */
> metricAddrBlkTlv.typeExtension := outRREQ.MetricType
> }
>
> However, if there is general agreement to use the simpler
> formulation, we could just have
>
> /* Build Metric Address Block TLV */
> metricAddrBlkTlv := outRREQ.MetricType
> metricAddrBlkTlv.value := outRREQ.origAddrMetric
>
> Your first example is better - if using the default metric, we dont use
the type extension at all, I guess?
> on the understanding that for non-default types, the
> assignment somehow translates into an RFC 5444 no-op.
>
>
> ==========================
> Non-MetricType related:
> ==========================
> In A.1.1 we have if (rte exists) then create route...oops!
>
>
> if (!rte exists)
> ......
>
>
>
> To match earlier sections we should put loopfree within the "rule from
> 8.1" code and update that to say if seqnum is the same, AND (if cost is
> less, OR (if cost is same or greater and LoopFree is true)) then update
> route entry.
>
>
> Done. I got rid of pseudocode in section 8.1 that basically duplicates
> this, since Chris was complaining about it.
>
>
> In Fetch_Route_Table_Entry, we compare to "MetricType" but the m should
> not be capitalised, since metricType (lower case m) is what is passed to
> this function.
>
>
> Done.
>
>
> For LoopFree, we should probably replace R2 and R1 with AdvRte and Rte
> for consistency
>
> In Update_Rte_Msg_Table, we should be storing metric too under "if
> (entry does not exist)":
> entry.Metric := rteMsg.origAddrMetric(for RREQ) or
> rteMsg.targAddrMetric(for RREP)
> We also have "rteMsg.OrigSeqNum" and "rteMsg.TargSeqNum" where the o and t
> should be lower case to match the definitions at the start of Appendix A.
> Although it seems that the definition of rteMsg parameters is inconsistent
> with the definition of AdvRte and Route parameters, perhaps they should all
> be capitalised throughout the appendix? And maybe we should define
> parameters of the RteMsgTable entries up there too?
> If you want:
> The RteMsg table entries have the following parameters as described in
> Section 6.5:
> RteMsgTableEntry.MessageType
> RteMsgTableEntry.OrigAddr
> RteMsgTableEntry.TargAddr
> RteMsgTableEntry.OrigSeqNum (RREQ only)
> RteMsgTableEntry.TargSeqNum (RREP only)
> RteMsgTableEntry.MetricType
> RteMsgTableEntry.Metric
> RteMsgTableEntry.Timestamp
>
>
> I'm not sure about this just right now, maybe we can return to it?
>
> The list of parameters, of course. I noticed you added the line about
storing metric.
> In A.3.3 we need an extra line between metricAddrBlkTlv.value :=
> outRREP.targAddrMetric and the validity time line.
>
> In A.4 we should be consistent with the other sections using capital
> letters for parameters.
>
>
> I tried to fix a couple of things here but I am not sure I found what you
> meant.
>
> I meant that for rteMsg (and therefore for outRREQ, inRREQ, outRREP,
inRREP), and also for RERR we have:
rteMsg.hopLimit
rteMsg.hopCount
outRERR.metricType, etc.
(i.e. lower case letter after the .)
but for other structures we use a capital, e.g.:
AdvRte.MetricType
Route.Address
RteMsgTable entry.MessageType
I thought we should use a consistent approach.
> In A.4.1 we should somehow stick in that the RERR isn't generated if
> only Idle routes are affected. Or just remove
> OR
> (rte.State == Idle AND ENABLE_IDLE_IN_RERR)
> for now? or after the foreach (rte in route table)
> add:
> /* repeat for-loop for Idle routes if num-broken-addr > 0 AND
> ENABLE_IDLE_IN_RERR */
>
>
> I added this test:
> if (rte.State == Active)
> {
> doGenerate := TRUE
> }
>
> Looks good.
Regards,
Vicky.
I'll send out the revised version pretty soon....
>
>
> Regards,
> Charlie P.
>
>
>
>
>>
>> On 3/19/2015 4:52 AM, Victoria Mercieca wrote:
>>
>>
>>
>>>
>>>
>>> This could be changed to say "Lower layer trigger that a link is no
>>> longer bidirectional"
>>>
>>> or "Lower layer triggers, for example message reception or link
>> status notifications".
>> The fact we are receiving L2 frames could be seen as a confirmation of
>> bidirectionality?
>>
>>
>> Done.
>>
>>
>>>
>>> I would have explicitly said NHDP HELLO message, not just message later
>>> in that section (though it is the only NHDP message).
>>>
>>>
>>> That enlargement fits well in Section 4 during the discussion where NHDP
>>> is mentioned, and I put it there.
>>>
>>> Easily added in the list though?
>>
>>
>> I didn't think it fit nicely into that list, but on your recommendation
>> I put it there also.
>>
>>
>>> Blacklist removal seems to be MAY at one point, and SHOULD at another.
>>>
>>> Fixed.
>>>
>>> Also that’s discussing things that trigger additions to blacklists (e.g.
>>> failure to receive RREP_Ack) and removals due to timeouts. Might there not
>>> also be specific reasons to remove from blacklists (e.g. receiving NHDP
>>> HELLO that indicates that if using that option)?
>>>
>>> Yes, this is a good point.
>>>
>>> True, we have already put in that if RREP_Ack is received from a
>> router on the blacklist, it is removed from the blacklist. Maybe in Section
>> 6.2 where we discuss adding to the blacklist if we receive notification of
>> a non-bidirectional link, we also mention "If any of these triggers signal
>> that bidirectional connectivity exists to a blacklisted router, the router
>> can be immediately removed from the blacklist." or something similar?
>>
>>
>> Done.
>>
>>
>> It seems rather a lack of progress that we are still at the only
>>> form of metric that an implementation will know what to do with is a hop
>>> count.
>>>
>>> That's not true. It is known how to use other metrics, and in fact
>>> specifications for AODV were written for the other metrics. However those
>>> specifications have been considered to be out of scope for the base
>>> document.
>>>
>>> Well, almost. Most of the discussion of metrics appears to require only
>>> an increasing function. Except the penultimate bullet in the first list in
>>> Section 8.1 appears to be explicitly additive. Once you decide on additive,
>>> you can make the whole metrics decision much more open.
>>>
>>> Having gone to the trouble of including mechanism for alternative
>>> metrics, it would seem a shame to make such a severe restriction.
>>>
>>>
>>> Is it enough to say that the current draft will support any additive
>> metric / increasing metric, using Cost(R) = AdvRte.Metric + Cost(L), where
>> Cost(L) > 0, but also that LoopFree may be further optimised for other
>> metrics? Not mentioning specific metric types but making it extra clear
>> that its possible with the current spec to use a different metric. Note -
>> our Section 7.4 doesnt actually say a different LoopFree may also be
>> required.
>>
>>
>>
>> I’d go with Henning’s suggestion to fix a single data type (32 bit
>>> integer perhaps) which would replace the second paragraph of 7.4 and make
>>> things much more easily extensible.
>>>
>>>
>>> A single data type for metric cannot accurately represent the costs of
>>> the routes. All you can get is monotonicity. Or maybe I misunderstand?
>>>
>>> Section 8.1, from the numbered points onwards, appears to give three
>>> descriptions of how you handle competing routes. One is good, if completely
>>> rigorous. Two is fine if the other one is more human readable (and possibly
>>> belongs in an earlier section). Three appears more than is wanted. (I also
>>> note the use of both AND and && in the second.) Note that I haven’t
>>> actually checked if all three are consistent (that’s not trivial when they
>>> switch between formulae and terms like stale) let alone if they work. (I
>>> recall someone has done some work on the latter point, is that
>>> published/referenceable?)
>>>
>>> I agree with this, and perhaps the pseudo-code version is better left
>> in the appendix in A.1.1? And we may not need the summary at the end of 8.1?
>> We do have inconsistency with where LoopFree is checked. It is step 3 in
>> section 8.1, which means it is currently not checked if the advertised
>> route is lower cost, and is only checked if its repairing an invalid route.
>>
>>
>> Well, actually, that's pretty much right. I'm not sure about deleting
>> the summary.
>> Any summary can be viewed as duplicative. However, there are people in
>> the world
>> who like to get an overall understanding of what's going on, even at the
>> cost of a
>> few sentences.
>>
>> But in the appendix we check LoopFree first before even checking
>> SeqNum. I suppose the correct thing would be to check it after determining
>> the incoming information is not stale.
>>
>>
>> Not really, because if the information is not stale and the cost is less
>> we want to
>> use the route without calling LoopFree().
>>
>>
>>
>> Suggestions:
>>
>> 8.1:
>> Switch step 2 and 3.
>> Add in to the new Step 2: "If LoopFree returns true, continue to step 3."
>> The final point in the new Step 3 should be: "If the advertised route's
>> cost is the same or greater than the stored route, but the stored route's
>> state is Invalid, the advertised route can be used to repair the Invalid
>> route, since it has been proven LoopFree in step 2."
>> Remove the rest of the section. At least remove the pseudo-code since
>> this is slightly inconsistent as is, and is present in a more accurate form
>> in the appendix.
>>
>> A.1.1:
>>
>> /* rules from 8.1 */
>>
>> if (AdvRte.SeqNum < Route.SeqNum) /* advertised route is stale */
>>
>> return null;
>>
>> if (!LoopFree(advRte, rte))
>> { /* incoming route cannot be guaranteed loop free */
>> return null;
>> }
>> if (
>> (advRte.SeqNum > Route.SeqNum) /* stored route is stale, use
>> advRte */
>> OR
>> ((advRte.SeqNum == Route.SeqNum) /* same SeqNum */
>> AND
>> [(advRte.Cost < Route.Metric) /* advRte is better */
>>
>> OR
>> ((Route.State == Invalid))] /* advRte can repair stored */
>>
>> {
>> Update_Route_Table_Entry (rte, advRte);
>> }
>>
>>
>>
>> One publication is cited. Another publication about AODVv1 would
>>> still be valid and could be cited.
>>>
>>> Section 8.3 first paragraph I think would be better separating out
>>> AODVv2 from IP. Whether to forward a packet is a decision made consulting
>>> IP’s table, not AODVv2’s use of invalid. I think that means you should
>>> indicate that invalid routes should be struck from IP’s table.
>>>
>>> Well, not exactly, because invalid routes still contain valid sequence
>>> number information.
>>>
>>> So we have a local table which includes all the routes, including
>> Invalid ones, and we insert or remove entries in the main IP routing table
>> depending on state, so that the IP routing table contains all the AODVv2
>> routing table's Active and Idle routes but not the Invalid ones.
>>
>>
>> That is not enforced by the specification. In our implementation we
>> modified the
>> IP route table, and it is by no means incorrect to do so.
>>
>>
>>
>>>
>>> Third paragraph in 8.3 precursor table appears from left field. (Unless
>>> I’ve missed an earlier reference, I haven’t done an electronic search).
>>> While postponing details is fine, the fact that there is such a thing,
>>> related to a route, and might matter, wants flagging up earlier. At the
>>> least indicated as a possible addition in Section 6.1.
>>>
>>>
>>> The precursors are indeed mentioned as an optional part of the route
>>> table entry. I added a cross reference in section 6.1 also. What more
>>> should be done? It was earlier thought that specification details about
>>> optional features should be in the optional section. For myself, I think
>>> the feature is important enough to belong in the main body of the text, but
>>> I had earlier been requested to describe it as optional ...
>>>
>>> Maybe to soften it, change to "Implementations which support the
>> optional feature of precursor lists as described in Section 12.2 MUST also
>> expunge the route's precursor list at the time that the route itself is
>> expunged."? Or is this an implementation detail we dont really need to
>> put here? We could have a Delete_Route_Table_Entry in the appendix where we
>> stress that the precursors list should be expunged?
>>
>>
>> I don't really see what's wrong with keeping normative text about
>> precursors in
>> the section about precursors, and can't understand moving it to an
>> appendix.
>>
>>
>>
>>>
>>> Section 8.4 invalid route may be expunged. What’s the intended status of
>>> the least recently used comment? Is that a may within a may, or a should
>>> within a may (you may do this, if you do, you should do it that way), or
>>> just a suggestion?
>>>
>>>
>>> It is a MAY and a SHOULD within the MAY.
>>>
>>> Does this make that clearer: "Any Invalid route MAY be expunged, but
>> the least recently used Invalid route SHOULD be expunged first."?
>>
>>
>> I used a slightly modified version of your text. But
>> really the existing text was pretty unambiguous.
>>
>>
>>
>>
>>>
>>> The binary exponential backoff on page 25 needs more about what is and
>>> is not proposed (and how its parameters – not specified – relate to other
>>> parametesr).
>>>
>>>
>>> Here is the text from RFC 3561:
>>>
>>> To reduce congestion in a network, repeated attempts by a source node
>>> at route discovery for a single destination MUST utilize a binary
>>> exponential backoff. The first time a source node broadcasts a RREQ,
>>> it waits NET_TRAVERSAL_TIME milliseconds for the reception of a RREP.
>>> If a RREP is not received within that time, the source node sends a
>>> new RREQ. When calculating the time to wait for the RREP after
>>> sending the second RREQ, the source node MUST use a binary
>>> exponential backoff. Hence, the waiting time for the RREP
>>> corresponding to the second RREQ is 2 * NET_TRAVERSAL_TIME
>>> milliseconds. If a RREP is not received within this time period,
>>> another RREQ may be sent, up to RREQ_RETRIES additional attempts
>>> after the first RREQ. For each additional attempt, the waiting time
>>> for the RREP is multiplied by 2, so that the time conforms to a
>>> binary exponential backoff.
>>>
>>>
>>> Would that be sufficient?
>>>
>>> "SHOULD utilize a binary exponential backoff as described in RFC 3561." ?
>>
>> Or:
>>
>> "SHOULD utilize a binary exponential backoff where the wait time is doubled
>> for each retry based on an initial value of RREQ_WAIT_TIME."
>>
>>
>> SHOULD utilize a
>> binary exponential backoff, as described in <xref
>> target="RFC3561"/>,
>> where the initial wait time is RREQ_WAIT_TIME and the
>> wait time is doubled for each retry based.
>>
>>
>>>
>>> I’m not at all sure about the packet queuing comments. Do we have good
>>> experience here? Is affixed buffer right? Is that “usually positive”
>>> comment known to be so? I know enough about buffering to know I don’t know
>>> enough about buffering, and that there are some counterintuitive results
>>> in the field.
>>>
>>>
>>> I have had quite a bit of experience with buffering. This draft is not
>>> the place to review those results,
>>> but one thing for sure is that TCP session initiation packets ought to
>>> be buffered.
>>>
>>>
>>> I haven’t read section 9 in any detail yet, but I will note that figure
>>> 1 is really about message contents, not message structure (or format).
>>>
>>>
>>> The contents of the message are organized according to the structure in
>>> the figure. We could mix up the contents in any order, and they
>>> would still truthfully be the contents. The ordering of the contents
>>> follows the structure as shown. I'm not sure how to make this clearer.
>>>
>>>
>>> I agree with you, it mirrors what the RFC5444 message will look like,
>>> and this is why we explain the messages in this order. Maybe using the word
>>> "format" makes it seem like this should be more specific, but it's for
>>> RFC5444 to handle the specifics of the format. We could replace "have the
>>> following general format:" with "have the following contents:" and it would
>>> appease Chris here without really making much difference? The reader gets
>>> the idea of the message structure anyway from the way we've written the
>>> rest of the section.
>>
>>
>> First, let's try replacing "general format" by "general structure" and
>> see if he likes it.
>>
>>
>>
>
>
Other related posts:
