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

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

[chengang]

From: Chen Gang <chengang@emindsoft.com.cn>

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@gmail.com>
---
 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
 
-- 
1.9.3

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@emindsoft.com.cn wrote:
> From: Chen Gang <chengang@emindsoft.com.cn>
> 
> 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@gmail.com>
> ---
>  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.

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

[Andrew Morton]

On Wed,  5 Oct 2016 21:40:10 +0800 chengang@emindsoft.com.cn wrote:

> 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.

hm.  Why is this an improvement?

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

[Chen Gang]

On 10/21/16 11:41, Andrew Morton wrote:
> On Wed,  5 Oct 2016 21:40:10 +0800 chengang@emindsoft.com.cn wrote:
> 
>> 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.
> 
> hm.  Why is this an improvement?
> 

For me, it will let code a little more understanding, a little simpler,
and let the code a little more extendable (when kernel members really
needs comp2_t in future, they need not have to treat it as __u32).

Only when comp2_t is really used in api header in future, kernel has to
know about it, but kernel still can keep original code no touch. So for
me, our changing is harmless.

Thanks.
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.

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

[Chen Gang]

On 10/22/16 06:53, Chen Gang wrote:
> 
> On 10/21/16 11:41, Andrew Morton wrote:
>> On Wed,  5 Oct 2016 21:40:10 +0800 chengang@emindsoft.com.cn wrote:
>>
>>> 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.
>>
>> hm.  Why is this an improvement?
>>
> 
> For me, it will let code a little more understanding, a little simpler,
> and let the code a little more extendable (when kernel members really
> needs comp2_t in future, they need not have to treat it as __u32).
> 
> Only when comp2_t is really used in api header in future, kernel has to
> know about it, but kernel still can keep original code no touch. So for
> me, our changing is harmless.
> 

Oh sorry, for "Only when comp2_t is really used in api header in future",
we may need encode_comp2_t, but in kernel wide, this changing is very
small.

At present, only encode_comp2_t uses comp2_t, and it is only called by
fill_ac in an area, and the goal of fill_ac is to encode etime to ac (
comp2_t is the intermediate generation).

And I guess, we have very small chance to use comp2_t in uapi header in
future, so now, encode_comp2_t can be removed, when we really need it,
we can revert to encode_comp2_t and let encode_ac_etime_hilo call it.

Thanks
-- 
Chen Gang (陈刚)

Managing Natural Environments is the Duty of Human Beings.