In D59910#1376490, @kib wrote:

What do you mean by inconsistency? Do you want _MNTPOINT when fhstat("/")?

In D59910#1376450, @olce wrote:

EDIT: Mmm... but this shouldn't be a problem inasmuch as our specific flags are not sent over NFS.

In D59910#1376391, @markj wrote:

No, you can get a handle with getfh() and friends, and if you're using NFS, you don't even need to be root to obtain one.

You need PRIV_VFS_GETFH in any case, so I don't see how this is true.

In D59910#1376145, @kib wrote:

Reporting root of a file system is different from reporting a mount point. And the request was to provide the feature to identify mount points.

In D59910#1376112, @kib wrote:

In D59910#1376084, @olce wrote

The check on VIRF_MOUNTPOINT is indeed necessary if the handle points to an underlying vnode, but the check on VV_ROOT should be performed also, if for anything for consistency with fstatat().

No, VV_ROOT vnode is not a mountpoint when it is instantiated by ino number, instead of lookup. It is below mountpoint.

In D59910#1375689, @markj wrote:

Should kern_fhstat() be updated too?

In D59910#1375235, @kib wrote:

post hook there would be a significant obfuscation.

It's not particularly pretty that this common code is duplicated, even if there are only a few calls to VOP_STAT() in the tree. Could you please put it instead in a new vop_stat_post() function? That would prevent possible future bugs when new calls to VOP_STAT() are introduced.

Upload the correct diff (previous one was missing #include <string.h>).

Switch to strchrnul() to find / separators. Further simplifications.

Note: Ideally, at some point, we should add some way to print the IP blocks per generation (separate change/revision).

Generally, you have to test if the driver is already attached. Here, the driver could attach through PCI or through ACPI, but it shouldn't attach through both. This is not supposed to happen, but better be safe than sorry, especially when dealing with hardware. So please add a check to both probe methods that device_get_unit(dev) is 0, else bail out (because there's already one driver/device). Check on acpi_get_handle(dev) still looks like a good idea, even if it does not handle the redundancy anymore.

In D54881#1372525, @ziaee wrote:

There is no spite or resentment behind my frustrated comment, please do not read into it too deeply.

In D59788#1372277, @obiwac wrote:

does it not make more sense to go through amdsmu_products forwards and just reverse the list? then the list can read more naturally as "the product will be the first one that matches an element in this list"

In D54881#1368936, @ziaee wrote:

You forgot to put this in the release notes, and omitted that I worked with you on this in the commit log. Both of these make my life harder. Please do not do this.

Typo in the commit message: "Thsi is intended".

Good catch!

Always printing a warning is fine for the time being. Having a separate function for the check is neat.

I'm fine with the principle of adding a warning. It's just that the current implementation is too rough, as it basically applies to all architectures whereas this concerns only amd64, and also the warning will be issued on Intel platforms as well, which is really confusing.

Thanks!

See inline comment for a comment improvement suggestion.

I'd also advise to disclose the AI used (for anything) with an "Assisted by:" tag.

Looks really great now! Thanks!

By tunable, I mean a sysctl knob (+ loader tunable).

I really think the original comment, although longer, was more informative. Could you please restore it (with amendments about the new findings and change)?

By the way, could you perhaps move all reads to constraint_obj->Package.Elements[] together close to the package size check? That makes it easier to verify that the check is correct.

That's certainly good enough. We can add even more checks later.

I'd suggest some function renames, but otherwise looks fine. I might do some followup changes.

In D59408#1364378, @mchoo wrote:

sysctl_kern_slice() lets you set any slice greater or equal to 1. When sched_slice is set to 4 load is 5, the first if condition passes because SCHED_SLICE_MIN_DIVISOR is set to 6 by macro definition. Second if condition also passes and tdq_slice() returns 0.

Thanks! (Suggested by: olce appreciated.)

In D59406#1365004, @mchoo wrote:

schedcpu()'s call to decay_cpu() happens regardless of awake, that is, even sleeping threads are decayed for the first tick of ts_slptime.

Slice is number of ticks a thread is allowed to run, thus having slice
value of 0 shouldn't be accepted as threads will never run. In
tdq_slice(), when sched_slice > load, the function will return 0.

On second thought, there is a balance between getting a consistent dump image and temporary disruption of the live system for live dumps. It's already the case that they do not guarantee a fully consistent image. And they don't try to stop CPUs, the scheduler, etc. As you point out, minidump is not set, which in this line of thinking is not surprising.

Well, I don't find the new comment less vague. By contrast, the description you're giving in the current commit message is immediately intelligible.

Commit message: "As the comment doesn't reflect the code correctly, drop it." is superfluous, I'd just drop it.

Grammar.

This was never committed enabled. Presumably, it was used for debugging by defining it to KTR_SCHED or some other KTR_* constant. We can revisit if we need something similar in the future.

Please rephrase though the assertion description (see inline comment).

Mmm, that's not true since we have been having live (mini)dump. On the contrary, it is perhaps time to put back dumping here and in ULE.

Suggestion: Rework the end of the commit message for something more concise, such as "is now completely removed, we should remove this tunable." => "and KSE was removed years ago.".

Mmm... Certainly, schedclock() should have been updated to sched_clock() in commit b43179fbe815 ("Create a new scheduler api..."), which renamed the function. But then, it is still technically true even today that sched_clock() actually updates the mentioned fields, it's just that it does so indirectly (through sched_clock_tick(), of which sched_clock() is the only caller). Keeping the mention of the upper-level function / entry point to the scheduler could be seen as clearer. The recently-introduced possibility to have multiple schedulers compiled in the same kernel lead to some ugly renames, and that's why sched_clock() was renamed to sched_4bsd_clock(). Conceptually, though, the interface function triggering the updates is still sched_clock() (it's how it is referenced from outside 4BSD). I'd slightly prefer having sched_clock() in the comment.

Sorry for the long delay before coming back to this. I'm now quite in a hurry to get this in, so that I can work on some other CPU/chipset generation. A comment a while ago indicated that you had more generations in stock, let's also examine that just after this one is done. I can help polishing and committing them if you do not have time to handle them.

In D59309#1360723, @markj wrote:

Bytes trailing the nul terminator in the device_name field are leaked.

Use of memset() should be systematic on stack-allocated structures for security reasons. The compiler normally eliminates the dead stores.

Please check inline comment, there may be a leak.