Re: [RFC] An idea for general kernel post-processing automation in FreeBSD

On May 24, 2023, at 22:34, Hans Petter Selasky <hps@selasky.org> wrote:

> On 5/25/23 02:00, Mark Millard wrote:
>> I took a look at a . . ./sys/modules/mlx4/mlx4_main.o via nm
>> and it showed a different convention for creating uniqueness
>> for the type of context involved:
>> static ssize_t set_port_type(struct device *dev,
>>      struct device_attribute *attr,
>>      const char *buf, size_t count)
>> {
>> . . .
>> static DEFINE_MUTEX(set_port_type_mutex);
>> . . .
>> ended up with the static routine's name prefixed,
>> with a "." separator:
>> 0000000000006da0 t set_port_type
>> 0000000000000018 d set_port_type.__set_sysinit_set_sym_set_port_type_mutex_sx_sysinit_sys_init
>> 0000000000000008 d set_port_type.__set_sysuninit_set_sym_set_port_type_mutex_sx_sysuninit_sys_uninit
>> 0000000000000020 b set_port_type.set_port_type_mutex
>> 00000000000004a8 d set_port_type.set_port_type_mutex_args
>> 00000000000004c0 d set_port_type.set_port_type_mutex_sx_sysinit_sys_init
>> 00000000000004d8 d set_port_type.set_port_type_mutex_sx_sysuninit_sys_uninit
> 
> Hi,
> 
> That makes sense. If the set_port_type function is optimized out, the sysinit goes away aswell. Like function garbage collection in the linker. Also I see a sysuninit call there, which is not done in Linux. Technically, that leaves room, this mutex may be used after it is destroyed, depending on how the set_port_type function is used.
> 
> For my patchset, the change to move those sysinits structures and the mutex variable outside the function, is correct.
> 
> Looking at LINT for AMD64, this is so to speak the only place in the whole of FreeBSD, where a (static) SYSINIT() is placed inside a function:
> 
> # nm /usr/obj/vol/freebsd-src/amd64.amd64/sys/LINT/kernel |
> # grep -F "." | grep __set_sysinit
> ffffffff83908638 d set_port_type.__set_sysinit_set_sym_set_port_type_mutex_sx_sysinit_sys_init
> 
> Even though the commit message for 805d759338a2be939fffc8bf3f25cfaab981a9be may be interpreted differently, my good feeling about is was right. We have one place in the kernel where we do things differently than other places, and that means a bug due to this variable being declared in a different way, may go unnoticed, because this is not the most common use-case being tested.

The pre-existing code expresses explicitly that no other
routine is allowed to have its own use of the mutex, a
design choice enforced by the compiler as things are
written. (The purpose of the limitation to block scope.)

Your proposed change removes the compiler enforcement of
that design, allowing use of the mutex by other code in
mlx4_main.c without any notification by the compiler.

Your proposal has a direction of being more fragile for
bad changes without having to be explicit in code updates
about the change of status.

> Jess, even though I may not always be able to exactly explain things to others, like you demand, why is this change made, explain or revert, I see I have a talent to fix things ahead of time, and get problems out of the way, before the storm hits. A mutex being removed based on optimization flags passed to the linker and compiler, causing a synchronization bug, may take a long time to detect, figure out and resolve. And it might end you never figure out the reason for a bug, and give up simply.
> 
> 
> Regarding the sysinits at compile time issue. As long as the sysinits are all done in the same way, and the committer takes responsibility for fixing and handling bugs that show up, I don't see any problem about the added complexity.
> 
> 
> 
> A good place to work, is not where the workers are considered replacable, like when you request my changes to be understood by yourself, so in case I die, someone else can take over. Every FreeBSD developer is uniq and important to the project.
> 



===
Mark Millard
marklmi at yahoo.com