Looks good to me.

In D44817#1021811, @glebius wrote:

Looks like driver does "hardware" accounting and software accounting at the same time. That's why we see correct counts without your patches. If vtnet_accum_stats() provides us with all what we need, then the driver should declare IFCAP_HWSTATS in its capabilities. That will turn off software accounting by the network stack. May I ask you to add this and also combine D44816 and D44817 into a single review?

Looks good to me.

What about stub functions for !NUMA ?
For example, converting

static inline int
vm_page_domain(vm_page_t m)
{
#ifdef NUMA
	int domn, segind;

Bite out the translation for errors from kernel linker.

In D44552#1018569, @olce wrote:

Responding to your comment in D44581 here:

In D44581#1018346, @zlei wrote:

The initial version of EUNSDEP is defined as (ELAST + 1) in sys/kern/kern_linker.c. I have to admit it is more a hack but I did not find a good candidate from 'sys/sys/error.h`.

To proceed, I'm proposing:

1. Re-use error EXDEV for unsatisfied dependency, thus omit unneeded translations, but it should be documented in kldload(2). This is preferred.
2. Re-use error EXDEV, but translate it to ENOEXEC. This is less ideal but prevent KPI breakage.
3. Define EUNSDEP in sys/kern/kern_linker.c, document it is only used internally and should not be leaked (even to other subsystem). Obviously translation to ENOEXEC is needed. Not preferred.

Following my last message in D44581, I would just rule out the first two, and in fact also replace the third with an even more radical option: Just forego EUNSDEP entirely. With the first two options ruled out, the only remaining purpose of this change is its initial one, that is to say, to print error messages allowing to distinguish between loading errors (including required modules' load failures), which is what the new switch(error) block containing the printf's already accomplishes. This code (without EUNSDEP) is enough to output the chain of dependencies in the kernel log. Could you please also split it, with only a single printf mentioning the dependency error remaining (the only important point here is to print both the names of the top-level module to load and that of the dependency that failed), and move the rest to linker_load_module(), for reasons explained in an earlier comment (better reporting for in-kernel loads and top-level modules)?

I think I see what you meant. I'll try that.

As for

with a signature allowing to pass back a text describing the error.

I think that is perfect, but given linker_load_module() can be called recursively, the signature should be designed carefully. IMO a failure from kernel linkers should be rare thus one of the above simpler fixes should be adequate. It is not perfect but user can still get the real reason from dmesg or /var/log/message.

I would just give up on that in that revision, this would be a separate change anyway, way more useful to users than just returning EUNSDEP, but it requires some (hopefully short) design discussion. This way, we also stop delaying the current change, which is useful to users on its own, and that I personally would like to see in, e.g., 14.1-RELEASE.

As propagation error from other subsystem is a common issue. Actually languages such as C++ / Java do that well. Those languages have type exception and exceptions can be wrapped up ( nested exceptions ). I think C can mimic but that is not elegance IMO.
+1 for 14.1-RELEASE .

Looks good to me.

In D44581#1017956, @kib wrote:

Adding anything to errno is the wrong direction. If you want to get arbitrary error back from kldload(2), then perhaps kldload2(2) is due, with a signature allowing to pass back a text describing the error.
This would be similar to the interface of rtld itself, where dl*() functions do not try to extend errno but provide dlerror() with the description of the problem.

In D44581#1017690, @kib wrote:

In D44581#1017679, @glebius wrote:

In D44581#1017675, @kib wrote:

I do not like it. Errno has the defined scope of providing the communication between kernel and userspace. Some of the 'out of band' values like ERESTART and EJUSTRETURN and valid extensions because they modify the kernel->user edge behavior, all other in the list are hacks.
More, the hack you propose is for very special and local situation, where error can be propagated by other means, if ever needed.

Would you like if it get a positive value and goes under #ifndef _POSIX_SOURCE like other our errnos that are extensions?

This is a no-starter at all. It would increase the sys_errlist[] size which is de-facto ABI break with all the consequeneces.

I spotted this while working on D44581 which intends to introduce new kernel-only error code EUNSDEP and on D44552 which uses it.

As in D44552 kern_kldload() will return the new error. It should not be leaked to the userspace. I was evaluating the impact and @olce pointed out the macro request_module is consuming kern_kldload().

In D44583#1017691, @bz wrote:

In D44583#1017689, @emaste wrote:

I don't know if the code path in drm_encoder_slave.c actually uses this

I don't think the change in this review should have any effect - we just go from discarding the returned fileid to not requesting it at all.

Yes, I understand; hence accepted. I was just curious how (and why) this got spotted given it's been sitting there like this for a decade. If there are (public) consumers I missed it would be good to know as I am still curious as to why it can hang a system (and if it does why has no one noticed it in the past; e.g. what the invariants are).

In D44633#1017635, @glebius wrote:

Reducing multiple error codes to one is losing information and it is confusing regardless of how experienced a user is. Even in case of the least experienced user, they would file a problem report or ask on mailing lists, forums, or whatever, they will come asking for help with reduced information. First thing to help such a user would be to write a dtrace script or ask him to patch kernel with printfs to get the actual error.

In D44552#1017135, @olce wrote:

In D44552#1017019, @zlei wrote:

Rechecked this, CURRENT and stable/14 vfs_byname_kld() has translated error to ENODEV. As for stable/13 it will be leaked.

Ah, I had forgotten the added translation to ENODEV in vfs_byname_kld(), but not in stable/13 indeed.

I would conclude, an error in domain A has the context, so when it goes cross to another domain B the context get lost, then probably it should be translated (to a proper error for domain B).

That seems a good principle in general but then there's the question of where the "crossing" happens. kern_*() interfaces were initially meant to be called from syscall glue code (adaptation for alternate syscall tables, e.g., 32-bit on 64-bit or Linux), which in particular performs copyin/copyout, and my current view is that they can also be used to make some subsystem functionality available to other subsystem with a kind of relatively formal interface. So, kern_*() functions should control relatively strictly their return codes and not, e.g., let new ones leak to callers, which also implies new return codes should land in the appropriate manual pages. In other words, this means that kern_*() and sys_*() should generally be treated the same, unless you have good reasons, and return the same codes (barring additional ones the syscall glue code can return). And, concerning the "crossing" part, if kern_*() is considered a kind of public interface to the rest of the kernel, crossing happens in fact in there and must be handled by it, and not the caller. Now, it is true that KPI restrictions are less stringent than syscall API restrictions, which I'd guess could be a common "reason" for divergence between sys_*() and kern_*().

In D44633#1017494, @glebius wrote:

Why do we really want to lose a meaningful error code and reduce all errors into one value? IMHO, it is counterproductive. And if we got a new error code type, why can't we propagate it to userland, too?

@olce
As a side effect, the error from kern_kldload() is lost, so I think you are right , we should emit logs from kernel about the error, to help diagnosing.

In D44552#1016696, @olce wrote:

In D44552#1016580, @zlei wrote:

Move translating errno EUNSDEP from kern_kldload() to sys_kldload()

I really think doing this in this review is ill-advised. And as is stands, the current change has impacts so should be amended.

Sorry for that.

After carefully checking, I can conclude it is safe to move the translation to sys_kldload(), as consumers of kern_kldload() either have done the translation themself, or totally ignore the return value, e.g. request_module in sys/compat/linuxkpi/common/include/linux/kmod.h, [1] and [2]. For the latter the current change will not make it worse.

I agree with the second part (some ignore the return value, so don't really care), but not with the first. Unless I'm mistaken, there are almost direct return paths to userland in these two cases:

• vfs_byname_kld()

Rechecked this, CURRENT and stable/14 vfs_byname_kld() has translated error to ENODEV. As for stable/13 it will be leaked.

• ngc_send()

ngcontrol_protosw.pr_send == ngc_send so EUNSDEP will be leaked. Really sorry I did not read the code carefully.

so EUNSDEP is going to be leaked.

Could you revert that modification? In other words, kern_kldload() still should translate the error itself, for now.

Another option could be refactor linker_file_unload() into two functions. One for normal unload and another is for abnormal ones such as unloading partially linked file.
Almost all abnormal unload requires the flag LINKER_UNLOAD_FORCE and at that moment the reference count of file should be right one. The logic will be much clear.

The only in-tree consumers of kld_unload_try and kld_unload even handlers are sdt and dtrace. Hopefully these even handlers are not abused by out-tree consumers.

Address @imp 's comments.

Move translating errno EUNSDEP from kern_kldload() to sys_kldload()

Stop translating errno from linker_reference_module() .

Moved the errno into sys/sys/errno.h, see D44581 .

Hi @imp , does this need broader attention ?

Looks good to me.

Made a reverse call graph to help evaluating the impact of this change:

linker_load_dependencies() --> linker_load_module()
	LINKER_LOAD_FILE()
		linker_load_file()
			linker_load_module()
				linker_reference_module()
					loadimage()
						TASK_INIT(&fwload_task, 0, loadimage, (void *)&fwli)
				kern_kldload() # syscall from userland

Looks good to me.

Generally looks good to me.

Looks good to me.

Looks good to me.

Looks good to me.

Looks good to me.