On Mon, Nov 20, 2023 at 12:44 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@nec.com> wrote:
Hi Chengen,

thank you for the update.

On 2023/11/17 12:45, Chengen Du wrote:
> A kernel commit 7ac07a26dea7 (zram: preparation for multi-zcomp support)
> in Linux replaces "compressor" with "comp_algs" in the zram struct.
> If not fixed, the issue triggers the following error:
>    rd: WARNING: Some pages are swapped out to zram. Please run mod -s zram.
>    rd: invalid user virtual address: ffff7d23f010  type: "64-bit UVADDR"
>
> Signed-off-by: Chengen Du <chengen.du@canonical.com>
> ---
>   defs.h     |  1 +
>   diskdump.c | 56 +++++++++++++++++++++++++++++++++++-------------------
>   2 files changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/defs.h b/defs.h
> index 788f63a..2cae5b6 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -2227,6 +2227,7 @@ struct offset_table {                    /* stash of commonly-used offsets */
>       long module_memory_size;
>       long irq_data_irq;
>       long zspage_huge;
> +     long zram_comp_algs;
>   };
>   
>   struct size_table {         /* stash of commonly-used sizes */
> diff --git a/diskdump.c b/diskdump.c
> index 0fe46f4..d7e4380 100644
> --- a/diskdump.c
> +++ b/diskdump.c
> @@ -2757,6 +2757,8 @@ diskdump_device_dump_info(FILE *ofp)
>   
>   static ulong ZRAM_FLAG_SHIFT;
>   static ulong ZRAM_FLAG_SAME_BIT;
> +static ulong ZRAM_COMP_PRIORITY_BIT1;
> +static ulong ZRAM_COMP_PRIORITY_MASK;
>   
>   static void
>   zram_init(void)
> @@ -2765,6 +2767,8 @@ zram_init(void)
>   
>       MEMBER_OFFSET_INIT(zram_mempoll, "zram", "mem_pool");
>       MEMBER_OFFSET_INIT(zram_compressor, "zram", "compressor");
> +     if (INVALID_MEMBER(zram_compressor))
> +             MEMBER_OFFSET_INIT(zram_comp_algs, "zram", "comp_algs");
>       MEMBER_OFFSET_INIT(zram_table_flag, "zram_table_entry", "flags");
>       if (INVALID_MEMBER(zram_table_flag))
>               MEMBER_OFFSET_INIT(zram_table_flag, "zram_table_entry", "value");
> @@ -2782,6 +2786,8 @@ zram_init(void)
>   
>       ZRAM_FLAG_SHIFT = 1 << zram_flag_shift;
>       ZRAM_FLAG_SAME_BIT = 1 << (zram_flag_shift+1);
> +     ZRAM_COMP_PRIORITY_BIT1 = ZRAM_FLAG_SHIFT + 7;
> +     ZRAM_COMP_PRIORITY_MASK = 0x3;
>   
>       if (CRASHDEBUG(1))
>               fprintf(fp, "zram_flag_shift: %ld\n", zram_flag_shift);
> @@ -2980,13 +2986,15 @@ try_zram_decompress(ulonglong pte_val, unsigned char *buf, ulong len, ulonglong
>       unsigned char *outbuf = NULL;
>       ulong zram, zram_table_entry, sector, index, entry, flags, size,
>               outsize, off;
> +     int comp_alg_unavail;
>   
> -     if (INVALID_MEMBER(zram_compressor)) {
> +     comp_alg_unavail = INVALID_MEMBER(zram_compressor) && INVALID_MEMBER(zram_comp_algs);
> +     if (comp_alg_unavail) {

This variable saves nothing, and looking around, I found that we don't
need to use the zram.comp* members.  The offset of zram.mem_pool has
been used since the first implementation [1], so I would like to change
this to INVALID_MEMBER(zram_mem_pool) and attached it.

So the patch is ok.

Acked-by: Kazuhito Hagio <k-hagio-ab@nec.com>

[1] https://github.com/crash-utility/crash/commit/b12bdd36cf7c


Lianbo, zram related offsets have some typos, irregular names and lack
of "help -o" output.  I made the patch 2/2 in this opportunity, could
you review this too?

 
Sorry for the late response, Kazu and Chengen.

These two patches in attachment are fine to me, only two comments:
For patch [1], it has two related kernel commits, another one is: 84b33bf78889 ("zram: introduce recompress sysfs knob")

For patch[2], I saw the crash extensions include the defs.h, could you please check if the relevant variables are referenced directly in the crash extensions? If no, it does not break compatibility.

Otherwise, for the patch[1] and [2]: Ack.

Thanks.
Lianbo

Thanks,
Kazu