[lttng-dev] [PATCH lttng-modules 0/1] Introduce configure script to describe changes in linux kernel interface

Thu Jul 6 17:10:48 EDT 2023

Hi,

I gave 2 derivatives as an example, but in fact there are more than 10. 
We will end up with a bigger if condition that the actual function 
implementation. And this is also not a sustainable solution in case 
there are new derivatives coming after the fix. It will be hard to 
maintain. Plus, to achieve this we have to release a new kernel version 
to expose the suffixes in utsrelease.h and getting the fix on the user 
side would require a reboot to upgrade their kernel version.

While the current solution based on versioning may be easier to 
understand from the developer perspective, it adds technical depth to 
the implementation and in fact it requires more maintenance than 
expected, also on the distribution side. When there is a change on the 
kernel side, you need to check if it landed in older kernel versions and 
based on that you need to add a new interval in the if condition.
Now, looking at the configure script approach, while maybe not as clear 
as an ifdef, only one change would be enough to solve this problem. The 
LTTng module does not have to care about each distribution's way of 
versioning and the distributions don't have to deal with issues on their 
side if they use a version that the LTTng module did not take into 
account because you were not aware of it.
I think maintaining it won't be that hard if it's readable and 
documented properly. I am more than happy to improve it and help with 
maintaining it if you agree on this.

All the best,
Roxana

