In D38898#885672, @freebsd_igalic.co wrote:

In D38898#885654, @jmg wrote:

So, I think what might be wrong is that you don't have another virtio driver to "unstick" the virtio queue. FreeBSD only has drivers for balloon, block, console, network, entropy and scsi. I looked at your config, and the only other device you define that is supported by FreeBSD is balloon. In my config, I'm using both network and block so it's possible that it's working here only because there is additional disk/network traffic causing the rng events to get dequeued...

you can see (part? of) my config at https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=269874
so there is a network driver, but there might not have been much going on

regardless, the fact that virtio_random requires some other driver to unstick its poll queue sounds like a bug in our virtio implementation.

In D38898#885646, @freebsd_igalic.co wrote:

rebuilt with this patch, here's the disas:

`Dump of assembler code for function vtrnd_read:

0xffffffff82b05340 <+0>:     55      push   %rbp
0xffffffff82b05341 <+1>:     48 89 e5        mov    %rsp,%rbp
0xffffffff82b05344 <+4>:     41 57   push   %r15
0xffffffff82b05346 <+6>:     41 56   push   %r14
0xffffffff82b05348 <+8>:     41 54   push   %r12
0xffffffff82b0534a <+10>:    53      push   %rbx
0xffffffff82b0534b <+11>:    48 83 ec 10     sub    $0x10,%rsp
0xffffffff82b0534f <+15>:    48 8b 1d 3a 1f 00 00    mov    0x1f3a(%rip),%rbx        # 0xffffffff82b07290 <g_vtrnd_softc>
0xffffffff82b05356 <+22>:    45 31 f6        xor    %r14d,%r14d
0xffffffff82b05359 <+25>:    48 85 db        test   %rbx,%rbx
0xffffffff82b0535c <+28>:    74 4e   je     0xffffffff82b053ac <vtrnd_read+108>
0xffffffff82b0535e <+30>:    80 7b 20 00     cmpb   $0x0,0x20(%rbx)
0xffffffff82b05362 <+34>:    75 48   jne    0xffffffff82b053ac <vtrnd_read+108>
0xffffffff82b05364 <+36>:    41 89 f4        mov    %esi,%r12d
0xffffffff82b05367 <+39>:    49 89 ff        mov    %rdi,%r15
0xffffffff82b0536a <+42>:    48 8b 7b 10     mov    0x10(%rbx),%rdi
0xffffffff82b0536e <+46>:    48 8d 75 dc     lea    -0x24(%rbp),%rsi
0xffffffff82b05372 <+50>:    e8 29 be f0 fd  call   0xffffffff80a111a0 <virtqueue_dequeue>
0xffffffff82b05377 <+55>:    48 85 c0        test   %rax,%rax
0xffffffff82b0537a <+58>:    74 30   je     0xffffffff82b053ac <vtrnd_read+108>

[...]

0xffffffff82b053ac <+108>:   44 89 f0        mov    %r14d,%eax
0xffffffff82b053af <+111>:   48 83 c4 10     add    $0x10,%rsp
0xffffffff82b053b3 <+115>:   5b      pop    %rbx
0xffffffff82b053b4 <+116>:   41 5c   pop    %r12
0xffffffff82b053b6 <+118>:   41 5e   pop    %r14
0xffffffff82b053b8 <+120>:   41 5f   pop    %r15
0xffffffff82b053ba <+122>:   5d      pop    %rbp
0xffffffff82b053bb <+123>:   c3      ret

End of assembler dump.

This change was inspired by noticing that CPU usage was significantly increased when running w/ the virtio_random module loaded. W/o it loaded, the harvest thread was consuming ~.02% cpu, but when it was loaded, it was consuming ~1% CPU (and there's a bug report of it consuming 100% cpu: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=269823 which this would partially fix).

not sure how it included a file AFTER the commit hash, but arc is
weird.. hopefully this will work..

done.

implement kib's comments

wrong change made it, this fully addresses comments

address style comments

Ok, will update the patch in a second, also forgot to include the output of the result:

root@freebsd:~ # kenv | grep ^smbios
smbios.bios.reldate="02/06/2015"
smbios.bios.revision="0.0"
smbios.bios.vendor="EFI Development Kit II / OVMF"
smbios.bios.version="0.0.0"
smbios.chassis.maker="QEMU"
smbios.chassis.type="Other"
smbios.chassis.version="virt-7.1"
smbios.memory.enabled="4194304"
smbios.socket.enabled="1"
smbios.socket.populated="1"
smbios.system.maker="QEMU"
smbios.system.product="QEMU Virtual Machine"
smbios.system.version="virt-7.1"
smbios.version="3.0"

So, spent a few minutes looking into this, my testing was done on:

FreeBSD gate4 14.0-CURRENT FreeBSD 14.0-CURRENT #0 main-n255198-1907e1c07c3: Thu May  5 07:52:56 UTC 2022     root@releng1.nyi.freebsd.org:/usr/obj/usr/src/amd64.amd64/sys/GENERIC amd64

New patch has addressed a few of the comments.

Addresses a few of the comments.

Thanks, will address a few of the comments.

I'll second the usage of Em or Sy, I'm a bit nervous about Sy, being symbolic, and how it'll format when using a proportional font (e.g. printed).

Thanks for catching the compile issue. I hacked the modules to compile w/ debug, so missed the bug compiling w/o.

• make compile w/o USB_DEBUG defined...

update to support VLAN MTU...

Let me know more about the CDCE_IND_SIZE_MAX constant.

No, this needs to be updated to the new website.

looks good.

In D29748#667598, @trasz wrote:

Makes perfect sense, but growfs depends on awk(1), which might be on a separate /usr, which gets mounted by mountcritlocal, which runs after root.

I wonder if it would be possible to somehow extend geom(8) to make the walk through the hierarchy easier, to not require complicated text processing?

In D29748#667254, @trasz wrote:

Do you remember any details about the bug, or the testing setup?

The problem with current ordering is that it's not really defined when the sysctl script runs - it has no dependencies, so it's up to rcorder(8), and from what I've seen it's not very stable about sorting.

Looking at the comment in the original commit:

# TODO: Figure out where this should really be ordered.
# I suspect it should go just after fsck but before mountcritlocal
# but it's hard to tell for sure because of the bug described
# below.

the phrase "This file derived from" has no history in our tree, I'd prefer the phrase I proposed.

Thank you.

Ok, the results from an arm64 run (A53, Pine64 A64-LTS).

In D29007#650040, @cem wrote:

In D29007#649973, @jmg wrote:

In D29007#649700, @cem wrote:

Is 128 a reasonable chunk here? How does it relate to the internal buffering of FILE?

I'm going to run some perf numbers on ARM, but w/ the attached file w/ benchmarks, it looks like the chunk should be closer to 16.

That sounds about right if we're optimizing for single conversions (%d, %g, whatever) at the start of long input strings. Does the small size penalize larger conversions much? It seems like we'll be bouncing around _vsscanf_read every N bytes now. (I wonder how common the GTA use case actually is.)

In D29007#649700, @cem wrote:

Is 128 a reasonable chunk here? How does it relate to the internal buffering of FILE?

strlen tests pass:

freebsd@test:/usr/tests $ kyua test lib/libc/string/strlen_test
lib/libc/string/strlen_test:strlen_basic  ->  passed  [0.004s]
lib/libc/string/strlen_test:strlen_huge  ->  passed  [0.027s]
lib/libc/string/strlen_test:strnlen_basic  ->  passed  [0.004s]

address comments.

• address brooks's comments,

style(9) uses \0 exclusively, so I guess I'll change.

• use a more common spelling of NUL...

Attached is a set of benchmarks. First number is chunk size, second number is number of 0's in the string that is sscanf'd.
Bench (results from an amd64, AMD PRO A10-8770E, 2.8GHz):

The strnlen will be committed as a separate commit, likely before this change, as this change depends upon it for performance.

As "more" correct term was never provided, and phk also didn't like this change, no point in keeping this open.

I have no plans to work on this in the future. Someone else is free to take this over and get it committed.

In D26538#632510, @emaste wrote:

Compat with what? There is no code in the FreeBSD source tree that uses it, and no releases have this ioctl. It hasn't even been merged to 12-stable yet.

Future backwards compat - i.e. introduce the new ioctl if and when we extend if_data.

In D26538#632352, @markj wrote:

In D26538#632166, @jmg wrote:

I know this was closed and committed, but I want to get some more discussion on this.

I happen to be reviewing this for my own code (to get at _hwassist), and noticed that the code does not check how much space the program allocated for the if_data structure. This means that as the interface stands, it cannot be expanded, ever in the future, as it'll break old programs when they use this.

Why can't we change the ioctl number and keep the old one for compat?

I know this was closed and committed, but I want to get some more discussion on this.

can't say how correct this is, but it'll be great to have it fixed!

Yes, can we please get this committed ASAP? I just spent some time debugging this problem. (Other solution would be for base not to read /etc/make.conf, but I have a feeling this commit is more likely.)

• minor spelling and grammar fixups. (was originally included)
• revert this change... (release.sh)
• remove whitespace at end of the copyright lines..