Re: bcmintc(4) diff for raspberry pi3
On 2021/05/22 12:06, Mark Kettenis wrote: > Can't find my raspberry pi3 right now. But here is a diff that avoids > spinning with interrupts disabled while trying to grab the kernel lock > for it. I'd appreciate it if somebody could give this a spin for me. > Just checking whether it works normally for a bit would be fine. Seems OK so far. booting sd0a:/bsd: 8935756+1804632+570032+831592 [648870+109+1086336+636102]=0xf99638 type 0x0 pa 0x0 va 0x0 pages 0x1 attr 0x8 type 0x7 pa 0x1000 va 0x1000 pages 0x1ff attr 0x8 type 0x2 pa 0x20 va 0x20 pages 0x4000 attr 0x8 type 0x7 pa 0x420 va 0x420 pages 0x3cf5 attr 0x8 type 0x9 pa 0x7ef5000 va 0x7ef5000 pages 0x16 attr 0x8 type 0x7 pa 0x7f0b000 va 0x7f0b000 pages 0x3119d attr 0x8 type 0x2 pa 0x390a8000 va 0x390a8000 pages 0xd5b attr 0x8 type 0x4 pa 0x39e03000 va 0x39e03000 pages 0x1 attr 0x8 type 0x2 pa 0x39e04000 va 0x39e04000 pages 0x3 attr 0x8 type 0x7 pa 0x39e07000 va 0x39e07000 pages 0x1 attr 0x8 type 0x2 pa 0x39e08000 va 0x39e08000 pages 0x100 attr 0x8 type 0x1 pa 0x39f08000 va 0x39f08000 pages 0x2a attr 0x8 type 0x0 pa 0x39f32000 va 0x39f32000 pages 0x7 attr 0x8 type 0x4 pa 0x39f39000 va 0x39f39000 pages 0x1 attr 0x8 type 0x6 pa 0x39f3a000 va 0x5144e2a000 pages 0x1 attr 0x8008 type 0x4 pa 0x39f3b000 va 0x39f3b000 pages 0x2 attr 0x8 type 0x0 pa 0x39f3d000 va 0x39f3d000 pages 0x1 attr 0x8 type 0x6 pa 0x39f3e000 va 0x5144e2e000 pages 0x3 attr 0x8008 type 0x4 pa 0x39f41000 va 0x39f41000 pages 0x1 attr 0x8 type 0x6 pa 0x39f42000 va 0x5144e32000 pages 0x4 attr 0x8008 type 0x0 pa 0x39f46000 va 0x39f46000 pages 0x1 attr 0x8 type 0x4 pa 0x39f47000 va 0x39f47000 pages 0x1 attr 0x8 type 0x0 pa 0x39f48000 va 0x39f48000 pages 0x1 attr 0x8 type 0x4 pa 0x39f49000 va 0x39f49000 pages 0x2 attr 0x8 type 0x0 pa 0x39f4b000 va 0x39f4b000 pages 0x1 attr 0x8 type 0x4 pa 0x39f4c000 va 0x39f4c000 pages 0x2 attr 0x8 type 0x2 pa 0x39f4e000 va 0x39f4e000 pages 0x1402 attr 0x8 type 0x5 pa 0x3b35 va 0x514624 pages 0x10 attr 0x8008 type 0x2 pa 0x3b36 va 0x3b36 pages 0xa0 attr 0x8 type 0xb pa 0x3f10 va 0x514625 pages 0x1 attr 0x8000 [ using 2372384 bytes of bsd ELF symbol table ] Copyright (c) 1982, 1986, 1989, 1991, 1993 The Regents of the University of California. All rights reserved. Copyright (c) 1995-2021 OpenBSD. All rights reserved. https://www.OpenBSD.org OpenBSD 6.9-current (GENERIC.MP) #3: Sat May 22 14:49:21 BST 2021 st...@mala.spacehopper.org:/sys/arch/arm64/compile/GENERIC.MP real mem = 956895232 (912MB) avail mem = 895025152 (853MB) random: good seed from bootblocks mainbus0 at root: Raspberry Pi 3 Model B Rev 1.2 cpu0 at mainbus0 mpidr 0: ARM Cortex-A53 r0p4 cpu0: 32KB 64b/line 2-way L1 VIPT I-cache, 32KB 64b/line 4-way L1 D-cache cpu0: 512KB 64b/line 16-way L2 cache cpu0: CRC32,ASID16 cpu1 at mainbus0 mpidr 1: ARM Cortex-A53 r0p4 cpu1: 32KB 64b/line 2-way L1 VIPT I-cache, 32KB 64b/line 4-way L1 D-cache cpu1: 512KB 64b/line 16-way L2 cache cpu1: CRC32,ASID16 cpu2 at mainbus0 mpidr 2: ARM Cortex-A53 r0p4 cpu2: 32KB 64b/line 2-way L1 VIPT I-cache, 32KB 64b/line 4-way L1 D-cache cpu2: 512KB 64b/line 16-way L2 cache cpu2: CRC32,ASID16 cpu3 at mainbus0 mpidr 3: ARM Cortex-A53 r0p4 cpu3: 32KB 64b/line 2-way L1 VIPT I-cache, 32KB 64b/line 4-way L1 D-cache cpu3: 512KB 64b/line 16-way L2 cache cpu3: CRC32,ASID16 efi0 at mainbus0: UEFI 2.8 efi0: Das U-Boot rev 0x20210400 apm0 at mainbus0 simplefb0 at mainbus0: 656x416, 32bpp wsdisplay0 at simplefb0 mux 1 wsdisplay0: screen 0-5 added (std, vt100 emulation) "system" at mainbus0 not configured "axi" at mainbus0 not configured simplebus0 at mainbus0: "soc" bcmclock0 at simplebus0 bcmmbox0 at simplebus0 bcmgpio0 at simplebus0 bcmaux0 at simplebus0 bcmdmac0 at simplebus0: DMA0 DMA2 DMA4 DMA5 DMA8 DMA9 DMA10 bcmintc0 at simplebus0 pluart0 at simplebus0: console bcmsdhost0 at simplebus0: 250 MHz base clock sdmmc0 at bcmsdhost0: 4-bit, sd high-speed, mmc high-speed, dma dwctwo0 at simplebus0 bcmdog0 at simplebus0 bcmrng0 at simplebus0 bcmtemp0 at simplebus0 "local_intc" at simplebus0 not configured sdhc0 at simplebus0 sdhc0: SDHC 3.0, 200 MHz base clock sdmmc1 at sdhc0: 4-bit, sd high-speed, mmc high-speed "firmware" at simplebus0 not configured "power" at simplebus0 not configured "mailbox" at simplebus0 not configured "gpiomem" at simplebus0 not configured "fb" at simplebus0 not configured "vcsm" at simplebus0 not configured "virtgpio" at simplebus0 not configured "clocks" at mainbus0 not configured "phy" at mainbus0 not configured "arm-pmu" at mainbus0 not configured agtimer0 at mainbus0: 19200 kHz "leds" at mainbus0 not configured "fixedregulator_3v3" at mainbus0 not configured "fixedregulator_5v0" at mainbus0 not configured "bootloader" at mainbus0 not configured dt: 443 probes usb0 at dwctwo0: USB revision 2.0 sdmmc0: can't enable card uhub0 at usb0 configuration 1 interface 0 "Broadcom DWC2 root hub" rev 2.00/1.00
Re: Extend history for getpagesize(3)
Why would anyone care about that stupid decision? This function is never going to be deleted from the namespace because it is critical, and the alternatives offered will continue to be less portable beyond the lifespan of the people who made that irrelevant and uneducated decision. But more to the point there is absolutely no reason to even hint a discouraging word at people from calling it, because if they need it and think they might not have it, they will tend to make worse decisions. Emil Engler wrote: > The man page for the obsolete function getpagesize(3) still lacks > information regarding its removal (and existance) in SUS. This diff > makes this more clear. > > Index: lib/libc/gen/getpagesize.3 > === > RCS file: /cvs/src/lib/libc/gen/getpagesize.3,v > retrieving revision 1.11 > diff -u -p -u -p -r1.11 getpagesize.3 > --- lib/libc/gen/getpagesize.3 5 Jun 2013 03:39:22 - 1.11 > +++ lib/libc/gen/getpagesize.3 22 May 2021 14:42:55 - > @@ -27,7 +27,7 @@ > .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > .\" SUCH DAMAGE. > .\" > -.Dd $Mdocdate: June 5 2013 $ > +.Dd $Mdocdate: May 22 2021 $ > .Dt GETPAGESIZE 3 > .Os > .Sh NAME > @@ -61,5 +61,9 @@ hardware page size. > .Sh HISTORY > The > .Fn getpagesize > -function call appeared in > +function call first appeared in > .Bx 4.2 . > +It was part of the > +.St -susv2 > +as a legacy feature and was removed in > +.St -susv3 . >
Extend history for getpagesize(3)
The man page for the obsolete function getpagesize(3) still lacks information regarding its removal (and existance) in SUS. This diff makes this more clear. Index: lib/libc/gen/getpagesize.3 === RCS file: /cvs/src/lib/libc/gen/getpagesize.3,v retrieving revision 1.11 diff -u -p -u -p -r1.11 getpagesize.3 --- lib/libc/gen/getpagesize.3 5 Jun 2013 03:39:22 - 1.11 +++ lib/libc/gen/getpagesize.3 22 May 2021 14:42:55 - @@ -27,7 +27,7 @@ .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF .\" SUCH DAMAGE. .\" -.Dd $Mdocdate: June 5 2013 $ +.Dd $Mdocdate: May 22 2021 $ .Dt GETPAGESIZE 3 .Os .Sh NAME @@ -61,5 +61,9 @@ hardware page size. .Sh HISTORY The .Fn getpagesize -function call appeared in +function call first appeared in .Bx 4.2 . +It was part of the +.St -susv2 +as a legacy feature and was removed in +.St -susv3 .
Re: amd64: softintr_dispatch: remove kernel lock
On Sat, May 22, 2021 at 02:33:47PM +0200, Mark Kettenis wrote: > > Date: Sat, 22 May 2021 11:11:38 + > > From: Visa Hankala > > > > On Wed, May 19, 2021 at 05:11:09PM -0500, Scott Cheloha wrote: > > > Hi, > > > > > > visa@ says I need to unlock softintr_dispatch() before I can > > > unlock softclock(), so let's do that. > > > > > > Additionally, when we call softintr_disestablish() we want to wait for > > > the underlying softintr handle to finish running if it is running. > > > > > > We can start with amd64. > > > > > > I think this approach will work: > > > > > > - Keep a pointer to the running softintr, if any, in the queue. NULL > > > the pointer when we return from sih_func(). > > > > > > - Take/release the kernel lock if the SI_MPSAFE flag is present when > > > we enter/leave sih_func(). > > > > > > - If the handle is running when you call softintr_disestablish(), spin > > > until the handle isn't running anymore and retry. > > > > > > There is no softintr manpage but I think it is understood that > > > softintr_disestablish() is only safe to call from a process context, > > > otherwise you may deadlock. Maybe we should do splassert(IPL_NONE)? > > > > > > We could probably sleep here instead of spinning. We'd have to change > > > every softintr_disestablish() implementation to do that, though. > > > Otherwise you'd have different behavior on different platforms. > > > > I think your diff does not pay enough attention to the fact that soft > > interrupts are handled by all CPUs. I think the diff that I posted > > a while ago [1] is better in that respect. > > > > Two biggest things that I do not like in my original diff are > > synchronization of handler execution, and use of the SMR barrier. > > > > [1] https://marc.info/?l=openbsd-tech=162092714911609 > > > > The kernel lock has guaranteed that at most one CPU is able to run > > a given soft interrupt handler at a time. My diff used a mutex to > > prevent concurrent execution. However, it is wasteful to spin. It would > > be more economical to let the current runner of the handler re-execute > > the code. > > > > The SMR barrier in softintr_disestablish() was a trick to drain any > > pending activity. However, it made me feel uneasy because I have not > > checked every caller of softintr_disestablish(). My main worry is not > > the latency but unexpected side effects. > > > > Below is a revised diff that improves the above two points. > > > > When a soft interrupt handler is scheduled, it is assigned to a CPU. > > That CPU will keep running the handler as long as there are pending > > requests. Once all pending requests have been drained, the CPU > > relinquishes its hold of the handler. This provides natural > > serialization. > > > > Now softintr_disestablish() uses spinning for draining activity. > > I still have slight qualms about this, though, because the feature > > has not been so explicit before. Integration with witness(4) might be > > in order. > > > > softintr_disestablish() uses READ_ONCE() to enforce reloading of the > > value in the busy-wait loop. This way the variable does not need to be > > volatile. (As yet another option, CPU_BUSY_CYCLE() could always > > imply memory clobbering, which should make an optimizing compiler > > redo the load.) For consistency with this READ_ONCE(), WRITE_ONCE() is > > used whenever the variable is written, excluding the initialization. > > > > The patch uses a single mutex for access serialization. The old code > > has used one mutex per each soft IPL level, but I am not sure how > > useful that has been. I think it would be better to have a separate > > mutex for each CPU. However, the increased code complexity might not > > be worthwhile at the moment. Even having the per-CPU queues has > > a whiff of being somewhat overkill. > > A few comments: > > * Looking at amd64 in isolation does not make sense. Like a lot of MD > code in OpenBSD the softintr code was copied from whatever > Net/FreeBSD had at the time, with no attempt at unification (it > works, check it in, don't go back to clean it up). However, with > powerpc64 and riscv64 we try to do things a little bit better in > that area. So arm64, powerpc64 and riscv64 share the same softintr > implementation that already implements softintr_establish_flags() > with SOFTINTR_ESTABLISH_MPSAFE. Now we haven't used that flag > anywhere in our tree yet, so the code might be completely busted. > But it may make a lot of sense to migrate other architectures to the > same codebase. I know that the implementations should be unified, or made machine independent as far as possible. I have focused on amd64's code to get the MP-safe processing working sooner on that platform - at the cost of increased code debt. > * The softintr_disestablish() function isn't used a lot in our tree. > It may make sense to postpone worrying about safely disestablishing > mpsafe soft interrupts for now and simply
vmd(8): skip inspecting non-udp packets on local ifs
tech@ & krw (since your code in question was imported to vmd),
I found strange behavior running tcpbench(1) to measure the connection
between a vmd guest and my host, as well as guest-to-guest. In short,
it's some bogus logic in how vmd tries to intercept dhcp/bootp on local
interfaces. Diff at the bottom addresses the issue, some background:
Running tcpbench(1) for ~20-30s on my machine, vmd (with -v debug
logging) barfs a bunch of lines like:
5 udp packets in 5 too long - dropped
The tcpbench(1) throughput stalls out at that point and reports 0 Mbps
avg bandwidth measurements.
If anyone wants to reproduce, use an OpenBSD guest and just run:
[host]$ tcpbench -s
[guest]$ tcpbench -t 180 100.64.x.2
Where 'x' is the appropriate value for your guest's local interface.
reyk@ imported packet.c from dhclient(8), but there's no validation that
the packet being inspected is an IP/UDP packet vs. IP/TCP, leading to
bogus logic related to inspecing UDP header attributes. In dhclient(8),
the decode_udp_ip_header function is used in a place where a bpf capture
buffer has already made sure it's a UDP packet (see sbin/dhclient/bpf.c).
In addition, there was a lot of stateful counting and checking we just
don't need in vmd(8), so I've ripped that out as well. It makes no sense
in this context.
OK?
Index: packet.c
===
RCS file: /cvs/src/usr.sbin/vmd/packet.c,v
retrieving revision 1.1
diff -u -p -r1.1 packet.c
--- packet.c19 Apr 2017 15:38:32 - 1.1
+++ packet.c22 May 2021 14:15:09 -
@@ -220,12 +220,6 @@ decode_udp_ip_header(unsigned char *buf,
unsigned char *data;
u_int32_t ip_len;
u_int32_t sum, usum;
- static unsigned int ip_packets_seen;
- static unsigned int ip_packets_bad_checksum;
- static unsigned int udp_packets_seen;
- static unsigned int udp_packets_bad_checksum;
- static unsigned int udp_packets_length_checked;
- static unsigned int udp_packets_length_overflow;
int len;
/* Assure that an entire IP header is within the buffer. */
@@ -236,17 +230,11 @@ decode_udp_ip_header(unsigned char *buf,
return (-1);
ip = (struct ip *)(buf + offset);
- ip_packets_seen++;
+ if (ip->ip_p != IPPROTO_UDP)
+ return (-1);
/* Check the IP header checksum - it should be zero. */
if (wrapsum(checksum(buf + offset, ip_len, 0)) != 0) {
- ip_packets_bad_checksum++;
- if (ip_packets_seen > 4 && ip_packets_bad_checksum != 0 &&
- (ip_packets_seen / ip_packets_bad_checksum) < 2) {
- log_info("%u bad IP checksums seen in %u packets",
- ip_packets_bad_checksum, ip_packets_seen);
- ip_packets_seen = ip_packets_bad_checksum = 0;
- }
return (-1);
}
@@ -274,7 +262,6 @@ decode_udp_ip_header(unsigned char *buf,
if (buflen < offset + ip_len + sizeof(*udp))
return (-1);
udp = (struct udphdr *)(buf + offset + ip_len);
- udp_packets_seen++;
/* Assure that the entire UDP packet is within the buffer. */
if (buflen < offset + ip_len + ntohs(udp->uh_ulen))
@@ -286,20 +273,8 @@ decode_udp_ip_header(unsigned char *buf,
* UDP header and the data. If the UDP checksum field is zero,
* we're not supposed to do a checksum.
*/
- udp_packets_length_checked++;
len = ntohs(udp->uh_ulen) - sizeof(*udp);
if ((len < 0) || (len + data > buf + buflen)) {
- udp_packets_length_overflow++;
- if (udp_packets_length_checked > 4 &&
- udp_packets_length_overflow != 0 &&
- (udp_packets_length_checked /
- udp_packets_length_overflow) < 2) {
- log_info("%u udp packets in %u too long - dropped",
- udp_packets_length_overflow,
- udp_packets_length_checked);
- udp_packets_length_overflow =
- udp_packets_length_checked = 0;
- }
return (-1);
}
if (len + data != buf + buflen)
@@ -313,15 +288,7 @@ decode_udp_ip_header(unsigned char *buf,
2 * sizeof(ip->ip_src),
IPPROTO_UDP + (u_int32_t)ntohs(udp->uh_ulen);
- udp_packets_seen++;
if (usum && usum != sum) {
- udp_packets_bad_checksum++;
- if (udp_packets_seen > 4 && udp_packets_bad_checksum != 0 &&
- (udp_packets_seen / udp_packets_bad_checksum) < 2) {
- log_info("%u bad udp checksums in %u packets",
- udp_packets_bad_checksum, udp_packets_seen);
- udp_packets_seen = udp_packets_bad_checksum = 0;
- }
return (-1);
smtp: more tls options
The new -T option in smtp(1) allows to plug more TLS parameters easily.
For completeness and consistency, this diff adds the following:
cafile=: override the default root certificates
nosni: disable SNI completely
noverify: synonym for -C that can be recycled
servername=: synonym for -S (undocumented) that can be recycled too
Eric.
Index: smtp.1
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp.1,v
retrieving revision 1.11
diff -u -p -r1.11 smtp.1
--- smtp.1 22 May 2021 12:16:06 - 1.11
+++ smtp.1 22 May 2021 12:49:17 -
@@ -53,6 +53,10 @@ This option requires a TLS or STARTTLS
.Ar server .
.It Fl C
Do not require server certificate to be valid.
+This flag is deprecated.
+Use
+.Dq Fl T No noverify
+instead.
.It Fl F Ar from
Set the return-path (MAIL FROM) for the SMTP transaction.
Default to the current username.
@@ -105,12 +109,21 @@ Refer to
.Xr tls_config_set_ciphers 3
for
.Ar value .
+.It noverify
+Do not require server certificate to be valid.
+.It nosni
+Disable Server Name Indication (SNI).
.It protocols Ns = Ns Ar value
Specify the protocols to use.
Refer to
.Xr tls_config_parse_protocols 3
for
.Ar value .
+.It servername Ns = Ns Ar value
+Use
+.Ar value
+for Server Name Indication (SNI).
+Defaults to the specified server hostname.
.El
.It Fl v
Be more verbose.
Index: smtpc.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v
retrieving revision 1.16
diff -u -p -r1.16 smtpc.c
--- smtpc.c 22 May 2021 09:09:07 - 1.16
+++ smtpc.c 22 May 2021 12:36:11 -
@@ -48,6 +48,8 @@ static struct smtp_mail mail;
static const char *servname = NULL;
static struct tls_config *tls_config;
+static int nosni = 0;
+static const char *cafile = NULL;
static const char *protocols = NULL;
static const char *ciphers = NULL;
@@ -65,25 +67,53 @@ static void
parse_tls_options(char *opt)
{
static char * const tokens[] = {
-#define CIPHERS 0
+#define CAFILE 0
+ "cafile",
+#define CIPHERS 1
"ciphers",
-#define PROTOCOLS 1
+#define NOSNI 2
+ "nosni",
+#define NOVERIFY 3
+ "noverify",
+#define PROTOCOLS 4
"protocols",
+#define SERVERNAME 5
+ "servername",
NULL };
char *value;
while (*opt) {
switch (getsubopt(, tokens, )) {
+ case CAFILE:
+ if (value == NULL)
+ fatalx("missing value for cafile");
+ cafile = value;
+ break;
case CIPHERS:
if (value == NULL)
fatalx("missing value for ciphers");
ciphers = value;
break;
+ case NOSNI:
+ if (value != NULL)
+ fatalx("no value expected for nosni");
+ nosni = 1;
+ break;
+ case NOVERIFY:
+ if (value != NULL)
+ fatalx("no value expected for noverify");
+ params.tls_verify = 0;
+ break;
case PROTOCOLS:
if (value == NULL)
fatalx("missing value for protocols");
protocols = value;
break;
+ case SERVERNAME:
+ if (value == NULL)
+ fatalx("missing value for servername");
+ servname = value;
+ break;
case -1:
if (suboptarg)
fatalx("invalid TLS option \"%s\"", suboptarg);
@@ -208,7 +238,10 @@ main(int argc, char **argv)
if (ciphers && tls_config_set_ciphers(tls_config, ciphers) == -1)
fatalx("tls_config_set_ciphers: %s",
tls_config_error(tls_config));
- if (tls_config_set_ca_file(tls_config, tls_default_ca_cert_file()) ==
-1)
+
+ if (cafile == NULL)
+ cafile = tls_default_ca_cert_file();
+ if (tls_config_set_ca_file(tls_config, cafile) == -1)
fatal("tls_set_ca_file");
if (!params.tls_verify) {
tls_config_insecure_noverifycert(tls_config);
@@ -332,7 +365,8 @@ parse_server(char *server)
if (servname == NULL)
servname = host;
- params.tls_servname = servname;
+ if (nosni == 0)
+ params.tls_servname = servname;
memset(, 0, sizeof(hints));
hints.ai_family = AF_UNSPEC;
Re: ACPI aml_rwgsb() fix
On Thu, May 20, 2021 at 12:19:37AM +0200, Mark Kettenis wrote: > My last change to dsdt.c broke one or two of my cheap little Intel > "Atom" laptops. Seems my interpretation of the ACPI standard wasn't > quite right. I went back to the original bug report and I think I > understand a bit better what the AML in that report is trying to do. > So here is a diff that fixes things. > > Theo, can you try this on that Dell Precision 3640? > > Tests on other hardware are welcome, especially on laptops. ok tb
Re: amd64: softintr_dispatch: remove kernel lock
Am Sat, May 22, 2021 at 02:33:47PM +0200 schrieb Mark Kettenis: > > Date: Sat, 22 May 2021 11:11:38 + > > From: Visa Hankala > > > > On Wed, May 19, 2021 at 05:11:09PM -0500, Scott Cheloha wrote: > > > Hi, > > > > > > visa@ says I need to unlock softintr_dispatch() before I can > > > unlock softclock(), so let's do that. > > > > > > Additionally, when we call softintr_disestablish() we want to wait for > > > the underlying softintr handle to finish running if it is running. > > > > > > We can start with amd64. > > > > > > I think this approach will work: > > > > > > - Keep a pointer to the running softintr, if any, in the queue. NULL > > > the pointer when we return from sih_func(). > > > > > > - Take/release the kernel lock if the SI_MPSAFE flag is present when > > > we enter/leave sih_func(). > > > > > > - If the handle is running when you call softintr_disestablish(), spin > > > until the handle isn't running anymore and retry. > > > > > > There is no softintr manpage but I think it is understood that > > > softintr_disestablish() is only safe to call from a process context, > > > otherwise you may deadlock. Maybe we should do splassert(IPL_NONE)? > > > > > > We could probably sleep here instead of spinning. We'd have to change > > > every softintr_disestablish() implementation to do that, though. > > > Otherwise you'd have different behavior on different platforms. > > > > I think your diff does not pay enough attention to the fact that soft > > interrupts are handled by all CPUs. I think the diff that I posted > > a while ago [1] is better in that respect. > > > > Two biggest things that I do not like in my original diff are > > synchronization of handler execution, and use of the SMR barrier. > > > > [1] https://marc.info/?l=openbsd-tech=162092714911609 > > > > The kernel lock has guaranteed that at most one CPU is able to run > > a given soft interrupt handler at a time. My diff used a mutex to > > prevent concurrent execution. However, it is wasteful to spin. It would > > be more economical to let the current runner of the handler re-execute > > the code. > > > > The SMR barrier in softintr_disestablish() was a trick to drain any > > pending activity. However, it made me feel uneasy because I have not > > checked every caller of softintr_disestablish(). My main worry is not > > the latency but unexpected side effects. > > > > Below is a revised diff that improves the above two points. > > > > When a soft interrupt handler is scheduled, it is assigned to a CPU. > > That CPU will keep running the handler as long as there are pending > > requests. Once all pending requests have been drained, the CPU > > relinquishes its hold of the handler. This provides natural > > serialization. > > > > Now softintr_disestablish() uses spinning for draining activity. > > I still have slight qualms about this, though, because the feature > > has not been so explicit before. Integration with witness(4) might be > > in order. > > > > softintr_disestablish() uses READ_ONCE() to enforce reloading of the > > value in the busy-wait loop. This way the variable does not need to be > > volatile. (As yet another option, CPU_BUSY_CYCLE() could always > > imply memory clobbering, which should make an optimizing compiler > > redo the load.) For consistency with this READ_ONCE(), WRITE_ONCE() is > > used whenever the variable is written, excluding the initialization. > > > > The patch uses a single mutex for access serialization. The old code > > has used one mutex per each soft IPL level, but I am not sure how > > useful that has been. I think it would be better to have a separate > > mutex for each CPU. However, the increased code complexity might not > > be worthwhile at the moment. Even having the per-CPU queues has > > a whiff of being somewhat overkill. > > A few comments: > > * Looking at amd64 in isolation does not make sense. Like a lot of MD > code in OpenBSD the softintr code was copied from whatever > Net/FreeBSD had at the time, with no attempt at unification (it > works, check it in, don't go back to clean it up). However, with > powerpc64 and riscv64 we try to do things a little bit better in > that area. So arm64, powerpc64 and riscv64 share the same softintr > implementation that already implements softintr_establish_flags() > with SOFTINTR_ESTABLISH_MPSAFE. Now we haven't used that flag > anywhere in our tree yet, so the code might be completely busted. > But it may make a lot of sense to migrate other architectures to the > same codebase. > > * The softintr_disestablish() function isn't used a lot in our tree. > It may make sense to postpone worrying about safely disestablishing > mpsafe soft interrupts for now and simply panic if someone tries to > do this. > > * Wouldn't it make sense for an mpsafe soft interrupt to protect > itself from running simultaniously on multiple CPUs? It probably > already needs some sort of lock to protect the
Re: amd64: softintr_dispatch: remove kernel lock
> Date: Sat, 22 May 2021 11:11:38 + > From: Visa Hankala > > On Wed, May 19, 2021 at 05:11:09PM -0500, Scott Cheloha wrote: > > Hi, > > > > visa@ says I need to unlock softintr_dispatch() before I can > > unlock softclock(), so let's do that. > > > > Additionally, when we call softintr_disestablish() we want to wait for > > the underlying softintr handle to finish running if it is running. > > > > We can start with amd64. > > > > I think this approach will work: > > > > - Keep a pointer to the running softintr, if any, in the queue. NULL > > the pointer when we return from sih_func(). > > > > - Take/release the kernel lock if the SI_MPSAFE flag is present when > > we enter/leave sih_func(). > > > > - If the handle is running when you call softintr_disestablish(), spin > > until the handle isn't running anymore and retry. > > > > There is no softintr manpage but I think it is understood that > > softintr_disestablish() is only safe to call from a process context, > > otherwise you may deadlock. Maybe we should do splassert(IPL_NONE)? > > > > We could probably sleep here instead of spinning. We'd have to change > > every softintr_disestablish() implementation to do that, though. > > Otherwise you'd have different behavior on different platforms. > > I think your diff does not pay enough attention to the fact that soft > interrupts are handled by all CPUs. I think the diff that I posted > a while ago [1] is better in that respect. > > Two biggest things that I do not like in my original diff are > synchronization of handler execution, and use of the SMR barrier. > > [1] https://marc.info/?l=openbsd-tech=162092714911609 > > The kernel lock has guaranteed that at most one CPU is able to run > a given soft interrupt handler at a time. My diff used a mutex to > prevent concurrent execution. However, it is wasteful to spin. It would > be more economical to let the current runner of the handler re-execute > the code. > > The SMR barrier in softintr_disestablish() was a trick to drain any > pending activity. However, it made me feel uneasy because I have not > checked every caller of softintr_disestablish(). My main worry is not > the latency but unexpected side effects. > > Below is a revised diff that improves the above two points. > > When a soft interrupt handler is scheduled, it is assigned to a CPU. > That CPU will keep running the handler as long as there are pending > requests. Once all pending requests have been drained, the CPU > relinquishes its hold of the handler. This provides natural > serialization. > > Now softintr_disestablish() uses spinning for draining activity. > I still have slight qualms about this, though, because the feature > has not been so explicit before. Integration with witness(4) might be > in order. > > softintr_disestablish() uses READ_ONCE() to enforce reloading of the > value in the busy-wait loop. This way the variable does not need to be > volatile. (As yet another option, CPU_BUSY_CYCLE() could always > imply memory clobbering, which should make an optimizing compiler > redo the load.) For consistency with this READ_ONCE(), WRITE_ONCE() is > used whenever the variable is written, excluding the initialization. > > The patch uses a single mutex for access serialization. The old code > has used one mutex per each soft IPL level, but I am not sure how > useful that has been. I think it would be better to have a separate > mutex for each CPU. However, the increased code complexity might not > be worthwhile at the moment. Even having the per-CPU queues has > a whiff of being somewhat overkill. A few comments: * Looking at amd64 in isolation does not make sense. Like a lot of MD code in OpenBSD the softintr code was copied from whatever Net/FreeBSD had at the time, with no attempt at unification (it works, check it in, don't go back to clean it up). However, with powerpc64 and riscv64 we try to do things a little bit better in that area. So arm64, powerpc64 and riscv64 share the same softintr implementation that already implements softintr_establish_flags() with SOFTINTR_ESTABLISH_MPSAFE. Now we haven't used that flag anywhere in our tree yet, so the code might be completely busted. But it may make a lot of sense to migrate other architectures to the same codebase. * The softintr_disestablish() function isn't used a lot in our tree. It may make sense to postpone worrying about safely disestablishing mpsafe soft interrupts for now and simply panic if someone tries to do this. * Wouldn't it make sense for an mpsafe soft interrupt to protect itself from running simultaniously on multiple CPUs? It probably already needs some sort of lock to protect the handler and other code running in process context on other CPUs. And some handlers may be safe to run simultaniously anyway. * I think we should avoid MAXCPU arrays if we can; adding stuff to struct cpu_info is probably a better approach here. Cheers, Mark
Re: amd64: softintr_dispatch: remove kernel lock
On Wed, May 19, 2021 at 05:11:09PM -0500, Scott Cheloha wrote:
> Hi,
>
> visa@ says I need to unlock softintr_dispatch() before I can
> unlock softclock(), so let's do that.
>
> Additionally, when we call softintr_disestablish() we want to wait for
> the underlying softintr handle to finish running if it is running.
>
> We can start with amd64.
>
> I think this approach will work:
>
> - Keep a pointer to the running softintr, if any, in the queue. NULL
> the pointer when we return from sih_func().
>
> - Take/release the kernel lock if the SI_MPSAFE flag is present when
> we enter/leave sih_func().
>
> - If the handle is running when you call softintr_disestablish(), spin
> until the handle isn't running anymore and retry.
>
> There is no softintr manpage but I think it is understood that
> softintr_disestablish() is only safe to call from a process context,
> otherwise you may deadlock. Maybe we should do splassert(IPL_NONE)?
>
> We could probably sleep here instead of spinning. We'd have to change
> every softintr_disestablish() implementation to do that, though.
> Otherwise you'd have different behavior on different platforms.
I think your diff does not pay enough attention to the fact that soft
interrupts are handled by all CPUs. I think the diff that I posted
a while ago [1] is better in that respect.
Two biggest things that I do not like in my original diff are
synchronization of handler execution, and use of the SMR barrier.
[1] https://marc.info/?l=openbsd-tech=162092714911609
The kernel lock has guaranteed that at most one CPU is able to run
a given soft interrupt handler at a time. My diff used a mutex to
prevent concurrent execution. However, it is wasteful to spin. It would
be more economical to let the current runner of the handler re-execute
the code.
The SMR barrier in softintr_disestablish() was a trick to drain any
pending activity. However, it made me feel uneasy because I have not
checked every caller of softintr_disestablish(). My main worry is not
the latency but unexpected side effects.
Below is a revised diff that improves the above two points.
When a soft interrupt handler is scheduled, it is assigned to a CPU.
That CPU will keep running the handler as long as there are pending
requests. Once all pending requests have been drained, the CPU
relinquishes its hold of the handler. This provides natural
serialization.
Now softintr_disestablish() uses spinning for draining activity.
I still have slight qualms about this, though, because the feature
has not been so explicit before. Integration with witness(4) might be
in order.
softintr_disestablish() uses READ_ONCE() to enforce reloading of the
value in the busy-wait loop. This way the variable does not need to be
volatile. (As yet another option, CPU_BUSY_CYCLE() could always
imply memory clobbering, which should make an optimizing compiler
redo the load.) For consistency with this READ_ONCE(), WRITE_ONCE() is
used whenever the variable is written, excluding the initialization.
The patch uses a single mutex for access serialization. The old code
has used one mutex per each soft IPL level, but I am not sure how
useful that has been. I think it would be better to have a separate
mutex for each CPU. However, the increased code complexity might not
be worthwhile at the moment. Even having the per-CPU queues has
a whiff of being somewhat overkill.
Index: arch/amd64/amd64/softintr.c
===
RCS file: src/sys/arch/amd64/amd64/softintr.c,v
retrieving revision 1.10
diff -u -p -r1.10 softintr.c
--- arch/amd64/amd64/softintr.c 11 Sep 2020 09:27:09 - 1.10
+++ arch/amd64/amd64/softintr.c 22 May 2021 11:07:23 -
@@ -36,13 +36,37 @@
#include
#include
+#include
#include
+#include
+#include
+#include
#include
#include
-struct x86_soft_intr x86_soft_intrs[X86_NSOFTINTR];
+struct x86_soft_intr_percpu;
+
+struct x86_soft_intrhand {
+ TAILQ_ENTRY(x86_soft_intrhand) sih_q;
+ void(*sih_fn)(void *);
+ void*sih_arg;
+ struct x86_soft_intr_percpu *sih_owner;
+ int sih_which;
+ unsigned short sih_flags;
+ unsigned short sih_pending;
+};
+
+#define SIF_DYING 0x0001
+#define SIF_MPSAFE 0x0002
+
+struct x86_soft_intr_percpu {
+ TAILQ_HEAD(, x86_soft_intrhand) sip_queue[X86_NSOFTINTR];
+} __aligned(CACHELINESIZE);
+
+struct x86_soft_intr_percpu x86_soft_intr_percpu[MAXCPUS];
+struct mutex softintr_lock = MUTEX_INITIALIZER(IPL_HIGH);
const int x86_soft_intr_to_ssir[X86_NSOFTINTR] = {
SIR_CLOCK,
@@ -58,14 +82,13 @@ const int x86_soft_intr_to_ssir[X86_NSOF
void
softintr_init(void)
{
- struct x86_soft_intr *si;
- int i;
+ struct x86_soft_intr_percpu *sip;
+ int i, j;
- for (i = 0; i < X86_NSOFTINTR; i++) {
-
bcmintc(4) diff for raspberry pi3
Can't find my raspberry pi3 right now. But here is a diff that avoids
spinning with interrupts disabled while trying to grab the kernel lock
for it. I'd appreciate it if somebody could give this a spin for me.
Just checking whether it works normally for a bit would be fine.
Thanks,
Mark
Index: arch/arm64/dev/bcm2836_intr.c
===
RCS file: /cvs/src/sys/arch/arm64/dev/bcm2836_intr.c,v
retrieving revision 1.11
diff -u -p -r1.11 bcm2836_intr.c
--- arch/arm64/dev/bcm2836_intr.c 15 May 2021 11:30:27 - 1.11
+++ arch/arm64/dev/bcm2836_intr.c 22 May 2021 10:03:37 -
@@ -96,8 +96,8 @@ struct intrsource {
struct bcm_intc_softc {
struct devicesc_dev;
- struct intrsourcesc_bcm_intc_handler[INTC_NIRQ];
- uint32_t sc_bcm_intc_imask[INTC_NBANK][NIPL];
+ struct intrsourcesc_handler[INTC_NIRQ];
+ uint32_t sc_imask[INTC_NBANK][NIPL];
int32_t sc_localcoremask[MAXCPUS];
bus_space_tag_t sc_iot;
bus_space_handle_t sc_ioh;
@@ -115,11 +115,11 @@ intbcm_intc_splraise(int new);
voidbcm_intc_setipl(int new);
voidbcm_intc_calc_mask(void);
void *bcm_intc_intr_establish(int, int, struct cpu_info *,
-int (*)(void *), void *, char *);
+ int (*)(void *), void *, char *);
void *bcm_intc_intr_establish_fdt(void *, int *, int, struct cpu_info *,
-int (*)(void *), void *, char *);
+ int (*)(void *), void *, char *);
void *l1_intc_intr_establish_fdt(void *, int *, int, struct cpu_info *,
-int (*)(void *), void *, char *);
+ int (*)(void *), void *, char *);
voidbcm_intc_intr_disestablish(void *);
voidbcm_intc_irq_handler(void *);
voidbcm_intc_intr_route(void *, int , struct cpu_info *);
@@ -204,7 +204,7 @@ bcm_intc_attach(struct device *parent, s
ARM_LOCAL_INT_MAILBOX(i), 0);
for (i = 0; i < INTC_NIRQ; i++) {
- TAILQ_INIT(>sc_bcm_intc_handler[i].is_list);
+ TAILQ_INIT(>sc_handler[i].is_list);
}
bcm_intc_calc_mask();
@@ -239,13 +239,13 @@ bcm_intc_intr_enable(int irq, int ipl)
struct bcm_intc_softc *sc = bcm_intc;
if (IS_IRQ_BANK0(irq))
- sc->sc_bcm_intc_imask[0][ipl] |= (1 << IRQ_BANK0(irq));
+ sc->sc_imask[0][ipl] |= (1 << IRQ_BANK0(irq));
else if (IS_IRQ_BANK1(irq))
- sc->sc_bcm_intc_imask[1][ipl] |= (1 << IRQ_BANK1(irq));
+ sc->sc_imask[1][ipl] |= (1 << IRQ_BANK1(irq));
else if (IS_IRQ_BANK2(irq))
- sc->sc_bcm_intc_imask[2][ipl] |= (1 << IRQ_BANK2(irq));
+ sc->sc_imask[2][ipl] |= (1 << IRQ_BANK2(irq));
else if (IS_IRQ_LOCAL(irq))
- sc->sc_bcm_intc_imask[3][ipl] |= (1 << IRQ_LOCAL(irq));
+ sc->sc_imask[3][ipl] |= (1 << IRQ_LOCAL(irq));
else
printf("%s: invalid irq number: %d\n", __func__, irq);
}
@@ -256,13 +256,13 @@ bcm_intc_intr_disable(int irq, int ipl)
struct bcm_intc_softc *sc = bcm_intc;
if (IS_IRQ_BANK0(irq))
- sc->sc_bcm_intc_imask[0][ipl] &= ~(1 << IRQ_BANK0(irq));
+ sc->sc_imask[0][ipl] &= ~(1 << IRQ_BANK0(irq));
else if (IS_IRQ_BANK1(irq))
- sc->sc_bcm_intc_imask[1][ipl] &= ~(1 << IRQ_BANK1(irq));
+ sc->sc_imask[1][ipl] &= ~(1 << IRQ_BANK1(irq));
else if (IS_IRQ_BANK2(irq))
- sc->sc_bcm_intc_imask[2][ipl] &= ~(1 << IRQ_BANK2(irq));
+ sc->sc_imask[2][ipl] &= ~(1 << IRQ_BANK2(irq));
else if (IS_IRQ_LOCAL(irq))
- sc->sc_bcm_intc_imask[3][ipl] &= ~(1 << IRQ_LOCAL(irq));
+ sc->sc_imask[3][ipl] &= ~(1 << IRQ_LOCAL(irq));
else
printf("%s: invalid irq number: %d\n", __func__, irq);
}
@@ -279,8 +279,7 @@ bcm_intc_calc_mask(void)
for (irq = 0; irq < INTC_NIRQ; irq++) {
int max = IPL_NONE;
int min = IPL_HIGH;
- TAILQ_FOREACH(ih, >sc_bcm_intc_handler[irq].is_list,
- ih_list) {
+ TAILQ_FOREACH(ih, >sc_handler[irq].is_list, ih_list) {
if (ih->ih_ipl > max)
max = ih->ih_ipl;
@@ -288,7 +287,7 @@ bcm_intc_calc_mask(void)
min = ih->ih_ipl;
}
- sc->sc_bcm_intc_handler[irq].is_irq = max;
+ sc->sc_handler[irq].is_irq = max;
if (max == IPL_NONE)
min = IPL_NONE;
@@ -369,16 +368,16 @@ bcm_intc_setipl(int new)
bus_space_write_4(sc->sc_iot, sc->sc_ioh, INTC_DISABLE_BANK2,
0x);
bus_space_write_4(sc->sc_iot, sc->sc_ioh, INTC_ENABLE_BANK0,
- sc->sc_bcm_intc_imask[0][new]);
vmd: Fix grammar for random lladdr
Hi
The grammar for lladdr of interfaces is according to the manpage:
[locked] lladdr [etheraddr]
This implies that `locked lladdr' is OK but looking at parse.y this
does not seem to be the case. Making it optional would lead to a
`lladdr' all by itself being valid, which I find weird. So I copied
the way ifconfig does it and now the syntax is:
[locked] lladdr etheraddr|random
so to have a random locked lladdr one would have to write
locked lladdr random
Is this a good approach?
Best,
Martin
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/vmd/parse.y,v
retrieving revision 1.56
diff -u -p -r1.56 parse.y
--- parse.y 23 Sep 2020 19:18:18 - 1.56
+++ parse.y 22 May 2021 07:55:18 -
@@ -685,14 +685,16 @@ string: STRING string {
lladdr : STRING{
struct ether_addr *ea;
- if ((ea = ether_aton($1)) == NULL) {
+ if (strcmp($1, "random") == 0) {
+ memset($$, 0, ETHER_ADDR_LEN);
+ } else if ((ea = ether_aton($1)) != NULL) {
+ memcpy($$, ea, ETHER_ADDR_LEN);
+ } else {
yyerror("invalid address: %s\n", $1);
free($1);
YYERROR;
}
free($1);
-
- memcpy($$, ea, ETHER_ADDR_LEN);
}
;
Index: vm.conf.5
===
RCS file: /cvs/src/usr.sbin/vmd/vm.conf.5,v
retrieving revision 1.56
diff -u -p -r1.56 vm.conf.5
--- vm.conf.5 1 Mar 2021 14:27:44 - 1.56
+++ vm.conf.5 22 May 2021 07:55:18 -
@@ -237,10 +237,12 @@ The
must not be longer than 15 characters or end with a digit,
as described in
.Xr ifconfig 8 .
-.It Oo Cm locked Oc Cm lladdr Op Ar etheraddr
+.It Oo Cm locked Oc Cm lladdr Ar etheraddr Ns | Ns Cm random
Change the link layer address (MAC address) of the interface on the
VM guest side.
-If not specified, a randomized address will be assigned by
+If
+.Cm random
+is specified, a randomized address will be assigned by
.Xr vmd 8 .
If the
.Cm locked
