Subject: [Fwd: Re: [rohc] draft-ietf-rohc-sigcomp-06]

From: "West, Mark (ITN)" <[email-masked]>
Date: Thu, 16 May 2002 23:06:21 +0100

I guess this probably ought to go to the list as well..?!

'Reply' / 'Reply to all' -- it's an easy mistake to make... (well, it is 
for me)

;-)

Mark.

-------- Original Message --------
Subject: Re: [rohc] draft-ietf-rohc-sigcomp-06
Date: Thu, 16 May 2002 14:27:44 +0100
From: Mark West <[email-masked]>
To: Carsten Bormann <[email-masked]>
References: <[email-masked]> <[email-masked]>

Hi Carsten,

 >
 > thanks for your comments!
 > Now let me pick them apart :-)

Well, that's what they're there for..!

(BTW, apologies for not suggesting re-wording in many places, but I was
slightly deadline constrained, and wanted to ask as many dumb questions
as possible!)

 >
 >> 3.3, page 10
 >> 1st para: "an endpoint _generally_ behaves in a predictable manner".
 >> In what way can it be unpredictable?
 >
 >
 > It might have a bad RAM chip.
 >
 > No, really: It might want to change state_memory_size when it runs into
 > memory shortage.  This is not predictable for the compressor, so it's
 > really bad behavior, but it is one strategy for graceful degradation, so
 > we didn't want to rule it out.  This unpredictability can be handled
 > with feedback (in optimistic or in pessimistic ways).
 >
 > This discussion MUST go into the UDVM user guide...

Makes sense.  So the text in -06 can stand...

 >
 >> 3rd para: "locally available state" -> "locally_available_state"
 >
 >
 > I think this is not a parameter with a single value per se, but rather
 > was meant to be descriptive for the set of locally available state.
 > (Note that there is no other mention of "locally_available_state" 
either.)

ok, I'll go along with that!

 >
 >> 3.3.1, page 10
 >> Is there any upper limit on decompression_memory_size?
 >
 >
 > practically, it's 128K, because we can't announce higher values in
 > feedback with the current encoding.  That allows for a 64K compressed
 > (!) message and a 64K UDVM, which should be fine (and there's always
 > continuous mode if it's not).
 >

My mistake -- I didn't spot that decompression_memory_size used the same
encoding as state_memory_size.

 >> Page 11
 >> Does cycles_per_bit refer to compressed or uncompressed bits?
 >> (I think that this is explicit later, but it worth clarifying here,
 >> even if it is obvious!)
 >
 >
 > Definitions says:
 >
 >  >>
 > SigComp message
 >
 > A message sent from the compressor dispatcher to the decompressor
 > dispatcher. In case of a message-based transport such as UDP, a
 > SigComp message corresponds to exactly one datagram. For a stream-
 > based transport such as TCP, the SigComp messages are separated by
 > reserved delimiters.
 > <<
 >
 > Then 3.3.1 goes on:
 >
 >  >>
 > The cycles_per_bit parameter specifies the number of "UDVM cycles"
 > available to decompress each bit in a SigComp message.
 > <<
 >
 > That's explicit enough for me.  Do you have a proposal for better 
wording?

No, I think that it's ok.  I was just getting myself confused!

 >> 3.3.2, page 12
 >> Regarding backwards compatibility, should this read "then it MUST be"?
 >
 >
 > This is a promise by the IETF, not a mandate on the implementers...
 > (In future versions, it might become one.)
 >

ok.

 >> 4.2.1, page 14
 >> "In general, this alternative is an implementation choice." -> "In
 >> general, the strategy used is an implementation choice."
 >
 >
 > Sure. (My bad -- I tend to use the original 1624 meaning of
 > "alternative" and forget that this has been pretty much lost in the
 > mists of time.)
 >
 > Even clearer:
 >
 > In general, which of the strategies is used is an implementation choice.
 >

Your text looks fine...

 >> 6.2, page 20
 >> Point 5 and the 2nd last paragraph seem to duplicate text about the
 >> use of retention priority 65535
 >
 >
 > Repetitions are good for learning :-)
 > I think the two sentences address different things, though, so I would
 > really like to leave them as is.
 >

