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

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.

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
>> [email protected]
>> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>
_______________________________________________
lttng-dev mailing list
[email protected]
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev