linsysfs: Reimplement bus scan code.

[dchagin]

Summary

The linsysfs code continues to grow, so split bus scan code to a separate
functions, add some helpers.
The primary goal is to scan bus as much as it's needed. The first scan create
the directory structure, other scans can use it, finding nodes by pci device.
Rewrote scsi and drm code using new helpers.

1. Yes, the change is big, but it doesn't make sense to me to optimize code

and then completely rewrite it.

2. The drm code needed more attention, as it derefences device_t to lkpi

struct device. Perhaps here make sence to do differently.

[kib]

If this works, I am fine with it.

sys/compat/linsysfs/linsysfs.c

79

Fix style, continuation should use 4-space indent.

97

What protects the list manipulations?

sys/compat/linsysfs/linsysfs.h

31

[dchagin]

fixed style issues

[dchagin]

sys/compat/linsysfs/linsysfs.c

97

What protects the list manipulations?

For now it's not protected, as well as bus scan code,
both used only at the module load/unload time.
I plan to add protection, ponders how to handle device add/remove.

[kib]

sys/compat/linsysfs/linsysfs.c

97

Giant is most likely held there, but you allocate sleepable.

I think it is worth adding a comment mentioning the locking state. Also, you might consider starting using bus_topo_lock() there, even if formally.

[dchagin]

locking done, now bus scan code unsleepable,
there is still a problem with tracking the life of refs to device_t,
however while the drm-kmod doesn't know how to unload, that's ok

[manu]

In D38545#878789, @dchagin wrote:

locking done, now bus scan code unsleepable,
there is still a problem with tracking the life of refs to device_t,
however while the drm-kmod doesn't know how to unload, that's ok

It knows how to unload, however it's true that currently this panic but I'm working on fixing this and keep it stable as we need this for work.

[dchagin]

In D38545#878793, @manu wrote:

In D38545#878789, @dchagin wrote:

locking done, now bus scan code unsleepable,
there is still a problem with tracking the life of refs to device_t,
however while the drm-kmod doesn't know how to unload, that's ok

It knows how to unload, however it's true that currently this panic but I'm working on fixing this and keep it stable as we need this for work.

ah, I wrote as well as I could, I don't mean to criticize, I read Linux code every day and I understand how hard your work is.

[dchagin]

linsysfs.h fixed

[kib]

sys/compat/linsysfs/linsysfs.c

86

So the possible failure is not caused by any persistent conditions, it is just transient internal unability to allocate memory. It is bad, IMO.

Just lock with bus_topo for now.

[manu]

In D38545#878812, @dchagin wrote:

In D38545#878793, @manu wrote:

In D38545#878789, @dchagin wrote:

locking done, now bus scan code unsleepable,
there is still a problem with tracking the life of refs to device_t,
however while the drm-kmod doesn't know how to unload, that's ok

It knows how to unload, however it's true that currently this panic but I'm working on fixing this and keep it stable as we need this for work.

ah, I wrote as well as I could, I don't mean to criticize, I read Linux code every day and I understand how hard your work is.

Don't worry I didn't take it badly, I was just saying that you shouldn't rely on the fact that currently drm-kmod doesn't unload because this is a bug :)

[dchagin]

done

[kib]

I still do not understand what are you doing.

You need to lock the newbus subsystem to safely access it, no? The newbus lock is bus_topo_lock(). You need to use it, then you can reuse it for your structures. The only quirk is that right now bus_topo_lock() is Giant so it is dropped on sleep.

sys/compat/linsysfs/linsysfs.h

34

Indents there definitely do not follow style

[dchagin]

In D38545#879586, @kib wrote:

I still do not understand what are you doing.

You need to lock the newbus subsystem to safely access it, no? The newbus lock is bus_topo_lock(). You need to use it, then you can reuse it for your structures. The only quirk is that right now bus_topo_lock() is Giant so it is dropped on sleep.

bus_topo_lock is acquired below in linsysfs_bus_scan,
however, I have a one question before any changes further, does device tree persistent? If not, should I after any sleep, rescan device tree?
Also, do we have plan to replace Giant here by normal mutex? If the tree is not persistent, this complicates things

[kib]

In D38545#879862, @dchagin wrote:

In D38545#879586, @kib wrote:

I still do not understand what are you doing.

You need to lock the newbus subsystem to safely access it, no? The newbus lock is bus_topo_lock(). You need to use it, then you can reuse it for your structures. The only quirk is that right now bus_topo_lock() is Giant so it is dropped on sleep.

