Re: [PATCH v5 1/9] x86/intel_pstate: add some calculation related support

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/09/15 03:32, Wei Wang wrote:
> The added calculation related functions will be used in the intel_pstate.c.
> They are copied from the Linux kernel(commit 2418f4f2, f3002134, eb18cba7).
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
>  xen/arch/x86/oprofile/op_model_athlon.c |  9 ----
>  xen/include/asm-x86/div64.h             | 79 +++++++++++++++++++++++++++++++++
>  xen/include/xen/kernel.h                | 23 ++++++++++
>  3 files changed, 102 insertions(+), 9 deletions(-)
>
>  changes in v5:
>  1) add clamp(), a type checking variant of clamp_t();
>  2) remove the private copy of clamp() in op_model_athlon.c.
>
> diff --git a/xen/arch/x86/oprofile/op_model_athlon.c b/xen/arch/x86/oprofile/op_model_athlon.c
> index c0a81ed..4122eee 100644
> --- a/xen/arch/x86/oprofile/op_model_athlon.c
> +++ b/xen/arch/x86/oprofile/op_model_athlon.c
> @@ -103,15 +103,6 @@ static u64 ibs_op_ctl;
>  #define IBS_FETCH_CODE                  13
>  #define IBS_OP_CODE                     14
>  
> -#define clamp(val, min, max) ({			\
> -	typeof(val) __val = (val);		\
> -	typeof(min) __min = (min);		\
> -	typeof(max) __max = (max);		\
> -	(void) (&__val == &__min);		\
> -	(void) (&__val == &__max);		\
> -	__val = __val < __min ? __min: __val;	\
> -	__val > __max ? __max: __val; })
> -
>  /*
>   * 16-bit Linear Feedback Shift Register (LFSR)
>   */
> diff --git a/xen/include/asm-x86/div64.h b/xen/include/asm-x86/div64.h
> index dd49f64..6ba03cb 100644
> --- a/xen/include/asm-x86/div64.h
> +++ b/xen/include/asm-x86/div64.h
> @@ -11,4 +11,83 @@
>      __rem;                                      \
>  })
>  

Comment to explain the functionality?

> +static inline uint64_t div_u64_rem(uint64_t dividend, uint32_t divisor,
> +                                      uint32_t *remainder)

Alignment

> +{
> +    *remainder = do_div(dividend, divisor);
> +    return dividend;
> +}
> +
> +static inline uint64_t div_u64(uint64_t dividend, uint32_t  divisor)


> +
> +/**
>   * container_of - cast a member of a structure out to the containing structure
>   *
>   * @ptr:	the pointer to the member.

Too many spaces before "divisor"

> +{
> +    uint32_t remainder;
> +
> +    return div_u64_rem(dividend, divisor, &remainder);
> +}
> +
> +/*
> + * div64_u64 - unsigned 64bit divide with 64bit divisor
> + * @dividend:    64bit dividend
> + * @divisor:    64bit divisor

Alignment also looks wonky here.

> + *
> + * This implementation is a modified version of the algorithm proposed
> + * by the book 'Hacker's Delight'.  The original source and full proof
> + * can be found here and is available for use without restriction.
> + *
> + * 'http://www.hackersdelight.org/HDcode/newCode/divDouble.c.txt'
> + */
> +static inline uint64_t div64_u64(uint64_t dividend, uint64_t divisor)
> +{
> +    uint32_t high = divisor >> 32;
> +    uint64_t quot;
> +
> +    if ( high == 0 )
> +        quot = div_u64(dividend, divisor);
> +    else
> +    {
> +        int n = 1 + fls(high);
> +
> +        quot = div_u64(dividend >> n, divisor >> n);
> +
> +        if ( quot != 0 )
> +            quot--;
> +        if ( (dividend - quot * divisor) >= divisor )
> +            quot++;
> +    }
> +    return quot;
> +}
> +

Comment?

> +static inline int64_t div_s64_rem(int64_t dividend, int32_t divisor,
> +                                     int32_t *remainder)

Alignment.

> +{
> +    int64_t quotient;
> +
> +    if ( dividend < 0 )
> +    {
> +        quotient = div_u64_rem(-dividend, ABS(divisor),
> +                        (uint32_t *)remainder);

Alignment.

> +        *remainder = -*remainder;
> +        if ( divisor > 0 )
> +            quotient = -quotient;
> +    }
> +    else
> +    {
> +        quotient = div_u64_rem(dividend, ABS(divisor),
> +                        (uint32_t *)remainder);

Alignment.

> +        if ( divisor < 0 )
> +            quotient = -quotient;
> +    }
> +    return quotient;
> +}
> +
> +/*
> + * div_s64 - signed 64bit divide with 32bit divisor
> + */
> +static inline int64_t div_s64(int64_t dividend, int32_t divisor)
> +{
> +    int32_t remainder;
> +
> +    return div_s64_rem(dividend, divisor, &remainder);
> +}
> +
>  #endif
> diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h
> index 548b64d..9812698 100644
> --- a/xen/include/xen/kernel.h
> +++ b/xen/include/xen/kernel.h
> @@ -43,6 +43,29 @@
>  #define MAX(x,y) ((x) > (y) ? (x) : (y))
>  
>  /**
> + * clamp - return a value clamped to a given range with strict typechecking
> + * @val: current value
> + * @lo: lowest allowable value
> + * @hi: highest allowable value
> + *
> + * This macro does strict typechecking of lo/hi to make sure they are of the
> + * same type as val.  See the unnecessary pointer comparisons.
> + */
> +#define clamp(val, lo, hi) min((typeof(val))max(val, lo), hi)

This is a change of behaviour from the clamp() you removed, as this now
evaluates its arguments multiple times.

Please use a ({ }) style macro to avoid evaluating the arguments
multiple times.

> +
> +/*
> + * clamp_t - return a value clamped to a given range using a given type
> + * @type: the type of variable to use
> + * @val: current value
> + * @lo: minimum allowable value
> + * @hi: maximum allowable value
> + *
> + * This macro does no typechecking and uses temporary variables of type
> + * 'type' to make all the comparisons.
> + */
> +#define clamp_t(type, val, lo, hi) min_t(type, max_t(type, val, lo), hi)

And here as well please.

~Andrew