Re: suggest to run rpki-client hourly
On Tue, Apr 14, 2020 at 01:05:44PM -0600, Todd C. Miller wrote:
> On Mon, 13 Apr 2020 21:45:21 -0600, Bob Beck wrote:
>
> > Like this one plenty. I think it's ok the values change on reload.
>
> Here's a cleaned up version that includes the man page. The random
> interval can now be one of "~", "low~high", "low~", or "~high" where
> if low and/or high are omitted, the appropriate value for the field
> is used. Multiple random intervals can be used, separated by a
> comma.
>
> - todd
>
hi. i think the doc parts read fine.
a couple of points:
- in STANDARDS, i guess stick to using Ql rather than Dq
- the system crontab and the example in acme-client.1 should probably be
changed too
jmc
> Index: usr.sbin/cron/crontab.5
> ===
> RCS file: /cvs/src/usr.sbin/cron/crontab.5,v
> retrieving revision 1.37
> diff -u -p -u -r1.37 crontab.5
> --- usr.sbin/cron/crontab.5 6 Jan 2020 19:44:09 - 1.37
> +++ usr.sbin/cron/crontab.5 14 Apr 2020 19:01:33 -
> @@ -144,7 +144,18 @@ For example,
> .Ar hour
> entry specifies execution at hours 8, 9, 10 and 11.
> .Pp
> -Step values can be used in conjunction with ranges.
> +A random value (within the legal range) may be obtained by using the
> +.Ql ~
> +character in a field.
> +The interval of the random value may be specified explicitly, for example
> +.Dq 0~30
> +will result in a random value between 0 and 30 inclusive.
> +If either (or both) of the numbers on either side of the
> +.Ql ~
> +are omitted, the appropriate limit (low or high) for the field will be used.
> +.Pp
> +Step values can be used in conjunction with ranges (but not random ranges
> +which represent a single number).
> Following a range with
> .No / Ns Ar number
> specifies skips of
> @@ -318,6 +329,9 @@ MAILTO=paul
> 23 0-23/2 * * * echo "run 23 minutes after midn, 2am, 4am ..., everyday"
>
> 5 4 * * sun echo "run at 5 after 4 every sunday"
> +
> +# run hourly at a random time within the first 30 minutes of the hour
> +0~30 * * * * /usr/libexec/spamd-setup
> .Ed
> .Sh SEE ALSO
> .Xr crontab 1 ,
> @@ -337,6 +351,10 @@ field may use 7 to represent Sunday.
> .It
> Ranges may include
> .Dq steps .
> +.It
> +Random intervals are supported using the
> +.Dq ~
> +character.
> .It
> Months or days of the week can be specified by name.
> .It
> Index: usr.sbin/cron/entry.c
> ===
> RCS file: /cvs/src/usr.sbin/cron/entry.c,v
> retrieving revision 1.49
> diff -u -p -u -r1.49 entry.c
> --- usr.sbin/cron/entry.c 13 Jun 2018 11:27:30 - 1.49
> +++ usr.sbin/cron/entry.c 14 Apr 2020 19:03:47 -
> @@ -450,33 +450,29 @@ static int
> get_range(bitstr_t *bits, int low, int high, const char *names[],
> int ch, FILE *file)
> {
> - /* range = number | number "-" number [ "/" number ]
> + /* range = number | number* "~" number* | number "-" number ["/" number]
>*/
>
> int i, num1, num2, num3;
>
> + num1 = low;
> + num2 = high;
> +
> if (ch == '*') {
> - /* '*' means "first-last" but can still be modified by /step
> + /* '*' means [low, high] but can still be modified by /step
>*/
> - num1 = low;
> - num2 = high;
> ch = get_char(file);
> if (ch == EOF)
> return (EOF);
> } else {
> - ch = get_number(, low, names, ch, file, ",- \t\n");
> - if (ch == EOF)
> - return (EOF);
> -
> - if (ch != '-') {
> - /* not a range, it's a single number.
> - */
> - if (EOF == set_element(bits, low, high, num1)) {
> - unget_char(ch, file);
> + if (ch != '~') {
> + ch = get_number(, low, names, ch, file, ",-~
> \t\n");
> + if (ch == EOF)
> return (EOF);
> - }
> - return (ch);
> - } else {
> + }
> +
> + switch (ch) {
> + case '-':
> /* eat the dash
>*/
> ch = get_char(file);
> @@ -488,6 +484,37 @@ get_range(bitstr_t *bits, int low, int h
> ch = get_number(, low, names, ch, file, "/, \t\n");
> if (ch == EOF || num1 > num2)
> return (EOF);
> + break;
> + case '~':
> + /* eat the tilde
> + */
> + ch = get_char(file);
> + if (ch == EOF)
> + return (EOF);
> +
> + /* get the (optional) number following the tilde
> + */
> + ch = get_number(, low, names,
Re: cpu utilisation bars for top(1)
Hi,
On Mon, Apr 13, 2020 at 07:18:34PM +0100, Stuart Henderson wrote:
> It would make better use of the screen space if it worked like systat vm -
> multiple characters | (interrupt), @ (spin), = (sys), > (user) ... this fits
> a lot more information into the same space than just using a single char to
> represent any type of cpu use.
Ah, gotcha. Here's what I've done:
- Use different symbols for different CPU states, like systat(1) does.
- Made "combined" mode work. In doing so, put the code that draws the
bars into a function.
- There are now some long lines that are hard to wrap. I was tempted to factor
out the code that writes the textual CPU states into its own function, as
that'd reduce nesting and duplication. However, I've refrained fow now.
Example output (with system under load -- compiling LLVM):
```
load averages: 5.49, 5.35, 5.06 arrakis.home 21:04:53
153 processes: 6 running, 142 idle, 1 dead, 4 on processor up 2 days, 7:49
CPU0: @>>>
CPU1: @@@
CPU2: @>>>
CPU3: @>>>
Memory: Real: 2972M/16G act/tot Free: 7053M Cache: 11G Swap: 0K/1028M
PID USERNAME PRI NICE SIZE RES STATE WAIT TIMECPU COMMAND
...
```
Diff follows, thanks.
Index: display.c
===
RCS file: /cvs/src/usr.bin/top/display.c,v
retrieving revision 1.61
diff -u -p -r1.61 display.c
--- display.c 27 Oct 2019 13:52:26 - 1.61
+++ display.c 14 Apr 2020 19:55:33 -
@@ -106,6 +106,11 @@ extern struct process_select ps;
int header_status = true;
+/* stuff for graphical display of CPU utilisation */
+int cpu_bars = false;
+static char cpubar_chars[] = { '|', '@', '=', '>' };
+static char cpubar_order[] = { CP_INTR, CP_SPIN, CP_SYS, CP_USER };
+
static int
empty(void)
{
@@ -376,6 +381,30 @@ cpustates_tag(int cpu)
}
void
+draw_cpubar(long long states[], int width, int num_cpus)
+{
+#define NUM_CPUBAR_CHARS(V, NCPUS, PP) V / NCPUS / 10 * PP
+ int i, st, num_chars;
+ double pperc;
+
+ width = width < 0 ? 0 : width;
+ pperc = width / 100.0; /* characters per-percent */
+
+ for (i = 0; i < sizeof(cpubar_order) / sizeof(cpubar_order[0]); i++) {
+ st = cpubar_order[i];
+ num_chars = NUM_CPUBAR_CHARS(states[st], num_cpus, pperc);
+
+ /* CP_USER is merged with CP_NICE, like in systat(1) */
+ if (st == CP_USER)
+ num_chars += NUM_CPUBAR_CHARS(states[CP_NICE],
num_cpus, pperc);
+
+ while (num_chars-- > 0) {
+ addch(cpubar_chars[i]);
+ }
+ }
+}
+
+void
i_cpustates(int64_t *ostates, int *online)
{
int i, first, cpu, cpu_line;
@@ -384,7 +413,7 @@ i_cpustates(int64_t *ostates, int *onlin
char **names, *thisname;
if (combine_cpus) {
- static double *values;
+ static long long *values;
if (!values) {
values = calloc(num_cpustates, sizeof(*values));
if (!values)
@@ -412,14 +441,19 @@ i_cpustates(int64_t *ostates, int *onlin
clrtoeol();
printwp("%-3d CPUs: ", ncpuonline);
- while ((thisname = *names++) != NULL) {
- if (*thisname != '\0') {
- value = values[i++] / ncpuonline;
- /* if percentage is >= 1000, print it
as 100% */
- printwp((value >= 1000 ? "%s%4.0f%% %s"
:
- "%s%4.1f%% %s"), first++ == 0 ? ""
: ", ",
- value / 10., thisname);
+ if (!cpu_bars) {
+ while ((thisname = *names++) != NULL) {
+ if (*thisname != '\0') {
+ value = (double) values[i++] /
ncpuonline;
+ /* if percentage is >= 1000,
print it as 100% */
+ printwp((value >= 1000 ?
"%s%4.0f%% %s" :
+ "%s%4.1f%% %s"), first++ ==
0 ? "" : ", ",
+ value / 10., thisname);
+ }
}
+ } else {
+ /* "NNN CPUs: " is 10 characters */
+ draw_cpubar(values, screen_width - 10,
ncpuonline);
}
putn();
Re: suggest to run rpki-client hourly
On Mon, 13 Apr 2020 21:45:21 -0600, Bob Beck wrote:
> Like this one plenty. I think it's ok the values change on reload.
Here's a cleaned up version that includes the man page. The random
interval can now be one of "~", "low~high", "low~", or "~high" where
if low and/or high are omitted, the appropriate value for the field
is used. Multiple random intervals can be used, separated by a
comma.
- todd
Index: usr.sbin/cron/crontab.5
===
RCS file: /cvs/src/usr.sbin/cron/crontab.5,v
retrieving revision 1.37
diff -u -p -u -r1.37 crontab.5
--- usr.sbin/cron/crontab.5 6 Jan 2020 19:44:09 - 1.37
+++ usr.sbin/cron/crontab.5 14 Apr 2020 19:01:33 -
@@ -144,7 +144,18 @@ For example,
.Ar hour
entry specifies execution at hours 8, 9, 10 and 11.
.Pp
-Step values can be used in conjunction with ranges.
+A random value (within the legal range) may be obtained by using the
+.Ql ~
+character in a field.
+The interval of the random value may be specified explicitly, for example
+.Dq 0~30
+will result in a random value between 0 and 30 inclusive.
+If either (or both) of the numbers on either side of the
+.Ql ~
+are omitted, the appropriate limit (low or high) for the field will be used.
+.Pp
+Step values can be used in conjunction with ranges (but not random ranges
+which represent a single number).
Following a range with
.No / Ns Ar number
specifies skips of
@@ -318,6 +329,9 @@ MAILTO=paul
23 0-23/2 * * * echo "run 23 minutes after midn, 2am, 4am ..., everyday"
5 4 * * sun echo "run at 5 after 4 every sunday"
+
+# run hourly at a random time within the first 30 minutes of the hour
+0~30 * * * * /usr/libexec/spamd-setup
.Ed
.Sh SEE ALSO
.Xr crontab 1 ,
@@ -337,6 +351,10 @@ field may use 7 to represent Sunday.
.It
Ranges may include
.Dq steps .
+.It
+Random intervals are supported using the
+.Dq ~
+character.
.It
Months or days of the week can be specified by name.
.It
Index: usr.sbin/cron/entry.c
===
RCS file: /cvs/src/usr.sbin/cron/entry.c,v
retrieving revision 1.49
diff -u -p -u -r1.49 entry.c
--- usr.sbin/cron/entry.c 13 Jun 2018 11:27:30 - 1.49
+++ usr.sbin/cron/entry.c 14 Apr 2020 19:03:47 -
@@ -450,33 +450,29 @@ static int
get_range(bitstr_t *bits, int low, int high, const char *names[],
int ch, FILE *file)
{
- /* range = number | number "-" number [ "/" number ]
+ /* range = number | number* "~" number* | number "-" number ["/" number]
*/
int i, num1, num2, num3;
+ num1 = low;
+ num2 = high;
+
if (ch == '*') {
- /* '*' means "first-last" but can still be modified by /step
+ /* '*' means [low, high] but can still be modified by /step
*/
- num1 = low;
- num2 = high;
ch = get_char(file);
if (ch == EOF)
return (EOF);
} else {
- ch = get_number(, low, names, ch, file, ",- \t\n");
- if (ch == EOF)
- return (EOF);
-
- if (ch != '-') {
- /* not a range, it's a single number.
-*/
- if (EOF == set_element(bits, low, high, num1)) {
- unget_char(ch, file);
+ if (ch != '~') {
+ ch = get_number(, low, names, ch, file, ",-~
\t\n");
+ if (ch == EOF)
return (EOF);
- }
- return (ch);
- } else {
+ }
+
+ switch (ch) {
+ case '-':
/* eat the dash
*/
ch = get_char(file);
@@ -488,6 +484,37 @@ get_range(bitstr_t *bits, int low, int h
ch = get_number(, low, names, ch, file, "/, \t\n");
if (ch == EOF || num1 > num2)
return (EOF);
+ break;
+ case '~':
+ /* eat the tilde
+*/
+ ch = get_char(file);
+ if (ch == EOF)
+ return (EOF);
+
+ /* get the (optional) number following the tilde
+*/
+ ch = get_number(, low, names, ch, file, ", \t\n");
+ if (ch == EOF)
+ ch = get_char(file);
+ if (ch == EOF || num1 > num2) {
+ unget_char(ch, file);
+ return (EOF);
+ }
+
+ /* get a random number in the interval [num1, num2]
+*/
+
Re: switch powerpc to MI mplock
> Date: Fri, 10 Apr 2020 09:31:24 +0200
> From: Martin Pieuchot
>
> In order to reduce the differences with other architecture and to be able
> to use WITNESS on powerpc I'm proposing the diff below that makes use of
> the MI mp (ticket) lock implementation for powerpc.
>
> This has been tested by Peter J. Philipp but I'd like to have more tests
> before proceeding.
>
> As explained previously the pmap code, which is using a recursive
> spinlock to protect the hash, still uses the old lock implementation with
> this diff.
>
> Please fire your MP macppc and report back :o)
Hopefully we can get rid of the recursive pmap lock at some point.
But for now, ok kettenis@
> Index: arch/powerpc/include/mplock.h
> ===
> RCS file: /cvs/src/sys/arch/powerpc/include/mplock.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 mplock.h
> --- arch/powerpc/include/mplock.h 4 Dec 2017 09:51:03 - 1.3
> +++ arch/powerpc/include/mplock.h 9 Apr 2020 16:21:55 -
> @@ -27,25 +27,27 @@
> #ifndef _POWERPC_MPLOCK_H_
> #define _POWERPC_MPLOCK_H_
>
> +#define __USE_MI_MPLOCK
> +
> /*
> * Really simple spinlock implementation with recursive capabilities.
> * Correctness is paramount, no fancyness allowed.
> */
>
> -struct __mp_lock {
> +struct __ppc_lock {
> volatile struct cpu_info *mpl_cpu;
> volatile long mpl_count;
> };
>
> #ifndef _LOCORE
>
> -void __mp_lock_init(struct __mp_lock *);
> -void __mp_lock(struct __mp_lock *);
> -void __mp_unlock(struct __mp_lock *);
> -int __mp_release_all(struct __mp_lock *);
> -int __mp_release_all_but_one(struct __mp_lock *);
> -void __mp_acquire_count(struct __mp_lock *, int);
> -int __mp_lock_held(struct __mp_lock *, struct cpu_info *);
> +void __ppc_lock_init(struct __ppc_lock *);
> +void __ppc_lock(struct __ppc_lock *);
> +void __ppc_unlock(struct __ppc_lock *);
> +int __ppc_release_all(struct __ppc_lock *);
> +int __ppc_release_all_but_one(struct __ppc_lock *);
> +void __ppc_acquire_count(struct __ppc_lock *, int);
> +int __ppc_lock_held(struct __ppc_lock *, struct cpu_info *);
>
> #endif
>
> Index: arch/powerpc/powerpc/lock_machdep.c
> ===
> RCS file: /cvs/src/sys/arch/powerpc/powerpc/lock_machdep.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 lock_machdep.c
> --- arch/powerpc/powerpc/lock_machdep.c 5 Mar 2020 09:28:31 -
> 1.8
> +++ arch/powerpc/powerpc/lock_machdep.c 9 Apr 2020 16:21:01 -
> @@ -27,7 +27,7 @@
> #include
>
> void
> -__mp_lock_init(struct __mp_lock *lock)
> +__ppc_lock_init(struct __ppc_lock *lock)
> {
> lock->mpl_cpu = NULL;
> lock->mpl_count = 0;
> @@ -43,7 +43,7 @@ extern int __mp_lock_spinout;
> #endif
>
> static __inline void
> -__mp_lock_spin(struct __mp_lock *mpl)
> +__ppc_lock_spin(struct __ppc_lock *mpl)
> {
> #ifndef MP_LOCKDEBUG
> while (mpl->mpl_count != 0)
> @@ -55,14 +55,14 @@ __mp_lock_spin(struct __mp_lock *mpl)
> CPU_BUSY_CYCLE();
>
> if (nticks == 0) {
> - db_printf("__mp_lock(%p): lock spun out\n", mpl);
> + db_printf("__ppc_lock(%p): lock spun out\n", mpl);
> db_enter();
> }
> #endif
> }
>
> void
> -__mp_lock(struct __mp_lock *mpl)
> +__ppc_lock(struct __ppc_lock *mpl)
> {
> /*
>* Please notice that mpl_count gets incremented twice for the
> @@ -92,18 +92,18 @@ __mp_lock(struct __mp_lock *mpl)
> }
> ppc_intr_enable(s);
>
> - __mp_lock_spin(mpl);
> + __ppc_lock_spin(mpl);
> }
> }
>
> void
> -__mp_unlock(struct __mp_lock *mpl)
> +__ppc_unlock(struct __ppc_lock *mpl)
> {
> int s;
>
> #ifdef MP_LOCKDEBUG
> if (mpl->mpl_cpu != curcpu()) {
> - db_printf("__mp_unlock(%p): not held lock\n", mpl);
> + db_printf("__ppc_unlock(%p): not held lock\n", mpl);
> db_enter();
> }
> #endif
> @@ -118,14 +118,14 @@ __mp_unlock(struct __mp_lock *mpl)
> }
>
> int
> -__mp_release_all(struct __mp_lock *mpl)
> +__ppc_release_all(struct __ppc_lock *mpl)
> {
> int rv = mpl->mpl_count - 1;
> int s;
>
> #ifdef MP_LOCKDEBUG
> if (mpl->mpl_cpu != curcpu()) {
> - db_printf("__mp_release_all(%p): not held lock\n", mpl);
> + db_printf("__ppc_release_all(%p): not held lock\n", mpl);
> db_enter();
> }
> #endif
> @@ -140,13 +140,13 @@ __mp_release_all(struct __mp_lock *mpl)
> }
>
> int
> -__mp_release_all_but_one(struct __mp_lock *mpl)
> +__ppc_release_all_but_one(struct __ppc_lock *mpl)
> {
> int rv = mpl->mpl_count - 2;
>
> #ifdef MP_LOCKDEBUG
> if (mpl->mpl_cpu != curcpu()) {
> - db_printf("__mp_release_all_but_one(%p): not held lock\n", mpl);
> + db_printf("__ppc_release_all_but_one(%p): not held lock\n",
> mpl);
>
net80211: mira fix for MIMO
This change affects MIMO-capable drivers: iwm(4), iwx(4), and athn(4) Prevent MiRA from jumping from very high to very low rates while switching ratesets when probing. MiRA does multi-phase probing where it first probes within the current rateset (e.g. MCS 8-15 aka MIMO2) and then switches to another rateset (e.g. MCS 0-7 aka SISO) to see if that rateset contains an even better rate. When switching to another rateset, MiRA is supposed to begin probing within the new rateset at a rate which is a close equivalent to the "best" rate in the rateset it is switching away from. E.g. if MCS 15 has been determined as the best MIMO2 rate, we should start probing at MCS 7 when switching to the SISO rateset. But because of a bug we start at the lowest rate (MCS 0) in this case, which causes a noticable drop in throughput. Keeps average throughput closer to the ideal test case of running at a fixed high MIMO rate (provided that rate actually works best). diff 875a69c55949fd83d2e1a833af1fbc2174a52ee0 /usr/src (staged changes) blob - 29df16879e093b5774cfc010d9ad488e5a4af97e blob + fb2f7c3f9553b3c473f25597d0005bd43c928b15 --- sys/net80211/ieee80211_mira.c +++ sys/net80211/ieee80211_mira.c @@ -670,6 +670,9 @@ ieee80211_mira_probe_next_rateset(struct ieee80211_mir break; } } + /* If all rates are lower the maximum rate is the closest match. */ + if (i == rsnext->nrates) + ni->ni_txmcs = rsnext->max_mcs; /* Add rates from the next rateset as candidates. */ mn->candidate_rates |= (1 << ni->ni_txmcs);
Re: tcpdump: print nhrp packets
On Tue, Apr 14, 2020 at 01:49:32PM +1000, David Gwynne wrote: > > > > On 13 Apr 2020, at 19:03, Remi Locherer wrote: > > > > Hi, > > > > I recently looked into NHRP (RFC 2332) and noticed that our tcpdump does > > not have a printer for it. So I added support for NHRP to tcpdump. > > > > Initially I was surprised: I expected a simpler protocol! But it is from > > the 90's with all the protocols from then in mind (frame relay, ATM, ...). > > > > I tested with public available pcap files and compared the output with > > wirshark. > > https://packetlife.net/captures/protocol/nhrp/ > > https://www.networkingwithfish.com/fun-in-the-lab-sniffer-tracing-a-dmvpn-tunnel-startup/ > > > > The output looks like this: > > > > 08:34:45.647483 172.16.25.2 > 172.16.15.2: gre NHRP: reg request, id 7 [tos > > 0xc0] > > 08:34:45.671422 172.16.15.2 > 172.16.25.2: gre NHRP: reg reply, id 7 [tos > > 0xc0] > > > > 08:47:16.138679 172.16.15.2 > 172.16.25.2: gre NHRP: res request, id 6 [tos > > 0xc0] > > 08:47:16.148863 172.16.25.2 > 172.16.15.2: gre NHRP: res reply, id 6 [tos > > 0xc0] > > > > With -v set: > > > > 08:34:45.647483 172.16.25.2 > 172.16.15.2: gre [] 2001 NHRP: reg request, > > id 7, hopcnt 255, src nbma 172.16.25.2, 192.168.0.2 -> 192.168.0.1 (code 0, > > pl 255, mtu 1514, htime 7200, pref 0) [tos 0xc0] (ttl 254, id 22, len 116) > > 08:34:45.671422 172.16.15.2 > 172.16.25.2: gre [] 2001 NHRP: reg reply, id > > 7, hopcnt 255, src nbma 172.16.25.2, 192.168.0.2 -> 192.168.0.1 (code 0, pl > > 255, mtu 1514, htime 7200, pref 0) [tos 0xc0] (ttl 255, id 7, len 136) > > > > 08:47:16.138679 172.16.15.2 > 172.16.25.2: gre [] 2001 NHRP: res request, > > id 6, hopcnt 254, src nbma 172.16.45.2, 192.168.0.4 -> 192.168.0.2 (code 0, > > pl 0, mtu 1514, htime 7200, pref 0) [tos 0xc0] (ttl 254, id 20, len 116) > > 08:47:16.148863 172.16.25.2 > 172.16.15.2: gre [] 2001 NHRP: res reply, id > > 6, hopcnt 255, src nbma 172.16.45.2, 192.168.0.4 -> 192.168.0.2 (code 0, pl > > 32, mtu 1514, htime 7199, pref 0, nbma 172.16.25.2, proto 192.168.0.2) [tos > > 0xc0] (ttl 255, id 31, len 144) > > > > Extensions are not parsed and printed. > > > > It would be nice to get pcaps with expamles that use address or protocol > > combinations other than GRE and IPv4. > > > > Comments, OKs? > > Can you print the addresses when -v is not set too? > > Otherwise I'm keen. > Like this? tcpdump -n: 08:47:16.068855 172.16.25.2 > 172.16.15.2: gre NHRP: res request, id 8, src nbma 172.16.25.2, 192.168.0.2 -> 192.168.0.4 (code 0) [tos 0xc0] 08:47:16.150679 172.16.15.2 > 172.16.25.2: gre NHRP: res reply, id 8, src nbma 172.16.25.2, 192.168.0.2 -> 192.168.0.4 (code 0, nbma 172.16.45.2, proto 192.168.0.4) [tos 0xc0] tcpdump -nv: 08:47:16.068855 172.16.25.2 > 172.16.15.2: gre [] 2001 NHRP: res request, id 8, hopcnt 255, src nbma 172.16.25.2, 192.168.0.2 -> 192.168.0.4 (code 0, pl 0, mtu 1514, htime 7200, pref 0) [tos 0xc0] (ttl 255, id 29, len 96) 08:47:16.150679 172.16.15.2 > 172.16.25.2: gre [] 2001 NHRP: res reply, id 8, hopcnt 254, src nbma 172.16.25.2, 192.168.0.2 -> 192.168.0.4 (code 0, pl 32, mtu 1514, htime 7199, pref 0, nbma 172.16.45.2, proto 192.168.0.4) [tos 0xc0] (ttl 254, id 21, len 164) Index: Makefile === RCS file: /cvs/src/usr.sbin/tcpdump/Makefile,v retrieving revision 1.64 diff -u -p -r1.64 Makefile --- Makefile3 Dec 2019 01:43:33 - 1.64 +++ Makefile28 Mar 2020 17:07:22 - @@ -48,7 +48,7 @@ SRCS= tcpdump.c addrtoname.c privsep.c p print-bgp.c print-ospf6.c print-ripng.c print-rt6.c print-stp.c \ print-etherip.c print-lwres.c print-lldp.c print-cdp.c print-pflog.c \ print-pfsync.c pf_print_state.c print-ofp.c ofp_map.c \ - print-udpencap.c print-carp.c \ + print-udpencap.c print-carp.c print-nhrp.c \ print-802_11.c print-iapp.c print-mpls.c print-slow.c print-usbpcap.c \ gmt2local.c savestr.c setsignal.c in_cksum.c Index: interface.h === RCS file: /cvs/src/usr.sbin/tcpdump/interface.h,v retrieving revision 1.83 diff -u -p -r1.83 interface.h --- interface.h 3 Dec 2019 01:43:33 - 1.83 +++ interface.h 28 Mar 2020 17:07:22 - @@ -217,6 +217,7 @@ extern void ppp_ether_if_print(u_char *, extern void gre_print(const u_char *, u_int); extern void vxlan_print(const u_char *, u_int); extern void nsh_print(const u_char *, u_int); +extern void nhrp_print(const u_char *, u_int); extern void icmp_print(const u_char *, u_int, const u_char *); extern void ieee802_11_if_print(u_char *, const struct pcap_pkthdr *, const u_char *); Index: print-ether.c === RCS file: /cvs/src/usr.sbin/tcpdump/print-ether.c,v retrieving revision 1.37 diff -u -p -r1.37 print-ether.c --- print-ether.c 24 Jan 2020 22:46:36 - 1.37 +++
Re: Simplify NET_LOCK() variations
On 14/04/20(Tue) 10:47, Claudio Jeker wrote: > On Tue, Apr 14, 2020 at 10:08:54AM +0200, Martin Pieuchot wrote: > > Thanks for all the inputs, updated diff below. > > > > On 13/04/20(Mon) 03:20, Alexandr Nedvedicky wrote: > > > On Sun, Apr 12, 2020 at 07:02:43PM +0200, Mark Kettenis wrote: > > > > > From: "Theo de Raadt" > > > > > Date: Sun, 12 Apr 2020 10:28:59 -0600 > > > > > > > > > > > + if ((p->p_flag & P_SYSTEM) && > > > > > > + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0)) > > > > > > > > > > Wow that is ugly. > > > > > > > > A better approach might be to store a pointer to the softnet task's > > > > struct proc in a global variable and check that. That is what we do > > > > for the pagedaemon for example. > > > > Diff below implements that by introducing the in_taskq() function, any > > comment on that approach? > > > > > I'm not sure same thing would work for network task. Currently > > > there is a single instance of pagedaemon, however we hope to > > > have more network tasks running in parallel. > > > > Let's concentrate on the present. More work is required to have > > multiple network tasks running in parallel anyway ;) > > > > Diff below uses NET_RLOCK_IN_SOFTNET() with a corresponding KASSERT() > > and converts all the places where tasks are enqueued on the "softnet" > > taskq. > > > > Any other comment or concern? > > I'm very concerned about this. I think this is a complicated workaround > that will haunt us later on. I don't really understand what goes on there > and the names don't help me that much. It just makes this even more > confusing. Thanks for raising your concern. In November 2017 we introduced a reader version of the NET_LOCK(). The first goal was to allow ifconfig(8) to not block on each and every ioctl(2) it does. This, combined with the conversion of various ioctls to take a read lock improved the responsiveness of ifconfig(8) under load. This designed has been working since then (6.2-current to 6.6-current) and allowed us to make progress by auditing some parts of the ioctl(2) and later sysctl(2) paths and mark them as "read only". During this release cycle, and after 4 releases working with the above mentioned designed, a programming mistake has been made. This mistake has, in my opinion, nothing to do with design. However it made it clear that we do not understand what the current architecture is. So the goal of this renaming is to explicitly state what the current locks are for. > Shouldn't we fix the network stack so that it actually can use read locks > and with that multiple concurrent threads instead? Here the main problem > seems to be that solock needs to finally grow up to a real individual lock. Sure we can. This isn't something orthogonal to this diff. However throwing this diff away and going back to a single NET_LOCK() everywhere would, in a way, disregard the efforts we've put in auditing, refactoring and marking part of it as "read only". > Are there other issues with multiple softnet task that we know off? The one we know are easy: - pf(4) - pfsync(4) These one could be differed to a different context or limited to a single thread if pf(4) is ready: - ipsec(4) - socket delivery But honestly as long as the signal handling and select/poll/kqueue aren't KERNEL_LOCK()-free trying to use multiple threads in the network doesn't seem the best way forward to me. The fact that socket delivery is still somehow serialized might hide bugs. So going multi-threads before that seems like a way to make bugs harder to fix. On top of that, I'd also argue that the priority should be pushing the limits of the KERNEL_LOCK(). I don't see the point for OpenBSD, a general purpose OS, to have a fully parallel network stack if the rest of the kernel is still big locked. So, IMHO network hackers should better concentrate on: - unix sockets - routing sockets - interrupt handlers of !IPL_MPSAFE drivers - ARP - pfsync, pipex, etc. - physical ioctl path Is it clearer?
Re: Simplify NET_LOCK() variations
On Tue, Apr 14, 2020 at 10:08:54AM +0200, Martin Pieuchot wrote:
> Thanks for all the inputs, updated diff below.
>
> On 13/04/20(Mon) 03:20, Alexandr Nedvedicky wrote:
> > On Sun, Apr 12, 2020 at 07:02:43PM +0200, Mark Kettenis wrote:
> > > > From: "Theo de Raadt"
> > > > Date: Sun, 12 Apr 2020 10:28:59 -0600
> > > >
> > > > > + if ((p->p_flag & P_SYSTEM) &&
> > > > > + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))
> > > >
> > > > Wow that is ugly.
> > >
> > > A better approach might be to store a pointer to the softnet task's
> > > struct proc in a global variable and check that. That is what we do
> > > for the pagedaemon for example.
>
> Diff below implements that by introducing the in_taskq() function, any
> comment on that approach?
>
> > I'm not sure same thing would work for network task. Currently
> > there is a single instance of pagedaemon, however we hope to
> > have more network tasks running in parallel.
>
> Let's concentrate on the present. More work is required to have
> multiple network tasks running in parallel anyway ;)
>
> Diff below uses NET_RLOCK_IN_SOFTNET() with a corresponding KASSERT()
> and converts all the places where tasks are enqueued on the "softnet"
> taskq.
>
> Any other comment or concern?
I'm very concerned about this. I think this is a complicated workaround
that will haunt us later on. I don't really understand what goes on there
and the names don't help me that much. It just makes this even more
confusing.
Shouldn't we fix the network stack so that it actually can use read locks
and with that multiple concurrent threads instead? Here the main problem
seems to be that solock needs to finally grow up to a real individual lock.
Are there other issues with multiple softnet task that we know off?
> Index: kern/kern_task.c
> ===
> RCS file: /cvs/src/sys/kern/kern_task.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 kern_task.c
> --- kern/kern_task.c 19 Dec 2019 17:40:11 - 1.27
> +++ kern/kern_task.c 14 Apr 2020 07:49:38 -
> @@ -47,6 +47,7 @@ struct taskq {
> unsigned int tq_nthreads;
> unsigned int tq_flags;
> const char *tq_name;
> + struct proc **tq_threads;
>
> struct mutex tq_mtx;
> struct task_list tq_worklist;
> @@ -55,6 +56,7 @@ struct taskq {
> #endif
> };
>
> +struct proc *systqproc;
> static const char taskq_sys_name[] = "systq";
>
> struct taskq taskq_sys = {
> @@ -64,6 +66,7 @@ struct taskq taskq_sys = {
> 1,
> 0,
> taskq_sys_name,
> + ,
> MUTEX_INITIALIZER(IPL_HIGH),
> TAILQ_HEAD_INITIALIZER(taskq_sys.tq_worklist),
> #ifdef WITNESS
> @@ -74,6 +77,7 @@ struct taskq taskq_sys = {
> #endif
> };
>
> +struct proc *systqmpproc;
> static const char taskq_sys_mp_name[] = "systqmp";
>
> struct taskq taskq_sys_mp = {
> @@ -83,6 +87,7 @@ struct taskq taskq_sys_mp = {
> 1,
> TASKQ_MPSAFE,
> taskq_sys_mp_name,
> + ,
> MUTEX_INITIALIZER(IPL_HIGH),
> TAILQ_HEAD_INITIALIZER(taskq_sys_mp.tq_worklist),
> #ifdef WITNESS
> @@ -129,6 +134,8 @@ taskq_create(const char *name, unsigned
> tq->tq_nthreads = nthreads;
> tq->tq_name = name;
> tq->tq_flags = flags;
> + tq->tq_threads = mallocarray(nthreads, sizeof(*tq->tq_threads),
> + M_DEVBUF, M_WAITOK|M_ZERO);
>
> mtx_init_flags(>tq_mtx, ipl, name, 0);
> TAILQ_INIT(>tq_worklist);
> @@ -172,6 +179,8 @@ taskq_destroy(struct taskq *tq)
> }
> mtx_leave(>tq_mtx);
>
> + free(tq->tq_threads, M_DEVBUF,
> + tq->tq_nthreads * sizeof(*tq->tq_threads));
> free(tq, M_DEVBUF, sizeof(*tq));
> }
>
> @@ -186,6 +195,8 @@ taskq_create_thread(void *arg)
> switch (tq->tq_state) {
> case TQ_S_DESTROYED:
> mtx_leave(>tq_mtx);
> + free(tq->tq_threads, M_DEVBUF,
> + tq->tq_nthreads * sizeof(*tq->tq_threads));
> free(tq, M_DEVBUF, sizeof(*tq));
> return;
>
> @@ -356,7 +367,17 @@ taskq_thread(void *xtq)
> {
> struct taskq *tq = xtq;
> struct task work;
> - int last;
> + int last, i;
> +
> + mtx_enter(>tq_mtx);
> + for (i = 0; i < tq->tq_nthreads; i++) {
> + if (tq->tq_threads[i] == NULL) {
> + tq->tq_threads[i] = curproc;
> + break;
> + }
> + }
> + KASSERT(i < tq->tq_nthreads);
> + mtx_leave(>tq_mtx);
>
> if (ISSET(tq->tq_flags, TASKQ_MPSAFE))
> KERNEL_UNLOCK();
> @@ -381,4 +402,17 @@ taskq_thread(void *xtq)
> wakeup_one(>tq_running);
>
> kthread_exit(0);
> +}
> +
> +int
> +in_taskq(struct taskq *tq)
> +{
> + int i;
> +
> + for (i = 0; i < tq->tq_nthreads; i++) {
> + if (curproc == tq->tq_threads[i])
> +
Re: Simplify NET_LOCK() variations
Thanks for all the inputs, updated diff below.
On 13/04/20(Mon) 03:20, Alexandr Nedvedicky wrote:
> On Sun, Apr 12, 2020 at 07:02:43PM +0200, Mark Kettenis wrote:
> > > From: "Theo de Raadt"
> > > Date: Sun, 12 Apr 2020 10:28:59 -0600
> > >
> > > > + if ((p->p_flag & P_SYSTEM) &&
> > > > + (strncmp(p->p_p->ps_comm, "softnet", 7) == 0))
> > >
> > > Wow that is ugly.
> >
> > A better approach might be to store a pointer to the softnet task's
> > struct proc in a global variable and check that. That is what we do
> > for the pagedaemon for example.
Diff below implements that by introducing the in_taskq() function, any
comment on that approach?
> I'm not sure same thing would work for network task. Currently
> there is a single instance of pagedaemon, however we hope to
> have more network tasks running in parallel.
Let's concentrate on the present. More work is required to have
multiple network tasks running in parallel anyway ;)
Diff below uses NET_RLOCK_IN_SOFTNET() with a corresponding KASSERT()
and converts all the places where tasks are enqueued on the "softnet"
taskq.
Any other comment or concern?
Index: kern/kern_task.c
===
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.27
diff -u -p -r1.27 kern_task.c
--- kern/kern_task.c19 Dec 2019 17:40:11 - 1.27
+++ kern/kern_task.c14 Apr 2020 07:49:38 -
@@ -47,6 +47,7 @@ struct taskq {
unsigned int tq_nthreads;
unsigned int tq_flags;
const char *tq_name;
+ struct proc **tq_threads;
struct mutex tq_mtx;
struct task_list tq_worklist;
@@ -55,6 +56,7 @@ struct taskq {
#endif
};
+struct proc *systqproc;
static const char taskq_sys_name[] = "systq";
struct taskq taskq_sys = {
@@ -64,6 +66,7 @@ struct taskq taskq_sys = {
1,
0,
taskq_sys_name,
+ ,
MUTEX_INITIALIZER(IPL_HIGH),
TAILQ_HEAD_INITIALIZER(taskq_sys.tq_worklist),
#ifdef WITNESS
@@ -74,6 +77,7 @@ struct taskq taskq_sys = {
#endif
};
+struct proc *systqmpproc;
static const char taskq_sys_mp_name[] = "systqmp";
struct taskq taskq_sys_mp = {
@@ -83,6 +87,7 @@ struct taskq taskq_sys_mp = {
1,
TASKQ_MPSAFE,
taskq_sys_mp_name,
+ ,
MUTEX_INITIALIZER(IPL_HIGH),
TAILQ_HEAD_INITIALIZER(taskq_sys_mp.tq_worklist),
#ifdef WITNESS
@@ -129,6 +134,8 @@ taskq_create(const char *name, unsigned
tq->tq_nthreads = nthreads;
tq->tq_name = name;
tq->tq_flags = flags;
+ tq->tq_threads = mallocarray(nthreads, sizeof(*tq->tq_threads),
+ M_DEVBUF, M_WAITOK|M_ZERO);
mtx_init_flags(>tq_mtx, ipl, name, 0);
TAILQ_INIT(>tq_worklist);
@@ -172,6 +179,8 @@ taskq_destroy(struct taskq *tq)
}
mtx_leave(>tq_mtx);
+ free(tq->tq_threads, M_DEVBUF,
+ tq->tq_nthreads * sizeof(*tq->tq_threads));
free(tq, M_DEVBUF, sizeof(*tq));
}
@@ -186,6 +195,8 @@ taskq_create_thread(void *arg)
switch (tq->tq_state) {
case TQ_S_DESTROYED:
mtx_leave(>tq_mtx);
+ free(tq->tq_threads, M_DEVBUF,
+ tq->tq_nthreads * sizeof(*tq->tq_threads));
free(tq, M_DEVBUF, sizeof(*tq));
return;
@@ -356,7 +367,17 @@ taskq_thread(void *xtq)
{
struct taskq *tq = xtq;
struct task work;
- int last;
+ int last, i;
+
+ mtx_enter(>tq_mtx);
+ for (i = 0; i < tq->tq_nthreads; i++) {
+ if (tq->tq_threads[i] == NULL) {
+ tq->tq_threads[i] = curproc;
+ break;
+ }
+ }
+ KASSERT(i < tq->tq_nthreads);
+ mtx_leave(>tq_mtx);
if (ISSET(tq->tq_flags, TASKQ_MPSAFE))
KERNEL_UNLOCK();
@@ -381,4 +402,17 @@ taskq_thread(void *xtq)
wakeup_one(>tq_running);
kthread_exit(0);
+}
+
+int
+in_taskq(struct taskq *tq)
+{
+ int i;
+
+ for (i = 0; i < tq->tq_nthreads; i++) {
+ if (curproc == tq->tq_threads[i])
+ return (1);
+ }
+
+ return (0);
}
Index: net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.603
diff -u -p -r1.603 if.c
--- net/if.c12 Apr 2020 07:04:03 - 1.603
+++ net/if.c14 Apr 2020 07:34:49 -
@@ -250,6 +250,47 @@ struct task if_input_task_locked = TASK_
struct rwlock netlock = RWLOCK_INITIALIZER("netlock");
/*
+ * Reader version of NET_LOCK() to be used in "softnet" thread only.
+
+ * The "softnet" thread should be the only thread processing packets
+ * without holding an exclusive lock. This is done to allow read-only
+ * ioctl(2) to not block.
+ */
+void
+NET_RLOCK_IN_SOFTNET(void)
+{
+
