Page MenuHomeFreeBSD

Minor speedup for load_kld function
Needs ReviewPublic

Authored by crees on Dec 28 2018, 2:55 PM.

Details

Summary

Remove now unnecessary kldstat check before attempting to load modules.

Since r233109, kldload has the -n option, which silently ignores options
that are already loaded.

https://lists.freebsd.org/pipermail/freebsd-rc/2018-December/003899.html

Test Plan
[crees@pegasus]~/workspace/src/head% sudo sh
# . libexec/rc/rc.subr
# kldstat |grep cuse
# load_kld cuse4bsd
# kldstat |grep cuse
15    1 0xffffffff818c3000 40a0     cuse.ko
# load_kld cuse4bsd
# load_kld doesntexist
kldload: can't load doesntexist: No such file or directory
sh: WARNING: Unable to load kernel module doesntexist
# kldunload cuse
# load_kld -m nothing -e noop /boot/modules/cuse4bsd.ko
# kldstat |grep cuse
15    1 0xffffffff818c3000 4c80     cuse4bsd.ko
# load_kld -m nothing -e noop /boot/modules/cuse4bsd.ko
# load_kld -m nothing -e noop /boot/modules/cuse4bsd.ko
# load_kld -m nothing -e noop /boot/modules/cuse4bsd.ko
# kldstat |grep cuse
15    1 0xffffffff818c3000 4c80     cuse4bsd.ko
#

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

crees created this revision.Dec 28 2018, 2:55 PM
crees edited the test plan for this revision. (Show Details)Dec 28 2018, 2:56 PM
crees edited the summary of this revision. (Show Details)
bcr accepted this revision.Dec 28 2018, 3:01 PM
bcr added a subscriber: bcr.

OK for the man page changes, .Dd bump upon commit. ;-)

This revision is now accepted and ready to land.Dec 28 2018, 3:01 PM
rgrimes accepted this revision.Apr 5 2019, 9:01 PM
rgrimes added a subscriber: rgrimes.

I have performed a visual only review.

crees added a comment.Apr 5 2019, 9:17 PM

I have performed a visual only review.

Greatly appreciated. Can I take that as approval by a src developer?

I have performed a visual only review.

Greatly appreciated. Can I take that as approval by a src developer?

I am sorry, I do not have that authority at this time.

jilles accepted this revision.Apr 5 2019, 10:06 PM
jilles added a subscriber: jilles.
jilles added inline comments.
libexec/rc/rc.subr
1866 ↗(On Diff #52366)

Debug output will be different, but I think it is enough to mention that in the commit message.

crees added a comment.Apr 7 2019, 6:24 PM

Fantastic, thanks for the approval and tip.

This revision was automatically updated to reflect the committed changes.
crees reopened this revision.Apr 10 2019, 8:22 AM
crees added a subscriber: avg.

I've reverted the patch for now (thanks again Jilles for your approval).

If the zfs.ko module is loaded, a zfs module exists in the kernel, but I suspect that zfs doesn't exist in a kernel with it compiled in; only zfsctrl, based on what avg has suggested (Andriy, hope you don't mind me subscribing you).

I think that changing required_modules to zfsctrl in the zfs script should fix this, but I'll need to verify that before trying to commit again, as well as verify that zfs.ko is loaded if it doesn't.

avg added a comment.Apr 10 2019, 12:04 PM

If the zfs.ko module is loaded, a zfs module exists in the kernel, but I suspect that zfs doesn't exist in a kernel with it compiled in; only zfsctrl, based on what avg has suggested

Once again, you are confusing a name of a loadable file with a name of a module. Which is not surprising as that duality is confusing. E.g., a single loadable file can contains multiple modules.
Whether ZFS is compiled into the kernel or loaded via zfs.ko, the module name is still zfsctrl. It's the module name.

Thanks very much for the explanation-- I think my terminology is just off here (I do understand what you mean). I have some ideas to get this to work, but I'll get back when I've tried them, may not be for a few days.