On Thu, May 11, 2023 at 8:38 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@nec.com> wrote:
On 2023/05/10 18:03, lijiang wrote:
> Hi, Kazu
> Thank you for the patch.
> On Wed, May 10, 2023 at 3:09 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@nec.com>
> wrote:
>
>> From: Kazuhito Hagio <k-hagio-ab@nec.com>
>>
>> The current comparison macros for kernel version shift minor number only
>> 8 bits.  This can cause an unexpected result on kernels with revision
>> number over 255, e.g. Linux 4.14.314.
>>
>>
> For this case, I saw kernel deal with it as below:
>
> #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + ((c) > 255 ?
> 255 : (c)))
>
> Can you try to imitate the above macro definition and help to confirm if it
> can also work for your case? If yes, it should be good to follow up kernel
> change.

um, if we need to check a revision number over 255, the KERNEL_VERSION
macro will not work.  For example,

 
Do you mean the following changes can not work?

#define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 16) + \
                             (kt->kernel_version[1] << 8) + \
                             ((kt->kernel_version[2] > 255) ? 255 : (kt->kernel_version[2])))
#define LINUX(x,y,z) (((uint)(x) << 16) + ((uint)(y) << 8) + ((uint)(z) > 255 ? 255 : (uint)(z)))

   if (THIS_KERNEL_VERSION < LINUX(4,14,300))

and yet, why do you think it is good to follow the kernel one?


I saw that similar cases were handled in the kernel, and the kernel did not change the bit shift operations.
... 
Makefile:1334: echo '#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) +  \
        ((c) > 255 ? 255 : (c)))';                                       \
tools/lib/bpf/bpf_helpers.h:74:#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + ((c) > 255 ? 255 : (c)))
... 

Thanks.
Lianbo

(Although I don't know the purpose and usage of the macro in kernel,
probably it's enough for kernel or tools.)

btw, makedumpfile has the same shift values as the patch since 2007.

#define KVER_MAJ_SHIFT 24
#define KVER_MIN_SHIFT 16
#define KERNEL_VERSION(x,y,z) (((x) << KVER_MAJ_SHIFT) | ((y) << KVER_MIN_SHIFT) | (z))

Thanks,
Kazu