router: configuration for async packet processing

[rohrerj]

This pullrequest is the second out of 3 with the goal to restructure the border router as described in the Design Document.

This pullrequest contains the implementation for the configurability.

---

This change is 

[matzf]

Reviewed 9 of 10 files at r1, all commit messages.
Reviewable status: 9 of 10 files reviewed, 5 unresolved discussions (waiting on @rohrerj)

a discussion (no related file):
Please update the documentation for these variables (in doc/manuals/router.rst).
Follow the existing format, and the format in the manual for the control service ( #4361 ). Roughly, this would look like this:

.. program:: router-conf-toml

.. object:: general
   # ... 

# new stuff starts here:
.. object:: router
   .. option:: router.receive_buffer_size = <int> (Default: 1048576)
      # description
   .. option:: router.write_buffer_size = <int> (Default: ...)
      # ....
   .. option:: router.num_processors = <int> (Default: GOMAXPROCS)
      # ....
   # etc.

---

---

router/config/config.go line 75 at r1 (raw file):

func (cfg *RouterConfig) InitDefaults() {
	if cfg.ReceiveBufferSize == 0 {
		cfg.ReceiveBufferSize = 1 << 20

Shouldn't we be treating the receive and send buffer sizes somewhat consistently? I can't think of a good reason why 0 doesn't mean the same thing for both. I'd change that 0 for receive buffer size to also not set a specific receive buffer size. (This needs a small change in private/underlay/conn/conn.go, initConnUDP).

---

router/config/config.go line 92 at r1 (raw file):

}

type RunConfig struct {

This is only used locally in the API of the router package, I think it makes sense to keep it there.

---

router/config/sample.go line 25 at r1 (raw file):

# (default 0)
send_buffer_size = 0

num_processors is missing here.

---

tools/topology/go.py line 106 at r1 (raw file):

                'num_slow_processors': 1,
                'batch_size': 256
            }

If these are all set to the defaults, it may be better not to specify anything at all -- fewer places to change the default value.

[rohrerj]

Reviewable status: 2 of 12 files reviewed, 4 unresolved discussions (waiting on @matzf)

a discussion (no related file):

Previously, matzf (Matthias Frei) wrote…

Please update the documentation for these variables (in doc/manuals/router.rst).
Follow the existing format, and the format in the manual for the control service ( #4361 ). Roughly, this would look like this:

.. program:: router-conf-toml

.. object:: general
   # ... 

# new stuff starts here:
.. object:: router
   .. option:: router.receive_buffer_size = <int> (Default: 1048576)
      # description
   .. option:: router.write_buffer_size = <int> (Default: ...)
      # ....
   .. option:: router.num_processors = <int> (Default: GOMAXPROCS)
      # ....
   # etc.

Done.

---

---

router/config/config.go line 75 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

Shouldn't we be treating the receive and send buffer sizes somewhat consistently? I can't think of a good reason why 0 doesn't mean the same thing for both. I'd change that 0 for receive buffer size to also not set a specific receive buffer size. (This needs a small change in private/underlay/conn/conn.go, initConnUDP).

Right. They should be consistent. Because '0' means now "use system default" we cannot use '0' to to set the receive buffer to 1<<20. Either the new default value would be to use system default (0) or set the value to 1<<20 with the python topology generator. Or do you see some third option?

---

router/config/sample.go line 25 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

num_processors is missing here.

Done.

---

tools/topology/go.py line 106 at r1 (raw file):

Previously, matzf (Matthias Frei) wrote…

If these are all set to the defaults, it may be better not to specify anything at all -- fewer places to change the default value.

Done.

[matzf]

Reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rohrerj)

---

doc/manuals/router.rst line 132 at r2 (raw file):
"processors" is a bit too vague IMO. Perhaps expand this, and also expand a little bit what goroutines are (apart from this feature, an operator does not need to be familiar with this concept).

Proposal:

option:: router.num_processors = (Default: GOMAXPROCS)

Number of goroutines started for SCION packets processing.
These goroutines make the routing decision for the SCION packets by inspecting, validating and updating the path information in the packet header. Packets are processed asynchronously from the corresponding read/write operations on the individual interface sockets.

Goroutines <https://en.wikipedia.org/wiki/Go_(programming_language)#Concurrency:_goroutines_and_channels>_ are the Go pramming language's light-weight user-space concurrency primitives. Go's runtime schedules goroutines on top of a fixed number of kernel threads. The number of kernel threads is controlled by the GOMAXPROCS environment variable. See also https://pkg.go.dev/runtime#hdr-Environment_Variables_.

By default, the router uses GOMAXPROCS packet processor goroutines, i.e. exactly one goroutine for each kernel thread created by the runtime.

---

doc/manuals/router.rst line 136 at r2 (raw file):

   .. option:: router.num_slow_processors = <int> (Default: 1)

      The number of slow-path processors.

Ah, I just realized that this is configuring something that is not yet implemented. My preference would be to leave this away entirely for now (i.e. remove num_slow_processors from the documentation, and all the configuration structs) and add it back together with the implementation of the feature -- this will be very easy, as all the other stuff for the config is already in place.
Alternatively, it would perhaps be ok to have note here saying that this is not doing anything yet.

---

router/config/config.go line 75 at r1 (raw file):

Previously, rohrerj wrote…

Right. They should be consistent. Because '0' means now "use system default" we cannot use '0' to to set the receive buffer to 1<<20. Either the new default value would be to use system default (0) or set the value to 1<<20 with the python topology generator. Or do you see some third option?

Default 0 seems good enough.
FWIW, that can't set anything other than 0 as default here is a shortcoming of the configuration "framework" -- we'd need a chance to set the default before unmarshalling the config. No big deal here though.

The python generator is just a development tool, this doesn't really play a role here.

[rohrerj]

Reviewable status: 11 of 12 files reviewed, 2 unresolved discussions (waiting on @matzf)

---

doc/manuals/router.rst line 132 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

"processors" is a bit too vague IMO. Perhaps expand this, and also expand a little bit what goroutines are (apart from this feature, an operator does not need to be familiar with this concept).

Proposal:

option:: router.num_processors = (Default: GOMAXPROCS)

Number of goroutines started for SCION packets processing.
These goroutines make the routing decision for the SCION packets by inspecting, validating and updating the path information in the packet header. Packets are processed asynchronously from the corresponding read/write operations on the individual interface sockets.

Goroutines <https://en.wikipedia.org/wiki/Go_(programming_language)#Concurrency:_goroutines_and_channels>_ are the Go pramming language's light-weight user-space concurrency primitives. Go's runtime schedules goroutines on top of a fixed number of kernel threads. The number of kernel threads is controlled by the GOMAXPROCS environment variable. See also https://pkg.go.dev/runtime#hdr-Environment_Variables_.

By default, the router uses GOMAXPROCS packet processor goroutines, i.e. exactly one goroutine for each kernel thread created by the runtime.

Right. I didn't know how detailed the description there should be. Thank you for the proposal, I copied it.

---

doc/manuals/router.rst line 136 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Ah, I just realized that this is configuring something that is not yet implemented. My preference would be to leave this away entirely for now (i.e. remove num_slow_processors from the documentation, and all the configuration structs) and add it back together with the implementation of the feature -- this will be very easy, as all the other stuff for the config is already in place.
Alternatively, it would perhaps be ok to have note here saying that this is not doing anything yet.

Since that part is already reviewed and the slow-path should be implemented by end of the month, I think it should be enough to just add a note saying that it's not doing anything at the moment.

[matzf]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rohrerj)

---

doc/manuals/router.rst line 132 at r2 (raw file):

Previously, rohrerj wrote…

Right. I didn't know how detailed the description there should be. Thank you for the proposal, I copied it.

Ok. If you look at the rendered result, it looks a bit dense now: https://scion--4374.org.readthedocs.build/en/4374/manuals/router.html#cmdoption-router-conf-toml-arg-router.num_processors

• Please add the blank lines for paragraphs.

• The See also. link looks a bit odd. I'd either keep the full URL in the text, or give it some title like

  See also the `go runtime documentation <link>`_.
  

• Format GOMAXPROC as "code" by enclosing it in double backticks.

[rohrerj]

Reviewable status: 11 of 12 files reviewed, 1 unresolved discussion (waiting on @matzf)

---

doc/manuals/router.rst line 132 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Ok. If you look at the rendered result, it looks a bit dense now: https://scion--4374.org.readthedocs.build/en/4374/manuals/router.html#cmdoption-router-conf-toml-arg-router.num_processors

• Please add the blank lines for paragraphs.

• The See also. link looks a bit odd. I'd either keep the full URL in the text, or give it some title like

  See also the `go runtime documentation <link>`_.
  

• Format GOMAXPROC as "code" by enclosing it in double backticks.

Done.

[matzf]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rohrerj)

---

doc/manuals/router.rst line 141 at r4 (raw file):

      goroutines on top of a fixed number of kernel threads. The number of kernel threads is controlled by
      the ``GOMAXPROCS`` environment variable. See also the `go runtime documentation <https://pkg.go.dev/runtime#hdr-Environment_Variables>`_.
      

Nit. Trailing whitespace.

[matzf]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @rohrerj)