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

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

[Roxana Nicolescu via lttng-dev]

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

-- 
2.34.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Roxana Nicolescu via lttng-dev]

This script is intented to be used for checking the changes in linux
kernel headers and keep the lttng module up-to-date.

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.

Configure script is supposed to make this change easier to integrate.
We check the linux kernel header (include/trace/events/jbd2.h) if
the type of tid, transaction and head variable has changes 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.

Signed-off-by: Roxana Nicolescu <roxana.nicolescu@canonical.com>
---
 README.md                             |   3 +-
 configure                             |  36 +++++++++
 include/instrumentation/events/jbd2.h | 110 ++++++--------------------
 3 files changed, 61 insertions(+), 88 deletions(-)
 create mode 100755 configure

diff --git a/README.md b/README.md
index 06480d3b..231ba956 100644
--- a/README.md
+++ b/README.md
@@ -29,6 +29,7 @@ Building
 To build and install LTTng-modules, you will need to have your kernel
 headers available (or access to your full kernel source tree), and do:
 
+    ./configure
     make
     sudo make modules_install
     sudo depmod -a
@@ -36,7 +37,7 @@ headers available (or access to your full kernel source tree), and do:
 The above commands will build LTTng-modules against your
 current kernel. If you need to build LTTng-modules against a custom
 kernel, do:
-
+    KERNELDIR=/path/to/custom/kernel ./configure
     make KERNELDIR=/path/to/custom/kernel
     sudo make KERNELDIR=/path/to/custom/kernel modules_install
     sudo depmod -a kernel_version
diff --git a/configure b/configure
new file mode 100755
index 00000000..a9fcd422
--- /dev/null
+++ b/configure
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+if [ -z "${KERNELDIR}" ]; then
+    KERNELDIR=/lib/modules/$(uname -r)/build
+fi
+
+INCLUDE_DIR="include/generated"
+if test ! -d "${INCLUDE_DIR}"; then
+	mkdir "${INCLUDE_DIR}"
+fi
+
+LINUX_HEADER_FILE="${KERNELDIR}/include/trace/events/jbd2.h"
+
+CONFIG_H_OUTPUT_FILE="${INCLUDE_DIR}/config.h"
+# cleanup old config file
+> ${CONFIG_H_OUTPUT_FILE}
+
+if test -f "${LINUX_HEADER_FILE}"; then
+    grep -q "tid_t tid" ${LINUX_HEADER_FILE}
+    status=$?
+    if [ $status -eq 0 ]; then
+        echo "#define TID_IS_TID_T 1" >> ${CONFIG_H_OUTPUT_FILE}
+    fi
+
+    grep -q "[[:blank:]]\+__field([[:blank:]]\+tid_t,[[:blank:]]\+transaction" ${LINUX_HEADER_FILE}
+    status=$?
+    if [ $status -eq 0 ]; then
+        echo "#define TRANSACTION_IS_TID_T 1" >> ${CONFIG_H_OUTPUT_FILE}
+    fi
+
+    grep -q "[[:blank:]]\+__field([[:blank:]]\+tid_t,[[:blank:]]\+head" ${LINUX_HEADER_FILE}
+    status=$?
+    if [ $status -eq 0 ]; then
+        echo "#define HEAD_IS_TID_T 1" >> ${CONFIG_H_OUTPUT_FILE}
+    fi
+fi
diff --git a/include/instrumentation/events/jbd2.h b/include/instrumentation/events/jbd2.h
index be217ffa..6f9bd013 100644
--- a/include/instrumentation/events/jbd2.h
+++ b/include/instrumentation/events/jbd2.h
@@ -8,6 +8,7 @@
 #include <lttng/tracepoint-event.h>
 #include <linux/jbd2.h>
 #include <lttng/kernel-version.h>
+#include <generated/config.h>
 
 #ifndef _TRACE_JBD2_DEF
 #define _TRACE_JBD2_DEF
@@ -27,25 +28,18 @@ LTTNG_TRACEPOINT_EVENT(jbd2_checkpoint,
 	)
 )
 
-#if (LTTNG_LINUX_VERSION_CODE >= LTTNG_KERNEL_VERSION(6,2,0) \
-	|| LTTNG_KERNEL_RANGE(5,4,229, 5,5,0) \
-	|| LTTNG_KERNEL_RANGE(5,10,163, 5,11,0) \
-	|| LTTNG_KERNEL_RANGE(5,15,87, 5,16,0) \
-	|| LTTNG_KERNEL_RANGE(6,0,18, 6,1,0) \
-	|| LTTNG_KERNEL_RANGE(6,1,4, 6,2,0))
-LTTNG_TRACEPOINT_EVENT_CLASS(jbd2_commit,
-
-	TP_PROTO(journal_t *journal, transaction_t *commit_transaction),
-
-	TP_ARGS(journal, commit_transaction),
+#ifdef TRANSACTION_IS_TID_T
+#define transaction_type tid_t
+#else
+#define transaction_type int
+#endif
 
-	TP_FIELDS(
-		ctf_integer(dev_t, dev, journal->j_fs_dev->bd_dev)
-		ctf_integer(char, sync_commit, commit_transaction->t_synchronous_commit)
-		ctf_integer(tid_t, transaction, commit_transaction->t_tid)
-	)
-)
+#ifdef HEAD_IS_TID_T
+#define head_type tid_t
 #else