Fair point about repetition!  I'm happy for the text to be left as is.

 >> 7.1, page 22
 >> Why is the 'T' bit called the 'T' bit?
 >
 >
 > Beats me.  (The -extended people probably can remember.)
 > Why didn't you ask why the F-bit is not a B-bit?
 >

Just curious! (I didn't ask about the F-bit [although should have!]
because I think I know why it's called an 'F' bit...)

I guess it doesn't really matter what the bits are called, but (where
not immediately obvious) I just raise the point as a matter of slight
pedantry!

 >> Why does the length of the feedback exclude the length field?
 >
 >
 > Because this is good protocol practice.
 > One less check to code.
 >
 > (This design rule became painfully obvious quite a while ago: There was
 > a bug in 4.2BSD once where a length field of zero caused the
 > implementation to advance the pointer by zero bytes and so to examine
 > the same field again and again in a tight loop -- in kernel mode. I'm
 > not online right now and so can't look it up -- great story for my
 > students though...)
 >
 > Also, it gives you one more byte you can put there :-)
 >

I figured it might be something like that!  And I don't see any need to
change it.  (Nice story, BTW)

 >> 7.2, page 23
 >> 1st para, is the uniqueness criterion the sole basis for the
 >> compressor to choose how many bytes of state-identifier to send?
 >
 >
 > I believe so (actually, it's a tradeoff between this and the compressed
 > size).  Of course, this includes the security considerations.
 >

ok.

 >> 3rd para: "in every currently stored state item" -> "in every
 >> currently stored state item within the compartment"
 >> (Assuming that state is matched 'per compartment')
 >
 >
 > No, it can't -- we don't know the compartment until *after* the message
 > is decompressed.  That is the main reason the whole state thing is more
 > complex than it looked on the napkin we were designing it on...
 >
 > Should we add words to make this clearer?
 >

Of course -- forgive my confusion.  I think that it is worth being clear
(in general I recall that the draft is) about what is per-compartment
and what is 'global'.

 >> Page 24
 >> 1st para: is it worth making a more general statement somewhere about
 >> the storage of 16-bit values in the UDVM memory (i.e. MSB precedes LSB)?
 >
 >
 > Well, the sequence of things in the document is slightly strange:
 > 7 is about the message format, 8 introduces the UDVM.
 > For some reason, the "useful values" slipped into 7.
 > I'm not sure I want to fix this now, though.
 >

Yes, I skipped around a bit when I reviewed the draft, but you're right
-- the order is slightly odd.

I guess it seems odd that there is no clear statement (that I remember
seeing) about the byte-ordering within the UDVM.  Having said that,
since it *is* stated, I leave it to your editorial discretion ;-)

 >> Page 25
 >> 1st para: There is some repetition of text about the 'no special
 >> significance' of the 'useful values' area
 >
 >
 > Here again, I believe repetition is good -- this is a note, anyway.
 >

ok

 >> 7.3, page 25
 >> Would it be clearer to have a diagram to indicate the code_len and
 >> destination address fields?
 >
 >
 > I thought we had that -- in the intro to section 7.
 >

So you do -- my mistake.

<pedantry>
Could reference the figure?
</pedantry>

 >> 8, page 26
 >> How would you define 'typical' and 'often' in the context of 'sub 100
 >> byte algorithms'?
 >
 >
 > I believe the first two paras of a new section are allowed to be ad copy
 >   even in an RFC :-)
 >

ok!

 >> 8.1, page 27
 >> Is it worth adding a note about how registers are initialised (i.e.
 >> that they should be!)?
 >
 >
 > There are initialized with zeros, as all UDVM core except for the Useful
 > Values.
 > If that is not good for a decompressor, it needs to provide its own
 > initialization.
 >
 > (I gather you want these points made in a note?)

I think the fact that they are initialised to 0 is clearly made.
Although it may be equally obvious that a decopmressor needs to
initialise them to more sensible values, I'd suggest a note like:
"These registers may need to be set to suitable values by the decompressor."

 >
 >> 8.2, page 27
 >> Does the decompressor dispatcher have to buffer the entire message
 >> prior to invoking the UDVM?  (It states explicitly elsewhere that it
 >> doesn't, but the 1st paragraph of 8.2 might be slightly misleading in
 >> this case).
 >
 >
 > Hmm. Fix how?

How about:

"When the UDVM bytecode is first executed the dispatcher conceptually
contains the remaining SigComp message after the header has been used to
initialize the UDVM as per Chapter 7.  However, note that the dispatcher
need not contain the entire message prior to starting decompression."

 >
 >> The 'reserved' bits in the input_bit_order register are only reserved
 >> for version 0x01 of the UDVM.  (Last para of this section should also
 >> account for this in checking that the value is in the range 0 - 7).
 >
 >
 > Isn't that true for all "reserved" fields?
 > The whole spec is about 0x01!
 > (I.e., is there a change that needs to be done in many places?)
 >

Well, some places explicitly state something like "this is reserved in
version 0x01..." and some places don't.  So, I guess we could either
make an overall note (there is some text on page 12 about version 0x01)
and/or try and be consistent throughout...

 > The last para could have a "In UDVM version 0x01, ..." added at the 
front.
 >
 >> 8.7, page 33
 >> Point 3: "more that the" -> "more than the"
 >
 >
 > Fixed.
 >
 >> Point 6: What is an 'unknown operand'?
 >
 >
 > An operand that cannot be decoded according to the current bit
 > assignments. (Hmm, is that less clear than "unknown instruction"?)
 >

My mistake -- the text is clear, but I had failed to spot that there are
clearly bit-patterns that are 'illegal'.

 >> 9, page 34
 >> I count 36 instructions, not 33!
 >
 >
 > Oops.  These parts of the text should be automatically generated, but
 > trying to respect the RFC-editor's minimal usage of nroff I didn't. Next
 > version will be run through m4 before nroffing :-)
 >
 >> 9.1.3, page 37
 >> Is the restriction that the sort be natural (i.e. preserving original
 >> order) a useful/necessary one?
 >
 >
 > Yes, we discussed this.  If you don't fully specify the behavior, the
 > compressor cannot use the result as a decompression table.  We don't
 > really care for "stable" sorting (this is the term I learned for this),
 > but it is the simplest behavior to specify (and there are good
 > algorithms that do this).

Makes sense.  (I must have missed the discussion!)

 >
 >> 9.2.3, page 39
 >> Description of POP restates the definition of "and then".
 >
 >
 > Repetition is good...

ok.
ok.

 >
 >> 9.2.4, page 39
 >> What happens if the start of the source / destination lie outside of
 >> the range specified in byte_copy_left, byte_copy_right?
 >
 >
 > Not much that would be surprising from 8.4:
 >
 > Set k := m + 1 (modulo 2^16)
 > If k = byte_copy_right then set n := byte_copy_left, else set n := k
 >
 > If byte_copy_right is not hit, there is no effect.
 >

But do we care that the copy might start outside the circular buffer.
And if m > byte_copy_right at the start, then the copy will run off the
end of memory...

Should this be checked as a pre-condition, or do we ignore it, or do we
'coerce' the value to be in range, or..?

 >> 9.2.5, page 40
 >> The trailing ", if any" makes it slightly unclear (I think) what
 >> happens if no data is copied.  Is it worth explicitly stating that
 >> destination is unchanged in this case?
 >
 >
 > append "(i.e., if the value of the length operand is zero, the value of
 > the destination operand is not changed.)"?
 >

I'm happy with that.

 >> 9.3.5, page 42
 >> last para: "continues at the following" -> "continues instruction
 >> execution at the following"
 >
 >
 > Sounds good to me.
 >
 >> 9.4.4, page 44
 >> Point 2: reference to 8.1 should be to 8.2
 >
 >
 > Good catch.  Applies to 9.4.3, too.
 >

 >> Is there any particular reason why decompression failure is called if
 >> state_length is 0 but state_begin is non-zero?
 >
 >
 > Hmm.  Must have made sense to me once, but does not make sense to me now
 > -- but it's been a 20-hour working day...

It doesn't really matter!  It just seems like a slightly odd
restriction, but I'm not going to argue about it...

 >
 >> The last paragraph describing the address at which execution resumes
 >> is a little confusing.  It makes sense, but I had to read it a couple
 >> of times to make sure that I got it...
 >
 >
 > The text could probably be improved considerably by introducing
 > derived_state_length, derived_state_instruction and
 > derived_state_address and referring to those where these are meant
 > instead of the operands.

Yes.  Do you want a suggestion for text, or are you happy to wordsmith it?

 >
 >> 9.4.6, page 46
 >> "Decompression failure must occur" -> "Decompression failure MUST occur"
 >>
 >> 9.4.7, page 46
 >> "Decompression failure must occur" -> "Decompression failure MUST occur"
 >
 >
 > Good points.
 >
 >> 9.4.9, page 47/8
 >> Given the definition of STATE-CREATE, and the fact that multiple
 >> states can be created, why is it necessary for the END-MESSAGE
 >> instruction to also be able to create state?
 >
 >
 > I don't like that either, but then, it's been the choice of the majority
 > of the authors, and it does not really make a difference.

Fair enough, it's just a bit... messy!

 >
 >> The processing of STATE-FREE requests states that "upon reaching the
 >> END-MESSAGE instruction these values are used to byte copy..."
 >> Are the values copied at the point of STATE-FREE execution or at the
 >> point of calling END-MESSAGE (requiring the state identifiers to be
 >> preserved in UDVM memory)?
 >
 >
 > The values of the operands are stashed away into some hidden UDVM
 > storage during STATE-FREE, the things they point to are not.

 >
 > "the UDVM
 > simply buffers the two supplied operands until the END-MESSAGE
 > instruction is reached"
 >
 > Idea for a wording change?

No, it's ok -- I checked the text for STATE-FREE and it also says that
the "operands are buffered" (rather than the data), so I think that
covers it.

 >
 >  > Figure 1 - split across pages 5/6
 >  >
 >  > Figure 4, diagram split across pages 22/23
 >  >
 >
 >> Figure 12, split across pages 48/9
 >>
 >> Figure 13, split across pages 50/1
 >
 >
 > Sorry I didn't bother to fix these -- RFC editor will impose a new page
 > layout anyway.
 >

Fine.

 >> 10.1, page 52
 >> "as it always can be circumvented" -> "as it can always be circumvented"
 >
 >
 > Wouldn't that become a "split infinitive", a bane of those English
 > teachers I never had :-)?
 >

I'm not sure!  I don't have my copy of Fowler's English Usage on me...

You could always have "as this can be circumvented in any event by..."

;-)

Mark.
-- 
Mark A. West, Consultant Engineer
Roke Manor Research Ltd., Romsey, Hants.  SO51 0ZN
Phone +44 (0)1794 833311   Fax  +44 (0)1794 833433

(Yes, I do know that my disclaimer is in an attachment.  And, no, I
didn't ask for it to be that way)

-- 
Mark A. West, Consultant Engineer
Roke Manor Research Ltd., Romsey, Hants.  SO51 0ZN
Phone +44 (0)1794 833311   Fax  +44 (0)1794 833433

(Yes, I do know that my disclaimer is in an attachment.  And, no, I 
didn't ask for it to be that way)