doc: improve the SIG doc with the text from the ietf drafts #4641
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @jiceatscion and @oncilla)
doc/sig.rst line 80 at r3 (raw file):
How about adding back this sentence?
The Session ID and Stream ID are chosen by the sender but the tuple MUST be unique within a session.
doc/sig.rst line 25 at r3 (raw file):
==================== IP packets are encapsulated into SIG frames, which are sent as SCION/UDP datagrams.
IN the draft we had these few lines that explained reasons to use SIG framing instead of other protocols. How about we leave them somewhere?
Code snippet:
This protocol is designed to:
- provide independence from the underlying SCION path MTU which can increase and decrease over time.
- provide fast detection of packet loss and subsequent recovery of decapsulation for packets that weren't lost.
- support for multiple streams within a framing session such that independent packet sequences be tunneled in parallel.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @nicorusti and @oncilla)
doc/sig.rst line 25 at r3 (raw file):
Previously, nicorusti (Nicola Rustignoli) wrote…
IN the draft we had these few lines that explained reasons to use SIG framing instead of other protocols. How about we leave them somewhere?
done
doc/sig.rst line 80 at r3 (raw file):
Previously, nicorusti (Nicola Rustignoli) wrote…
How about adding back this sentence?
The Session ID and Stream ID are chosen by the sender but the tuple MUST be unique within a session.
I actually said this twice already in the new text. Once on line 38 (of the raw doc), and once on line 78.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @jiceatscion, @nicorusti, and @oncilla)
doc/sig.rst line 36 at r4 (raw file):
There may be multiple IP packets in a single SIG frame, and a single IP packet may be split into multiple SIG frames. The ingress SIG initiates unidirectional packet flows to the egress SIG simply by sending the corresponding SIG frames. There is no handshake. The egress SIG, should it accept the traffic, instanciates the necessary resources on-demand to process each flow. Each such flow forms an independent sequence of packets (a stream) ordered by an incrementing sequence number. Between a given SIG ingress/egress pair, a (session ID, stream ID) pair uniquely identifies a stream.
Suggestion:
instantiatesdoc/sig.rst line 45 at r4 (raw file):
- The egress SIG reassembles and forward packets from each stream, ordered by frame sequence number and by packet within each frame. The session ID part of the (session ID, stream ID) pair is used to differentiate traffic classes. The egress SIG does not interpret session IDs, but dedicates, as much as possible, processing resources to each session (and not to each individual stream). The ingress SIG takes advantage of this by using different sessions for different traffic classes, thereby allowing them to receive a fair share of the egress processing resources. As a result lesser used sessions (presumably with higher priority traffic) receive relatively more processing resources.
I wouldn't associate session ID with traffic class. That's up to the sending SIG to determine.
Also, the text about processing resources and fair share I wouldn't put here (not even sure it's true). That's such a low level implementation detail that I'd wouldn't want anyone relying on this
Code quote:
The session ID part of the (session ID, stream ID) pair is used to differentiate traffic classes. The egress SIG does not interpret session IDs, but dedicates, as much as possible, processing resources to each session (and not to each individual stream). The ingress SIG takes advantage of this by using different sessions for different traffic classes, thereby allowing them to receive a fair share of the egress processing resources. As a result lesser used sessions (presumably with higher priority traffic) receive relatively more processing resources.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @nicorusti, @oncilla, and @shitz)
doc/sig.rst line 45 at r4 (raw file):
Previously, shitz (Samuel Hitz) wrote…
I wouldn't associate session ID with traffic class. That's up to the sending SIG to determine.
Also, the text about processing resources and fair share I wouldn't put here (not even sure it's true). That's such a low level implementation detail that I'd wouldn't want anyone relying on this
Sure, it's up to the SIGs. I'm just trying to give a hint of what it's for. That is what I saw in the code when trying to figure it out. Btw, the name sessionID doesn't seem particularly relevant; it seems almost as if the field used to be that but was repurposed. I was wondering if we should mention that the name is historical. But then, may be I am over-interpreting. How would you suggest we explain this? What is the intent of separating these 8 bits from the rest?
doc/sig.rst line 36 at r4 (raw file):
There may be multiple IP packets in a single SIG frame, and a single IP packet may be split into multiple SIG frames. The ingress SIG initiates unidirectional packet flows to the egress SIG simply by sending the corresponding SIG frames. There is no handshake. The egress SIG, should it accept the traffic, instanciates the necessary resources on-demand to process each flow. Each such flow forms an independent sequence of packets (a stream) ordered by an incrementing sequence number. Between a given SIG ingress/egress pair, a (session ID, stream ID) pair uniquely identifies a stream.
Done.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @jiceatscion, @nicorusti, and @oncilla)
doc/sig.rst line 45 at r4 (raw file):
Previously, jiceatscion wrote…
Sure, it's up to the SIGs. I'm just trying to give a hint of what it's for. That is what I saw in the code when trying to figure it out. Btw, the name sessionID doesn't seem particularly relevant; it seems almost as if the field used to be that but was repurposed. I was wondering if we should mention that the name is historical. But then, may be I am over-interpreting. How would you suggest we explain this? What is the intent of separating these 8 bits from the rest?
You are right that the name sessionID isn't particularly relevant given these are only unidirectional session.
I would simply say that the semantics of the sessionID is not defined. One usecase is to map different traffic classes to different session IDs, but that's just an example. FWIW, we mapped different traffic classes to different session IDs that we can collect traffic stats per traffic class. Traffic class is a configuration concept and we wanted a 1-to-1 mapping to something that we have on the dataplane (and thus in dataplane metrics). The part about resource usage on the egress SIG I would leave out.
Does that make sense?
|
Yes, it does. Thanks a bunch. I'll rewrite along those lines.
…On Thu, Dec 19, 2024 at 4:45 PM Samuel Hitz ***@***.***> wrote:
***@***.**** requested changes on this pull request.
*Reviewable <https://reviewable.io/reviews/scionproto/scion/4641>*
status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on
@jiceatscion <https://github.com/jiceatscion>, @nicorusti
<https://github.com/nicorusti>, and @oncilla <https://github.com/oncilla>)
------------------------------
*doc/sig.rst line 45 at r4
<https://reviewable.io/reviews/scionproto/scion/4641#-OEIY__y2gpO7VXpFCad:-OEUZhf24gr_v3FVRegX:b-mx3a2z>
(raw file
<https://github.com/scionproto/scion/blob/32bd399725452c9d27ff4255f8d51297ed85c37e/doc/sig.rst#L45>):*
*Previously, jiceatscion wrote…*
Sure, it's up to the SIGs. I'm just trying to give a hint of what it's
for. That is what I saw in the code when trying to figure it out. Btw, the
name sessionID doesn't seem particularly relevant; it seems almost as if
the field used to be that but was repurposed. I was wondering if we should
mention that the name is historical. But then, may be I am
over-interpreting. How would you suggest we explain this? What is the
intent of separating these 8 bits from the rest?
You are right that the name sessionID isn't particularly relevant given
these are only unidirectional session.
I would simply say that the semantics of the sessionID is not defined. One
usecase is to map different traffic classes to different session IDs, but
that's just an example. FWIW, we mapped different traffic classes to
different session IDs that we can collect traffic stats per traffic class.
Traffic class is a configuration concept and we wanted a 1-to-1 mapping to
something that we have on the dataplane (and thus in dataplane metrics).
The part about resource usage on the egress SIG I would leave out.
Does that make sense?
—
Reply to this email directly, view it on GitHub
<#4641 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BBLEYOHUH7GJNVCKD7VRMG32GLS3FAVCNFSM6AAAAABQCANL4CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMJVGMYDSMBXG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
JEAN-CHRISTOPHE HUGLY
Software Engineer
+41 79 455 68 69
***@***.*** ***@***.***>
scion.org <http://www.scion.org/>
SCION Association <https://www.linkedin.com/company/scion-association/>
<https://www.scion.org/>
|
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @nicorusti, @oncilla, and @shitz)
doc/sig.rst line 45 at r4 (raw file):
Previously, shitz (Samuel Hitz) wrote…
You are right that the name sessionID isn't particularly relevant given these are only unidirectional session.
I would simply say that the semantics of the sessionID is not defined. One usecase is to map different traffic classes to different session IDs, but that's just an example. FWIW, we mapped different traffic classes to different session IDs that we can collect traffic stats per traffic class. Traffic class is a configuration concept and we wanted a 1-to-1 mapping to something that we have on the dataplane (and thus in dataplane metrics). The part about resource usage on the egress SIG I would leave out.
Does that make sense?
Yes, it does. Thanks a bunch. I'll rewrite along those lines.
done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nicorusti and @oncilla)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nicorusti and @oncilla)
doc/sig.rst line 80 at r3 (raw file):
Previously, jiceatscion wrote…
I actually said this twice already in the new text. Once on line 38 (of the raw doc), and once on line 78.
nico?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jiceatscion and @oncilla)
doc/sig.rst line 80 at r3 (raw file):
Previously, jiceatscion wrote…
nico?
Sounds good, thanks!
Fixes #4640