bus_topo_lock is acquired below in linsysfs_bus_scan,
however, I have a one question before any changes further, does device tree persistent? If not, should I after any sleep, rescan device tree?
Also, do we have plan to replace Giant here by normal mutex? If the tree is not persistent, this complicates things

What do you mean by persistence? Imagine that somebody plugs in a USB/COM adapter, or removes a hot-pluggable PCIe device. Of course, it is reflected in the newbus structure.

On the other hand, right now newbus is not really locked. It is somewhat protected by the Giant, but because Giant is dropped for sleep, the protection is somewhat cheesy. Locking newbus for real is quite non-trivial, because the changes affect all drivers, and this is a lot of testing. Another hard aspect is that many drivers use Giant special properties (recursion, autodrop on sleep), which is impossible to emulate with regular locks. At least several attempts to lock newbus failed.

bus_topo_lock() is mostly an indicator for the places which needs attention for newbus locking. This is why I want it to be present in your changes, and why I am opposed to add specific local locking scheme.

[imp]

In D38545#879892, @kib wrote:

In D38545#879862, @dchagin wrote:

In D38545#879586, @kib wrote:

I still do not understand what are you doing.

You need to lock the newbus subsystem to safely access it, no? The newbus lock is bus_topo_lock(). You need to use it, then you can reuse it for your structures. The only quirk is that right now bus_topo_lock() is Giant so it is dropped on sleep.

bus_topo_lock is acquired below in linsysfs_bus_scan,
however, I have a one question before any changes further, does device tree persistent? If not, should I after any sleep, rescan device tree?
Also, do we have plan to replace Giant here by normal mutex? If the tree is not persistent, this complicates things

What do you mean by persistence? Imagine that somebody plugs in a USB/COM adapter, or removes a hot-pluggable PCIe device. Of course, it is reflected in the newbus structure.

On the other hand, right now newbus is not really locked. It is somewhat protected by the Giant, but because Giant is dropped for sleep, the protection is somewhat cheesy. Locking newbus for real is quite non-trivial, because the changes affect all drivers, and this is a lot of testing. Another hard aspect is that many drivers use Giant special properties (recursion, autodrop on sleep), which is impossible to emulate with regular locks. At least several attempts to lock newbus failed.

bus_topo_lock() is mostly an indicator for the places which needs attention for newbus locking. This is why I want it to be present in your changes, and why I am opposed to add specific local locking scheme.

Kib is right: there are plans for making this better. Experiments look promising, but I've not had enough time to torture test them. Having bus_topo_lock() etc will show where we may need to change or at least analyze

[dchagin]

In D38545#879892, @kib wrote:

In D38545#879862, @dchagin wrote:

In D38545#879586, @kib wrote:

I still do not understand what are you doing.

You need to lock the newbus subsystem to safely access it, no? The newbus lock is bus_topo_lock(). You need to use it, then you can reuse it for your structures. The only quirk is that right now bus_topo_lock() is Giant so it is dropped on sleep.

bus_topo_lock is acquired below in linsysfs_bus_scan,
however, I have a one question before any changes further, does device tree persistent? If not, should I after any sleep, rescan device tree?
Also, do we have plan to replace Giant here by normal mutex? If the tree is not persistent, this complicates things

What do you mean by persistence? Imagine that somebody plugs in a USB/COM adapter, or removes a hot-pluggable PCIe device. Of course, it is reflected in the newbus structure.

On the other hand, right now newbus is not really locked. It is somewhat protected by the Giant, but because Giant is dropped for sleep, the protection is somewhat cheesy. Locking newbus for real is quite non-trivial, because the changes affect all drivers, and this is a lot of testing. Another hard aspect is that many drivers use Giant special properties (recursion, autodrop on sleep), which is impossible to emulate with regular locks. At least several attempts to lock newbus failed.

bus_topo_lock() is mostly an indicator for the places which needs attention for newbus locking. This is why I want it to be present in your changes, and why I am opposed to add specific local locking scheme.

Thakn you for clarification. I understand.
I spent some time to adapt linsysfs and others to ifAPI. And found that anyway I need some generic way to create pseudofs nodes in an unsleepable context. As there two places here now where it needed - bus scan code, as Giant would be replaced after all, and the listnics code which should be rewrited to iterate over ifnets in the net epoch. Also both of this functions should start by eventhandler, to handle device attach/detach etc, not only at mount time.
I ponders the best way I can use, may be thread taskqueue?
Or I missing something?

