Re: [PATCH] uapi: linux: acct: Remove redundant type comp2_t from kernel

[Chen Gang]

Hello:

For me if necessary, can add additional comments for it:

  If kernel will really need comp2_t in the future (I guess not), kernel
  can define it as a union with structures (need not must be the same as
  the user mode), which will be easier for kernel using. e.g.

	#pragma push(1)
	typedef union {
	#if ACCT_BYTEORDER == 0x80 /* big endian */
		struct {
			__u8  pad0;
			__u8  ac_etime_hi;
			__u16 ac_etime_lo;
		};
		struct {
			__u32 pad1 : 8;
			__u32 exp  : 5;
			__u32 mant : 19;
		};
	#else
		struct {
			__u16 ac_etime_lo;
			__u8  ac_etime_hi;
		};
		struct {
			__u32 mant : 19;
			__u32 exp  : 5;
		};
	#endif
		__u32 v;
	} comp2_t;
	#pragma pop()

  Some members may be not interested in bits definition (mant, exp), I
  guess, it is because of performance reason (but I am not quite sure).
  But ac_etime_lo and ac_etime_hi are useful.

  Although this definition may be better than u32, we can not change the
  uapi -- which may cause user mode application generate building break,
  e.g. some informal usage -- use u32 directly for shift operations.


Thanks.

On 10/5/16 21:40, chengang@xxxxxxxxxxxxxxxx wrote:
> From: Chen Gang <chengang@xxxxxxxxxxxxxxxx>
> 
> In api itself, kernel does not use it -- it is divided into ac_etime_hi
> and ac_etime_lo. So kernel side only need generate the correct
> ac_etime_hi and ac_etime_lo, but need not know about comp2_t.
> 
> At present, kernel use normal u64 type for it, when kernel provdes it to
> outside, kernel can translate it into ac_etime_hi and ac_etime_lo,
> directly, but need not notice about comp2_t, in fact.
> 
> The patch notices about coding styles:
> 
>  - Use 1 instead of 1ul, since type int is enough.
> 
>  - Use white space between operator and constant or macro.
> 
>  - Initialize variables directly, when declare it, since they need be
>    initialized and can be initialized by constant (include constant
>    macros).
> 
>  - Remove redundant empty line.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@xxxxxxxxx>
> ---
>  include/uapi/linux/acct.h |  6 ++++--
>  kernel/acct.c             | 31 ++++++++++++++-----------------
>  2 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/include/uapi/linux/acct.h b/include/uapi/linux/acct.h
> index df2f9a0..97acdd4 100644
> --- a/include/uapi/linux/acct.h
> +++ b/include/uapi/linux/acct.h
> @@ -24,12 +24,15 @@
>   *  comp_t is a 16-bit "floating" point number with a 3-bit base 8
>   *  exponent and a 13-bit fraction.
>   *  comp2_t is 24-bit with 5-bit base 2 exponent and 20 bit fraction
> - *  (leading 1 not stored).
> + *  (leading 1 not stored). And it is described as ac_etime_hi and
> + *  ac_etime_lo in kernel.
>   *  See linux/kernel/acct.c for the specific encoding systems used.
>   */
>  
>  typedef __u16	comp_t;
> +#ifndef __KERNEL__
>  typedef __u32	comp2_t;
> +#endif	/* __KERNEL */
>  
>  /*
>   *   accounting file record
> @@ -120,5 +123,4 @@ struct acct_v3
>  #define AHZ		(HZ)
>  #endif	/* __KERNEL */
>  
> -
>  #endif /* _UAPI_LINUX_ACCT_H */
> diff --git a/kernel/acct.c b/kernel/acct.c
> index 74963d1..f707a10 100644
> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -338,7 +338,7 @@ static comp_t encode_comp_t(unsigned long value)
>  
>  #if ACCT_VERSION == 1 || ACCT_VERSION == 2
>  /*
> - * encode an u64 into a comp2_t (24 bits)
> + * encode an u64 into ac_etime_hi and ac_etime_lo (24 bits)
>   *
>   * Format: 5 bit base 2 exponent, 20 bits mantissa.
>   * The leading bit of the mantissa is not stored, but implied for
> @@ -348,15 +348,15 @@ static comp_t encode_comp_t(unsigned long value)
>  
>  #define MANTSIZE2       20                      /* 20 bit mantissa. */
>  #define EXPSIZE2        5                       /* 5 bit base 2 exponent. */
> -#define MAXFRACT2       ((1ul << MANTSIZE2) - 1) /* Maximum fractional value. */
> -#define MAXEXP2         ((1 << EXPSIZE2) - 1)    /* Maximum exponent. */
> +#define MAXFRACT2       ((1 << MANTSIZE2) - 1)  /* Maximum fractional value. */
> +#define MAXEXP2         ((1 << EXPSIZE2) - 1)   /* Maximum exponent. */
>  
> -static comp2_t encode_comp2_t(u64 value)
> +static void encode_ac_etime_hilo(u64 value, acct_t *ac)
>  {
> -	int exp, rnd;
> +	int exp = (value > (MAXFRACT2 >> 1));
> +	int rnd = 0;
> +	u32 etime;
>  
> -	exp = (value > (MAXFRACT2>>1));
> -	rnd = 0;
>  	while (value > MAXFRACT2) {
>  		rnd = value & 1;
>  		value >>= 1;
> @@ -373,10 +373,12 @@ static comp2_t encode_comp2_t(u64 value)
>  
>  	if (exp > MAXEXP2) {
>  		/* Overflow. Return largest representable number instead. */
> -		return (1ul << (MANTSIZE2+EXPSIZE2-1)) - 1;
> +		etime = (1 << (MANTSIZE2 + EXPSIZE2 - 1)) - 1;
>  	} else {
> -		return (value & (MAXFRACT2>>1)) | (exp << (MANTSIZE2-1));
> +		etime = (value & (MAXFRACT2 >> 1)) | (exp << (MANTSIZE2 - 1));
>  	}
> +	ac->ac_etime_hi = etime >> 16;
> +	ac->ac_etime_lo = (u16) etime;
>  }
>  #endif
>  
> @@ -436,17 +438,12 @@ static void fill_ac(acct_t *ac)
>  				(unsigned long) elapsed : (unsigned long) -1l);
>  #endif
>  #if ACCT_VERSION == 1 || ACCT_VERSION == 2
> -	{
> -		/* new enlarged etime field */
> -		comp2_t etime = encode_comp2_t(elapsed);
> -
> -		ac->ac_etime_hi = etime >> 16;
> -		ac->ac_etime_lo = (u16) etime;
> -	}
> +	/* new enlarged etime field */
> +	encode_ac_etime_hilo(elapsed, ac);
>  #endif
>  	do_div(elapsed, AHZ);
>  	ac->ac_btime = get_seconds() - elapsed;
> -#if ACCT_VERSION==2
> +#if ACCT_VERSION == 2
>  	ac->ac_ahz = AHZ;
>  #endif
>  
> 

-- 
Chen Gang (éå)

Managing Natural Environments is the Duty of Human Beings.