Subject: [Gen-art] Gen-ART Last Call review of draft-ietf-manet-olsrv2-dat-metric-08

From: Paul Kyzivat <[email-masked]>
Date: Fri, 20 Nov 2015 15:41:47 -0500

[In progress - not ready for sending]

I am the assigned Gen-ART reviewer for this draft. The General Area 
Review Team (Gen-ART) reviews all IETF documents being processed by the 
IESG for the IETF Chair. Please treat these comments just like any other 
last call comments. For more information, please see the FAQ at 
<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-manet-olsrv2-dat-metric-08
Reviewer: Paul Kyzivat
Review Date: TBS
IETF LC End Date: 2015-11-23
IESG Telechat date: 2015-12-03

Summary:

This draft is basically ready for publication as an Experimental RFC, 
but has nits that should be fixed before publication.

Major: 0
Minor: 0
Nits:  5

Nits:

* Section 1:

"Appendix A contains a few possible steps to improve the DAT metric."

This is the first occurrence of the "DAT" acronym. It took me a bit to 
realize this must be referring to "Directional AirTime". Could you 
please define the acronym *before* the first use? (Perhaps in the prior 
paragraph where "Directional Airtime" is first used.)

* Section 2:

"These networks employ link layer retransmission to increase the 
delivery probability and multiple unicast data rates."

I'm not sure how to parse this sentence. Is it intended to be:

"These networks employ link layer retransmission to increase (the 
delivery probability) and (multiple unicast data rates)."

OR "These networks employ link layer retransmission to (increase the 
delivery probability) and (multiple unicast data rates)."

Either way I don't understand what "multiple unicast data rates" means. 
Can you clarify this?

* Section 7:

You call these *constants*, in contrast to the *parameters* defined in 
section 6. But you then suggest conditions under which they could be 
changed. Perhaps they should simply be included with the parameters, but 
with strong warnings about diverging from the recommended values.

Else, it would be good to define these *before* the parameters, because 
that would avoid the forward reference to DAT_MAXIMUM_LOSS.

* Section 9.3/9.4:

Are you considering these to be mutually exclusive? Or is a HELLO 
message processed first by 9.3, and then by 9.4?

Since there is considerable overlap in processing between the two, and 
it would presumably be wrong to do that twice, I guess you must assume 
them to be mutually exclusive. But I presume HELLO messages arrive in 
incoming packets, so 9.3 sounds like it ought to apply to them.

If I guessed right, then I suggest revising 9.3 to start "For each 
incoming packet that is not a HELLO message, ..."

* Section 10.2:

IIUC steps 5.3 & 5.4 are just the hard way of saying:

   bitrate := MAX(L_DAT_rx_bitrate, DAT_MINIMUM_BITRATE)

---

From: Henning Rogge <[email-masked]>
Date: Tue, 24 Nov 2015 14:16:35 +0100

> paragraph where "Directional Airtime" is first used.)

DAT was explained in the Terminology section... I moved it to the first 
place in this section.

> delivery probability) and (multiple unicast data rates)."

I will split the sentence apart into two parts to make this more clear.

> Either way I don't understand what "multiple unicast data rates" means.

Wifi (802.11) provides multiple datarates for unicast traffic. I will 
change the sentence to make it easier to understand.

> with strong warnings about diverging from the recommended values.

Parameters can be chosen by a local implementation/installation... but 
constants need to be the same over the whole Mesh.

> that would avoid the forward reference to DAT_MAXIMUM_LOSS.

I will switch the Parameter and the Constant section.

> message processed first by 9.3, and then by 9.4?

the RFC5444 packet (which contains the Hello Message, but that is 
unimportant for this processing part) is processed by 9.3

the Hello Message is processed by 9.4

> incoming packets, so 9.3 sounds like it ought to apply to them.

The trouble is (thank you to Thomas Clausen for pointing this out) that 
some RFC5444 implementations don't have access to the packet sequence 
number. So this metric (which needs the validity time value of the Hello 
Message anyways) contains a fallback that provides a loss estimate based 
on Hello Intervals if a node does not provide a Packet sequence number.

>    bitrate := MAX(L_DAT_rx_bitrate, DAT_MINIMUM_BITRATE)

I transformed both IF conditions into a MIN() and a MAX() statement.

Henning Rogge

-- 
Diplom-Informatiker Henning Rogge , Fraunhofer-Institut für
Kommunikation, Informationsverarbeitung und Ergonomie FKIE
Kommunikationssysteme (KOM)
Fraunhofer Straße 20, 53343 Wachtberg, Germany
Telefon +49 228 9435-961,   Fax +49 228 9435 685
mailto:[email-masked] http://www.fkie.fraunhofer.de

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 24 Nov 2015 10:00:11 -0500

