Re: igc(4) remove unnecessary PHY ID checks
On Wed, Jan 25, 2023 at 03:17:48PM +0100, Moritz Buhl wrote:
>
> Dear tech,
>
> Looking into various reports on errors with mini routers that use
> igc(4), I found the following diff in FreeBSD:
> https://github.com/freebsd/freebsd-src/commit/29d7f1ff579579711dd5a3325480728b8ed45f8c
>
> I additionally deleted the igc_set_d0_lplu_state_i225 and
> igc_set_d3_lplu_state_i225 functions since they are no longer used.
>
> I am not convinced that this diff really fixes any of the issues,
> however I do appreciate feedback.
I225 devices do have only one phy vendor, Linux also removes PHY ID checking:
https://github.com/torvalds/linux/commit/7c496de538eebd8212dc2a3c9a468386b264d0d4
> Ok?
ok kevlo@
> mbuhl
>
> Index: dev/pci/igc_i225.c
> ===
> RCS file: /cvs/src/sys/dev/pci/igc_i225.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 igc_i225.c
> --- dev/pci/igc_i225.c11 May 2022 06:14:15 - 1.3
> +++ dev/pci/igc_i225.c10 Jan 2023 09:26:04 -
> @@ -163,16 +163,7 @@ igc_init_phy_params_i225(struct igc_hw *
> goto out;
>
> ret_val = igc_get_phy_id(hw);
> - /* Verify phy id and set remaining function pointers */
> - switch (phy->id) {
> - case I225_I_PHY_ID:
> - default:
> - phy->type = igc_phy_i225;
> - phy->ops.set_d0_lplu_state = igc_set_d0_lplu_state_i225;
> - phy->ops.set_d3_lplu_state = igc_set_d3_lplu_state_i225;
> - /* TODO - complete with GPY PHY information */
> - break;
> - }
> + phy->type = igc_phy_i225;
>
> out:
> return ret_val;
> @@ -1104,66 +1095,6 @@ igc_init_hw_i225(struct igc_hw *hw)
>
> ret_val = igc_init_hw_base(hw);
> return ret_val;
> -}
> -
> -/*
> - * igc_set_d0_lplu_state_i225 - Set Low-Power-Link-Up (LPLU) D0 state
> - * @hw: pointer to the HW structure
> - * @active: true to enable LPLU, false to disable
> - *
> - * Note: since I225 does not actually support LPLU, this function
> - * simply enables/disables 1G and 2.5G speeds in D0.
> - */
> -int
> -igc_set_d0_lplu_state_i225(struct igc_hw *hw, bool active)
> -{
> - uint32_t data;
> -
> - DEBUGFUNC("igc_set_d0_lplu_state_i225");
> -
> - data = IGC_READ_REG(hw, IGC_I225_PHPM);
> -
> - if (active) {
> - data |= IGC_I225_PHPM_DIS_1000;
> - data |= IGC_I225_PHPM_DIS_2500;
> - } else {
> - data &= ~IGC_I225_PHPM_DIS_1000;
> - data &= ~IGC_I225_PHPM_DIS_2500;
> - }
> -
> - IGC_WRITE_REG(hw, IGC_I225_PHPM, data);
> - return IGC_SUCCESS;
> -}
> -
> -/*
> - * igc_set_d3_lplu_state_i225 - Set Low-Power-Link-Up (LPLU) D3 state
> - * @hw: pointer to the HW structure
> - * @active: true to enable LPLU, false to disable
> - *
> - * Note: since I225 does not actually support LPLU, this function
> - * simply enables/disables 100M, 1G and 2.5G speeds in D3.
> - */
> -int
> -igc_set_d3_lplu_state_i225(struct igc_hw *hw, bool active)
> -{
> - uint32_t data;
> -
> - DEBUGFUNC("igc_set_d3_lplu_state_i225");
> -
> - data = IGC_READ_REG(hw, IGC_I225_PHPM);
> -
> - if (active) {
> - data |= IGC_I225_PHPM_DIS_100_D3;
> - data |= IGC_I225_PHPM_DIS_1000_D3;
> - data |= IGC_I225_PHPM_DIS_2500_D3;
> - } else {
> - data &= ~IGC_I225_PHPM_DIS_100_D3;
> - data &= ~IGC_I225_PHPM_DIS_1000_D3;
> - data &= ~IGC_I225_PHPM_DIS_2500_D3;
> - }
> -
> - IGC_WRITE_REG(hw, IGC_I225_PHPM, data);
> - return IGC_SUCCESS;
> }
>
> /**
> Index: dev/pci/igc_i225.h
> ===
> RCS file: /cvs/src/sys/dev/pci/igc_i225.h,v
> retrieving revision 1.1
> diff -u -p -r1.1 igc_i225.h
> --- dev/pci/igc_i225.h31 Oct 2021 14:52:57 - 1.1
> +++ dev/pci/igc_i225.h10 Jan 2023 09:37:40 -
> @@ -27,8 +27,6 @@ voidigc_release_swfw_sync_i225(struct i
> int igc_set_ltr_i225(struct igc_hw *, bool);
> int igc_init_hw_i225(struct igc_hw *);
> int igc_setup_copper_link_i225(struct igc_hw *);
> -int igc_set_d0_lplu_state_i225(struct igc_hw *, bool);
> -int igc_set_d3_lplu_state_i225(struct igc_hw *, bool);
> int igc_set_eee_i225(struct igc_hw *, bool, bool, bool);
>
> #define ID_LED_DEFAULT_I225 \
> Index: dev/pci/igc_phy.c
> ===
> RCS file: /cvs/src/sys/dev/pci/igc_phy.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 igc_phy.c
> --- dev/pci/igc_phy.c 11 May 2022 06:14:15 - 1.2
> +++ dev/pci/igc_phy.c 10 Jan 2023 09:34:11 -
> @@ -307,8 +307,7 @@ igc_phy_setup_autoneg(struct igc_hw *hw)
> return ret_val;
> }
>
> - if ((phy->autoneg_mask & ADVERTISE_2500_FULL) &&
> - hw->phy.id == I225_I_PHY_ID) {
> + if (phy->autoneg_mask & ADVERTISE_2500_FULL)
dmtimer(4), macppc, powerpc64, riscv64: init stathz, profhz like other platforms
Almost every platform initializes stathz and profhz like this:
stathz = hz;
profhz = stathz * 10;
This patch brings a few stragglers in line with everyone else.
This does not change stathz and profhz on the listed platforms: hz is
100 in every case.
ok?
Index: macppc/macppc/clock.c
===
RCS file: /cvs/src/sys/arch/macppc/macppc/clock.c,v
retrieving revision 1.51
diff -u -p -r1.51 clock.c
--- macppc/macppc/clock.c 29 Nov 2022 00:58:05 - 1.51
+++ macppc/macppc/clock.c 27 Jan 2023 03:20:17 -
@@ -196,8 +196,8 @@ cpu_initclocks(void)
intrstate = ppc_intr_disable();
- stathz = 100;
- profhz = 1000; /* must be a multiple of stathz */
+ stathz = hz;
+ profhz = stathz * 10;
clockintr_init(CL_RNDSTAT);
dec_nsec_cycle_ratio = ticks_per_sec * (1ULL << 32) / 10;
Index: powerpc64/powerpc64/clock.c
===
RCS file: /cvs/src/sys/arch/powerpc64/powerpc64/clock.c,v
retrieving revision 1.7
diff -u -p -r1.7 clock.c
--- powerpc64/powerpc64/clock.c 29 Nov 2022 01:04:44 - 1.7
+++ powerpc64/powerpc64/clock.c 27 Jan 2023 03:20:17 -
@@ -98,8 +98,8 @@ cpu_initclocks(void)
dec_nsec_cycle_ratio = tb_freq * (1ULL << 32) / 10;
dec_nsec_max = UINT64_MAX / dec_nsec_cycle_ratio;
- stathz = 100;
- profhz = 1000; /* must be a multiple of stathz */
+ stathz = hz;
+ profhz = stathz * 10;
clockintr_init(CL_RNDSTAT);
evcount_attach(&clock_count, "clock", NULL);
Index: riscv64/riscv64/clock.c
===
RCS file: /cvs/src/sys/arch/riscv64/riscv64/clock.c,v
retrieving revision 1.7
diff -u -p -r1.7 clock.c
--- riscv64/riscv64/clock.c 3 Dec 2022 15:03:49 - 1.7
+++ riscv64/riscv64/clock.c 27 Jan 2023 03:20:17 -
@@ -92,8 +92,8 @@ cpu_initclocks(void)
timer_nsec_cycle_ratio = tb_freq * (1ULL << 32) / 10;
timer_nsec_max = UINT64_MAX / timer_nsec_cycle_ratio;
- stathz = 100;
- profhz = 1000; /* must be a multiple of stathz */
+ stathz = hz;
+ profhz = stathz * 10;
clockintr_init(CL_RNDSTAT);
riscv_intc_intr_establish(IRQ_TIMER_SUPERVISOR, 0,
Index: armv7/omap/dmtimer.c
===
RCS file: /cvs/src/sys/arch/armv7/omap/dmtimer.c,v
retrieving revision 1.16
diff -u -p -r1.16 dmtimer.c
--- armv7/omap/dmtimer.c17 Jan 2023 02:32:07 - 1.16
+++ armv7/omap/dmtimer.c27 Jan 2023 03:20:18 -
@@ -230,8 +230,8 @@ dmtimer_cpu_initclocks(void)
{
struct dmtimer_softc*sc = dmtimer_cd.cd_devs[1];
- stathz = 100;
- profhz = 1000;
+ stathz = hz;
+ profhz = stathz * 10;
clockintr_init(CL_RNDSTAT);
sc->sc_ticks_per_second = TIMER_FREQUENCY; /* 32768 */
Re: sshd reorder fails when comp72.tgz not installed
Ah of course. I will move the missing files into base. Brian Conway wrote: > Just a heads up, I was trying a few install variations of -current and > noticed sshd reorder failing on boot on one of them. Unpacking > /usr/share/relink/usr/sbin/sshd/sshd.tar and running `make -f Makefile.relink > relink` by hand yielded: > > ld: error: cannot open crt0.o: No such file or directory > cc: error: linker command failed with exit code 1 (use -v to see invocation) > > Sure enough, that particular install had base72 but not comp72. > > # tar ztf comp72.tgz|grep crt0.o > ./usr/lib/crt0.o > ./usr/lib/gcrt0.o > ./usr/lib/rcrt0.o > > Not sure if this is worth being picky over, as non-compiler installs are > rare/not recommended, but I wanted to make a mention just in case. Thanks. > > dmesg (amd64 VMM): > > OpenBSD 7.2-current (GENERIC) #949: Thu Jan 26 09:54:34 MST 2023 > dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC > real mem = 1593823232 (1519MB) > avail mem = 1526300672 (1455MB) > random: good seed from bootblocks > mpath0 at root > scsibus0 at mpath0: 256 targets > mainbus0 at root > bios0 at mainbus0: SMBIOS rev. 2.4 @ 0xf36e0 (10 entries) > bios0: vendor SeaBIOS version "1.14.0p0-OpenBSD-vmm" date 01/01/2011 > bios0: OpenBSD VMM > acpi at bios0 not configured > cpu0 at mainbus0: (uniprocessor) > cpu0: Intel(R) Core(TM) i5-8259U CPU @ 2.30GHz, 2304.01 MHz, 06-8e-0a > cpu0: > FPU,VME,DE,PSE,TSC,MSR,PAE,CX8,SEP,PGE,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SSE3,PCLMUL,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,PAGE1GB,LONG,LAHF,ABM,3DNOWP,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,RDSEED,ADX,SMAP,CLFLUSHOPT,MD_CLEAR,MELTDOWN > cpu0: 32KB 64b/line 8-way D-cache, 32KB 64b/line 8-way I-cache, 256KB > 64b/line 4-way L2 cache, 6MB 64b/line 12-way L3 cache > cpu0: smt 0, core 0, package 0 > cpu0: using VERW MDS workaround > pvbus0 at mainbus0: OpenBSD > pvclock0 at pvbus0 > pci0 at mainbus0 bus 0 > pchb0 at pci0 dev 0 function 0 "OpenBSD VMM Host" rev 0x00 > virtio0 at pci0 dev 1 function 0 "Qumranet Virtio RNG" rev 0x00 > viornd0 at virtio0 > virtio0: irq 3 > virtio1 at pci0 dev 2 function 0 "Qumranet Virtio Network" rev 0x00 > vio0 at virtio1: address fe:e1:bb:d1:69:3f > virtio1: irq 5 > virtio2 at pci0 dev 3 function 0 "Qumranet Virtio Storage" rev 0x00 > vioblk0 at virtio2 > scsibus1 at vioblk0: 1 targets > sd0 at scsibus1 targ 0 lun 0: > sd0: 24576MB, 512 bytes/sector, 50331648 sectors > virtio2: irq 6 > virtio3 at pci0 dev 4 function 0 "OpenBSD VMM Control" rev 0x00 > vmmci0 at virtio3 > virtio3: irq 7 > isa0 at mainbus0 > isadma0 at isa0 > com0 at isa0 port 0x3f8/8 irq 4: ns8250, no fifo > com0: console > vscsi0 at root > scsibus2 at vscsi0: 256 targets > softraid0 at root > scsibus3 at softraid0: 256 targets > root on sd0a (4d3a796cdb2bafb8.a) swap on sd0b dump on sd0b > > Brian Conway >
sshd reorder fails when comp72.tgz not installed
Just a heads up, I was trying a few install variations of -current and noticed sshd reorder failing on boot on one of them. Unpacking /usr/share/relink/usr/sbin/sshd/sshd.tar and running `make -f Makefile.relink relink` by hand yielded: ld: error: cannot open crt0.o: No such file or directory cc: error: linker command failed with exit code 1 (use -v to see invocation) Sure enough, that particular install had base72 but not comp72. # tar ztf comp72.tgz|grep crt0.o ./usr/lib/crt0.o ./usr/lib/gcrt0.o ./usr/lib/rcrt0.o Not sure if this is worth being picky over, as non-compiler installs are rare/not recommended, but I wanted to make a mention just in case. Thanks. dmesg (amd64 VMM): OpenBSD 7.2-current (GENERIC) #949: Thu Jan 26 09:54:34 MST 2023 dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC real mem = 1593823232 (1519MB) avail mem = 1526300672 (1455MB) random: good seed from bootblocks mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 2.4 @ 0xf36e0 (10 entries) bios0: vendor SeaBIOS version "1.14.0p0-OpenBSD-vmm" date 01/01/2011 bios0: OpenBSD VMM acpi at bios0 not configured cpu0 at mainbus0: (uniprocessor) cpu0: Intel(R) Core(TM) i5-8259U CPU @ 2.30GHz, 2304.01 MHz, 06-8e-0a cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,CX8,SEP,PGE,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,SSE3,PCLMUL,SSSE3,FMA3,CX16,SSE4.1,SSE4.2,MOVBE,POPCNT,AES,XSAVE,AVX,F16C,RDRAND,HV,NXE,PAGE1GB,LONG,LAHF,ABM,3DNOWP,ITSC,FSGSBASE,BMI1,AVX2,SMEP,BMI2,ERMS,RDSEED,ADX,SMAP,CLFLUSHOPT,MD_CLEAR,MELTDOWN cpu0: 32KB 64b/line 8-way D-cache, 32KB 64b/line 8-way I-cache, 256KB 64b/line 4-way L2 cache, 6MB 64b/line 12-way L3 cache cpu0: smt 0, core 0, package 0 cpu0: using VERW MDS workaround pvbus0 at mainbus0: OpenBSD pvclock0 at pvbus0 pci0 at mainbus0 bus 0 pchb0 at pci0 dev 0 function 0 "OpenBSD VMM Host" rev 0x00 virtio0 at pci0 dev 1 function 0 "Qumranet Virtio RNG" rev 0x00 viornd0 at virtio0 virtio0: irq 3 virtio1 at pci0 dev 2 function 0 "Qumranet Virtio Network" rev 0x00 vio0 at virtio1: address fe:e1:bb:d1:69:3f virtio1: irq 5 virtio2 at pci0 dev 3 function 0 "Qumranet Virtio Storage" rev 0x00 vioblk0 at virtio2 scsibus1 at vioblk0: 1 targets sd0 at scsibus1 targ 0 lun 0: sd0: 24576MB, 512 bytes/sector, 50331648 sectors virtio2: irq 6 virtio3 at pci0 dev 4 function 0 "OpenBSD VMM Control" rev 0x00 vmmci0 at virtio3 virtio3: irq 7 isa0 at mainbus0 isadma0 at isa0 com0 at isa0 port 0x3f8/8 irq 4: ns8250, no fifo com0: console vscsi0 at root scsibus2 at vscsi0: 256 targets softraid0 at root scsibus3 at softraid0: 256 targets root on sd0a (4d3a796cdb2bafb8.a) swap on sd0b dump on sd0b Brian Conway
Re: don't remove known vmd vm's on failure
On Sun, Jan 15, 2023 at 09:08:29AM -0500, Dave Voutila wrote:
>
> Dave Voutila writes:
>
> > It turns out not only does vmd have numerous error paths for handling
> > when something is amiss with a guest, most of the paths don't check if
> > it's a known vm defined in vm.conf.
> >
> > As a result, vmd often removes the vm from the SLIST of vm's meaning
> > one can't easily attempt to start it again or see it in vmctl's status
> > output.
> >
> > A simple reproduction:
> >
> > 1. define a vm with memory > 4gb in vm.conf
> > 2. run vmd in the foreground (doas vmd -d) so it's not started by rc.d
> > 3. try to start with `vmctl start -c ${vm_name}`, you should trigger
> > an ENOMEM and get the "Cannot allocate memory" message from vmctl.
> > 4. try to start the same vm again...now you get EPERM!
> > 5. the vm is no longer visible in the output from `vmctl status` :(
> >
> > The problem is most of the error paths call vm_remove, which not only
> > tears down the vm via vm_stop, but also removes it from the vm list and
> > frees it. Only clean stops or restarts seem to perform this check
> > currently.
> >
> > Below diff refactors into checking if the vm is defined in the global
> > config before deciding to call vm_stop or vm_remove.
>
> Slight tweak... __func__->caller to actually pass the correct name to
> vm_{stop,remove}() from vm_terminate()
>
Finally getting caught up. ok mlarkin on this if you didn't commit it
already.
-ml
>
> diff refs/heads/master refs/heads/vmd-accounting
> commit - d4e23fe7544b01187ebf3ac8ae32e955445ee666
> commit + 46503195403bfab50cd34bd8682f35a17d54d03d
> blob - 6bffb2519a31464836aa573dbccb7aa14ea97722
> blob + f30dc14de1ff9d5cf121cbc08b6db183a06d0c07
> --- usr.sbin/vmd/vmd.c
> +++ usr.sbin/vmd/vmd.c
> @@ -67,6 +67,8 @@ struct vmd *env;
> int vm_claimid(const char *, int, uint32_t *);
> void start_vm_batch(int, short, void*);
>
> +static inline void vm_terminate(struct vmd_vm *, const char *);
> +
> struct vmd *env;
>
> static struct privsep_proc procs[] = {
> @@ -395,14 +397,14 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
> errno = vmr.vmr_result;
> log_warn("%s: failed to forward vm result",
> vcp->vcp_name);
> - vm_remove(vm, __func__);
> + vm_terminate(vm, __func__);
> return (-1);
> }
> }
>
> if (vmr.vmr_result) {
> log_warnx("%s: failed to start vm", vcp->vcp_name);
> - vm_remove(vm, __func__);
> + vm_terminate(vm, __func__);
> errno = vmr.vmr_result;
> break;
> }
> @@ -410,7 +412,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
> /* Now configure all the interfaces */
> if (vm_priv_ifconfig(ps, vm) == -1) {
> log_warn("%s: failed to configure vm", vcp->vcp_name);
> - vm_remove(vm, __func__);
> + vm_terminate(vm, __func__);
> break;
> }
>
> @@ -441,10 +443,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
> log_info("%s: sent vm %d successfully.",
> vm->vm_params.vmc_params.vcp_name,
> vm->vm_vmid);
> - if (vm->vm_from_config)
> - vm_stop(vm, 0, __func__);
> - else
> - vm_remove(vm, __func__);
> + vm_terminate(vm, __func__);
> }
>
> /* Send a response if a control client is waiting for it */
> @@ -470,10 +469,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
> }
> if (vmr.vmr_result != EAGAIN ||
> vm->vm_params.vmc_bootdevice) {
> - if (vm->vm_from_config)
> - vm_stop(vm, 0, __func__);
> - else
> - vm_remove(vm, __func__);
> + vm_terminate(vm, __func__);
> } else {
> /* Stop VM instance but keep the tty open */
> vm_stop(vm, 1, __func__);
> @@ -509,7 +505,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
> imsg->hdr.peerid, -1, &vir, sizeof(vir)) == -1) {
> log_debug("%s: GET_INFO_VM failed for vm %d, removing",
> __func__, vm->vm_vmid);
> - vm_remove(vm, __func__);
> + vm_terminate(vm, __func__);
> return (-1);
> }
> break;
> @@ -545,7 +541,7 @@ vmd_dispatch_vmm(int fd, struct privsep_proc *p, struc
>
Replace selinfo structure by klist in sockbuf
No reason to keep it, selinfo is just wrapper to klist. netstat(1) and
libkvm use socket structure, but don't touch
so_{snd,rcv}.sb_sel.si_note.
Index: sys/kern/uipc_socket.c
===
RCS file: /cvs/src/sys/kern/uipc_socket.c,v
retrieving revision 1.297
diff -u -p -r1.297 uipc_socket.c
--- sys/kern/uipc_socket.c 23 Jan 2023 18:34:24 - 1.297
+++ sys/kern/uipc_socket.c 26 Jan 2023 18:39:31 -
@@ -176,8 +176,8 @@ socreate(int dom, struct socket **aso, i
if (prp->pr_type != type)
return (EPROTOTYPE);
so = soalloc(M_WAIT);
- klist_init(&so->so_rcv.sb_sel.si_note, &socket_klistops, so);
- klist_init(&so->so_snd.sb_sel.si_note, &socket_klistops, so);
+ klist_init(&so->so_rcv.sb_klist, &socket_klistops, so);
+ klist_init(&so->so_snd.sb_klist, &socket_klistops, so);
sigio_init(&so->so_sigio);
TAILQ_INIT(&so->so_q0);
TAILQ_INIT(&so->so_q);
@@ -303,8 +303,8 @@ sofree(struct socket *so, int keep_lock)
}
sigio_free(&so->so_sigio);
- klist_free(&so->so_rcv.sb_sel.si_note);
- klist_free(&so->so_snd.sb_sel.si_note);
+ klist_free(&so->so_rcv.sb_klist);
+ klist_free(&so->so_snd.sb_klist);
#ifdef SOCKET_SPLICE
if (so->so_sp) {
if (issplicedback(so)) {
@@ -2095,7 +2095,7 @@ void
sohasoutofband(struct socket *so)
{
pgsigio(&so->so_sigio, SIGURG, 0);
- KNOTE(&so->so_rcv.sb_sel.si_note, 0);
+ KNOTE(&so->so_rcv.sb_klist, 0);
}
int
@@ -2126,7 +2126,7 @@ soo_kqfilter(struct file *fp, struct kno
return (EINVAL);
}
- klist_insert_locked(&sb->sb_sel.si_note, kn);
+ klist_insert_locked(&sb->sb_klist, kn);
sounlock(so);
return (0);
@@ -2137,7 +2137,7 @@ filt_sordetach(struct knote *kn)
{
struct socket *so = kn->kn_fp->f_data;
- klist_remove(&so->so_rcv.sb_sel.si_note, kn);
+ klist_remove(&so->so_rcv.sb_klist, kn);
}
int
@@ -2178,7 +2178,7 @@ filt_sowdetach(struct knote *kn)
{
struct socket *so = kn->kn_fp->f_data;
- klist_remove(&so->so_snd.sb_sel.si_note, kn);
+ klist_remove(&so->so_snd.sb_klist, kn);
}
int
Index: sys/kern/uipc_socket2.c
===
RCS file: /cvs/src/sys/kern/uipc_socket2.c,v
retrieving revision 1.133
diff -u -p -r1.133 uipc_socket2.c
--- sys/kern/uipc_socket2.c 22 Jan 2023 12:05:44 - 1.133
+++ sys/kern/uipc_socket2.c 26 Jan 2023 18:39:31 -
@@ -226,8 +226,8 @@ sonewconn(struct socket *head, int conns
so->so_rcv.sb_lowat = head->so_rcv.sb_lowat;
so->so_rcv.sb_timeo_nsecs = head->so_rcv.sb_timeo_nsecs;
- klist_init(&so->so_rcv.sb_sel.si_note, &socket_klistops, so);
- klist_init(&so->so_snd.sb_sel.si_note, &socket_klistops, so);
+ klist_init(&so->so_rcv.sb_klist, &socket_klistops, so);
+ klist_init(&so->so_snd.sb_klist, &socket_klistops, so);
sigio_init(&so->so_sigio);
sigio_copy(&so->so_sigio, &head->so_sigio);
@@ -262,8 +262,8 @@ sonewconn(struct socket *head, int conns
if (persocket)
sounlock(so);
sigio_free(&so->so_sigio);
- klist_free(&so->so_rcv.sb_sel.si_note);
- klist_free(&so->so_snd.sb_sel.si_note);
+ klist_free(&so->so_rcv.sb_klist);
+ klist_free(&so->so_snd.sb_klist);
pool_put(&socket_pool, so);
return (NULL);
}
@@ -549,7 +549,7 @@ sowakeup(struct socket *so, struct sockb
}
if (sb->sb_flags & SB_ASYNC)
pgsigio(&so->so_sigio, SIGIO, 0);
- KNOTE(&sb->sb_sel.si_note, 0);
+ KNOTE(&sb->sb_klist, 0);
}
/*
Index: sys/kern/uipc_syscalls.c
===
RCS file: /cvs/src/sys/kern/uipc_syscalls.c,v
retrieving revision 1.209
diff -u -p -r1.209 uipc_syscalls.c
--- sys/kern/uipc_syscalls.c22 Jan 2023 12:05:44 - 1.209
+++ sys/kern/uipc_syscalls.c26 Jan 2023 18:39:31 -
@@ -326,7 +326,7 @@ doaccept(struct proc *p, int sock, struc
: (flags & SOCK_NONBLOCK ? FNONBLOCK : 0);
/* connection has been removed from the listen queue */
- KNOTE(&head->so_rcv.sb_sel.si_note, 0);
+ KNOTE(&head->so_rcv.sb_klist, 0);
if (persocket)
sounlock(head);
Index: sys/miscfs/fifofs/fifo_vnops.c
===
RCS file: /cvs/src/sys/miscfs/fifofs/fifo_vnops.c,v
retrieving revision 1.100
diff -u -p -r1.100 fifo_vnops.c
--- sys/miscfs/fifofs/fifo_vnops.c 22 Jan 2023 12:05:44 - 1.100
+++ sys/miscfs/fifofs/fifo_vnops.c 26 Jan 2023 18:39:31 -
@@ -504,7 +504,7 @@ fifo_kqfilter(void *v)
ap->a_kn->kn_hook = so;
- klist_insert(&sb->sb_sel.si_n
Re: strptime.c
On Thu, Jan 26, 2023 at 10:37 AM Todd C. Miller wrote: > On Thu, 26 Jan 2023 10:29:36 -0800, enh wrote: > > > yeah, but that's the copy & paste-o, no? (apologies if it's just too > early > > for me to be looking at code yet...) > > > > doesn't this need to be int64_t? > > > > int result = 0; > > Yes, it does, thanks. > > > https://github.com/openbsd/src/blob/master/lib/libc/time/strptime.c#L613 > > > > (i think the overflow checks are insufficient in both copies of the > > function too, especially around the min and max values, but this seems > like > > the biggest problem with the code.) > > The checks for underflow/overflow appear to be insufficient. > My inclination is to just convert everything to use strtonum(3). > yeah, that's one thing i liked the freebsd variant --- "don't make me think" :-) a quick glance at strtonum() lgtm. > - todd >
Re: Replace selwakeup() with KNOTE(9) in pppx(4) and pppac(4) layers
On Thu, Jan 26, 2023 at 03:28:07PM +, Visa Hankala wrote: > On Thu, Jan 26, 2023 at 01:57:56AM +0300, Vitaliy Makkoveev wrote: > > On Wed, Jan 25, 2023 at 10:43:50PM +0300, Vitaliy Makkoveev wrote: > > > visa@, mpi@, I'm asking you to review, because you are involved in the > > > kevent(9) development. > > > > > > Hrvoje, if you want to test this diff, you need to disable pipex(4) with > > > "net.pipex.enable=0". > > > > > > > I missed we already have klist_init_mutex(), so the redundant *_klistops > > removed from previous diff. > > I have a similar patch lying around, shown below. The main differences: > > * Use the same mutex for both read and write side of the klists. > Having dedicated mutexes feels overkill. > > * Use klist_insert() and klist_remove() for shorter code. > > * Share f_modify and f_process functions for read and write filters. > Looks more reasonable, ok by me.
Re: strptime.c
On Thu, 26 Jan 2023 10:29:36 -0800, enh wrote: > yeah, but that's the copy & paste-o, no? (apologies if it's just too early > for me to be looking at code yet...) > > doesn't this need to be int64_t? > > int result = 0; Yes, it does, thanks. > https://github.com/openbsd/src/blob/master/lib/libc/time/strptime.c#L613 > > (i think the overflow checks are insufficient in both copies of the > function too, especially around the min and max values, but this seems like > the biggest problem with the code.) The checks for underflow/overflow appear to be insufficient. My inclination is to just convert everything to use strtonum(3). - todd
Re: strptime.c
On Thu, Jan 26, 2023 at 9:42 AM Todd C. Miller wrote: > On Thu, 26 Jan 2023 09:36:52 -0800, enh wrote: > > > it's quite possible that this could use _conv_num64(), but it wasn't > > obvious to me that that function's correct? (i haven't thought too hard > > about the overflow logic, but just the fact that result is an `int` seems > > odd?) > > It just returns a boolean value, 1 for OK, 0 for not OK. There is > a result parameter for the output value. The code is effectively > the same as _conv_num. I dislike that it uses int64_t instead of > time_t though. > yeah, but that's the copy & paste-o, no? (apologies if it's just too early for me to be looking at code yet...) doesn't this need to be int64_t? int result = 0; https://github.com/openbsd/src/blob/master/lib/libc/time/strptime.c#L613 (i think the overflow checks are insufficient in both copies of the function too, especially around the min and max values, but this seems like the biggest problem with the code.) > - todd >
mg: M-x grep: don't append /dev/null to cmdline
One (midly annoying) feature of mg is that it appends /dev/null to the
grep invocation. I guess this was done so grep is never called with
only one file argument and so it always displays the filename in its
output.
However, this gets a bit annoying when one only types "grep -n -R foo"
and yields no results because only /dev/null was consulted.
My proposal is to add -H to the mix instead.
This poses a problem: it gets too easy to "hang" mg by forgetting the
path to the files: grep would read from stdin and since inherits the
cooked mode ^D and ^C don't work. Diff below works around the problem
in the easier way: redirecting stdin from /dev/null. (a follow-up
patch could rework compile_mode and clean it up a bit, avoiding the
cd'ing back and forth too, by just forking a child and redirect its
stdout with a pipe.)
(note that this issue is already present: M-x grep RET C-u "cat -" RET)
ok?
diff /home/op/w/mg.orig
commit - b83db95072ea94d64c0bb6027c4b3478e3400e5c
path + /home/op/w/mg.orig
blob - 016256f64d06be49304999fa4665018e4f823d31
file + grep.c
--- grep.c
+++ grep.c
@@ -69,14 +69,12 @@ grep(int f, int n)
struct buffer *bp;
struct mgwin*wp;
- (void)strlcpy(cprompt, "grep -n ", sizeof(cprompt));
+ (void)strlcpy(cprompt, "grep -Hn ", sizeof(cprompt));
if ((bufp = eread("Run grep: ", cprompt, NFILEN,
EFDEF | EFNEW | EFCR)) == NULL)
return (ABORT);
else if (bufp[0] == '\0')
return (FALSE);
- if (strlcat(cprompt, " /dev/null", sizeof(cprompt)) >= sizeof(cprompt))
- return (FALSE);
if ((bp = compile_mode("*grep*", cprompt)) == NULL)
return (FALSE);
@@ -189,7 +187,7 @@ compile_mode(const char *name, const char *command)
buf = NULL;
sz = 0;
- n = snprintf(qcmd, sizeof(qcmd), "%s 2>&1", command);
+ n = snprintf(qcmd, sizeof(qcmd), "%s 2>&1 = sizeof(qcmd))
return (NULL);
Re: strptime.c
On Thu, 26 Jan 2023 09:36:52 -0800, enh wrote: > it's quite possible that this could use _conv_num64(), but it wasn't > obvious to me that that function's correct? (i haven't thought too hard > about the overflow logic, but just the fact that result is an `int` seems > odd?) It just returns a boolean value, 1 for OK, 0 for not OK. There is a result parameter for the output value. The code is effectively the same as _conv_num. I dislike that it uses int64_t instead of time_t though. - todd
Re: strptime.c
well, the + if (bp == old_bp || errno == ERANGE || +((long) t) != n) return NULL; bit is the "most active ingredient" here, deliberately failing (rather than wrapping) if out of 32-bit range. it's quite possible that this could use _conv_num64(), but it wasn't obvious to me that that function's correct? (i haven't thought too hard about the overflow logic, but just the fact that result is an `int` seems odd?) actually, though, checking Android's git history, it seems like the strtol() came from FreeBSD along with the localtime_r() :-) (we only support ILP32 and LP64, so strtol() is fine, i think? on a 64-bit system it's 64-bit which is plenty, and on a 32-bit system, we'll get ERANGE for stuff that won't fit into 32 bits, which is also the desired behavior.) On Thu, Jan 26, 2023 at 6:49 AM Todd C. Miller wrote: > Is there a reason you didn't just change the gmtime_r() in the 's' > case to localtime_r()? That seems like the simplest fix. Using > strtol() for what may be a 64-bit value on an 32-bit system looks > wrong. > > - todd >
Re: Replace selwakeup() with KNOTE(9) in pppx(4) and pppac(4) layers
On Thu, Jan 26, 2023 at 01:57:56AM +0300, Vitaliy Makkoveev wrote:
> On Wed, Jan 25, 2023 at 10:43:50PM +0300, Vitaliy Makkoveev wrote:
> > visa@, mpi@, I'm asking you to review, because you are involved in the
> > kevent(9) development.
> >
> > Hrvoje, if you want to test this diff, you need to disable pipex(4) with
> > "net.pipex.enable=0".
> >
>
> I missed we already have klist_init_mutex(), so the redundant *_klistops
> removed from previous diff.
I have a similar patch lying around, shown below. The main differences:
* Use the same mutex for both read and write side of the klists.
Having dedicated mutexes feels overkill.
* Use klist_insert() and klist_remove() for shorter code.
* Share f_modify and f_process functions for read and write filters.
As pppx and pppac do not seem to have forced detaching, the calls of
klist_invalidate() are not needed.
Mutex assertions could be added in filt_pppx_write() and
filt_pppac_write() to make the locking assumption more explicit
(the code that calls these functions needs the lock). However, I have
omitted these assertions as they might not add much value.
diff --git a/sys/net/if_pppx.c b/sys/net/if_pppx.c
index a59c932d276..e4675973a06 100644
--- a/sys/net/if_pppx.c
+++ b/sys/net/if_pppx.c
@@ -56,7 +56,8 @@
#include
#include
#include
-#include
+#include
+#include
#include
#include
@@ -118,6 +119,7 @@ struct pppx_if;
* I immutable after creation
* K kernel lock
* N net lock
+ * m pxd_mtx
*/
struct pppx_dev {
@@ -125,10 +127,9 @@ struct pppx_dev {
int pxd_unit; /* [I] */
/* kq shizz */
- struct selinfo pxd_rsel;
- struct mutexpxd_rsel_mtx;
- struct selinfo pxd_wsel;
- struct mutexpxd_wsel_mtx;
+ struct mutexpxd_mtx;
+ struct klistpxd_rklist; /* [m] */
+ struct klistpxd_wklist; /* [m] */
/* queue of packets for userland to service - protected by splnet */
struct mbuf_queue pxd_svcq;
@@ -195,22 +196,28 @@ void pppxattach(int);
void filt_pppx_rdetach(struct knote *);
intfilt_pppx_read(struct knote *, long);
+intfilt_pppx_modify(struct kevent *, struct knote *);
+intfilt_pppx_process(struct knote *, struct kevent *);
const struct filterops pppx_rd_filtops = {
- .f_flags= FILTEROP_ISFD,
+ .f_flags= FILTEROP_ISFD | FILTEROP_MPSAFE,
.f_attach = NULL,
.f_detach = filt_pppx_rdetach,
.f_event= filt_pppx_read,
+ .f_modify = filt_pppx_modify,
+ .f_process = filt_pppx_process,
};
void filt_pppx_wdetach(struct knote *);
intfilt_pppx_write(struct knote *, long);
const struct filterops pppx_wr_filtops = {
- .f_flags= FILTEROP_ISFD,
+ .f_flags= FILTEROP_ISFD | FILTEROP_MPSAFE,
.f_attach = NULL,
.f_detach = filt_pppx_wdetach,
.f_event= filt_pppx_write,
+ .f_modify = filt_pppx_modify,
+ .f_process = filt_pppx_process,
};
struct pppx_dev *
@@ -257,8 +264,9 @@ pppxopen(dev_t dev, int flags, int mode, struct proc *p)
}
pxd->pxd_unit = minor(dev);
- mtx_init(&pxd->pxd_rsel_mtx, IPL_NET);
- mtx_init(&pxd->pxd_wsel_mtx, IPL_NET);
+ mtx_init(&pxd->pxd_mtx, IPL_NET);
+ klist_init_mutex(&pxd->pxd_rklist, &pxd->pxd_mtx);
+ klist_init_mutex(&pxd->pxd_wklist, &pxd->pxd_mtx);
LIST_INIT(&pxd->pxd_pxis);
mq_init(&pxd->pxd_svcq, 128, IPL_NET);
@@ -453,29 +461,24 @@ int
pppxkqfilter(dev_t dev, struct knote *kn)
{
struct pppx_dev *pxd = pppx_dev2pxd(dev);
- struct mutex *mtx;
struct klist *klist;
switch (kn->kn_filter) {
case EVFILT_READ:
- mtx = &pxd->pxd_rsel_mtx;
- klist = &pxd->pxd_rsel.si_note;
+ klist = &pxd->pxd_rklist;
kn->kn_fop = &pppx_rd_filtops;
break;
case EVFILT_WRITE:
- mtx = &pxd->pxd_wsel_mtx;
- klist = &pxd->pxd_wsel.si_note;
+ klist = &pxd->pxd_wklist;
kn->kn_fop = &pppx_wr_filtops;
break;
default:
return (EINVAL);
}
- kn->kn_hook = (caddr_t)pxd;
+ kn->kn_hook = pxd;
- mtx_enter(mtx);
- klist_insert_locked(klist, kn);
- mtx_leave(mtx);
+ klist_insert(klist, kn);
return (0);
}
@@ -483,18 +486,17 @@ pppxkqfilter(dev_t dev, struct knote *kn)
void
filt_pppx_rdetach(struct knote *kn)
{
- struct pppx_dev *pxd = (struct pppx_dev *)kn->kn_hook;
- struct klist *klist = &pxd->pxd_rsel.si_note;
+ struct pppx_dev *pxd = kn->kn_hook;
- mtx_enter(&pxd->pxd_rsel_mtx);
Re: strptime.c
Is there a reason you didn't just change the gmtime_r() in the 's' case to localtime_r()? That seems like the simplest fix. Using strtol() for what may be a 64-bit value on an 32-bit system looks wrong. - todd
Re: refactor mbuf parsing on driver level
On Thu, Jan 26, 2023 at 02:06:28PM +0300, Vitaliy Makkoveev wrote: > On Thu, Jan 26, 2023 at 11:37:51AM +0100, Christian Weisgerber wrote: > > Jan Klemkow: > > > > > we have several drivers which have to parse the content of mbufs. This > > > diff suggest a central parsing function for this. Thus, we can reduce > > > redundant code. > > > > > > I just start with ix(4) and ixl(4) because it was easy to test for me. > > > But, this could also improve em(4), igc(4), ale(4) and oce(4). > > > > Here's the corresponding change for em(4). > > This only affects 82575, 82576, i350, and i210. > > Tested on i210. > > > > ok? > > > > The ether_extract_headers() diff was reverted, because is wrong for the > cases other than tcp/udp/icmp. We need to fix it and recommit again > before continue. I'm already on the way, to fix this mess. I'll send a new diff soon. Sorry this inconvenience, Jan
Re: refactor mbuf parsing on driver level
On Thu, Jan 26, 2023 at 11:37:51AM +0100, Christian Weisgerber wrote: > Jan Klemkow: > > > we have several drivers which have to parse the content of mbufs. This > > diff suggest a central parsing function for this. Thus, we can reduce > > redundant code. > > > > I just start with ix(4) and ixl(4) because it was easy to test for me. > > But, this could also improve em(4), igc(4), ale(4) and oce(4). > > Here's the corresponding change for em(4). > This only affects 82575, 82576, i350, and i210. > Tested on i210. > > ok? > The ether_extract_headers() diff was reverted, because is wrong for the cases other than tcp/udp/icmp. We need to fix it and recommit again before continue.
Re: refactor mbuf parsing on driver level
Jan Klemkow:
> we have several drivers which have to parse the content of mbufs. This
> diff suggest a central parsing function for this. Thus, we can reduce
> redundant code.
>
> I just start with ix(4) and ixl(4) because it was easy to test for me.
> But, this could also improve em(4), igc(4), ale(4) and oce(4).
Here's the corresponding change for em(4).
This only affects 82575, 82576, i350, and i210.
Tested on i210.
ok?
consolidate mbuf header parsing on device driver layer
M sys/dev/pci/if_em.c | 12+ 34-
1 file changed, 12 insertions(+), 34 deletions(-)
diff 7c601e62aaac9d63b4ccf68f98d354af760b41a9
1c186b4a689624833baafe23d82bb535c4d5ad93
commit - 7c601e62aaac9d63b4ccf68f98d354af760b41a9
commit + 1c186b4a689624833baafe23d82bb535c4d5ad93
blob - 581c9fc2647dfed3e8514ab40b2a30885f63d752
blob + 8f25ef5cc5f18c4540aa5b44df9bad1e64d375bd
--- sys/dev/pci/if_em.c
+++ sys/dev/pci/if_em.c
@@ -2398,12 +2398,11 @@ em_tx_ctx_setup(struct em_queue *que, struct mbuf *mp,
em_tx_ctx_setup(struct em_queue *que, struct mbuf *mp, u_int head,
u_int32_t *olinfo_status, u_int32_t *cmd_type_len)
{
+ struct ether_extracted ext;
struct e1000_adv_tx_context_desc *TD;
- struct ether_header *eh = mtod(mp, struct ether_header *);
- struct mbuf *m;
uint32_t vlan_macip_lens = 0, type_tucmd_mlhl = 0, mss_l4len_idx = 0;
- int off = 0, hoff;
- uint8_t ipproto, iphlen;
+ int off = 0;
+ uint8_t iphlen;
*olinfo_status = 0;
*cmd_type_len = 0;
@@ -2418,44 +2417,26 @@ em_tx_ctx_setup(struct em_queue *que, struct mbuf *mp,
}
#endif
- vlan_macip_lens |= (sizeof(*eh) << E1000_ADVTXD_MACLEN_SHIFT);
-
- switch (ntohs(eh->ether_type)) {
- case ETHERTYPE_IP: {
- struct ip *ip;
+ ether_extract_headers(mp, &ext);
- m = m_getptr(mp, sizeof(*eh), &hoff);
- ip = (struct ip *)(mtod(m, caddr_t) + hoff);
+ vlan_macip_lens |= (sizeof(*ext.eh) << E1000_ADVTXD_MACLEN_SHIFT);
- iphlen = ip->ip_hl << 2;
- ipproto = ip->ip_p;
+ if (ext.ip4) {
+ iphlen = ext.ip4->ip_hl << 2;
type_tucmd_mlhl |= E1000_ADVTXD_TUCMD_IPV4;
if (ISSET(mp->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT)) {
*olinfo_status |= E1000_TXD_POPTS_IXSM << 8;
off = 1;
}
-
- break;
- }
#ifdef INET6
- case ETHERTYPE_IPV6: {
- struct ip6_hdr *ip6;
+ } else if (ext.ip6) {
+ iphlen = sizeof(*ext.ip6);
- m = m_getptr(mp, sizeof(*eh), &hoff);
- ip6 = (struct ip6_hdr *)(mtod(m, caddr_t) + hoff);
-
- iphlen = sizeof(*ip6);
- ipproto = ip6->ip6_nxt;
-
type_tucmd_mlhl |= E1000_ADVTXD_TUCMD_IPV6;
- break;
- }
#endif
- default:
+ } else {
iphlen = 0;
- ipproto = 0;
- break;
}
*cmd_type_len |= E1000_ADVTXD_DTYP_DATA | E1000_ADVTXD_DCMD_IFCS;
@@ -2464,21 +2445,18 @@ em_tx_ctx_setup(struct em_queue *que, struct mbuf *mp,
vlan_macip_lens |= iphlen;
type_tucmd_mlhl |= E1000_ADVTXD_DCMD_DEXT | E1000_ADVTXD_DTYP_CTXT;
- switch (ipproto) {
- case IPPROTO_TCP:
+ if (ext.tcp) {
type_tucmd_mlhl |= E1000_ADVTXD_TUCMD_L4T_TCP;
if (ISSET(mp->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) {
*olinfo_status |= E1000_TXD_POPTS_TXSM << 8;
off = 1;
}
- break;
- case IPPROTO_UDP:
+ } else if (ext.udp) {
type_tucmd_mlhl |= E1000_ADVTXD_TUCMD_L4T_UDP;
if (ISSET(mp->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) {
*olinfo_status |= E1000_TXD_POPTS_TXSM << 8;
off = 1;
}
- break;
}
if (!off)
--
Christian "naddy" Weisgerber na...@mips.inka.de
