Hi Guanyou, Thanks for your patch, just some minor comments: On Wed, Apr 24, 2024 at 5:24 PM Guanyou Chen <chenguanyou9338(a)gmail.com&gt; 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(a)lists.crash-utility.osci.io > To unsubscribe send an email to devel-leave(a)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

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(a)gmail.com&gt; wrote: > > Hi Tao, > > Attached updated [PATCH V2]. > > Tao Liu <ltao(a)redhat.com&gt; 于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(a)gmail.com&gt; 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(a)lists.crash-utility.osci.io >> > To unsubscribe send an email to devel-leave(a)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 >>