The remedies below seem ok to me. One comment inline.

>> incoming packets, so 9.3 sounds like it ought to apply to them.

So you are saying they are mutually exclusive: HELLO message by 9.4, all 
other packets by 9.3. Can you make the text clearer about this. As 
written it still could be interpreted as applying 9.3 to all packets, 
including HELLO, and then applying 9.4 to HELLO packets. (Even though 
this is nonsense if you think about it.)

	Thanks,
	Paul

---

From: Henning Rogge <[email-masked]>
Date: Tue, 24 Nov 2015 16:09:37 +0100

> this is nonsense if you think about it.)

No, it is NOT exclusive...

there is no Hello packet... there are just RFC5444 packets which might 
contain one (or in theory multiple) NHDP HELLO messages.

Its the same with IP and UDP... every UDP header is contained in an IP 
header. We are talking about two different levels of hierarchy.

DAT is processing both the RFC5444 packet header (which contains the 
packet sequence number) of ALL RFC5444 packets and the message content 
of the HELLO messages.

Henning Rogge

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 24 Nov 2015 11:32:49 -0500

Henning,

> packet sequence number) of ALL RFC5444 packets and the message content

OK. But my confusion remains, because there seem to be duplicate actions 
in both 9.3 and 9.4 that ought not be done twice. In particular:

9.3:
    1.  If L_DAT_last_pkt_seqno = UNDEFINED, then:

        1.  L_DAT_received[TAIL] := 1.

        2.  L_DAT_total[TAIL] := 1.

9.4:
    3.  If L_DAT_last_pkt_seqno = UNDEFINED, then:

        1.  L_DAT_received[TAIL] := L_DAT_received[TAIL] + 1.

        2.  L_DAT_total[TAIL] := L_DAT_total[TAIL] + 1.

So, if I do 9.3 for every packet, and then also do 9.4 for every HELLO 
message within that packet, then I could end up incrementing 
L_DAT_received[TAIL] *twice*.

	Thanks,
	Paul

---

From: Henning Rogge <[email-masked]>
Date: Wed, 25 Nov 2015 08:05:35 +0100

> L_DAT_received[TAIL] *twice*.

If you look at the head of section 9.3 you will notice that this section 
will only be processed in the presence of a packet sequence number. One 
of the first things 9.3 does is to set the L_DAT_last_pkt_seqno variable.

The part of 9.4 dealing with L_DAT_received[] and L_DAT_total[] is the 
fallback strategy when the packet sequence number is missing. As soon as 
we see a packet sequence number from a neighbor, this codepath will be 
deactivated because the L_DAT_last_pkt_seqno variable will never be 
reset back to UNDEFINED.

See also section 4) for a more detailed explanation:

...
"The metric normally measures the loss of a link by tracking the 
incoming [RFC5444] packet sequence numbers.  Without these packet
sequence numbers, the metric does calculate the loss of the link
based of received and lost [RFC5444] HELLO messages.  It uses the
incoming HELLO interval time (or if not present, the validity time)
to decide when a HELLO is lost."

Henning Rogge

---

From: Paul Kyzivat <[email-masked]>
Date: Wed, 25 Nov 2015 10:11:36 -0500

Henning,

I'm not trying to second guess the intent. My issues are solely with the 
presentation. And clearly this is not a subject I'm knowledgeable about, 
so I'm going solely from what I read in this draft without any other 
context. Somebody who has the proper context to be reading this may well 
not find it at all confusing. Nevertheless, see inline.

> of the first things 9.3 does is to set the L_DAT_last_pkt_seqno variable.

OK. My mistake. Because 9.3 sets L_DAT_last_pkt_seqno it won't get 
updated again in 9.4.

But then, what is the purpose step 3 of 9.4? If 9.3 is always applied 
before 9.4, then L_DAT_last_pkt_seqno can never be undefined in 9.4.

	Thanks,
	Paul

---

From: Henning Rogge <[email-masked]>
Date: Thu, 26 Nov 2015 13:30:28 +0100

> not find it at all confusing. Nevertheless, see inline.

No problem...

> before 9.4, then L_DAT_last_pkt_seqno can never be undefined in 9.4.

It could be that you have a neighbor that will never deliver a packet 
sequence number, so 9.3 will never apply.

Because I am not sure if packet (9.3) or message (9.4) processing is 
done first, I make sure to overwrite the data in the current element of 
the queue in 9.3 when I discover the packet sequence number.

Henning Rogge

P.S.: I will be away from work in December, so I added my private email 
[email-masked] to this thread.

---

From: Paul Kyzivat <[email-masked]>
Date: Mon, 30 Nov 2015 10:54:59 -0500

Henning,

OK. I'm now happy with -10.

	Thanks,
	Paul