Re: [PATCH v1 07/11] fs/proc/vmcore: introduce PROC_VMCORE_DEVICE_RAM to detect device RAM ranges in 2nd kernel

David Hildenbrand
2024-11-21T19:47:12Z

>>
>> That would work, but I don't completely like it.
>>
>> (a) I want s390x to select NEED_PROC_VMCORE_DEVICE_RAM instead. Staring at a
>> bunch of similar cases (git grep "config NEED" | grep Kconfig, git grep
>> "config ARCH_WANTS" | grep Kconfig), "select" is the common way to do it.
>>
>> So unless there is a pretty good reason, I'll keep
>> NEED_PROC_VMCORE_DEVICE_RAM as is.
>
> That's easy to satify, see below:

Yes, this is mostly what I have right now, except

>
> ============simple version=====
> fs/proc/Kconfig:
> config NEED_PROC_VMCORE_DEVICE_RAM
> def n

using "bool" here like other code. (I assume you meant "def_bool n",
"bool" seems to achieve the same thing)

>
> config PROC_VMCORE_DEVICE_RAM
> def_bool y
> depends on PROC_VMCORE && VIRTIO_MEM
> depends on NEED_PROC_VMCORE_DEVICE_RAM
>
> arch/s390/Kconfig:
> config S390
> select NEED_PROC_VMCORE_DEVICE_RAM
> ==============================
>
>>
>> (b) In the context of this patch, "depends on VIRTIO_MEM" does not make
>> sense. We could have an intermediate:
>>
>> config PROC_VMCORE_DEVICE_RAM
>> def_bool n
>> depends on PROC_VMCORE
>> depends on NEED_PROC_VMCORE_DEVICE_RAM
>>
>> And change that with VIRTIO_MEM support in the relevant patch.
>
> Oh, it's not comment for this patch, I made the simple version based on
> the whole patchset. When I had a glance at this patch, I also took
> several iterations to get it after I applied the whole patchset and
> tried to understand the whole code.

Makes sense, I'm figuring out how I can split that up.

If we can avoid the PROVIDE_* thing for now, great. Not a big fan of
that myself.

>
>>
>>
>> I faintly remember that we try avoiding such dependencies and prefer
>> selecting Kconfigs instead. Just look at the SPLIT_PTE_PTLOCKS mess we still
>> have to clean up. But as we don't expect that many providers for now, I
>> don't care.
>
> With the simple version, Kconfig learner as me can easily understand what
> they are doing. If it took you a couple of iterations to make them as
> you had mentioned earlier, and it took me several iterations to
> understand them, I believe there must be room to improve the presented
> ones in this patchset. These are only my humble opinion, and I am not
> aware of virtio-mem at all, I'll leave this to you and other virtio-mem
> dev to decide what should be taken. Thanks for your patience and
> provided information, I learned a lot from this discussion.

I hope I didn't express myself poorly: thanks a lot for the review and
the discussion! It helped to make the Kconfig stuff better. I'll get rid
of the PROVIDE_* thing for now and just depend on virtio-mem.

>
> ===================
> fs/proc/Kconfig:
> config PROVIDE_PROC_VMCORE_DEVICE_RAM
> def_bool n
>
> config NEED_PROC_VMCORE_DEVICE_RAM
> def_bool n
>
> config PROC_VMCORE_DEVICE_RAM
> def_bool y
> depends on PROC_VMCORE
> depends on NEED_PROC_VMCORE_DEVICE_RAM
> depends on PROVIDE_PROC_VMCORE_DEVICE_RAM
>
> drivers/virtio/Kconfig:
> config VIRTIO_MEM
> select PROVIDE_PROC_VMCORE_DEVICE_RAM if PROC_VMCORE
> ~~~~~~~~~~~~~~
>
> arch/s390/Kconfig:
> config S390
> select NEED_PROC_VMCORE_DEVICE_RAM if PROC_VMCORE
> ~~~~~~~~~~~~~~
> ========================
>
> One last thing I haven't got well, If PROC_VMCORE_DEVICE_RAM has had
> dependency on PROC_VMCORE, can we take off the ' if PROC_VMCORE' when
> select PROVIDE_PROC_VMCORE_DEVICE_RAM and NEED_PROC_VMCORE_DEVICE_RAM?

We could; it would mean that in a .config file you would end up with
"NEED_PROC_VMCORE_DEVICE_RAM=y" with "#PROC_VMCORE" and no notion of
"PROC_VMCORE_DEVICE_RAM".

I don't particularly like that -- needing something that apparently does
not exist. Not sure if there is a best practice here, staring at some
examples I don't seem to find a consistent rule. I can just drop it, not
the end of the world.

Did you get to look at the other code changes in this patch set? Your
feedback would be highly appreciated!

--
Cheers,

David / dhildenb