Hi Guanyou,

Thanks for the improvement. However I'd like to improve the commit log
as follows:

This patch is a refactoring on commit [1], and has no functional
change. The reason is that the structure of zspage has not changed,
just new bits have been introduced. So a union is better to reduce
code replication.

What do you think?

For the rest of the patch, ack.

Thanks,
Tao LIu

On Wed, May 8, 2024 at 11:06 AM Guanyou Chen <chenguanyou9338@gmail.com> wrote:
>
> Hi Tao,
>
> Attached updated [PATCH V2].
>
> Tao Liu <ltao@redhat.com> 于2024年5月3日周五 17:58写道：
>>
>> 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
>>