Re: em(4) multiqueue
On Thu, Apr 13, 2023 at 01:30:36PM -0500, Brian Conway wrote: > > Reviving this thread, apologies for discontinuity in mail readers: > https://marc.info/?t=16564219358 > > After rebasing on 7.3, my results have mirrored Hrvoje's testing at the end > of that thread. No issues with throughput, unusual latency, or reliability. > `vmstat -i` shows some level of balancing between the queues. I've been > testing on as many em(4) systems as I have access to, some manually, some in > a packet forwarder/firewall scenarios: > > em0 at pci1 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em1 at pci2 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em2 at pci3 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em3 at pci4 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em4 at pci5 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em5 at pci6 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > > em0 at pci1 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address > 00:0d:b9:... > em1 at pci2 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address > 00:0d:b9:... > em2 at pci3 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address > 00:0d:b9:... > > em0 at pci1 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address > 00:0d:b9:... > em1 at pci2 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address > 00:0d:b9:... > em2 at pci3 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address > 00:0d:b9:... Last time I tested (about a year go) on I211, rx locked up if I tried something like iperf3 or tcpbench. Don't know if you have a similar problem. > em0 at pci1 dev 0 function 0 "Intel 82574L" rev 0x00: msi, address > 68:05:ca:... > > The only questions I have are around queue identification. All the specs I've > been able to find indicate the I210 should have 4 queues, did Intel make a > cheaper version with 2 toward the end of production? Or could it be an I211 > masquerading as an I210 (and would that be bad for the driver)? > > Also, > https://www.mouser.com/pdfdocs/Intel_82574L_82574IT_GbE_Controller_brief.pdf > indicates that the 82574L should have 2 queues? > > Anyway, great work, please let me know if there's more I can do to help this > move forward. > > Brian Conway > Lead Software Engineer, Owner > RCE Software, LLC >
Re: em(4) multiqueue
On 2023/04/13 16:45, Sonic wrote: > Is this multiqueue support in 7.3 or does it require patching? > According to Intel the i211 should have 2 queues but I see no msi-x > support in dmesg: > em0 at pci1 dev 0 function 0 "Intel I211" rev 0x03: msi, address It is not committed, there's a diff.
Re: em(4) multiqueue
Is this multiqueue support in 7.3 or does it require patching? According to Intel the i211 should have 2 queues but I see no msi-x support in dmesg: em0 at pci1 dev 0 function 0 "Intel I211" rev 0x03: msi, address Thanks. Chris
Re: em(4) multiqueue
On Thu, Apr 13, 2023, at 2:45 PM, Stuart Henderson wrote: > On 2023/04/13 13:30, Brian Conway wrote: >> Reviving this thread, apologies for discontinuity in mail readers: >> https://marc.info/?t=16564219358 >> >> After rebasing on 7.3, my results have mirrored Hrvoje's testing at the end >> of that thread. No issues with throughput, unusual latency, or reliability. >> `vmstat -i` shows some level of balancing between the queues. I've been >> testing on as many em(4) systems as I have access to, some manually, some in >> a packet forwarder/firewall scenarios: >> >> em0 at pci1 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address >> 00:f1:f3:... >> em1 at pci2 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address >> 00:f1:f3:... >> em2 at pci3 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address >> 00:f1:f3:... >> em3 at pci4 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address >> 00:f1:f3:... >> em4 at pci5 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address >> 00:f1:f3:... >> em5 at pci6 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address >> 00:f1:f3:... >> >> em0 at pci1 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address >> 00:0d:b9:... >> em1 at pci2 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address >> 00:0d:b9:... >> em2 at pci3 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address >> 00:0d:b9:... >> >> em0 at pci1 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address >> 00:0d:b9:... >> em1 at pci2 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address >> 00:0d:b9:... >> em2 at pci3 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address >> 00:0d:b9:... >> >> em0 at pci1 dev 0 function 0 "Intel 82574L" rev 0x00: msi, address >> 68:05:ca:... >> >> The only questions I have are around queue identification. All the specs >> I've been able to find indicate the I210 should have 4 queues, did Intel >> make a cheaper version with 2 toward the end of production? Or could it be >> an I211 masquerading as an I210 (and would that be bad for the driver)? > > Is it a 2-cpu machine? Ah, you're right. The level of detail I provided was insufficient. >> Also, >> https://www.mouser.com/pdfdocs/Intel_82574L_82574IT_GbE_Controller_brief.pdf >> indicates that the 82574L should have 2 queues? > > No msix in your dmesg excerpt for that one I'll lug that one back out and take a look. Probably safe to assume a misunderstanding on my part. Thanks. -b
Re: em(4) multiqueue
On 2023/04/13 13:30, Brian Conway wrote: > Reviving this thread, apologies for discontinuity in mail readers: > https://marc.info/?t=16564219358 > > After rebasing on 7.3, my results have mirrored Hrvoje's testing at the end > of that thread. No issues with throughput, unusual latency, or reliability. > `vmstat -i` shows some level of balancing between the queues. I've been > testing on as many em(4) systems as I have access to, some manually, some in > a packet forwarder/firewall scenarios: > > em0 at pci1 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em1 at pci2 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em2 at pci3 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em3 at pci4 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em4 at pci5 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > em5 at pci6 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address > 00:f1:f3:... > > em0 at pci1 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address > 00:0d:b9:... > em1 at pci2 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address > 00:0d:b9:... > em2 at pci3 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address > 00:0d:b9:... > > em0 at pci1 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address > 00:0d:b9:... > em1 at pci2 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address > 00:0d:b9:... > em2 at pci3 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address > 00:0d:b9:... > > em0 at pci1 dev 0 function 0 "Intel 82574L" rev 0x00: msi, address > 68:05:ca:... > > The only questions I have are around queue identification. All the specs I've > been able to find indicate the I210 should have 4 queues, did Intel make a > cheaper version with 2 toward the end of production? Or could it be an I211 > masquerading as an I210 (and would that be bad for the driver)? Is it a 2-cpu machine? > Also, > https://www.mouser.com/pdfdocs/Intel_82574L_82574IT_GbE_Controller_brief.pdf > indicates that the 82574L should have 2 queues? No msix in your dmesg excerpt for that one
Re: keynote: ansi for b64_{ntop,pton} wrappers
On Thu, Apr 13, 2023 at 07:56:35PM +0200, Theo Buehler wrote:
> Silences -Wdeprecated-non-prototype warnings emitted by clang 15.
OK bluhm@
> Index: base64.c
> ===
> RCS file: /cvs/src/lib/libkeynote/base64.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 base64.c
> --- base64.c 29 Jun 2004 11:35:56 - 1.11
> +++ base64.c 4 Feb 2023 20:37:56 -
> @@ -34,11 +34,8 @@ int __b64_ntop(unsigned char const *, si
> int __b64_pton(char const *, unsigned char *, size_t);
>
> int
> -kn_encode_base64(src, srclength, target, targsize)
> -unsigned char const *src;
> -unsigned int srclength;
> -char *target;
> -unsigned int targsize;
> +kn_encode_base64(unsigned char const *src, unsigned int srclength, char
> *target,
> +unsigned int targsize)
> {
> int i;
>
> @@ -49,10 +46,7 @@ unsigned int targsize;
> }
>
> int
> -kn_decode_base64(src, target, targsize)
> -char const *src;
> -unsigned char *target;
> -unsigned int targsize;
> +kn_decode_base64(char const *src, unsigned char *target, unsigned int
> targsize)
> {
> int i;
>
Re: em(4) multiqueue
Reviving this thread, apologies for discontinuity in mail readers: https://marc.info/?t=16564219358 After rebasing on 7.3, my results have mirrored Hrvoje's testing at the end of that thread. No issues with throughput, unusual latency, or reliability. `vmstat -i` shows some level of balancing between the queues. I've been testing on as many em(4) systems as I have access to, some manually, some in a packet forwarder/firewall scenarios: em0 at pci1 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address 00:f1:f3:... em1 at pci2 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address 00:f1:f3:... em2 at pci3 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address 00:f1:f3:... em3 at pci4 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address 00:f1:f3:... em4 at pci5 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address 00:f1:f3:... em5 at pci6 dev 0 function 0 "Intel I210" rev 0x03, msix, 2 queues, address 00:f1:f3:... em0 at pci1 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address 00:0d:b9:... em1 at pci2 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address 00:0d:b9:... em2 at pci3 dev 0 function 0 "Intel I210" rev 0x03, msix, 4 queues, address 00:0d:b9:... em0 at pci1 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address 00:0d:b9:... em1 at pci2 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address 00:0d:b9:... em2 at pci3 dev 0 function 0 "Intel I211" rev 0x03, msix, 2 queues, address 00:0d:b9:... em0 at pci1 dev 0 function 0 "Intel 82574L" rev 0x00: msi, address 68:05:ca:... The only questions I have are around queue identification. All the specs I've been able to find indicate the I210 should have 4 queues, did Intel make a cheaper version with 2 toward the end of production? Or could it be an I211 masquerading as an I210 (and would that be bad for the driver)? Also, https://www.mouser.com/pdfdocs/Intel_82574L_82574IT_GbE_Controller_brief.pdf indicates that the 82574L should have 2 queues? Anyway, great work, please let me know if there's more I can do to help this move forward. Brian Conway Lead Software Engineer, Owner RCE Software, LLC
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Tue, Apr 11, 2023 at 05:50:43PM +0200, Otto Moerbeek wrote: > On Sun, Apr 09, 2023 at 12:17:35PM +0200, Otto Moerbeek wrote: > > > On Sun, Apr 09, 2023 at 10:08:25AM +0200, Claudio Jeker wrote: > > > > > On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > > > > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > > > > > > > > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > > > > > > > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > This is work in progress. I have to think if the flags to kdump > > > > > > > > I'm > > > > > > > > introducing should be two or a single one. > > > > > > > > > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS defined. > > > > > > > > If run > > > > > > > > with option D it dumps its state to a malloc.out file at exit. > > > > > > > > This > > > > > > > > state can be used to find leaks amongst other things. > > > > > > > > > > > > > > > > This is not ideal for pledged processes, as they often have no > > > > > > > > way to > > > > > > > > write files. > > > > > > > > > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > > > > > > > > > As kdump has no nice way to show those lines without all extras > > > > > > > > it > > > > > > > > normally shows, so add two options to it to just show the lines. > > > > > > > > > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > > > > > > > > > Run : > > > > > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > > > ... > > > > > > > > $ kdump -hu > > > > > > > > > > > > > > > > Feedback appreciated. > > > > > > > > > > > > I can't really comment on malloc(3) stuff, but I agree that > > > > > > utrace(2) is a good > > > > > > way to get information outside a pledged process. > > > > > > > > > > > > I tend to think it is safe to use it, as the pledged process need > > > > > > cooperation > > > > > > from outside to exfiltrate informations (a process with permission > > > > > > to call > > > > > > ktrace(2) on this pid). > > > > > > > > > > > > Please note it is a somehow generic problem: at least profiled > > > > > > processes would > > > > > > also get advantage of using it. > > > > > > > > > > > > > > > > > > Regarding kdump options, I think that -u option should implies -h > > > > > > (no header). > > > > > > > > > > > > Does it would make sens to considere a process using utrace(2) with > > > > > > several > > > > > > interleaved records for different sources ? A process with > > > > > > MALLOC_OPTIONS=D and > > > > > > profiling enabled for example ? An (another) option on kdump to > > > > > > filter on utrace > > > > > > label would be useful in such case, or have -u mandate a label to > > > > > > filter on. > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > $ kdump -u mallocdumpline > > > > > > > > > > > > and for profiling: > > > > > > > > > > > > $ kdump -u profil > gmon.out > > > > > > $ gprof your_program gmon.out > > > > > > > > > > > > Thanks. > > > > > > > > > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > > > > > part to make it more flexable for different purposes. > > > > > > > > Anothew aspect of safety is the information availble in the heap > > > > itself. I'm pretty sure the addresses of call sites into malloc are > > > > interesting to attackers. To prevent a program having access to those > > > > (even if they are stored inside the malloc meta data that is not > > > > directly accesible to a program), I'm making sure the recording only > > > > takes place if malloc option D is used. > > > > > > > > This is included in a diff with the kdump changes and a few other > > > > things below. I'm also compiling with MALLOC_STATS if !SMALL. > > > > > > > > usage is now: > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu a.out > > > > $ kdump -u malloc > > > > > > > > > > I would prefer if every utrace() call is a full line (in other words ulog > > > should be line buffered). It makes the regular kdump output more useable. > > > Right now you depend on kdump -u to put the lines back together. > > > > Right. Done line buffering in the new diff below. > > > > > Whenever I used utrace() I normally passed binary objects to the call so I > > > could enrich the ktrace with userland trace info. So if kdump -u is used > > > for more then just mallocstats it should have a true binary mode. For > > > example gmon.out is a binary format so the above example by semarie@ would > > > not work. As usual this can be solved in tree once that problem is hit. > > > > Right, we can do that when needed. > > > > This is approaching a state where I'm happy with it. So reviews, tests > > appreciated. > > > > I would like to have the possibility to record a caller deeper in the > > stack (as many program use wrappers to
keynote: ansi for b64_{ntop,pton} wrappers
Silences -Wdeprecated-non-prototype warnings emitted by clang 15.
Index: base64.c
===
RCS file: /cvs/src/lib/libkeynote/base64.c,v
retrieving revision 1.11
diff -u -p -r1.11 base64.c
--- base64.c29 Jun 2004 11:35:56 - 1.11
+++ base64.c4 Feb 2023 20:37:56 -
@@ -34,11 +34,8 @@ int __b64_ntop(unsigned char const *, si
int __b64_pton(char const *, unsigned char *, size_t);
int
-kn_encode_base64(src, srclength, target, targsize)
-unsigned char const *src;
-unsigned int srclength;
-char *target;
-unsigned int targsize;
+kn_encode_base64(unsigned char const *src, unsigned int srclength, char
*target,
+unsigned int targsize)
{
int i;
@@ -49,10 +46,7 @@ unsigned int targsize;
}
int
-kn_decode_base64(src, target, targsize)
-char const *src;
-unsigned char *target;
-unsigned int targsize;
+kn_decode_base64(char const *src, unsigned char *target, unsigned int targsize)
{
int i;
Re: fix iwm/iwx updatechan callbacks
On Wed, 12 Apr 2023 23:27:08 +0200, Stefan Sperling wrote: > The iwm_updatechan and iwx_updatechan callbacks are not reachable > because they were never wired up. Only the iwn driver already has > this callback pointer set as intended. > > With the patch below iwm/iwx should get triggered when an AP switches > between 20MHz and 40/80MHz channel width, as indicated by the > 11n HT-operation information element in beacons. Working fine here with: iwm0 at pci2 dev 0 function 0 "Intel AC 8260" rev 0x3a, msi iwm0: hw rev 0x200, fw ver 36.ca7b901d.0, address xx:xx:xx:xx:xx:xx though I don't think my AP switches channel widths. - todd
installer: move code into stop_watchdog()
start_watchdog() and reset_watchdog() exist, but stop logic is inlined.
Giving it a name seems better.
No functional change.
Feedback? Objection? OK?
Questions wrt. upgrade.site(5) prompted me to look at whether execution of
/upgrade.site execution is subject to this timeout: it is, finish_up()
runs $MODE.site and the watchdog gets stopped afterwards.
Index: install.sub
===
RCS file: /cvs/src/distrib/miniroot/install.sub,v
retrieving revision 1.1241
diff -u -p -r1.1241 install.sub
--- install.sub 7 Apr 2023 13:48:42 - 1.1241
+++ install.sub 13 Apr 2023 15:14:31 -
@@ -3452,11 +3452,8 @@ do_upgrade() {
# Perform final steps common to both an install and an upgrade.
finish_up
- if [ -f /tmp/wdpid ]; then
- kill -KILL "$(cat /tmp/wdpid)" 2>/dev/null
- # do not bother waiting
- rm -f /tmp/wdpid
- fi
+
+ stop_watchdog
}
check_unattendedupgrade() {
@@ -3483,6 +3480,7 @@ WATCHDOG_PERIOD_SEC=$((30 * 60))
# Restart the background timer.
reset_watchdog() {
local _pid
+
if [ -f /tmp/wdpid ]; then
_pid=$(cat /tmp/wdpid)
kill -KILL -$_pid 2>/dev/null
@@ -3501,6 +3499,14 @@ start_watchdog() {
) >/dev/null 2>&1 &
echo $! > /tmp/wdpid
set +m
+}
+
+stop_watchdog() {
+ if [ -f /tmp/wdpid ]; then
+ kill -KILL "$(cat /tmp/wdpid)" 2>/dev/null
+ # do not bother waiting
+ rm -f /tmp/wdpid
+ fi
}
# return if we only want internal functions
Re: bgpd cleanup imsg handling for communities
On Thu, Apr 13, 2023 at 03:48:40PM +0200, Claudio Jeker wrote:
> Seen while working on similar code. There is no need to make the
> composition of IMSG_CTL_SHOW_RIB_COMMUNITIES so complicated.
ok
>
> --
> :wq Claudio
>
> Index: rde.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.600
> diff -u -p -r1.600 rde.c
> --- rde.c 7 Apr 2023 13:49:03 - 1.600
> +++ rde.c 13 Apr 2023 13:47:51 -
> @@ -2730,16 +2730,10 @@ rde_dump_rib_as(struct prefix *p, struct
> struct rde_community *comm = prefix_communities(p);
> size_t len = comm->nentries * sizeof(struct community);
> if (comm->nentries > 0) {
> - if ((wbuf = imsg_create(ibuf_se_ctl,
> - IMSG_CTL_SHOW_RIB_COMMUNITIES, 0, pid,
> - len)) == NULL)
> + if (imsg_compose(ibuf_se_ctl,
> + IMSG_CTL_SHOW_RIB_COMMUNITIES, 0, pid, -1,
> + comm->communities, len) == -1)
> return;
> - if ((bp = ibuf_reserve(wbuf, len)) == NULL) {
> - ibuf_free(wbuf);
> - return;
> - }
> - memcpy(bp, comm->communities, len);
> - imsg_close(ibuf_se_ctl, wbuf);
> }
> for (l = 0; l < asp->others_len; l++) {
> if ((a = asp->others[l]) == NULL)
>
bgpd cleanup imsg handling for communities
Seen while working on similar code. There is no need to make the
composition of IMSG_CTL_SHOW_RIB_COMMUNITIES so complicated.
--
:wq Claudio
Index: rde.c
===
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.600
diff -u -p -r1.600 rde.c
--- rde.c 7 Apr 2023 13:49:03 - 1.600
+++ rde.c 13 Apr 2023 13:47:51 -
@@ -2730,16 +2730,10 @@ rde_dump_rib_as(struct prefix *p, struct
struct rde_community *comm = prefix_communities(p);
size_t len = comm->nentries * sizeof(struct community);
if (comm->nentries > 0) {
- if ((wbuf = imsg_create(ibuf_se_ctl,
- IMSG_CTL_SHOW_RIB_COMMUNITIES, 0, pid,
- len)) == NULL)
+ if (imsg_compose(ibuf_se_ctl,
+ IMSG_CTL_SHOW_RIB_COMMUNITIES, 0, pid, -1,
+ comm->communities, len) == -1)
return;
- if ((bp = ibuf_reserve(wbuf, len)) == NULL) {
- ibuf_free(wbuf);
- return;
- }
- memcpy(bp, comm->communities, len);
- imsg_close(ibuf_se_ctl, wbuf);
}
for (l = 0; l < asp->others_len; l++) {
if ((a = asp->others[l]) == NULL)
Re: bgpd first bunch of flowspec code
On Thu, Apr 13, 2023 at 02:17:48PM +0200, Theo Buehler wrote:
> On Wed, Apr 12, 2023 at 05:33:10PM +0200, Claudio Jeker wrote:
> > This is the first big amount of flowspec specific code.
> > It adds a new file (flowspec.c) which exposes basic API functions to work
> > with flowspec. Right now apart from the regress test nothing uses these
> > functions (but don't worry I have other diffs which make use of them).
> >
> > Flowspec encoding is super annoying. It is extremly flexible, the length
> > of individual components is encoded into the component, there are different
> > encodings for IPv4 and IPv6, some encodings make absolutly no sense, and the
> > total length can be up to 4095 bytes (yikes).
>
> This is complete madness.
>
> > Because of this the idea is to keep flowspec as NLRI blob, use this API to
> > validate the blob and provide a RFC compliant compare function.
> > Additionally provide functions to extract components and helpers to print
> > components.
>
> Yes, that makes sense.
>
> > I know some not so important functions are still missing but this is a
> > decent start. Especially formating a flowspec into a string is not fully
> > there yet.
>
> Here's a first round of feedback. I need to ponder this more since it
> involves a lot of scary pointer manipulations. I don't think my head
> will cooperate without doing something else for a while.
If you have better suggestions on how to work with such a blob I'm happy
to hear. I tried to keep the madness in one place and have a mostly sane
API on top.
> Unless it blocks you from further progress, I would prefer to wait with
> landing this for a day or two more.
No worries, I cherry-pick stuff out of my work tree. I can handle a few
more rebases :)
A few comments below:
> Maybe add
>
> #define FLOWSPEC_OP_NUM_FALSE 0x00
> #define FLOWSPEC_OP_NUM_TRUE 0x07
>
> and use them in the switch in flowspec_fmt_num_op().
Torn about that. The concept of TRUE and FALSE is so rediculously
stupid that I'm not sure it is something we should promote out of dark
basement it should remain in.
> > + case FLOWSPEC_TYPE_DEST:
> > + case FLOWSPEC_TYPE_SOURCE:
> > + if (!is_v6) {
>
> Maybe add a reference to RFC 4271, section 4.3 here. The generic references in
> RFC 8955 aren't helpful.
>
> > + if (len < vlen + 1)
> > + return -1;
> > + plen = buf[vlen];
> > + vlen += PREFIX_SIZE(plen);
> > + if (plen > 32 || len < vlen)
> > + return -1;
> > + } else {
> > + if (len < vlen + 2)
> > + return -1;
> > + plen = buf[vlen];
> > + off = buf[vlen + 1];
> > + if (plen > 128 || off >= plen)
> > + return -1;
> > + vlen += PREFIX_SIZE(plen - off) + 1; /* off is extra */
> > + if (len < vlen)
> > + return -1;
>
> It took me way too long to figure out I have to go look at RFC 8956,
> section 3.1. Starting from RFC 8955 I tried to find something about
> this mysterious off in RFC 4271 and was utterly confused for the better
> part of an hour...
>
> In short: please add a comment for the stupid. Future me will be grateful.
Sorry about that. I was also shocked by the ingenuity of the RFC 8956
authors. Especially since the offset MUST be 0 in many cases.
Guess the ASN.1 madness infected other groups at IETF as well.
> > +static long long
> > +extract_val(const uint8_t *comp, int len)
> > +{
> > + long long val = 0;
>
> I'm surprised val isn't a uint64_t.
I fixed that after I sent this version out. Let me fold it back into
this diff.
Updated version of the diff below. It includes now a few more formatting
functions.
--
:wq Claudio
Index: Makefile
===
RCS file: /cvs/src/usr.sbin/bgpd/Makefile,v
retrieving revision 1.38
diff -u -p -r1.38 Makefile
--- Makefile11 Jan 2023 13:53:17 - 1.38
+++ Makefile12 Apr 2023 15:14:49 -
@@ -5,7 +5,7 @@ SRCS= bgpd.c session.c log.c logmsg.c pa
rde.c rde_rib.c rde_decide.c rde_prefix.c mrt.c kroute.c control.c \
pfkey.c rde_update.c rde_attr.c rde_community.c printconf.c \
rde_filter.c rde_sets.c rde_aspa.c rde_trie.c pftable.c name2id.c \
- util.c carp.c timer.c rde_peer.c rtr.c rtr_proto.c
+ util.c carp.c timer.c rde_peer.c rtr.c rtr_proto.c flowspec.c
CFLAGS+= -Wall -I${.CURDIR}
CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
CFLAGS+= -Wmissing-declarations
Index: bgpd.h
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
retrieving revision 1.470
diff -u -p -r1.470 bgpd.h
--- bgpd.h 3 Apr 2023 10:48:00 - 1.470
+++ bgpd.h 13 Apr 2023 13:12:15 -
@@ -1087,18 +1087,23 @@ struct
Re: fix iwm/iwx updatechan callbacks
On 2023-04-12 23:27 +02, Stefan Sperling wrote: > The iwm_updatechan and iwx_updatechan callbacks are not reachable > because they were never wired up. Only the iwn driver already has > this callback pointer set as intended. > > With the patch below iwm/iwx should get triggered when an AP switches > between 20MHz and 40/80MHz channel width, as indicated by the > 11n HT-operation information element in beacons. > > Tests for regressions on any iwm/iwx devices would be welcome. > Seems to be working fine with on x395 with iwm0 at pci1 dev 0 function 0 "Intel Dual Band Wireless-AC 9260" rev 0x29, msix iwm0: hw rev 0x320, fw ver 46.ea3728ee.0, address 40:74:e0:38:11:11 ... but as far as I know my APs do not switch channel width. -- In my defence, I have been left unsupervised.
Re: bgpd first bunch of flowspec code
On Wed, Apr 12, 2023 at 05:33:10PM +0200, Claudio Jeker wrote:
> This is the first big amount of flowspec specific code.
> It adds a new file (flowspec.c) which exposes basic API functions to work
> with flowspec. Right now apart from the regress test nothing uses these
> functions (but don't worry I have other diffs which make use of them).
>
> Flowspec encoding is super annoying. It is extremly flexible, the length
> of individual components is encoded into the component, there are different
> encodings for IPv4 and IPv6, some encodings make absolutly no sense, and the
> total length can be up to 4095 bytes (yikes).
This is complete madness.
> Because of this the idea is to keep flowspec as NLRI blob, use this API to
> validate the blob and provide a RFC compliant compare function.
> Additionally provide functions to extract components and helpers to print
> components.
Yes, that makes sense.
> I know some not so important functions are still missing but this is a
> decent start. Especially formating a flowspec into a string is not fully
> there yet.
Here's a first round of feedback. I need to ponder this more since it
involves a lot of scary pointer manipulations. I don't think my head
will cooperate without doing something else for a while.
Unless it blocks you from further progress, I would prefer to wait with
landing this for a day or two more.
> --
> :wq Claudio
>
> Index: usr.sbin/bgpd/Makefile
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/Makefile,v
> retrieving revision 1.38
> diff -u -p -r1.38 Makefile
> --- usr.sbin/bgpd/Makefile11 Jan 2023 13:53:17 - 1.38
> +++ usr.sbin/bgpd/Makefile12 Apr 2023 15:14:49 -
> @@ -5,7 +5,7 @@ SRCS= bgpd.c session.c log.c logmsg.c pa
> rde.c rde_rib.c rde_decide.c rde_prefix.c mrt.c kroute.c control.c \
> pfkey.c rde_update.c rde_attr.c rde_community.c printconf.c \
> rde_filter.c rde_sets.c rde_aspa.c rde_trie.c pftable.c name2id.c \
> - util.c carp.c timer.c rde_peer.c rtr.c rtr_proto.c
> + util.c carp.c timer.c rde_peer.c rtr.c rtr_proto.c flowspec.c
> CFLAGS+= -Wall -I${.CURDIR}
> CFLAGS+= -Wstrict-prototypes -Wmissing-prototypes
> CFLAGS+= -Wmissing-declarations
> Index: usr.sbin/bgpd/bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.470
> diff -u -p -r1.470 bgpd.h
> --- usr.sbin/bgpd/bgpd.h 3 Apr 2023 10:48:00 - 1.470
> +++ usr.sbin/bgpd/bgpd.h 12 Apr 2023 15:14:49 -
> @@ -1087,18 +1087,22 @@ struct ext_comm_pairs {
> extern const struct ext_comm_pairs iana_ext_comms[];
>
> /* BGP flowspec defines RFC 8955 and 8956 */
> -#define FLOWSPEC_LEN_LIMIT 0xf0
> -#define FLOWSPEC_OP_EOL 0x80
> -#define FLOWSPEC_OP_AND 0x40
> -#define FLOWSPEC_OP_LEN_MASK 0x30
> -#define FLOWSPEC_OP_LEN_SHIFT4
> +#define FLOWSPEC_LEN_LIMIT 0xf0
> +#define FLOWSPEC_OP_EOL 0x80
> +#define FLOWSPEC_OP_AND 0x40
> +#define FLOWSPEC_OP_LEN_MASK 0x30
> +#define FLOWSPEC_OP_LEN_SHIFT4
> #define FLOWSPEC_OP_LEN(op) \
> (1 << (((op) & FLOWSPEC_OP_LEN_MASK) >> FLOWSPEC_OP_LEN_SHIFT))
> -#define FLOWSPEC_OP_NUM_LT 0x04
> -#define FLOWSPEC_OP_NUM_GT 0x02
> -#define FLOWSPEC_OP_NUM_EQ 0x01
> -#define FLOWSPEC_OP_BIT_NOT 0x02
> -#define FLOWSPEC_OP_BIT_MATCH0x01
> +#define FLOWSPEC_OP_NUM_LT 0x04
> +#define FLOWSPEC_OP_NUM_GT 0x02
> +#define FLOWSPEC_OP_NUM_EQ 0x01
> +#define FLOWSPEC_OP_NUM_LE (FLOWSPEC_OP_NUM_LT | FLOWSPEC_OP_NUM_EQ)
> +#define FLOWSPEC_OP_NUM_GE (FLOWSPEC_OP_NUM_GT | FLOWSPEC_OP_NUM_EQ)
> +#define FLOWSPEC_OP_NUM_NOT (FLOWSPEC_OP_NUM_GT | FLOWSPEC_OP_NUM_LT)
Maybe add
#define FLOWSPEC_OP_NUM_FALSE 0x00
#define FLOWSPEC_OP_NUM_TRUE0x07
and use them in the switch in flowspec_fmt_num_op().
> +#define FLOWSPEC_OP_NUM_MASK 0x07
> +#define FLOWSPEC_OP_BIT_NOT 0x02
> +#define FLOWSPEC_OP_BIT_MATCH0x01
>
> #define FLOWSPEC_TYPE_MIN1
> #define FLOWSPEC_TYPE_DEST 1
> @@ -1510,6 +1514,7 @@ int aspath_verify(void *, uint16_t, in
> #define AS_ERR_BAD -3
> #define AS_ERR_SOFT-4
> u_char *aspath_inflate(void *, uint16_t, uint16_t *);
> +int extract_prefix(const u_char *, int, void *, uint8_t, uint8_t);
> int nlri_get_prefix(u_char *, uint16_t, struct bgpd_addr *,
> uint8_t *);
> int nlri_get_prefix6(u_char *, uint16_t, struct bgpd_addr *,
> @@ -1532,6 +1537,15 @@ int af2aid(sa_family_t, uint8_t, uint8
> struct sockaddr *addr2sa(const struct bgpd_addr *, uint16_t, socklen_t
> *);
> void sa2addr(struct sockaddr *, struct bgpd_addr *, uint16_t *);
> const
Re: alphabetically order commands in bgpctl
On Thu, Apr 13, 2023 at 01:23:23PM +0200, Theo Buehler wrote:
> On Thu, Apr 13, 2023 at 12:25:46PM +0200, Claudio Jeker wrote:
> > bgpctl help output follows no clear order. I decided to sort all
> > keywords and flags alphabetically. Also fixup the manpage a bit since
> > some additions where added in the wrong spot.
> >
> > I think the output of 'bgpctl show rib help' is the worst (both before and
> > after). It is long and some keywords are not self-explanatory.
> > Still I prefer them to be alphabetically sorted.
>
> I'm ok with this, but see some comments below.
>
> > -.Xr bgpd 8
> > -itself.
>
> I think the previous two lines were removed by accident...
>
> > +.Xr bgpd 8
> > +itself.
>
> ... and added here by mistake.
Fixed
> > static const struct token t_show_mrt[] = {
> > { NOTOKEN, "", NONE, NULL},
> > - { ASTYPE, "as", AS_ALL, t_show_mrt_as},
> > - { ASTYPE, "source-as",AS_SOURCE, t_show_mrt_as},
> > - { ASTYPE, "transit-as", AS_TRANSIT, t_show_mrt_as},
> > - { ASTYPE, "peer-as", AS_PEER,t_show_mrt_as},
> > - { ASTYPE, "empty-as", AS_EMPTY, t_show_mrt},
> > { FLAG, "detail", F_CTL_DETAIL, t_show_mrt},
> > - { FLAG, "ssv", F_CTL_SSV, t_show_mrt},
> > - { KEYWORD, "neighbor", NONE, t_show_mrt_neigh},
> > { FLAG, "peers",F_CTL_NEIGHBORS,t_show_mrt},
> > + { FLAG, "ssv", F_CTL_SSV, t_show_mrt},
>
> Slightly confused here. This sorts FLAG first, then ASTYPE and KEYWORD.
> It breaks alphabetical order. This wasn't done in t_show_rib[].
Also fixed. I probably used a bad sort -k here.
--
:wq Claudio
Re: alphabetically order commands in bgpctl
On Thu, Apr 13, 2023 at 12:25:46PM +0200, Claudio Jeker wrote:
> bgpctl help output follows no clear order. I decided to sort all
> keywords and flags alphabetically. Also fixup the manpage a bit since
> some additions where added in the wrong spot.
>
> I think the output of 'bgpctl show rib help' is the worst (both before and
> after). It is long and some keywords are not self-explanatory.
> Still I prefer them to be alphabetically sorted.
I'm ok with this, but see some comments below.
> --
> :wq Claudio
>
> Index: bgpctl.8
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.8,v
> retrieving revision 1.107
> diff -u -p -r1.107 bgpctl.8
> --- bgpctl.8 12 Apr 2023 17:19:16 - 1.107
> +++ bgpctl.8 13 Apr 2023 10:21:05 -
> @@ -180,20 +180,20 @@ can be an IP address, in which case the
> or a flag:
> .Pp
> .Bl -tag -width tableXnumber -compact
> -.It Cm connected
> -Show only connected routes.
> -.It Cm static
> -Show only static routes.
> .It Cm bgp
> Show only routes originating from
> -.Xr bgpd 8
> -itself.
I think the previous two lines were removed by accident...
> -.It Cm nexthop
> -Show only routes required to reach a BGP nexthop.
> +.It Cm connected
> +Show only connected routes.
> .It Cm inet
> Show only IPv4 routes.
> .It Cm inet6
> Show only IPv6 routes.
> +.It Cm nexthop
> +Show only routes required to reach a BGP nexthop.
> +.It Cm static
> +Show only static routes.
> +.Xr bgpd 8
> +itself.
... and added here by mistake.
> .It Cm table Ar number
> Show the routing table with ID
> .Ar number
> @@ -317,6 +317,7 @@ Show RIB entry for this CIDR prefix.
> .Xc
> Show all entries in the specified range.
> .\".It Ar address/len Cm longer-prefixes
> +.\".It Ar address/len Cm or-longer
> .It Xo
> .Ar address Ns Li / Ns Ar len
> .Cm or-shorter
> @@ -326,20 +327,24 @@ Show all entries covering and including
> Show all entries with
> .Ar as
> anywhere in the AS path.
> +.It Cm avs Pq Ic valid | unknown | invalid
> +Show all entries with matching ASAP Validation State (AVS).
> .It Cm community Ar community
> Show all entries with community
> .Ar community .
> +.It Cm empty-as
> +Show all entries that are internal routes with no AS's in the AS path.
> .It Cm large-community Ar large-community
> Show all entries with large-community
> .Ar large-community .
> -.It Cm empty-as
> -Show all entries that are internal routes with no AS's in the AS path.
> .It Cm memory
> Show RIB memory statistics.
> .It Cm neighbor Ar peer
> Show only entries from the specified peer.
> .It Cm neighbor group Ar description
> Show only entries from the specified peer group.
> +.It Cm ovs Pq Ic valid | not-found | invalid
> +Show all entries with matching Origin Validation State (OVS).
> .It Cm path-id Ar pathid
> Show only entries which match the specified
> .Ar pathid .
> @@ -365,10 +370,6 @@ Show only entries from the specified RIB
> Show all entries with
> .Ar as
> anywhere but rightmost.
> -.It Cm ovs Pq Ic valid | not-found | invalid
> -Show all entries with matching Origin Validation State (OVS).
> -.It Cm avs Pq Ic valid | unknown | invalid
> -Show all entries with matching ASAP Validation State (AVS).
> .El
> .Pp
> Additionally, the following
> Index: parser.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
> retrieving revision 1.123
> diff -u -p -r1.123 parser.c
> --- parser.c 12 Apr 2023 17:19:16 - 1.123
> +++ parser.c 13 Apr 2023 10:21:05 -
> @@ -120,12 +120,12 @@ static const struct token t_communicatio
> static const struct token t_show_rib_path[];
>
> static const struct token t_main[] = {
> - { KEYWORD, "reload", RELOAD, t_communication},
> - { KEYWORD, "show", SHOW, t_show},
> { KEYWORD, "fib", FIB,t_fib},
> + { KEYWORD, "log", NONE, t_log},
> { KEYWORD, "neighbor", NEIGHBOR, t_neighbor},
> { KEYWORD, "network", NONE, t_network},
> - { KEYWORD, "log", NONE, t_log},
> + { KEYWORD, "reload", RELOAD, t_communication},
> + { KEYWORD, "show", SHOW, t_show},
> { ENDTOKEN, "", NONE, NULL}
> };
>
> @@ -133,17 +133,17 @@ static const struct token t_show[] = {
> { NOTOKEN, "", NONE, NULL},
> { KEYWORD, "fib", SHOW_FIB, t_show_fib},
> { KEYWORD, "interfaces", SHOW_INTERFACE, NULL},
> + { KEYWORD, "ip", NONE, t_show_ip},
> + { KEYWORD, "metrics", SHOW_METRICS, NULL},
> + { KEYWORD, "mrt", SHOW_MRT, t_show_mrt},
> { KEYWORD, "neighbor", SHOW_NEIGHBOR, t_show_neighbor},
> { KEYWORD,
alphabetically order commands in bgpctl
bgpctl help output follows no clear order. I decided to sort all
keywords and flags alphabetically. Also fixup the manpage a bit since
some additions where added in the wrong spot.
I think the output of 'bgpctl show rib help' is the worst (both before and
after). It is long and some keywords are not self-explanatory.
Still I prefer them to be alphabetically sorted.
--
:wq Claudio
Index: bgpctl.8
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.8,v
retrieving revision 1.107
diff -u -p -r1.107 bgpctl.8
--- bgpctl.812 Apr 2023 17:19:16 - 1.107
+++ bgpctl.813 Apr 2023 10:21:05 -
@@ -180,20 +180,20 @@ can be an IP address, in which case the
or a flag:
.Pp
.Bl -tag -width tableXnumber -compact
-.It Cm connected
-Show only connected routes.
-.It Cm static
-Show only static routes.
.It Cm bgp
Show only routes originating from
-.Xr bgpd 8
-itself.
-.It Cm nexthop
-Show only routes required to reach a BGP nexthop.
+.It Cm connected
+Show only connected routes.
.It Cm inet
Show only IPv4 routes.
.It Cm inet6
Show only IPv6 routes.
+.It Cm nexthop
+Show only routes required to reach a BGP nexthop.
+.It Cm static
+Show only static routes.
+.Xr bgpd 8
+itself.
.It Cm table Ar number
Show the routing table with ID
.Ar number
@@ -317,6 +317,7 @@ Show RIB entry for this CIDR prefix.
.Xc
Show all entries in the specified range.
.\".It Ar address/len Cm longer-prefixes
+.\".It Ar address/len Cm or-longer
.It Xo
.Ar address Ns Li / Ns Ar len
.Cm or-shorter
@@ -326,20 +327,24 @@ Show all entries covering and including
Show all entries with
.Ar as
anywhere in the AS path.
+.It Cm avs Pq Ic valid | unknown | invalid
+Show all entries with matching ASAP Validation State (AVS).
.It Cm community Ar community
Show all entries with community
.Ar community .
+.It Cm empty-as
+Show all entries that are internal routes with no AS's in the AS path.
.It Cm large-community Ar large-community
Show all entries with large-community
.Ar large-community .
-.It Cm empty-as
-Show all entries that are internal routes with no AS's in the AS path.
.It Cm memory
Show RIB memory statistics.
.It Cm neighbor Ar peer
Show only entries from the specified peer.
.It Cm neighbor group Ar description
Show only entries from the specified peer group.
+.It Cm ovs Pq Ic valid | not-found | invalid
+Show all entries with matching Origin Validation State (OVS).
.It Cm path-id Ar pathid
Show only entries which match the specified
.Ar pathid .
@@ -365,10 +370,6 @@ Show only entries from the specified RIB
Show all entries with
.Ar as
anywhere but rightmost.
-.It Cm ovs Pq Ic valid | not-found | invalid
-Show all entries with matching Origin Validation State (OVS).
-.It Cm avs Pq Ic valid | unknown | invalid
-Show all entries with matching ASAP Validation State (AVS).
.El
.Pp
Additionally, the following
Index: parser.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
retrieving revision 1.123
diff -u -p -r1.123 parser.c
--- parser.c12 Apr 2023 17:19:16 - 1.123
+++ parser.c13 Apr 2023 10:21:05 -
@@ -120,12 +120,12 @@ static const struct token t_communicatio
static const struct token t_show_rib_path[];
static const struct token t_main[] = {
- { KEYWORD, "reload", RELOAD, t_communication},
- { KEYWORD, "show", SHOW, t_show},
{ KEYWORD, "fib", FIB,t_fib},
+ { KEYWORD, "log", NONE, t_log},
{ KEYWORD, "neighbor", NEIGHBOR, t_neighbor},
{ KEYWORD, "network", NONE, t_network},
- { KEYWORD, "log", NONE, t_log},
+ { KEYWORD, "reload", RELOAD, t_communication},
+ { KEYWORD, "show", SHOW, t_show},
{ ENDTOKEN, "", NONE, NULL}
};
@@ -133,17 +133,17 @@ static const struct token t_show[] = {
{ NOTOKEN, "", NONE, NULL},
{ KEYWORD, "fib", SHOW_FIB, t_show_fib},
{ KEYWORD, "interfaces", SHOW_INTERFACE, NULL},
+ { KEYWORD, "ip", NONE, t_show_ip},
+ { KEYWORD, "metrics", SHOW_METRICS, NULL},
+ { KEYWORD, "mrt", SHOW_MRT, t_show_mrt},
{ KEYWORD, "neighbor", SHOW_NEIGHBOR, t_show_neighbor},
{ KEYWORD, "network", NETWORK_SHOW, t_network_show},
{ KEYWORD, "nexthop", SHOW_NEXTHOP, NULL},
{ KEYWORD, "rib", SHOW_RIB, t_show_rib},
- { KEYWORD, "tables", SHOW_FIB_TABLES, NULL},
- { KEYWORD, "ip", NONE, t_show_ip},
- { KEYWORD, "summary", SHOW_SUMMARY, t_show_summary},
- { KEYWORD, "sets",