+#define head_type int
+#endif
+
 LTTNG_TRACEPOINT_EVENT_CLASS(jbd2_commit,
 
 	TP_PROTO(journal_t *journal, transaction_t *commit_transaction),
@@ -55,10 +49,9 @@ LTTNG_TRACEPOINT_EVENT_CLASS(jbd2_commit,
 	TP_FIELDS(
 		ctf_integer(dev_t, dev, journal->j_fs_dev->bd_dev)
 		ctf_integer(char, sync_commit, commit_transaction->t_synchronous_commit)
-		ctf_integer(int, transaction, commit_transaction->t_tid)
+		ctf_integer(transaction_type, transaction, commit_transaction->t_tid)
 	)
 )
-#endif
 
 LTTNG_TRACEPOINT_EVENT_INSTANCE(jbd2_commit, jbd2_start_commit,
 
@@ -97,12 +90,6 @@ LTTNG_TRACEPOINT_EVENT_INSTANCE(jbd2_commit, jbd2_drop_transaction,
 )
 #endif
 
-#if (LTTNG_LINUX_VERSION_CODE >= LTTNG_KERNEL_VERSION(6,2,0) \
-	|| LTTNG_KERNEL_RANGE(5,4,229, 5,5,0) \
-	|| LTTNG_KERNEL_RANGE(5,10,163, 5,11,0) \
-	|| LTTNG_KERNEL_RANGE(5,15,87, 5,16,0) \
-	|| LTTNG_KERNEL_RANGE(6,0,18, 6,1,0) \
-	|| LTTNG_KERNEL_RANGE(6,1,4, 6,2,0))
 LTTNG_TRACEPOINT_EVENT(jbd2_end_commit,
 	TP_PROTO(journal_t *journal, transaction_t *commit_transaction),
 
@@ -111,24 +98,10 @@ LTTNG_TRACEPOINT_EVENT(jbd2_end_commit,
 	TP_FIELDS(
 		ctf_integer(dev_t, dev, journal->j_fs_dev->bd_dev)
 		ctf_integer(char, sync_commit, commit_transaction->t_synchronous_commit)
-		ctf_integer(tid_t, transaction, commit_transaction->t_tid)
-		ctf_integer(tid_t, head, journal->j_tail_sequence)
+		ctf_integer(transaction_type, transaction, commit_transaction->t_tid)
+		ctf_integer(head_type, head, journal->j_tail_sequence)
 	)
 )
-#else
-LTTNG_TRACEPOINT_EVENT(jbd2_end_commit,
-	TP_PROTO(journal_t *journal, transaction_t *commit_transaction),
-
-	TP_ARGS(journal, commit_transaction),
-
-	TP_FIELDS(
-		ctf_integer(dev_t, dev, journal->j_fs_dev->bd_dev)
-		ctf_integer(char, sync_commit, commit_transaction->t_synchronous_commit)
-		ctf_integer(int, transaction, commit_transaction->t_tid)
-		ctf_integer(int, head, journal->j_tail_sequence)
-	)
-)
-#endif
 
 LTTNG_TRACEPOINT_EVENT(jbd2_submit_inode_data,
 	TP_PROTO(struct inode *inode),
@@ -141,57 +114,21 @@ LTTNG_TRACEPOINT_EVENT(jbd2_submit_inode_data,
 	)
 )
 
-#if (LTTNG_LINUX_VERSION_CODE >= LTTNG_KERNEL_VERSION(6,2,0) \
-	|| LTTNG_KERNEL_RANGE(5,4,229, 5,5,0) \
-	|| LTTNG_KERNEL_RANGE(5,10,163, 5,11,0) \
-	|| LTTNG_KERNEL_RANGE(5,15,87, 5,16,0) \
-	|| LTTNG_KERNEL_RANGE(6,0,18, 6,1,0) \
-	|| LTTNG_KERNEL_RANGE(6,1,4, 6,2,0))
-LTTNG_TRACEPOINT_EVENT(jbd2_run_stats,
-	TP_PROTO(dev_t dev, tid_t tid,
-		 struct transaction_run_stats_s *stats),
-
-	TP_ARGS(dev, tid, stats),
-
-	TP_FIELDS(
-		ctf_integer(dev_t, dev, dev)
-		ctf_integer(tid_t, tid, tid)
-		ctf_integer(unsigned long, wait, stats->rs_wait)
-		ctf_integer(unsigned long, running, stats->rs_running)
-		ctf_integer(unsigned long, locked, stats->rs_locked)
-		ctf_integer(unsigned long, flushing, stats->rs_flushing)
-		ctf_integer(unsigned long, logging, stats->rs_logging)
-		ctf_integer(__u32, handle_count, stats->rs_handle_count)
-		ctf_integer(__u32, blocks, stats->rs_blocks)
-		ctf_integer(__u32, blocks_logged, stats->rs_blocks_logged)
-	)
-)
-
-LTTNG_TRACEPOINT_EVENT(jbd2_checkpoint_stats,
-	TP_PROTO(dev_t dev, tid_t tid,
-		 struct transaction_chp_stats_s *stats),
-
-	TP_ARGS(dev, tid, stats),
-
-	TP_FIELDS(
-		ctf_integer(dev_t, dev, dev)
-		ctf_integer(tid_t, tid, tid)
-		ctf_integer(unsigned long, chp_time, stats->cs_chp_time)
-		ctf_integer(__u32, forced_to_close, stats->cs_forced_to_close)
-		ctf_integer(__u32, written, stats->cs_written)
-		ctf_integer(__u32, dropped, stats->cs_dropped)
-	)
-)
+#ifdef TID_IS_TID_T
+#define tid_type tid_t
 #else
+#define tid_type unsigned long
+#endif
+
 LTTNG_TRACEPOINT_EVENT(jbd2_run_stats,
-	TP_PROTO(dev_t dev, unsigned long tid,
+	TP_PROTO(dev_t dev, tid_type tid,
 		 struct transaction_run_stats_s *stats),
 
 	TP_ARGS(dev, tid, stats),
 
 	TP_FIELDS(
 		ctf_integer(dev_t, dev, dev)
-		ctf_integer(unsigned long, tid, tid)
+		ctf_integer(tid_type, tid, tid)
 		ctf_integer(unsigned long, wait, stats->rs_wait)
 		ctf_integer(unsigned long, running, stats->rs_running)
 		ctf_integer(unsigned long, locked, stats->rs_locked)
@@ -204,21 +141,20 @@ LTTNG_TRACEPOINT_EVENT(jbd2_run_stats,
 )
 
 LTTNG_TRACEPOINT_EVENT(jbd2_checkpoint_stats,
-	TP_PROTO(dev_t dev, unsigned long tid,
+	TP_PROTO(dev_t dev, tid_type tid,
 		 struct transaction_chp_stats_s *stats),
 
 	TP_ARGS(dev, tid, stats),
 
 	TP_FIELDS(
 		ctf_integer(dev_t, dev, dev)
-		ctf_integer(unsigned long, tid, tid)
+		ctf_integer(tid_type, tid, tid)
 		ctf_integer(unsigned long, chp_time, stats->cs_chp_time)
 		ctf_integer(__u32, forced_to_close, stats->cs_forced_to_close)
 		ctf_integer(__u32, written, stats->cs_written)
 		ctf_integer(__u32, dropped, stats->cs_dropped)
 	)
 )
-#endif
 
 LTTNG_TRACEPOINT_EVENT(jbd2_update_log_tail,
 	TP_PROTO(journal_t *journal, tid_t first_tid,
-- 
2.34.1

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Michael Jeanson via lttng-dev]

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.

Regards,

Michael
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Mathieu Desnoyers via lttng-dev]

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@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Roxana Nicolescu via lttng-dev]

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

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

[Mathieu Desnoyers via lttng-dev]

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@lists.lttng.org 
>>> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
>> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

a
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

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

[Roxana Nicolescu via lttng-dev]

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

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

[Michael Jeanson via lttng-dev]

On 2023-07-06 17:10, Roxana Nicolescu wrote:
> 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

Hi Roxana,

We are still not convinced of the benefits of a configure script over the 
added complexity since the issue is specific to Ubuntu where you choose to 
maintain 10s of different kernel branches with diverging code and overlapping 
versions.

I made a patch [1] with version ranges that adds support for all kernels from 
the kinetic "linux" repo [2] including 'generic' and other flavours and 
'lowlatency'. It however doesn't cover 'linux-aws' or other repos which share 
ABI numbers with 'lowlatency'.

You can of course use the configure script approach in the Ubuntu package 
'lttng-modules-dkms' by adding your patch to the packaging code.

Regards,

Michael

[1] https://review.lttng.org/c/lttng-modules/+/10485
[2] https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/kinetic

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev