Re: [pim] John Scudder's No Objection on draft-ietf-pim-assert-packing-10: (with COMMENT)

[Toerless Eckert <[e-mail]>]

[resending, as the first mail send didn't have my replies]

reply to ballot position/details. Inline.

On Mon, Mar 20, 2023 at 02:18:24PM -0700, John Scudder via Datatracker wrote:
> John Scudder has entered the following ballot position for
> draft-ietf-pim-assert-packing-10: No Objection
> 
> When responding, please keep the subject line intact and reply to all
> email addresses included in the To and CC lines. (Feel free to cut this
> introductory paragraph, however.)
> 
> 
> Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ 
> for more information about how to handle DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-pim-assert-packing/
> 
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> # John Scudder, RTG AD, comments for candidate draft-ietf-pim-assert-packing-11
> commit fc9d64f CC @jgscudder
> 
> ## COMMENTS
> 
> (And some nits mixed in, marked as such.)
> 
> Thanks for addressing my earlier DISCUSS! I have a few residual comments, only
> one of which I think is problematic (my comment on §3.3.1.1). Although I hope
> you'll address that comment, I'm not keeping it as a DISCUSS point.
> 
> ### Section 3.3.1
> 
>    When using assert packing, the regular [RFC7761] Assert message
>    encoding with A=0 and P=0 is still allowed to be sent.  Routers are
>    free to choose which PackedAssert message format they send.
> 
> Do you mean "free to choose which Assert message format they send"?

Yes. Fixed in text.

> ### Section 3.3.1
> 
>    *  Implementations SHOULD be able to indicate to the operator (such
>       as through a YANG model) how many Assert and PackedAssert messages
>       were sent/received and how many assert records where sent/
>       received.
> 
> Nit: where -> were

Fixed.
> 
> ### Section 3.3.1
> 
>    *  Implementations that introduce support for PackedAsserts after
>       support for Asserts SHOULD support configuration that disables
>       PackedAssert operations.
> 
> I get what you are trying to do here, mainly because you explained it in email.
> Let me suggest some different wording. Maybe something like,
> 
>    *  A configuration option SHOULD be available to disable PackedAssert
>       operations. An exception exists for implementations that do not
>       have any native support for legacy Assert operations, these
>       implementations MAY omit the configuration option.

Thanks. Please check updated text.

> ### Section 3.3.1.1
> 
> I'm still having difficulty following this section, sorry.
> 
>    Asserts/PackedAsserts in this case are scheduled for serialization
>    with highest priority, such that they bypass any potentially earlier
>    scheduled other packets.
> 
> I guess this means, maintain a queue with at least two priority levels, with
> the high priority being reserved for reception-triggered assert records. (I
> know in your email you expressed allergy to the word "queue" but that's exactly
> what it seems like you're describing, without using the word.)

Right.
> 
>                              When there is no such Assert/PackedAssert
>    message scheduled for or being serialized,
> 
> The "such" is a point of difficulty here -- its referent seems to be "earlier
> scheduled other packets" which are eligible to be bypassed. Or does it mean
> reception-triggered asserts? I think you can fix this problem by spelling it
> out instead of indirecting through "such".

Yes, pls. see fixed text.


>                                               the router immediately
>    serializes an Assert or PackedAssert message without further assert
>    packing.
> 
> I'm also wondering what exactly you intend by "immediately serializes". Control
> plane software I'm familiar with lives up in userspace and uses the socket
> interface to push stuff in the direction of the network; for the messages to
> get written out to the wire it's not uncommon for several additional layers to
> be involved, different processors, a router backplane which itself may look
> like a network interface, etc. I think/hope you just intend that the control
> plane software should manage the order in which it writes things to the socket
> interface, but your use of phrases like "the router... serializes" makes it
> smell a little like you think this tuning is going to be applied all the way
> down to writing the packet to the wire.

Checked to "schedules". Yes, serializes was wrong.

>    If there are one or more reception triggered Assert/PackedAssert
>    messages already serializing and/or scheduled to be serialized on the
>    outgoing interface,
> 
> ... the concern continues with the above. In cases I'm familiar with, I don't
> have that level of detail available to me about the interface queues when I'm
> sitting up in the routing daemon.
> 
>                        then the router can use the time until the last
>    of those messages will have finished serializing for PIM processing
>    of further conditions that may result in additional reception
>    triggered assert records as well as packing of these assert records
>    without introducing additional delay.
> 
> But no matter what the paradigm is I still don't get what the fragment above is
> telling me. The best guess I have right now (but it is not a very strong guess)
> is that you're saying, during the time a message is being written to the wire,
> we can aggregate records, according to priority order, into the next-in-line
> message buffer. Although of course, I don't know the details of every control
> plane implementation, I find it somewhat difficult to believe that this level
> of optimization will ever see the light of day.
> 
> More colloquially, maybe what you're saying is, "please pack the buffers as
> full as can be done without inducing unnecessary latency"? And putting all my
> guesses on this point together, maybe what this subsection is saying is,
> "reception-triggered asserts should be prioritized above other asserts; when
> constructing messages to send, please pack the buffers as full as can be done
> without inducing unnecessary latency"?

There is a lot of ongoing work in other areas which i think does inverst a lot more
into processing optimized for low-latency than those classical routing protocol
control plane queues. Reminds me a bit of how as little as maybe 12 years or so,
even the TCP implementations for BGP happened to be potentially much worse than
other general purpose TCP implementations in some products. 

For example, AQM in hardware chips with algorithms like PIE (RFC8033) does take into
account time spent in queue to some degree, and CoDEL even more so. 

If you are stuck with a legacy queuing system for protocol queues, you could implement IMHO
simple workarounds based on simple protocol process level policers:

- Create a table of per-physcical  outgoing-interface simple "policers",
  tracking the number of bytes from high priority packets that we track
  as yet-to-be-serialized. Simply based on lock clock timestamp and
  tracked physical outgoing interface speed (aka: need to know if that one is
  100Mbps, Gigabit etc..). Whenever you check the policer at a later
  point in time, you simply deduct the number of bytes based on interface speed
  and elapsed clock time.

- Now, whenever your PIM code performs a "send assert" for those time critical
  asserts. we append the assert record to the appropriate list - interface, (S,G) or (*,G)
  Then we check if the list is at MTU size, if so, we create a packet and
  enqueue it, updating the policer.

- If we didn't send so far, we check the policer to see if the number of
  enqueued bytes on the interface ran below some implementation thrashold, such as an MTU, if so,
  then we pick whichever of the two lists (S,G), (*,G) has more bytes, pack it,
  send it, update policer. This step would also result in sending out just
  simple assert, if we hadn't remembered any more assert records then the one
  that triggered this processing in the first place.

Aka: we never need to remember more than one MTU worth of (S,G) assert records
per interface and one MTU worth of (*,G) per interface, but to appropriately 
time their sending, this most simple implementation would want to have almost
zero-cost access to some local timestamp (e.g.: from CPU).
And we need to figure out the outgoing interface speed somehow (100Mbps,
Gigabit, ...). 

And of course, if cpu/thread clock doesn't work, there are workaround for this too.
et.c. pp.

And of course somewhere down near the interface level, on linecards or the
like you hopefully do have at least high/low priority queue for protocol
packets. E.g.: PIM Hellos always belonged into the high priority queue in case you
had a few megabytes worth of PIM join/prunes.

> Also, nit: I would use a hyphen in "reception-triggered" since the two words
> are being used together as an adjective. Similarly in 3.3.1.2, "timer
> expiry-triggered".

Fixed.

> 
> ### Section 3.3.1.3
> 
> Nit, subsectons -> subsections
> Nit, be achieve -> be achieved

Fixed.

> ### Section 3.3.1.5
> 
> Nit, througput -> throughput

Fixed.

> ### Section 6
> 
> I guess it may be so obvious as to not need spelling out, but greenfield
> PackedAssert-only implementations (bullet 5 of §3.1.1) would be rendered
> useless if placed on a LAN with a legacy Assert-only implementation, because of
> the MUST in the first bullet of §3.3.1:
> 
>    *  When any PIM routers on the LAN have not signaled support for
>       Assert Packing, implementations MUST send only Asserts and MUST
>       NOT send PackedAsserts under any condition.

I was imaginign that brownfield implementations would create a new code path
for packed asserts so as not to create regression issues for the case when
packed asserts are not in use (not enabled, or on a LAN with some non-supporting
router). In a greenfield implementation i would just have one code-path, such
as the above described one, whether or not assert packing is enabled. I would
just need to think how to best build it so i have minimum amount of alternative
code paths whether assert-packing is enabled or not (e.g. setting packetization
threshold to 0 and only using assert packet format, but no packing).

And remember my code suggestions above was just to tackle the timing issue for the time
critical asserts. The ones created from timer expiry would still have
lower priority whether they are sent out as asserts or packed asserts. In that
respect a greenfield implementation would likely always perform better in the
priorization between timer expiry and reception triggeed case than a brownfield
implementation that may carefully keep old code running which may have not made
this distinction in timing between those two type of asserts.

> I wonder if this (the ability to completely silence such an implementation by
> advertising non-support for PackedAssert) represents an interesting enough
> attack that it's worth calling out in the Security Considerations? Perhaps this
> is already captured in some of the underlying spec SecCons, but I'm not sure.

I updated the security section accordingly, please check.

Thanks

Toerless

> ## Notes
> 
> This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
> [`ietf-comments` tool][ICT] to automatically convert this review into
> individual GitHub issues.
> 
> [ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
> [ICT]: https://github.com/mnot/ietf-comments