[monwarez_mailoo.org]

Is there another version of this patch ? By trying to rebase it against 15.0, I get the following panic:

Fatal trap 12: page fault while in kernel mode
cpuid = 1; apic id = 01
fault virtual address	= 0x68
fault code		= supervisor read data, page not present
instruction pointer	= 0x20:0xffffffff80a95c46
stack pointer	        = 0x28:0xfffffe00fac84670
frame pointer	        = 0x28:0xfffffe00fac846c0
code segment		= base 0x0, limit 0xfffff, type 0x1b
			= DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags	= interrupt enabled, resume, IOPL = 0
current process		= 30761 (kldload)
rdi: 0000000000000000 rsi: fffffe00fac846e8 rdx: ffffffff849e025b
rcx: ffffffff849de5e0  r8: 0000000000000000  r9: 0000000000000000
rax: 0000000000000000 rbx: fffff80011bcc000 rbp: fffffe00fac846c0
r10: 0000000000000000 r11: 998fffd1d1000000 r12: 0000000000000001
r13: 0000000000000000 r14: ffffffff849e025b r15: 0000000000000000
trap number		= 12
panic: page fault
cpuid = 1
time = 1785625206
KDB: stack backtrace:
#0 0xffffffff80bbeb2d at kdb_backtrace+0x5d
#1 0xffffffff80b71e26 at vpanic+0x136
#2 0xffffffff80b71ce3 at panic+0x43
#3 0xffffffff8107af99 at trap_pfault+0x3c9
#4 0xffffffff81050e98 at calltrap+0x8
#5 0xffffffff849ddcb1 at linsysfs_scsi_cb+0x131
#6 0xffffffff849ddd54 at linsysfs_bus_foreach+0x34
#7 0xffffffff849ddd96 at linsysfs_bus_foreach+0x76
#8 0xffffffff849ddd96 at linsysfs_bus_foreach+0x76
#9 0xffffffff849ddd96 at linsysfs_bus_foreach+0x76
#10 0xffffffff849ddd96 at linsysfs_bus_foreach+0x76
#11 0xffffffff849ddd96 at linsysfs_bus_foreach+0x76
#12 0xffffffff849ddd96 at linsysfs_bus_foreach+0x76
#13 0xffffffff849ddd96 at linsysfs_bus_foreach+0x76
#14 0xffffffff849dd6ce at linsysfs_init+0x3fe
#15 0xffffffff80a9600f at pfs_init+0x9f
#16 0xffffffff80c4d7ec at vfs_modevent+0x3cc
#17 0xffffffff80b4bab5 at module_register_init+0x85

And the backtrace looks like:

#0  __curthread () at /usr/src/sys/amd64/include/pcpu_aux.h:57
#1  doadump (textdump=<optimized out>) at /usr/src/sys/kern/kern_shutdown.c:399
#2  0xffffffff80b719a9 in kern_reboot (howto=260) at /usr/src/sys/kern/kern_shutdown.c:519
#3  0xffffffff80b71eb7 in vpanic (fmt=0xffffffff811d2313 "%s", ap=ap@entry=0xfffffe00fac84530) at /usr/src/sys/kern/kern_shutdown.c:974
#4  0xffffffff80b71ce3 in panic (fmt=<unavailable>) at /usr/src/sys/kern/kern_shutdown.c:887
#5  0xffffffff8107af99 in trap_fatal (frame=<optimized out>, eva=<optimized out>) at /usr/src/sys/amd64/amd64/trap.c:969
#6  0xffffffff8107af99 in trap_pfault (frame=0xfffffe00fac845b0, usermode=false, signo=<optimized out>, ucode=<optimized out>)
#7  <signal handler called>
#8  pfs_create_link (parent=0x0, opn=0xfffffe00fac846e8, name=0xffffffff849e025b "device", fill=0xffffffff849de5e0 <linsysfs_scsiname+80>, attr=0x0, vis=0x0, destroy=0x0, flags=1) at /usr/src/sys/fs/pseudofs/pseudofs.c:315
#9  0xffffffff849ddcb1 in linsysfs_cpuonline () from /boot/kernel/linsysfs.ko
#10 0x0000000000000000 in ?? ()

[kib]

The patch requires a lot of work. It is not complicated, but quite time consuming. Also, it requires access to dual-bool the machine with Linux and FreeBSD, to compare the outcome on the same system.