On Mon, Apr 11, 2022 at 3:12 PM HAGIO KAZUHITO(萩尾 一仁) <[email protected]> wrote:
> -----Original Message----- > > On Mon, Apr 11, 2022 at 12:30 PM HAGIO KAZUHITO(萩尾 一仁) < > [email protected] <mailto:[email protected]> > > > wrote: > > > > > > Hi Lianbo, > > > > -----Original Message----- > > > diff --git a/defs.h b/defs.h > > > index 81ac049..1e8360d 100644 > > > --- a/defs.h > > > +++ b/defs.h > > > @@ -4531,6 +4531,26 @@ struct machine_specific { > > > #define NUM_IN_BITMAP(bitmap, x) > (bitmap[(x)/BITS_PER_LONG] & NUM_TO_BIT(x)) > > > #define SET_BIT(bitmap, x) (bitmap[(x)/BITS_PER_LONG] |= > NUM_TO_BIT(x)) > > > > > > +static inline unsigned int __const_hweight8(unsigned long > w) > > > +{ > > > + return > > > + (!!((w) & (1ULL << 0))) + > > > + (!!((w) & (1ULL << 1))) + > > > + (!!((w) & (1ULL << 2))) + > > > + (!!((w) & (1ULL << 3))) + > > > + (!!((w) & (1ULL << 4))) + > > > + (!!((w) & (1ULL << 5))) + > > > + (!!((w) & (1ULL << 6))) + > > > + (!!((w) & (1ULL << 7))); > > > +} > > > + > > > +#define __const_hweight16(w) (__const_hweight8(w) + > __const_hweight8((w) >> 8)) > > > +#define __const_hweight32(w) (__const_hweight16(w) + > __const_hweight16((w) >> 16)) > > > +#define __const_hweight64(w) (__const_hweight32(w) + > __const_hweight32((w) >> 32)) > > > + > > > +#define hweight32(w) __const_hweight32(w) > > > +#define hweight64(w) __const_hweight64(w) > > > + > > > > > > > > > > > > No need to move the above code from sbitmap.c to defs.h, a > simple way is to implement a new function > > in > > > sbitmap.c and add its definition in defs.h, that will > > > be easy to call it in diskdump.c. For example: > > > > > > diff --git a/defs.h b/defs.h > > > index 81ac0498dac7..0c5115e71f1c 100644 > > > --- a/defs.h > > > +++ b/defs.h > > > @@ -5894,6 +5894,7 @@ typedef bool > (*sbitmap_for_each_fn)(unsigned int idx, void *p); > > > void sbitmap_for_each_set(const struct sbitmap_context *sc, > > > sbitmap_for_each_fn fn, void *data); > > > void sbitmap_context_load(ulong addr, struct sbitmap_context > *sc); > > > +unsigned long get_hweight64(unsigned long w); > > > > > > /* sbitmap_queue helpers */ > > > typedef bool (*sbitmapq_for_each_fn)(unsigned int idx, ulong > addr, void *p); > > > diff --git a/sbitmap.c b/sbitmap.c > > > index 286259f71d64..628cc00c0b6b 100644 > > > --- a/sbitmap.c > > > +++ b/sbitmap.c > > > @@ -71,6 +71,11 @@ static inline unsigned int > __const_hweight8(unsigned long w) > > > > > > #define BIT(nr) (1UL << (nr)) > > > > > > +unsigned long get_hweight64(unsigned long w) > > > +{ > > > + return hweight64(w); > > > +} > > > + > > > static inline unsigned long min(unsigned long a, unsigned long > b) > > > { > > > return (a < b) ? a : b; > > > > > > > > > //diskdump.c > > > ... > > > dd->valid_pages[i] += get_hweight64(tmp); > > > > > > ... > > > > > > How about the above suggestions? Shijie and Kazu. > > > > Thanks for the suggestion, but personally I don't think it has more > > benefits than moving them. What is the good point? > > > > > > > > It has fewer code changes. The bitmap operation can be maintained > together > > in the sbitmap.c and won't be scattered elsewhere. > > > > In the future, some new functions may be still extended for the bitmap > operations in the sbitmap.c, that > > will avoid adding more bitmap operations to defs.h. > > > > That's my concern. If that is not a problem, the v2 will be fine to me. > :-) > > Thanks for the explanation, I see. I think that it's not a problem > for now. > > Now at least the existing bitmap operations become common ones and they > do not use values in the sbitmap.c, it's not suitable to maintain them > there with a function to access. If new functions are extended, let's > optimize them at that time. > Thanks for your comment, Kazu. I have no other issue. Applied Thanks. Lianbo
-- Crash-utility mailing list [email protected] https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
