Hi Guanyou,

Thanks for your patch, just some minor comments:


On Wed, Apr 24, 2024 at 5:24 PM Guanyou Chen <chenguanyou9338@gmail.com> wrote:
>
> Hi LianBo
>
> We don't need struct zspage_5_17.

Could you please rewrite your commit message, so people can know what
the patch is trying to do without diving into the code? E.g.
refactoring the code by combining 2 structures into one etc.

>
> ---
>  defs.h     | 32 +++++++++++++++-----------------
>  diskdump.c | 15 ++++++---------
>  2 files changed, 21 insertions(+), 26 deletions(-)
>
> diff --git a/defs.h b/defs.h
> index 3cb8e63..01f316e 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -7407,28 +7407,26 @@ ulong try_zram_decompress(ulonglong pte_val, unsigned char *buf, ulong len, ulon
>  #define SECTORS_PER_PAGE        (1 << SECTORS_PER_PAGE_SHIFT)
>
>  struct zspage {
> -    struct {
> -        unsigned int fullness : 2;
> -        unsigned int class : 9;
> -        unsigned int isolated : 3;
> -        unsigned int magic : 8;
> +    union {
> +        unsigned int flag_bits;
> +        struct {
> +            unsigned int fullness : 2;
> +            unsigned int class : 9;
> +            unsigned int isolated : 3;
> +            unsigned int magic : 8;
> +        } v0;
> +        struct {
> +            unsigned int huge : 1;
> +            unsigned int fullness : 2;
> +            unsigned int class : 9;
> +            unsigned int isolated : 3;
> +            unsigned int magic : 8;
> +        } v5_17;
>      };
>      unsigned int inuse;
>      unsigned int freeobj;
>  };
>
> -struct zspage_5_17 {
> -   struct {
> -       unsigned int huge : 1;
> -       unsigned int fullness : 2;
> -       unsigned int class : 9;
> -       unsigned int isolated : 3;
> -       unsigned int magic : 8;
> -   };
> -   unsigned int inuse;
> -   unsigned int freeobj;
> -};
> -
>  /*
>   * makedumpfile.c
>   */
> diff --git a/diskdump.c b/diskdump.c
> index 3ae7bf2..a928a0e 100644
> --- a/diskdump.c
> +++ b/diskdump.c
> @@ -2819,7 +2819,6 @@ zram_object_addr(ulong pool, ulong handle, unsigned char *zram_buf)
>  {
>     ulong obj, off, class, page, zspage;
>     struct zspage zspage_s;
> -   struct zspage_5_17 zspage_5_17_s;
>     physaddr_t paddr;
>     unsigned int obj_idx, class_idx, size;
>     ulong pages[2], sizes[2];
> @@ -2833,15 +2832,13 @@ zram_object_addr(ulong pool, ulong handle, unsigned char *zram_buf)
>     readmem(page + OFFSET(page_private), KVADDR, &zspage,
>             sizeof(void *), "page_private", FAULT_ON_ERROR);
>
> +    readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "zspage", FAULT_ON_ERROR);
^^^^^
In your patch file attached, here is space instead of tab.

Thanks,
Tao Liu

>     if (VALID_MEMBER(zspage_huge)) {
> -       readmem(zspage, KVADDR, &zspage_5_17_s,
> -           sizeof(struct zspage_5_17), "zspage_5_17", FAULT_ON_ERROR);
> -       class_idx = zspage_5_17_s.class;
> -       zs_magic = zspage_5_17_s.magic;
> +       class_idx = zspage_s.v5_17.class;
> +       zs_magic = zspage_s.v5_17.magic;
>     } else {
> -       readmem(zspage, KVADDR, &zspage_s, sizeof(struct zspage), "zspage", FAULT_ON_ERROR);
> -       class_idx = zspage_s.class;
> -       zs_magic = zspage_s.magic;
> +       class_idx = zspage_s.v0.class;
> +       zs_magic = zspage_s.v0.magic;
>     }
>
>     if (zs_magic != ZSPAGE_MAGIC)
> @@ -2887,7 +2884,7 @@ zram_object_addr(ulong pool, ulong handle, unsigned char *zram_buf)
>
>  out:
>     if (VALID_MEMBER(zspage_huge)) {
> -       if (!zspage_5_17_s.huge)
> +       if (!zspage_s.v5_17.huge)
>             return (zram_buf + ZS_HANDLE_SIZE);
>     } else {
>         readmem(page, KVADDR, &obj, sizeof(void *), "page flags", FAULT_ON_ERROR);
> --
> 2.39.0
> --
> Crash-utility mailing list -- devel@lists.crash-utility.osci.io
> To unsubscribe send an email to devel-leave@lists.crash-utility.osci.io
> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> Contribution Guidelines: https://github.com/crash-utility/crash/wiki