From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755319AbcJFLAo (ORCPT ); Thu, 6 Oct 2016 07:00:44 -0400 Received: from out28-122.mail.aliyun.com ([115.124.28.122]:35402 "EHLO out28-122.mail.aliyun.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751693AbcJFLAf (ORCPT ); Thu, 6 Oct 2016 07:00:35 -0400 X-Greylist: delayed 304 seconds by postgrey-1.27 at vger.kernel.org; Thu, 06 Oct 2016 07:00:34 EDT X-Alimail-AntiSpam: AC=CONTINUE;BC=0.07445417|-1;FP=0|0|0|0|0|-1|-1|-1;HT=e02c03282;MF=chengang@emindsoft.com.cn;NM=1;PH=DS;RN=4;RT=4;SR=0;TI=SMTPD_---.71FIOFS_1475751321; Message-ID: <57F62F72.10002@emindsoft.com.cn> Date: Thu, 06 Oct 2016 19:03:14 +0800 From: Chen Gang User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: akpm@linux-foundation.org, dhowells@redhat.com CC: linux-kernel@vger.kernel.org, Chen Gang Subject: Re: [PATCH] uapi: linux: acct: Remove redundant type comp2_t from kernel References: <1475674810-1560-1-git-send-email-chengang@emindsoft.com.cn> In-Reply-To: <1475674810-1560-1-git-send-email-chengang@emindsoft.com.cn> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > 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 > --- > 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.