In D59355#1363034, @ali_mashtizadeh.com wrote:

Yes it’s non-standard but we already depend on a number of gnu extensions in our code. I think is just an error with the build flags, if you just set the length to 0 or 1 and it’s also accepted by gcc.

In D59380#1362825, @des wrote:

It will be significantly faster without locking.

In D59380#1362615, @des wrote:

In D59380#1362584, @rlibby wrote:

My understanding of what is making my use case slow (metalog on nfs) is that releasing the advisory lock results in a flush and nfs commit. This patch tries to reduce that by at least reducing the advisory lock acquire/release to once per install invocation (which admittedly only helps for the minority of cases where the install command line has multiple files). That is, I'm not exactly trying to reduce the stdio flushes, I'm trying to reduce the nfs commits. I don't think changing stdio buffering by itself would change this at all (and note the location of the fflush(), under the advisory lock). Getting rid of the advisory locks would probably make this much faster, regardless of stdio buffering, but I think that would break parallel install using the same metalog.

I glossed over the diff, I didn't realize the metalog was locked. That shouldn't be necessary as long as it's opened in append mode and line-buffered. I would suggest running some tests with just the locking and flushes removed and setvbuf(metafp, NULL, _IOLBF, 0) added.

@brd do you have an opinion on this?

In D59380#1362364, @des wrote:

I wonder if we could achieve the same goal more cleanly by adding a multi-line buffering mode to stdio. Call it _IOMBF and make it work like fully buffered, except that when the buffer is full we only flush up to the last newline in the buffer, and keep the leftovers. That probably means the last line is lost on close if not terminated, but that's fine if we document it.

ziaee feedback: use a zero-width space to keep the linter happy

Looks good.

In D59354#1362330, @ngie wrote:

A fix was submitted for this already: https://reviews.freebsd.org/D59346 .

Oops, I missed that too.

Looks good.

In D58336#1338226, @olce wrote:

Fine, though please note I have a lot of rework of hwpstate_intel(4) incoming, in particular dropping this hybrid core detection code, so if you plan more/significant work in this area please liaise with me.

Refresh after conflict resolution for 02c440e20404 ("hwpstate_intel: Minimize ifdef for i386 build")

Comparing to sys/x86/include/bus.h and sys/dev/ntb/ntb_hw/ntb_hw_*.c, they just call the bus_space_tag_t parameter tag, and sys/x86/include/bus.h calls the bus_space_handle_t parameter bsh while ntb calls it handle. I think you could simplify those names. A verbatim copy of ntb's definitions would seem fine.

In D58318#1337993, @jrtc27 wrote:

D58301 seems a better alternative to this?

kib feedback and earlier olce comment: CPU_FOREACH

kib feedback from D58336: just provide pc_small_core on i386

In D58332#1337715, @koinec_yahoo.co.jp wrote:

Thank you very much for creating this.
I failed to notice the missing handler for i386 when I made my initial proposal. I apologize for that.

In D58316#1337420, @markj wrote:

In D58316#1337411, @rlibby wrote:

Maybe one day we should plumb it for malloc_dbg / memguard_alloc too.

Do you use memguard at all? I haven't needed to reach for it since KASAN was added.

Maybe one day we should plumb it for malloc_dbg / memguard_alloc too.

I also wonder if we should have an option to add this redzone without KASAN. We could come up with a cheap-ish debugging feature to verify the redzone with uma_junk when we don't have KASAN. Not asking for that in this patch.

Add an assertion to item_ctor() that the returned item is properly
aligned. I couldn't see any pre-existing checks which verify this.

The other thing that SMR does in this respect is it puts free buckets at the tail of uzd_buckets in zone_put_bucket. Did you think about whether you want that for KASAN also, to extend the length of time before reuse further?

Looks good.

Looks good.

Looks fine modulo the uz_flags comment and style and history nits.

In D58268#1336651, @markj wrote:

In D58268#1336646, @rlibby wrote:

BTW git arc can fetch patches from phabricator and apply them to a local tree, though of course that can fail, e.g., if the patch in phabricator needs a rebase. The following invocation will fetch and apply all of the patches in this series:

$ git arc patch -c -s -r D58272

Kind of cumbersome versus fetching a git branch, but hey...

Style nits, but logic looks good.

Just for review convenience, do you have a public git branch with the patch set?

Logic LGTM.

kib feedback

tweak comment and log message

markj feedback

whitespace fixup

So, why mess with the types (int -> bool and int -> u_long)? Strictly speaking it is not necessary and I can revert it if you prefer. I note though that there is no TUNABLE_UINT() and a comment for TUNABLE_INT() suggests not to use it. I don't know why this is, but just converting to bool and u_long seems fine anyway.

Addressing problems uncovered in testing. I had assumed that the
mechanism used by debug.witness.witness_count actually worked, but it
didn't. I added a new patch D57613 to address the problem with
witness_count by itself, and then applied that fix here too. I also
fixed an alignment problem.

Looks good. I presume you've tested it?

Abandoning this for now, though I still think it would be fine to take. I addressed some of the underlying issues with D56894 and D56946.