[RFC] acpi: add acpitimer_delay(), acpihpet_delay()
Hi,
In the future when the LAPIC timer is run in oneshot mode there will
be no lapic_delay().
This is fine if you have a constant TSC, because we have tsc_delay().
This is *very* bad for older amd64 machines, because you are left with
i8254_delay().
I would like to offer a less awful delay(9) implementation for this
class of hardware. Otherwise we may trip over bizarre phantom bugs on
MP kernels because only one CPU can read the i8254 at a time.
I think patrick@ was struggling with some version of that problem last
year, but in a VM.
Real i386 hardware should be fine. Later models with an ACPI PM timer
will be fine using acpitimer_delay() instead of i8254_delay().
If this seems reasonable to people I will come back with a cleaned up
patch for testing.
Thoughts? Preferences?
-Scott
Here are the sample measurements from my 2017 laptop (kaby lake
refresh) running the attached patch. It takes longer than a
microsecond to read either of the ACPI timers. The PM timer is better
than the HPET. The HPET is a bit better than the i8254. I hope the
numbers are a little better on older hardware.
acpitimer_test_delay: expected 0.01000 actual 0.10638 error
0.09638
acpitimer_test_delay: expected 0.1 actual 0.15464 error
0.05464
acpitimer_test_delay: expected 0.00010 actual 0.000107619 error
0.07619
acpitimer_test_delay: expected 0.00100 actual 0.001007275 error
0.07275
acpitimer_test_delay: expected 0.01000 actual 0.010007891 error
0.07891
acpihpet_test_delay: expected 0.01000 actual 0.22208 error
0.21208
acpihpet_test_delay: expected 0.1 actual 0.31690 error
0.21690
acpihpet_test_delay: expected 0.00010 actual 0.000112647 error
0.12647
acpihpet_test_delay: expected 0.00100 actual 0.001021480 error
0.21480
acpihpet_test_delay: expected 0.01000 actual 0.010013736 error
0.13736
i8254_test_delay: expected 0.01000 actual 0.40110 error
0.39110
i8254_test_delay: expected 0.1 actual 0.39471 error
0.29471
i8254_test_delay: expected 0.00010 actual 0.000128031 error
0.28031
i8254_test_delay: expected 0.00100 actual 0.001024586 error
0.24586
i8254_test_delay: expected 0.01000 actual 0.010021859 error
0.21859
Index: dev/acpi/acpihpet.c
===
RCS file: /cvs/src/sys/dev/acpi/acpihpet.c,v
retrieving revision 1.26
diff -u -p -r1.26 acpihpet.c
--- dev/acpi/acpihpet.c 6 Apr 2022 18:59:27 - 1.26
+++ dev/acpi/acpihpet.c 15 Aug 2022 04:21:58 -
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
@@ -31,8 +32,9 @@ int acpihpet_attached;
int acpihpet_match(struct device *, void *, void *);
void acpihpet_attach(struct device *, struct device *, void *);
int acpihpet_activate(struct device *, int);
-
+void acpiphet_delay(u_int);
u_int acpihpet_gettime(struct timecounter *tc);
+void acpihpet_test_delay(u_int);
uint64_t acpihpet_r(bus_space_tag_t _iot, bus_space_handle_t _ioh,
bus_size_t _ioa);
@@ -262,7 +264,7 @@ acpihpet_attach(struct device *parent, s
freq = 1000ull / period;
printf(": %lld Hz\n", freq);
- hpet_timecounter.tc_frequency = (uint32_t)freq;
+ hpet_timecounter.tc_frequency = freq;
hpet_timecounter.tc_priv = sc;
hpet_timecounter.tc_name = sc->sc_dev.dv_xname;
tc_init(_timecounter);
@@ -273,10 +275,43 @@ acpihpet_attach(struct device *parent, s
acpihpet_attached++;
}
+void
+acpihpet_delay(u_int usecs)
+{
+ uint64_t d, s;
+ struct acpihpet_softc *sc = hpet_timecounter.tc_priv;
+
+ d = usecs * hpet_timecounter.tc_frequency / 100;
+ s = acpihpet_r(sc->sc_iot, sc->sc_ioh, HPET_MAIN_COUNTER);
+ while (acpihpet_r(sc->sc_iot, sc->sc_ioh, HPET_MAIN_COUNTER) - s < d)
+ CPU_BUSY_CYCLE();
+}
+
u_int
acpihpet_gettime(struct timecounter *tc)
{
struct acpihpet_softc *sc = tc->tc_priv;
return (bus_space_read_4(sc->sc_iot, sc->sc_ioh, HPET_MAIN_COUNTER));
+}
+
+void
+acpihpet_test_delay(u_int usecs)
+{
+ struct timespec ac, er, ex, t0, t1;
+
+ if (!acpihpet_attached) {
+ printf("%s: (no hpet attached)\n", __func__);
+ return;
+ }
+
+ nanouptime();
+ acpihpet_delay(usecs);
+ nanouptime();
+ timespecsub(, , );
+ NSEC_TO_TIMESPEC(usecs * 1000ULL, );
+ timespecsub(, , );
+ printf("%s: expected %lld.%09ld actual %lld.%09ld error %lld.%09ld\n",
+ __func__, ex.tv_sec, ex.tv_nsec, ac.tv_sec, ac.tv_nsec,
+ er.tv_sec, er.tv_nsec);
}
Index: dev/acpi/acpitimer.c
===
RCS file:
httpd: plug fd/memory leaks in error paths
I was looking for something different when I stumbled upon this.
- in server_file_index `escapedpath' is leaked if evbuffer_add_printf
fails, similarly the directory entries in the loop. `escapeduri'
is also leaked if escape_html fails.
- read_errdoc leaks a file descriptor if fstat fails or if the file
is empty.
both very unlikely but worth fixing IMHO.
For server_file_index I decided to just drop the `skip' variable and
just goto fail on errors. There shouldn't be any visible change from
it, except that we don't try to serve half page if we somehow exhaust
memory halfway through.
ok?
Index: server_file.c
===
RCS file: /home/cvs/src/usr.sbin/httpd/server_file.c,v
retrieving revision 1.74
diff -u -p -r1.74 server_file.c
--- server_file.c 4 Mar 2022 01:46:07 - 1.74
+++ server_file.c 14 Aug 2022 21:09:25 -
@@ -474,7 +474,7 @@ server_file_index(struct httpd *env, str
struct http_descriptor *desc = clt->clt_descreq;
struct server_config *srv_conf = clt->clt_srv_conf;
struct dirent **namelist, *dp;
- int namesize, i, ret, fd = -1, namewidth, skip;
+ int namesize = 0, i = 0, ret, fd = -1, namewidth;
int code = 500;
struct evbuffer *evb = NULL;
struct media_type*media;
@@ -510,9 +510,6 @@ server_file_index(struct httpd *env, str
if ((namesize = scandir(path, , NULL, alphasort)) == -1)
goto abort;
- /* Indicate failure but continue going through the list */
- skip = 0;
-
if ((escapedpath = escape_html(desc->http_path)) == NULL)
goto fail;
@@ -535,8 +532,10 @@ server_file_index(struct httpd *env, str
"\n"
"Index of %s\n"
"\n\n",
- escapedpath, style, escapedpath) == -1)
- skip = 1;
+ escapedpath, style, escapedpath) == -1) {
+ free(escapedpath);
+ goto fail;
+ }
free(escapedpath);
@@ -545,8 +544,7 @@ server_file_index(struct httpd *env, str
dp = namelist[i];
- if (skip ||
- fstatat(fd, dp->d_name, , 0) == -1) {
+ if (fstatat(fd, dp->d_name, , 0) == -1) {
free(dp);
continue;
}
@@ -558,8 +556,10 @@ server_file_index(struct httpd *env, str
if ((escapeduri = url_encode(dp->d_name)) == NULL)
goto fail;
- if ((escapedhtml = escape_html(dp->d_name)) == NULL)
+ if ((escapedhtml = escape_html(dp->d_name)) == NULL) {
+ free(escapeduri);
goto fail;
+ }
if (dp->d_name[0] == '.' &&
!(dp->d_name[1] == '.' && dp->d_name[2] == '\0')) {
@@ -571,7 +571,7 @@ server_file_index(struct httpd *env, str
strchr(escapeduri, ':') != NULL ? "./" : "",
escapeduri, escapedhtml,
MAXIMUM(namewidth, 0), " ", tmstr, "-") == -1)
- skip = 1;
+ goto fail;
} else if (S_ISREG(subst.st_mode)) {
if (evbuffer_add_printf(evb,
"%s%*s%s%20llu\n",
@@ -579,16 +579,16 @@ server_file_index(struct httpd *env, str
escapeduri, escapedhtml,
MAXIMUM(namewidth, 0), " ",
tmstr, subst.st_size) == -1)
- skip = 1;
+ goto fail;
}
free(escapeduri);
free(escapedhtml);
free(dp);
}
free(namelist);
+ namelist = NULL;
- if (skip ||
- evbuffer_add_printf(evb,
+ if (evbuffer_add_printf(evb,
"\n\n\n\n") == -1)
goto abort;
@@ -629,6 +629,9 @@ server_file_index(struct httpd *env, str
bufferevent_free(clt->clt_bev);
clt->clt_bev = NULL;
abort:
+ for (; i < namesize; i++)
+ free(namelist[i]);
+ free(namelist);
if (fd != -1)
close(fd);
if (evb != NULL)
Index: server_http.c
===
RCS file: /home/cvs/src/usr.sbin/httpd/server_http.c,v
retrieving revision 1.150
diff -u -p -r1.150 server_http.c
--- server_http.c 2 Mar 2022 11:10:43 - 1.150
+++ server_http.c 14 Aug 2022 18:42:33 -
@@ -1777,14 +1777,13 @@ read_errdoc(const char *root, const char
free(path);
if (fstat(fd, ) < 0) {
log_warn("%s: stat", __func__);
+ close(fd);
return (NULL);
}
Re: interface media without netlock
On Sun, Aug 14, 2022 at 10:35:34PM +0200, Jeremie Courreges-Anglas wrote:
> Works fine on GENERIC.MP.
OK bluhm@
> Index: dev/fdt/if_cad.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/if_cad.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 if_cad.c
> --- dev/fdt/if_cad.c 8 Mar 2022 16:13:08 - 1.11
> +++ dev/fdt/if_cad.c 14 Aug 2022 20:20:00 -
> @@ -550,12 +550,22 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
> {
> struct cad_softc *sc = ifp->if_softc;
> struct ifreq *ifr = (struct ifreq *)data;
> - int error = 0;
> + int error = 0, netlock_held = 1;
> int s;
>
> - NET_UNLOCK();
> + switch (cmd) {
> + case SIOCGIFMEDIA:
> + case SIOCSIFMEDIA:
> + case SIOCGIFSFFPAGE:
> + netlock_held = 0;
> + break;
> + }
> +
> + if (netlock_held)
> + NET_UNLOCK();
> rw_enter_write(>sc_cfg_lock);
> - NET_LOCK();
> + if (netlock_held)
> + NET_LOCK();
> s = splnet();
>
> switch (cmd) {
>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: interface media without netlock
> On 14 Aug 2022, at 23:35, Jeremie Courreges-Anglas wrote:
>
> On Sun, Aug 14 2022, Vitaliy Makkoveev wrote:
>> I propose to avoid netlock relocking for SIOCGIFSFFPAGE too.
>
> sigh, that seems to be needed indeed.
>
>> Also, since the relock should be avoided for many commands, I prefer
>> to have something like below:
>>
>> switch(cmd){
>> case SIOCGIFMEDIA:
>> case ...:
>> relock = 1;
>> break;
>> }
>>
>> if (relock)
>> NET_UNLOCK();
>
> Fine with me. "netlock_held" made more sense to me as the variable
> name, I hope you won't mind. ;)
>
No problem, ok by me.
> Works fine on GENERIC.MP.
>
>
> Index: dev/fdt/if_cad.c
> ===
> RCS file: /cvs/src/sys/dev/fdt/if_cad.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 if_cad.c
> --- dev/fdt/if_cad.c 8 Mar 2022 16:13:08 - 1.11
> +++ dev/fdt/if_cad.c 14 Aug 2022 20:20:00 -
> @@ -550,12 +550,22 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
> {
> struct cad_softc *sc = ifp->if_softc;
> struct ifreq *ifr = (struct ifreq *)data;
> - int error = 0;
> + int error = 0, netlock_held = 1;
> int s;
>
> - NET_UNLOCK();
> + switch (cmd) {
> + case SIOCGIFMEDIA:
> + case SIOCSIFMEDIA:
> + case SIOCGIFSFFPAGE:
> + netlock_held = 0;
> + break;
> + }
> +
> + if (netlock_held)
> + NET_UNLOCK();
> rw_enter_write(>sc_cfg_lock);
> - NET_LOCK();
> + if (netlock_held)
> + NET_LOCK();
> s = splnet();
>
> switch (cmd) {
>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: interface media without netlock
On Sun, Aug 14 2022, Vitaliy Makkoveev wrote:
> I propose to avoid netlock relocking for SIOCGIFSFFPAGE too.
sigh, that seems to be needed indeed.
> Also, since the relock should be avoided for many commands, I prefer
> to have something like below:
>
> switch(cmd){
> case SIOCGIFMEDIA:
> case ...:
> relock = 1;
> break;
> }
>
> if (relock)
> NET_UNLOCK();
Fine with me. "netlock_held" made more sense to me as the variable
name, I hope you won't mind. ;)
Works fine on GENERIC.MP.
Index: dev/fdt/if_cad.c
===
RCS file: /cvs/src/sys/dev/fdt/if_cad.c,v
retrieving revision 1.11
diff -u -p -r1.11 if_cad.c
--- dev/fdt/if_cad.c8 Mar 2022 16:13:08 - 1.11
+++ dev/fdt/if_cad.c14 Aug 2022 20:20:00 -
@@ -550,12 +550,22 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
{
struct cad_softc *sc = ifp->if_softc;
struct ifreq *ifr = (struct ifreq *)data;
- int error = 0;
+ int error = 0, netlock_held = 1;
int s;
- NET_UNLOCK();
+ switch (cmd) {
+ case SIOCGIFMEDIA:
+ case SIOCSIFMEDIA:
+ case SIOCGIFSFFPAGE:
+ netlock_held = 0;
+ break;
+ }
+
+ if (netlock_held)
+ NET_UNLOCK();
rw_enter_write(>sc_cfg_lock);
- NET_LOCK();
+ if (netlock_held)
+ NET_LOCK();
s = splnet();
switch (cmd) {
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: IPv6 hop-by-hop check in parallel
On 8/13/22 15:08, Alexander Bluhm wrote: Hi, While running forwarding in parallel, I have introduced a hard barrier for parallel local protocol processing. The packets are requeued from shared to exclusive netlock. Unless we unlock all protocol input routines at once, we need some mechanism from to move from one queue to the other. The problem is, that we have to remember next protocol field and offset of the parsed header chain. A simple example is IPv6 hop-by-hop options precessing. This code is MP safe and can be moved from ip6_local() to ip6_ours() to run in parallel. If there were any options, the offset and next protocol are stored in a mbuf tag. Without tag we know that it is a regular IPv6 header. Of course mbuf tags kill performance, but who uses hop-by-hop options anyway? pf drops such packets per default. ok? bluhm This may be totally irrelevant. I may not understand the problem at all. The problem of locking queue-to-queue transfers in a multiprocessor system was addressed at Alliant Computer a very long time ago. Other people probably invented other solutions. They implemented a lock for the queue links only in each type of queue at a higher lock priority than the lock on the data. The sending thread already held a lock on the packet data. It took the queue field lock, transferred the packet and released both locks. I can't be sure which lock(s) the receiving thread held. The point was that neither thread needed to lock both queues. geoff steckel
Re: interface media without netlock
I propose to avoid netlock relocking for SIOCGIFSFFPAGE too.
Also, since the relock should be avoided for many commands, I prefer
to have something like below:
switch(cmd){
case SIOCGIFMEDIA:
case ...:
relock = 1;
break;
}
if (relock)
NET_UNLOCK();
> On 14 Aug 2022, at 22:09, Jeremie Courreges-Anglas wrote:
>
> On Sun, Aug 14 2022, Jeremie Courreges-Anglas wrote:
>> On Thu, Jul 28 2022, Alexander Bluhm wrote:
>>> Hi,
>>>
>>> The netlock for SIOCSIFMEDIA and SIOCGIFMEDIA ioctl is not necessary.
>>> Legacy drivers run with kernel lock, interface media is MP safe or
>>> has kernel lock.
>>>
>>> ixl(4) talks about net lock but in fact has kernel lock.
>>>
>>> Problem is that smtpd(8) periodically checks media status. This
>>> stops network if netlock was taken.
>>>
>>> ok?
>>
>> I'm seeing fallout with cad(4) in the RAMDISK and GENERIC.MP kernels
>> (didn't try GENERIC). RAMDISK after
>>
>> OpenBSD 7.2-beta (RAMDISK) #140: Sat Aug 6 02:06:57 MDT 2022
>>
>> hangs some time after mounting root. On GENERIC.MP I get a panic:
>>
>> --8<--
>> Automatic boot in progress: starting file system checks.
>> /dev/sd0a (c92bac01c037a788.a): file system is clean; not checking
>> /dev/sd0m (c92bac01c037a788.m): file system is clean; not checking
>> /dev/sd0j (c92bac01c037a788.j): file system is clean; not checking
>> /dev/sd0d (c92bac01c037a788.d): file system is clean; not checking
>> /dev/sd0e (c92bac01c037a788.e): file system is clean; not checking
>> /dev/sd0f (c92bac01c037a788.f): file system is clean; not checking
>> /dev/sd0g (c92bac01c037a788.g): file system is clean; not checking
>> /dev/sd0h (c92bac01c037a788.h): file system is clean; not checking
>> /dev/sd0k (c92bac01c037a788.k): file system is clean; not checking
>> /dev/sd0l (c92bac01c037a788.l): file system is clean; not checking
>> /dev/sd0o (c92bac01c037a788.o): file system is clean; not checking
>> /dev/sd0n (c92bac01c037a788.n): file system is clean; not checking
>> /dev/sd0p (c92bac01c037a788.p): file system is clean; not checking
>> pf enabled
>> kern.bufcachepercent: 20 -> 80
>> sysctl: kern.allowdt: value is not available
>> ddb.console: 0 -> 1
>> starting network
>> panic: netlock: lock not held
>> Stopped at panic+0x106:addia0,zero,256TIDPIDUID
>> PR
>> FLAGS PFLAGS CPU COMMAND
>> *464996 1081 0 0x3 00 ifconfig
>> 492269 29052 0 0x14000 0x2001 zerothread
>> panic() at panic+0x106
>> rw_exit_write() at rw_exit_write+0x94
>> cad_ioctl() at cad_ioctl+0x34
>> ifioctl() at ifioctl+0xaaa
>> soo_ioctl() at soo_ioctl+0x152
>> sys_ioctl() at sys_ioctl+0x230
>> svc_handler() at svc_handler+0x284
>> do_trap_user() at do_trap_user+0x15c
>> cpu_exception_handler_user() at cpu_exception_handler_user+0x7c
>> end of kernel
>> end trace frame: 0x5408b2ae8, count: 6
>> https://www.openbsd.org/ddb.html describes the minimum info required in bug
>> reports. Insufficient info makes it difficult to find and fix bugs.
>>
>> -->8--
>>
>> The diff below fixes GENERIC.MP, I have yet to build and try RAMDISK.
>
> Done, the diff fixes bsd.rd too.
>
>> I'm not completely sure this is the best way to solve it but I can't
>> spot other uses of ifp->if_ioctl() not protected by the NET_LOCK() in
>> ifioctl(). Except SIOCGIFSFFPAGE but that seems fine...
>
> Thoughts? ok?
>
>> Index: dev/fdt/if_cad.c
>> ===
>> RCS file: /home/cvs/src/sys/dev/fdt/if_cad.c,v
>> retrieving revision 1.11
>> diff -u -p -r1.11 if_cad.c
>> --- dev/fdt/if_cad.c 8 Mar 2022 16:13:08 - 1.11
>> +++ dev/fdt/if_cad.c 14 Aug 2022 13:04:41 -
>> @@ -553,9 +553,11 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
>> int error = 0;
>> int s;
>>
>> -NET_UNLOCK();
>> +if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
>> +NET_UNLOCK();
>> rw_enter_write(>sc_cfg_lock);
>> -NET_LOCK();
>> +if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
>> +NET_LOCK();
>> s = splnet();
>>
>> switch (cmd) {
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: interface media without netlock
On Sun, Aug 14 2022, Jeremie Courreges-Anglas wrote:
> On Thu, Jul 28 2022, Alexander Bluhm wrote:
>> Hi,
>>
>> The netlock for SIOCSIFMEDIA and SIOCGIFMEDIA ioctl is not necessary.
>> Legacy drivers run with kernel lock, interface media is MP safe or
>> has kernel lock.
>>
>> ixl(4) talks about net lock but in fact has kernel lock.
>>
>> Problem is that smtpd(8) periodically checks media status. This
>> stops network if netlock was taken.
>>
>> ok?
>
> I'm seeing fallout with cad(4) in the RAMDISK and GENERIC.MP kernels
> (didn't try GENERIC). RAMDISK after
>
> OpenBSD 7.2-beta (RAMDISK) #140: Sat Aug 6 02:06:57 MDT 2022
>
> hangs some time after mounting root. On GENERIC.MP I get a panic:
>
> --8<--
> Automatic boot in progress: starting file system checks.
> /dev/sd0a (c92bac01c037a788.a): file system is clean; not checking
> /dev/sd0m (c92bac01c037a788.m): file system is clean; not checking
> /dev/sd0j (c92bac01c037a788.j): file system is clean; not checking
> /dev/sd0d (c92bac01c037a788.d): file system is clean; not checking
> /dev/sd0e (c92bac01c037a788.e): file system is clean; not checking
> /dev/sd0f (c92bac01c037a788.f): file system is clean; not checking
> /dev/sd0g (c92bac01c037a788.g): file system is clean; not checking
> /dev/sd0h (c92bac01c037a788.h): file system is clean; not checking
> /dev/sd0k (c92bac01c037a788.k): file system is clean; not checking
> /dev/sd0l (c92bac01c037a788.l): file system is clean; not checking
> /dev/sd0o (c92bac01c037a788.o): file system is clean; not checking
> /dev/sd0n (c92bac01c037a788.n): file system is clean; not checking
> /dev/sd0p (c92bac01c037a788.p): file system is clean; not checking
> pf enabled
> kern.bufcachepercent: 20 -> 80
> sysctl: kern.allowdt: value is not available
> ddb.console: 0 -> 1
> starting network
> panic: netlock: lock not held
> Stopped at panic+0x106:addia0,zero,256TIDPIDUID
> PR
> FLAGS PFLAGS CPU COMMAND
> *464996 1081 0 0x3 00 ifconfig
> 492269 29052 0 0x14000 0x2001 zerothread
> panic() at panic+0x106
> rw_exit_write() at rw_exit_write+0x94
> cad_ioctl() at cad_ioctl+0x34
> ifioctl() at ifioctl+0xaaa
> soo_ioctl() at soo_ioctl+0x152
> sys_ioctl() at sys_ioctl+0x230
> svc_handler() at svc_handler+0x284
> do_trap_user() at do_trap_user+0x15c
> cpu_exception_handler_user() at cpu_exception_handler_user+0x7c
> end of kernel
> end trace frame: 0x5408b2ae8, count: 6
> https://www.openbsd.org/ddb.html describes the minimum info required in bug
> reports. Insufficient info makes it difficult to find and fix bugs.
>
> -->8--
>
> The diff below fixes GENERIC.MP, I have yet to build and try RAMDISK.
Done, the diff fixes bsd.rd too.
> I'm not completely sure this is the best way to solve it but I can't
> spot other uses of ifp->if_ioctl() not protected by the NET_LOCK() in
> ifioctl(). Except SIOCGIFSFFPAGE but that seems fine...
Thoughts? ok?
> Index: dev/fdt/if_cad.c
> ===
> RCS file: /home/cvs/src/sys/dev/fdt/if_cad.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 if_cad.c
> --- dev/fdt/if_cad.c 8 Mar 2022 16:13:08 - 1.11
> +++ dev/fdt/if_cad.c 14 Aug 2022 13:04:41 -
> @@ -553,9 +553,11 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
> int error = 0;
> int s;
>
> - NET_UNLOCK();
> + if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
> + NET_UNLOCK();
> rw_enter_write(>sc_cfg_lock);
> - NET_LOCK();
> + if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
> + NET_LOCK();
> s = splnet();
>
> switch (cmd) {
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: interface media without netlock
On Thu, Jul 28 2022, Alexander Bluhm wrote:
> Hi,
>
> The netlock for SIOCSIFMEDIA and SIOCGIFMEDIA ioctl is not necessary.
> Legacy drivers run with kernel lock, interface media is MP safe or
> has kernel lock.
>
> ixl(4) talks about net lock but in fact has kernel lock.
>
> Problem is that smtpd(8) periodically checks media status. This
> stops network if netlock was taken.
>
> ok?
I'm seeing fallout with cad(4) in the RAMDISK and GENERIC.MP kernels
(didn't try GENERIC). RAMDISK after
OpenBSD 7.2-beta (RAMDISK) #140: Sat Aug 6 02:06:57 MDT 2022
hangs some time after mounting root. On GENERIC.MP I get a panic:
--8<--
Automatic boot in progress: starting file system checks.
/dev/sd0a (c92bac01c037a788.a): file system is clean; not checking
/dev/sd0m (c92bac01c037a788.m): file system is clean; not checking
/dev/sd0j (c92bac01c037a788.j): file system is clean; not checking
/dev/sd0d (c92bac01c037a788.d): file system is clean; not checking
/dev/sd0e (c92bac01c037a788.e): file system is clean; not checking
/dev/sd0f (c92bac01c037a788.f): file system is clean; not checking
/dev/sd0g (c92bac01c037a788.g): file system is clean; not checking
/dev/sd0h (c92bac01c037a788.h): file system is clean; not checking
/dev/sd0k (c92bac01c037a788.k): file system is clean; not checking
/dev/sd0l (c92bac01c037a788.l): file system is clean; not checking
/dev/sd0o (c92bac01c037a788.o): file system is clean; not checking
/dev/sd0n (c92bac01c037a788.n): file system is clean; not checking
/dev/sd0p (c92bac01c037a788.p): file system is clean; not checking
pf enabled
kern.bufcachepercent: 20 -> 80
sysctl: kern.allowdt: value is not available
ddb.console: 0 -> 1
starting network
panic: netlock: lock not held
Stopped at panic+0x106:addia0,zero,256TIDPIDUID PR
FLAGS PFLAGS CPU COMMAND
*464996 1081 0 0x3 00 ifconfig
492269 29052 0 0x14000 0x2001 zerothread
panic() at panic+0x106
rw_exit_write() at rw_exit_write+0x94
cad_ioctl() at cad_ioctl+0x34
ifioctl() at ifioctl+0xaaa
soo_ioctl() at soo_ioctl+0x152
sys_ioctl() at sys_ioctl+0x230
svc_handler() at svc_handler+0x284
do_trap_user() at do_trap_user+0x15c
cpu_exception_handler_user() at cpu_exception_handler_user+0x7c
end of kernel
end trace frame: 0x5408b2ae8, count: 6
https://www.openbsd.org/ddb.html describes the minimum info required in bug
reports. Insufficient info makes it difficult to find and fix bugs.
-->8--
The diff below fixes GENERIC.MP, I have yet to build and try RAMDISK.
I'm not completely sure this is the best way to solve it but I can't
spot other uses of ifp->if_ioctl() not protected by the NET_LOCK() in
ifioctl(). Except SIOCGIFSFFPAGE but that seems fine...
Index: dev/fdt/if_cad.c
===
RCS file: /home/cvs/src/sys/dev/fdt/if_cad.c,v
retrieving revision 1.11
diff -u -p -r1.11 if_cad.c
--- dev/fdt/if_cad.c8 Mar 2022 16:13:08 - 1.11
+++ dev/fdt/if_cad.c14 Aug 2022 13:04:41 -
@@ -553,9 +553,11 @@ cad_ioctl(struct ifnet *ifp, u_long cmd,
int error = 0;
int s;
- NET_UNLOCK();
+ if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
+ NET_UNLOCK();
rw_enter_write(>sc_cfg_lock);
- NET_LOCK();
+ if (cmd != SIOCGIFMEDIA && cmd != SIOCSIFMEDIA)
+ NET_LOCK();
s = splnet();
switch (cmd) {
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Add Intel Optane 900P/905P NVMe
Hi,
Intel Optane 905p NVMe devices were not yet recognized in CURRENT (e.g.,
https://ark.intel.com/content/www/us/en/ark/products/148607/intel-optane-ssd-905p-series-380gb-m-2-110mm-pcie-x4-20nm-3d-xpoint.html
).
I've managed to get it working with the following diff:
Index: pcidevs
===
RCS file: /cvs/src/sys/dev/pci/pcidevs,v
retrieving revision 1.2000
diff -u -p -u -p -r1.2000 pcidevs
--- pcidevs 2 Aug 2022 05:35:01 - 1.2000
+++ pcidevs 14 Aug 2022 13:46:32 -
@@ -4623,6 +4623,7 @@ product INTEL 6321ESB_ACM 0x2699 6321ESB
product INTEL 6321ESB_HDA 0x269a 6321ESB HD Audio
product INTEL 6321ESB_SMB 0x269b 6321ESB SMBus
product INTEL 6321ESB_IDE 0x269e 6321ESB IDE
+product INTEL NVME_6 0x2700 Optane 900P/905P NVMe
product INTEL WL_22500_1 0x2723 Wi-Fi 6 AX200
product INTEL WL_22500_9 0x2725 Wi-Fi 6 AX210
product INTEL WL_22500_10 0x2726 Wi-Fi 6 AX211
Index: pcidevs.h
===
RCS file: /cvs/src/sys/dev/pci/pcidevs.h,v
retrieving revision 1.1994
diff -u -p -u -p -r1.1994 pcidevs.h
--- pcidevs.h 2 Aug 2022 05:35:34 - 1.1994
+++ pcidevs.h 14 Aug 2022 13:48:44 -
@@ -4628,6 +4628,7 @@
#definePCI_PRODUCT_INTEL_6321ESB_HDA 0x269a /* 6321ESB HD
Audio */
#definePCI_PRODUCT_INTEL_6321ESB_SMB 0x269b /* 6321ESB
SMBus */
#definePCI_PRODUCT_INTEL_6321ESB_IDE 0x269e /* 6321ESB IDE
*/
+#define PCI_PRODUCT_INTEL_NVME_6 0x2700 /* Optane 900P/905P
NVMe */
#definePCI_PRODUCT_INTEL_WL_22500_10x2723 /* Wi-Fi 6
AX200 */
#definePCI_PRODUCT_INTEL_WL_22500_90x2725 /* Wi-Fi 6
AX210 */
#definePCI_PRODUCT_INTEL_WL_22500_10 0x2726 /* Wi-Fi 6
AX211 */
Index: pcidevs_data.h
===
RCS file: /cvs/src/sys/dev/pci/pcidevs_data.h,v
retrieving revision 1.1989
diff -u -p -u -p -r1.1989 pcidevs_data.h
--- pcidevs_data.h 2 Aug 2022 05:35:34 - 1.1989
+++ pcidevs_data.h 14 Aug 2022 13:49:55 -
@@ -15888,6 +15888,10 @@ static const struct pci_known_product pc
"6321ESB IDE",
},
{
+ PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_NVME_6,
+ "Optane 900P/905P NVMe",
+ },
+ {
PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_WL_22500_1,
"Wi-Fi 6 AX200",
},
I've also noticed the following in nvme_pci.c which seems to be related
to some kind of Intel Optane device:
81 static const struct pci_matchid nvme_msi_blacklist[] = {
82 { PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_OPTANE },
83 };
It is unclear to me if this is also relevant for the Optane 900P/905P
series, i.e., should the line "{ PCI_VENDOR_INTEL,
PCI_PRODUCT_INTEL_NVME_6 }," also be added there? [didn't notice any
problems yet without having the device included there...]
The affected parts of dmesg after applying the patch:
nvme1 at pci4 dev 0 function 0 "Intel Optane 900P/905P NVMe" rev 0x00:
msix, NVMe 1.0
nvme1: INTEL SSDPEL1D380GA, firmware E2010603, serial PHMC93620015380A
scsibus3 at nvme1: 2 targets, initiator 0
sd1 at scsibus3 targ 1 lun 0:
sd1: 362476MB, 512 bytes/sector, 742352688 sectors
Best regards
Andreas
Re: Remove unneeded kern.nselcoll sysctl
On Sun, 14 Aug 2022 11:13:35 -, Visa Hankala wrote: > Remove unneeded kern.nselcoll sysctl. > > The last use of this sysctl in base was removed over two weeks ago. > Debian Code Search does not show any uses of the KERN_NSELCOLL macro. OK millert@ - todd
Remove kqueue-related trace points from poll(2) and select(2)
kqueue-based poll(2) and select(2) seem to work fairly well. Because of
this, the ktrace points that display the internal translated events do
not appear very valuable any longer. They clog up and make traces
difficult to read. I think it is time to remove the trace points.
OK?
Index: sys/kern/sys_generic.c
===
RCS file: src/sys/kern/sys_generic.c,v
retrieving revision 1.148
diff -u -p -r1.148 sys_generic.c
--- sys/kern/sys_generic.c 5 Jul 2022 15:06:16 - 1.148
+++ sys/kern/sys_generic.c 14 Aug 2022 10:46:50 -
@@ -691,10 +691,7 @@ dopselect(struct proc *p, int nd, fd_set
/* Maximum number of events per iteration */
count = MIN(nitems(kev), nevents);
ready = kqueue_scan(, count, kev, timeout, p, );
-#ifdef KTRACE
- if (KTRPOINT(p, KTR_STRUCT))
- ktrevent(p, kev, ready);
-#endif
+
/* Convert back events that are ready. */
for (i = 0; i < ready && error == 0; i++)
error = pselcollect(p, [i], pobits, );
@@ -762,10 +759,6 @@ pselregister(struct proc *p, fd_set *pib
EV_SET(, fd, evf[msk],
EV_ADD|EV_ENABLE|__EV_SELECT,
evff[msk], 0, (void *)(p->p_kq_serial));
-#ifdef KTRACE
- if (KTRPOINT(p, KTR_STRUCT))
- ktrevent(p, , 1);
-#endif
error = kqueue_register(p->p_kq, , 0, p);
switch (error) {
case 0:
@@ -1001,10 +994,7 @@ doppoll(struct proc *p, struct pollfd *f
/* Maximum number of events per iteration */
count = MIN(nitems(kev), nevents);
ready = kqueue_scan(, count, kev, timeout, p, );
-#ifdef KTRACE
- if (KTRPOINT(p, KTR_STRUCT))
- ktrevent(p, kev, ready);
-#endif
+
/* Convert back events that are ready. */
for (i = 0; i < ready; i++)
ncollected += ppollcollect(p, [i], pl, nfds);
@@ -1057,10 +1047,6 @@ ppollregister_evts(struct proc *p, struc
KASSERT(pl->revents == 0);
-#ifdef KTRACE
- if (KTRPOINT(p, KTR_STRUCT))
- ktrevent(p, kevp, nkev);
-#endif
for (i = 0; i < nkev; i++, kevp++) {
again:
error = kqueue_register(p->p_kq, kevp, pollid, p);
Remove unneeded kern.nselcoll sysctl
Remove unneeded kern.nselcoll sysctl.
The last use of this sysctl in base was removed over two weeks ago.
Debian Code Search does not show any uses of the KERN_NSELCOLL macro.
OK?
Index: lib/libc/sys/sysctl.2
===
RCS file: src/lib/libc/sys/sysctl.2,v
retrieving revision 1.48
diff -u -p -r1.48 sysctl.2
--- lib/libc/sys/sysctl.2 31 Mar 2022 17:27:16 - 1.48
+++ lib/libc/sys/sysctl.2 14 Aug 2022 10:46:48 -
@@ -460,7 +460,6 @@ information.
.It Dv KERN_NGROUPS Ta "integer" Ta "no"
.It Dv KERN_NOSUIDCOREDUMP Ta "integer" Ta "yes"
.It Dv KERN_NPROCS Ta "integer" Ta "no"
-.It Dv KERN_NSELCOLL Ta "integer" Ta "no"
.It Dv KERN_NTHREADS Ta "integer" Ta "no"
.It Dv KERN_NUMVNODES Ta "integer" Ta "no"
.It Dv KERN_OSRELEASE Ta "string" Ta "no"
@@ -716,10 +715,6 @@ Whether a process may dump core after ch
.El
.It Dv KERN_NPROCS Pq Va kern.nprocs
The number of entries in the kernel process table.
-.It Dv KERN_NSELCOLL Pq Va kern.nselcoll
-Number of
-.Xr select 2
-collisions.
.It Dv KERN_NTHREADS Pq Va kern.nthreads
The number of entries in the kernel thread table.
.It Dv KERN_NUMVNODES Pq Va kern.numvnodes
Index: sys/kern/kern_sysctl.c
===
RCS file: src/sys/kern/kern_sysctl.c,v
retrieving revision 1.404
diff -u -p -r1.404 kern_sysctl.c
--- sys/kern/kern_sysctl.c 26 Jul 2022 14:53:45 - 1.404
+++ sys/kern/kern_sysctl.c 14 Aug 2022 10:46:50 -
@@ -298,7 +298,6 @@ const struct sysctl_bounded_args kern_va
{KERN_NFILES, , SYSCTL_INT_READONLY},
{KERN_TTYCOUNT, _count, SYSCTL_INT_READONLY},
{KERN_ARGMAX, _max, SYSCTL_INT_READONLY},
- {KERN_NSELCOLL, _zero, SYSCTL_INT_READONLY},
{KERN_POSIX1, _version, SYSCTL_INT_READONLY},
{KERN_NGROUPS, _max, SYSCTL_INT_READONLY},
{KERN_JOB_CONTROL, _one, SYSCTL_INT_READONLY},
Index: sys/sys/sysctl.h
===
RCS file: src/sys/sys/sysctl.h,v
retrieving revision 1.228
diff -u -p -r1.228 sysctl.h
--- sys/sys/sysctl.h13 May 2022 15:32:00 - 1.228
+++ sys/sys/sysctl.h14 Aug 2022 10:46:50 -
@@ -144,7 +144,7 @@ struct ctlname {
#define KERN_CPTIME40 /* array: cp_time */
#define KERN_NCHSTATS 41 /* struct: vfs cache statistics */
#define KERN_FORKSTAT 42 /* struct: fork statistics */
-#define KERN_NSELCOLL 43 /* int: select(2) collisions */
+/* was KERN_NSELCOLL 43 */
#define KERN_TTY 44 /* node: tty information */
#defineKERN_CCPU 45 /* int: ccpu */
#defineKERN_FSCALE 46 /* int: fscale */
@@ -237,7 +237,7 @@ struct ctlname {
{ "cp_time", CTLTYPE_STRUCT }, \
{ "nchstats", CTLTYPE_STRUCT }, \
{ "forkstat", CTLTYPE_STRUCT }, \
- { "nselcoll", CTLTYPE_INT }, \
+ { "gap", 0 }, \
{ "tty", CTLTYPE_NODE }, \
{ "ccpu", CTLTYPE_INT }, \
{ "fscale", CTLTYPE_INT }, \
Re: Split (*pr_usrreq)() by multiple handlers
On Sun, Aug 14, 2022 at 01:35:18AM +0300, Vitaliy Makkoveev wrote:
> The pr_usrreqs hunks. I replaced spaces by tab within `routesw'
> structure.
OK bluhm@
> Index: sys/kern/uipc_proto.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_proto.c,v
> retrieving revision 1.22
> diff -u -p -r1.22 uipc_proto.c
> --- sys/kern/uipc_proto.c 25 Feb 2022 23:51:03 - 1.22
> +++ sys/kern/uipc_proto.c 13 Aug 2022 21:59:52 -
> @@ -50,27 +50,21 @@ const struct protosw unixsw[] = {
>.pr_domain = ,
>.pr_protocol = PF_UNIX,
>.pr_flags = PR_CONNREQUIRED|PR_WANTRCVD|PR_RIGHTS,
> - .pr_usrreq = uipc_usrreq,
> - .pr_attach = uipc_attach,
> - .pr_detach = uipc_detach,
> + .pr_usrreqs= _usrreqs,
> },
> {
>.pr_type = SOCK_SEQPACKET,
>.pr_domain = ,
>.pr_protocol = PF_UNIX,
>.pr_flags = PR_ATOMIC|PR_CONNREQUIRED|PR_WANTRCVD|PR_RIGHTS,
> - .pr_usrreq = uipc_usrreq,
> - .pr_attach = uipc_attach,
> - .pr_detach = uipc_detach,
> + .pr_usrreqs= _usrreqs,
> },
> {
>.pr_type = SOCK_DGRAM,
>.pr_domain = ,
>.pr_protocol = PF_UNIX,
>.pr_flags = PR_ATOMIC|PR_ADDR|PR_RIGHTS,
> - .pr_usrreq = uipc_usrreq,
> - .pr_attach = uipc_attach,
> - .pr_detach = uipc_detach,
> + .pr_usrreqs= _usrreqs,
> }
> };
>
> Index: sys/kern/uipc_socket.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_socket.c,v
> retrieving revision 1.281
> diff -u -p -r1.281 uipc_socket.c
> --- sys/kern/uipc_socket.c13 Aug 2022 21:01:46 - 1.281
> +++ sys/kern/uipc_socket.c13 Aug 2022 21:59:52 -
> @@ -172,7 +172,7 @@ socreate(int dom, struct socket **aso, i
> prp = pffindproto(dom, proto, type);
> else
> prp = pffindtype(dom, type);
> - if (prp == NULL || prp->pr_attach == NULL)
> + if (prp == NULL || prp->pr_usrreqs == NULL)
> return (EPROTONOSUPPORT);
> if (prp->pr_type != type)
> return (EPROTOTYPE);
> @@ -1296,7 +1296,7 @@ sosplice(struct socket *so, int fd, off_
> if ((error = getsock(curproc, fd, )) != 0)
> return (error);
> sosp = fp->f_data;
> - if (sosp->so_proto->pr_usrreq != so->so_proto->pr_usrreq) {
> + if (sosp->so_proto->pr_usrreqs != so->so_proto->pr_usrreqs) {
> error = EPROTONOSUPPORT;
> goto frele;
> }
> Index: sys/kern/uipc_usrreq.c
> ===
> RCS file: /cvs/src/sys/kern/uipc_usrreq.c,v
> retrieving revision 1.167
> diff -u -p -r1.167 uipc_usrreq.c
> --- sys/kern/uipc_usrreq.c2 Jul 2022 11:49:23 - 1.167
> +++ sys/kern/uipc_usrreq.c13 Aug 2022 21:59:52 -
> @@ -126,6 +126,12 @@ int unp_rights; /* [R] file descriptors
> int unp_defer; /* [G] number of deferred fp to close by the GC task */
> int unp_gcing; /* [G] GC task currently running */
>
> +const struct pr_usrreqs uipc_usrreqs = {
> + .pru_usrreq = uipc_usrreq,
> + .pru_attach = uipc_attach,
> + .pru_detach = uipc_detach,
> +};
> +
> void
> unp_init(void)
> {
> Index: sys/net/pfkeyv2.c
> ===
> RCS file: /cvs/src/sys/net/pfkeyv2.c,v
> retrieving revision 1.234
> diff -u -p -r1.234 pfkeyv2.c
> --- sys/net/pfkeyv2.c 6 Jun 2022 14:45:41 - 1.234
> +++ sys/net/pfkeyv2.c 13 Aug 2022 21:59:52 -
> @@ -199,6 +199,12 @@ pfdatatopacket(void *data, int len, stru
> return (0);
> }
>
> +const struct pr_usrreqs pfkeyv2_usrreqs = {
> + .pru_usrreq = pfkeyv2_usrreq,
> + .pru_attach = pfkeyv2_attach,
> + .pru_detach = pfkeyv2_detach,
> +};
> +
> const struct protosw pfkeysw[] = {
> {
>.pr_type = SOCK_RAW,
> @@ -206,9 +212,7 @@ const struct protosw pfkeysw[] = {
>.pr_protocol = PF_KEY_V2,
>.pr_flags = PR_ATOMIC | PR_ADDR,
>.pr_output= pfkeyv2_output,
> - .pr_usrreq= pfkeyv2_usrreq,
> - .pr_attach= pfkeyv2_attach,
> - .pr_detach= pfkeyv2_detach,
> + .pr_usrreqs = _usrreqs,
>.pr_sysctl= pfkeyv2_sysctl,
> }
> };
> Index: sys/net/rtsock.c
> ===
> RCS file: /cvs/src/sys/net/rtsock.c,v
> retrieving revision 1.334
> diff -u -p -r1.334 rtsock.c
> --- sys/net/rtsock.c 28 Jun 2022 10:01:13 - 1.334
> +++ sys/net/rtsock.c 13 Aug 2022 21:59:52 -
> @@ -2401,6 +2401,12 @@ rt_setsource(unsigned int rtableid, stru
> * Definitions of protocols supported in the ROUTE domain.
> */
>
> +const struct pr_usrreqs route_usrreqs = {
> + .pru_usrreq = route_usrreq,
> + .pru_attach = route_attach,
> + .pru_detach = route_detach,
> +};
> +
> const struct protosw routesw[] = {
> {
>.pr_type = SOCK_RAW,
> @@ -2408,9 +2414,7 @@ const