On 04-07-2023 21:16, Mathieu Desnoyers wrote:
> On 7/4/23 14:39, Roxana Nicolescu wrote:
>> Hi,
>>
>> Thanks a lot for your feedback.
>>
>> I realize I did not say the reason why I did not go for 
>> LTTNG_UBUNTU_KERNEL_RANGE. We deliver a bunch of different
>> derivatives (inherited from the main kernel), each with its own
>> version and it's impossible to use LTTNG_UBUNTU_KERNEL_RANGE alone. 
>> Derivatives in the same cycle don't have the same version number, so
>> I cannot rely on the version alone to determine when a change has
>> happened. For example these are some kernels we released last cycle: 
>> - linux (main kernel): 5.19.0-46 - linux-kvm: 5.19.0-1026 -
>> linux-lowlatency: 5.19.0-1028 As you can see, linux-kvm and
>> linux-lowlatency versions are not the same, and linux-lowlatency from
>> 2 months ago version version number coincides with linux-kvm from
>> now, but they don't match the same base. I hope that explains it.
>>
>> Initially I thought about exposing the version of the main kernel in
>> the kernel headers that can be later used in the module, but then I
>> came across openvswitch and that's how I came up with the idea of an
>> initial configure step.
>>
>> But I totally understand if you think this is not worth it.
>
> LTTng modules use the UTS_UBUNTU_RELEASE_ABI from the Ubuntu 
> generated/utsrelease.h kernel headers to detect tracepoint 
> instrumentation changes. I don't understand why many kernel flavors 
> would have the same ABI number with different ABI semantics, but I 
> guess that's just how things are now.
>
> One way to solve this would be to detect the "-lowlatency" and "-kvm" 
> suffixes in the string within generated/utsrelease.h UTS_RELEASE, e.g.:
>
> #define UTS_RELEASE "5.15.0-76-lowlatency"
>
> This could be done by LTTng modules by implementing a script similar 
> to what we do for debian, fedora, rhel, and sle (see scripts/ in 
> lttng-modules).
>
> Then we could have:
>
> * LTTNG_UBUNTU_KERNEL_RANGE for kernels where all flavors have the same
>   kernel ABI.
>
> * LTTNG_UBUNTU_GENERIC_KERNEL_RANGE for generic kernels only, for
>   situations where the kernel ABI differ between flavors,
>
> * LTTNG_UBUNTU_LOWLATENCY_KERNEL_RANGE for lowlatency kernels only, for
>   situations where the kernel ABI differ between flavors,
>
> * LTTNG_UBUNTU_KVM_KERNEL_RANGE for kvm kernels only, for situations
>   where the kernel ABI differ between flavors.
>
> It would all have been simpler if the UTS_UBUNTU_RELEASE_ABI would 
> actually have been a versioned kernel ABI without different semantics 
> across kernel flavors, but considering the current situation we will 
> need to deal with this with scripts as we have done for other 
> distributions.
>
> Thanks,
>
> Mathieu
>
>>
>> All the best, Roxana
>>
>> On 04/07/2023 20:07, Mathieu Desnoyers wrote:
>>> On 7/4/23 11:35, Michael Jeanson via lttng-dev wrote:
>>>> On 2023-07-03 14:28, Roxana Nicolescu via lttng-dev wrote:
>>>>> This script described the changes in the linux kernel interface
>>>>> that affect compatibility with lttng-modules.
>>>>>
>>>>> It is introduced for a specific usecase where commit d87a7b4c77a9: 
>>>>> "jbd2: use the correct print format" broke the
>>>>> interface between the kernel and lttng-module. 3 variables changed 
>>>>> their type to tid_t (transaction, head and tid) in
>>>>> multiple function declarations. The lttng module was updated
>>>>> properly to ensure backwards compatibility by using the version
>>>>> of the kernel. But this change took into account only long term
>>>>> supported versions. As an example, ubuntu 5.19 kernels picked
>>>>> the linux kernel change from 5.15 without actually changing the
>>>>> linux kernel upstream version. This means the current tooling
>>>>> does not allow to fix the module for newer ubuntu 5.19
>>>>> kernels.
>>>>>
>>>>> This script is supposed to solve the problem mentioned above,
>>>>> but to also make this change easier to integrate. We check the
>>>>> linux kernel header (include/trace/events/jbd2.h) if the types
>>>>> of tid, transaction and head variable have changed to tid_t
>>>>> and define these 3 variables in 'include/generated/config.h': 
>>>>> TID_IS_TID_T 1 TRANSACTION_IS_TID_T 1 HEAD_IS_TID_T 1
>>>>>
>>>>> In 'include/instrumentation/events/jbd2.h' we then check these
>>>>> to define the proper type of transaction, head and tid
>>>>> variables that will be later used in the function declarations
>>>>> that need them.
>>>>>
>>>>> This change is meant to remove the dependency on linux kernel
>>>>> version and the outcome is a bit cleaner that before. As with
>>>>> the previous implementation, this may need changes in the future 
>>>>> if the kernel interface changes again.
>>>>>
>>>>> Note: This is a proposal for a simpler way of integrating linux
>>>>> kernel changes in lttng-modules. The implementation is very
>>>>> simple due to the fact that tid_t was introduced everywhere in
>>>>> one commit in include/trace/events/jbd2.h. I would like to get
>>>>> your opinion on this approach. If needed, it can be improved.
>>>>>
>>>>> Roxana Nicolescu (1): Introduce configure script to describe
>>>>> changes in linux kernel interface
>>>>>
>>>>> README.md                             |   3 +- configure
>>>>> |  36 +++++++++ include/instrumentation/events/jbd2.h | 110 
>>>>> ++++++-------------------- 3 files changed, 61 insertions(+),
>>>>> 88 deletions(-) create mode 100755 configure
>>>>>
>>>>
>>>> Hi Roxana,
>>>>
>>>> While I can see advantages to a configure script approach to
>>>> detect kernel source changes I don't think it's worth the added
>>>> complexity on top of our current kernel version range system.
>>>>
>>>> We already have an Ubuntu specific kernel range macro that 
>>>> supplements the upstream version with Ubuntu's kernel ABI
>>>> number:
>>>>
>>>> LTTNG_UBUNTU_KERNEL_RANGE(5,19,17,X, 6,0,0,0)
>>>>
>>>> I'll let Mathieu make the final call but I think that would be
>>>> the preferred approach.
>>>
>>> Indeed, many of the kernel tracepoint code changes we had to deal
>>> with in the past 10 years would not be easy to track with configure
>>>  scripts, so we would end up with not just one, but with a
>>> combination of two different mechanisms to adapt to kernel code
>>> changes.
>>>
>>> In order to keep things maintainable long-term, I prefer that we
>>> stay with the version-based approach as recommended by Michael.
>>>
>>> Thanks,
>>>
>>> Mathieu
>>>
>>>> Regards,
>>>>
>>>> Michael _______________________________________________ lttng-dev
>>>> mailing list lttng-dev at lists.lttng.org 
>>>> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>>>
>