Re: [PATCH v2] diskdump: Optimize the boot time

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



-----Original Message-----
> On Mon, Apr 11, 2022 at 12:30 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx <mailto:k-hagio-ab@xxxxxxx>
> > 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,
Kazu

> 
> 
> 	It has extra function calls and the difference of callee between
> 	hweight64() in sbitmap.c and get_hweight64() in diskdump.c, IMO
> 	it's not simple.
> 
> 
> 
> You are right. It has extra function calls.
> 
> Thanks.
> Lianbo
> 
> 
> 	Thanks,
> 	Kazu
> 
> 
> 

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux