Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320354 > unrolled thread

[PATCH] cputime: Fix timeval-->cputime conversion

Back to article view | Back to linux.kernel

Contents

  [PATCH] cputime: Fix timeval-->cputime conversion zengtao <prime.zeng@huawei.com> - 2016-01-28 08:10 +0100
    Re: [PATCH] cputime: Fix timeval-->cputime conversion Thomas Gleixner <tglx@linutronix.de> - 2016-01-28 09:30 +0100
      Re: [PATCH] cputime: Fix timeval-->cputime conversion Arnd Bergmann <arnd@arndb.de> - 2016-01-28 13:00 +0100
        RE: [PATCH] cputime: Fix timeval-->cputime conversion "Zengtao (B)" <prime.zeng@hisilicon.com> - 2016-01-29 04:20 +0100
          Re: [PATCH] cputime: Fix timeval-->cputime conversion Arnd Bergmann <arnd@arndb.de> - 2016-01-29 09:50 +0100
            RE: [PATCH] cputime: Fix timeval-->cputime conversion "Zengtao (B)" <prime.zeng@hisilicon.com> - 2016-01-30 03:40 +0100
            RE: [PATCH] cputime: Fix timeval-->cputime conversion "Zengtao (B)" <prime.zeng@hisilicon.com> - 2016-02-01 05:00 +0100
              RE: [PATCH] cputime: Fix timeval-->cputime conversion Thomas Gleixner <tglx@linutronix.de> - 2016-02-01 09:50 +0100
                RE: [PATCH] cputime: Fix timeval-->cputime conversion "Zengtao (B)" <prime.zeng@hisilicon.com> - 2016-02-02 04:00 +0100

#1320354 — [PATCH] cputime: Fix timeval-->cputime conversion

The structure:
struct timeval {
	__kernel_time_t		tv_sec;		/* seconds */
	__kernel_suseconds_t	tv_usec;	/* microseconds */
};
both __kernel_time_t and __kernel_suseconds_t are short than u64
when it is 32bit platform, so force u64 conversion here.

Signed-off-by: zengtao <prime.zeng@huawei.com>
---
 include/asm-generic/cputime_nsecs.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/asm-generic/cputime_nsecs.h b/include/asm-generic/cputime_nsecs.h
index 0419485..e2f7ff9 100644
--- a/include/asm-generic/cputime_nsecs.h
+++ b/include/asm-generic/cputime_nsecs.h
@@ -91,7 +91,8 @@ static inline void cputime_to_timespec(const cputime_t ct, struct timespec *val)
  */
 static inline cputime_t timeval_to_cputime(const struct timeval *val)
 {
-	u64 ret = val->tv_sec * NSEC_PER_SEC + val->tv_usec * NSEC_PER_USEC;
+	u64 ret = (u64)val->tv_sec * NSEC_PER_SEC +
+			val->tv_usec * NSEC_PER_USEC;
 	return (__force cputime_t) ret;
 }
 static inline void cputime_to_timeval(const cputime_t ct, struct timeval *val)
-- 
1.9.1

[toc] | [next] | [standalone]

#1320393

Cc'ing Arnd

On Thu, 28 Jan 2016, zengtao wrote:

> The structure:
> struct timeval {
> 	__kernel_time_t		tv_sec;		/* seconds */
> 	__kernel_suseconds_t	tv_usec;	/* microseconds */
> };
> both __kernel_time_t and __kernel_suseconds_t are short than u64
> when it is 32bit platform, so force u64 conversion here.
> 
> Signed-off-by: zengtao <prime.zeng@huawei.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

> ---
>  include/asm-generic/cputime_nsecs.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/asm-generic/cputime_nsecs.h b/include/asm-generic/cputime_nsecs.h
> index 0419485..e2f7ff9 100644
> --- a/include/asm-generic/cputime_nsecs.h
> +++ b/include/asm-generic/cputime_nsecs.h
> @@ -91,7 +91,8 @@ static inline void cputime_to_timespec(const cputime_t ct, struct timespec *val)
>   */
>  static inline cputime_t timeval_to_cputime(const struct timeval *val)
>  {
> -	u64 ret = val->tv_sec * NSEC_PER_SEC + val->tv_usec * NSEC_PER_USEC;
> +	u64 ret = (u64)val->tv_sec * NSEC_PER_SEC +
> +			val->tv_usec * NSEC_PER_USEC;
>  	return (__force cputime_t) ret;
>  }
>  static inline void cputime_to_timeval(const cputime_t ct, struct timeval *val)
> -- 
> 1.9.1
> 
> 

[toc] | [prev] | [next] | [standalone]

#1320608

On Thursday 28 January 2016 09:22:04 Thomas Gleixner wrote:
> Cc'ing Arnd
> 
> On Thu, 28 Jan 2016, zengtao wrote:
> 
> > The structure:
> > struct timeval {
> >       __kernel_time_t         tv_sec;         /* seconds */
> >       __kernel_suseconds_t    tv_usec;        /* microseconds */
> > };
> > both __kernel_time_t and __kernel_suseconds_t are short than u64
> > when it is 32bit platform, so force u64 conversion here.
> > 
> > Signed-off-by: zengtao <prime.zeng@huawei.com>
> 
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

This seems to miss timespec_to_cputime(), which has the same problem,
so only setitimer() is fixed, but not nanosleep() or timer_settime().

There should probably be some explanation in which cases this happens,
my reading is that can only occur on MIPS32 and ARM32 with "Full dynticks
CPU time accounting" enabled, which is required for CONFIG_NO_HZ_FULL,
so we need this backported to any kernel that includes
31c1fc818715 ("ARM: Kconfig: allow full nohz CPU accounting"), i.e.
v3.13 or higher, correct?

	Arnd

> >  include/asm-generic/cputime_nsecs.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/asm-generic/cputime_nsecs.h b/include/asm-generic/cputime_nsecs.h
> > index 0419485..e2f7ff9 100644
> > --- a/include/asm-generic/cputime_nsecs.h
> > +++ b/include/asm-generic/cputime_nsecs.h
> > @@ -91,7 +91,8 @@ static inline void cputime_to_timespec(const cputime_t ct, struct timespec *val)
> >   */
> >  static inline cputime_t timeval_to_cputime(const struct timeval *val)
> >  {
> > -     u64 ret = val->tv_sec * NSEC_PER_SEC + val->tv_usec * NSEC_PER_USEC;
> > +     u64 ret = (u64)val->tv_sec * NSEC_PER_SEC +
> > +                     val->tv_usec * NSEC_PER_USEC;
> >       return (__force cputime_t) ret;
> >  }
> >  static inline void cputime_to_timeval(const cputime_t ct, struct timeval *val)
> > -- 
> > 1.9.1
> > 
> > 

[toc] | [prev] | [next] | [standalone]

#1321455

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Thursday, January 28, 2016 7:52 PM
> To: Thomas Gleixner
> Cc: Zengtao (B); LKML; Frederic Weisbecker
> Subject: Re: [PATCH] cputime: Fix timeval-->cputime conversion
> 
> On Thursday 28 January 2016 09:22:04 Thomas Gleixner wrote:
> > Cc'ing Arnd
> >
> > On Thu, 28 Jan 2016, zengtao wrote:
> >
> > > The structure:
> > > struct timeval {
> > >       __kernel_time_t         tv_sec;         /* seconds */
> > >       __kernel_suseconds_t    tv_usec;        /* microseconds */
> > > };
> > > both __kernel_time_t and __kernel_suseconds_t are short than u64
> > > when it is 32bit platform, so force u64 conversion here.
> > >
> > > Signed-off-by: zengtao <prime.zeng@huawei.com>
> >
> > Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> 
> This seems to miss timespec_to_cputime(), which has the same problem,
> so only setitimer() is fixed, but not nanosleep() or timer_settime().
Yes, I have checked the code just now, the timespec_to_cputime() has the 
same problem.I found the origin issue through setitimer().And I think the 
timespec_to_cputime() only affects timer_settime(),by which means it affects 
nanosleep? 

> 
> There should probably be some explanation in which cases this happens,
> my reading is that can only occur on MIPS32 and ARM32 with "Full dynticks
> CPU time accounting" enabled, which is required for CONFIG_NO_HZ_FULL,
> so we need this backported to any kernel that includes
> 31c1fc818715 ("ARM: Kconfig: allow full nohz CPU accounting"), i.e.
> v3.13 or higher, correct?
Yes.
> 
> 	Arnd
> 
> > >  include/asm-generic/cputime_nsecs.h | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/asm-generic/cputime_nsecs.h
> b/include/asm-generic/cputime_nsecs.h
> > > index 0419485..e2f7ff9 100644
> > > --- a/include/asm-generic/cputime_nsecs.h
> > > +++ b/include/asm-generic/cputime_nsecs.h
> > > @@ -91,7 +91,8 @@ static inline void cputime_to_timespec(const
> cputime_t ct, struct timespec *val)
> > >   */
> > >  static inline cputime_t timeval_to_cputime(const struct timeval *val)
> > >  {
> > > -     u64 ret = val->tv_sec * NSEC_PER_SEC + val->tv_usec *
> NSEC_PER_USEC;
> > > +     u64 ret = (u64)val->tv_sec * NSEC_PER_SEC +
> > > +                     val->tv_usec * NSEC_PER_USEC;
> > >       return (__force cputime_t) ret;
> > >  }
> > >  static inline void cputime_to_timeval(const cputime_t ct, struct timeval
> *val)
> > > --
> > > 1.9.1
> > >
> > >

[toc] | [prev] | [next] | [standalone]

#1321579

On Friday 29 January 2016 03:12:37 Zengtao wrote:
> > -----Original Message-----
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > Sent: Thursday, January 28, 2016 7:52 PM
> > To: Thomas Gleixner
> > Cc: Zengtao (B); LKML; Frederic Weisbecker
> > Subject: Re: [PATCH] cputime: Fix timeval-->cputime conversion
> > 
> > On Thursday 28 January 2016 09:22:04 Thomas Gleixner wrote:
> > > Cc'ing Arnd
> > >
> > > On Thu, 28 Jan 2016, zengtao wrote:
> > >
> > > > The structure:
> > > > struct timeval {
> > > >       __kernel_time_t         tv_sec;         /* seconds */
> > > >       __kernel_suseconds_t    tv_usec;        /* microseconds */
> > > > };
> > > > both __kernel_time_t and __kernel_suseconds_t are short than u64
> > > > when it is 32bit platform, so force u64 conversion here.
> > > >
> > > > Signed-off-by: zengtao <prime.zeng@huawei.com>
> > >
> > > Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> > 
> > This seems to miss timespec_to_cputime(), which has the same problem,
> > so only setitimer() is fixed, but not nanosleep() or timer_settime().
> Yes, I have checked the code just now, the timespec_to_cputime() has the 
> same problem.I found the origin issue through setitimer().And I think the 
> timespec_to_cputime() only affects timer_settime(),by which means it affects 
> nanosleep? 

Reading that code again, I think it does not affect sys_nanosleep, but
it does affect sys_clock_nanosleep(CLOCK_PROCESS_CPUTIME_ID, ...)
along with timer_create/timer_settime with CLOCK_PROCESS_CPUTIME_ID.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1322233

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@arndb.de]
> Sent: Friday, January 29, 2016 4:46 PM
> To: Zengtao (B)
> Cc: Thomas Gleixner; LKML; Frederic Weisbecker
> Subject: Re: [PATCH] cputime: Fix timeval-->cputime conversion
> 
> On Friday 29 January 2016 03:12:37 Zengtao wrote:
> > > -----Original Message-----
> > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > Sent: Thursday, January 28, 2016 7:52 PM
> > > To: Thomas Gleixner
> > > Cc: Zengtao (B); LKML; Frederic Weisbecker
> > > Subject: Re: [PATCH] cputime: Fix timeval-->cputime conversion
> > >
> > > On Thursday 28 January 2016 09:22:04 Thomas Gleixner wrote:
> > > > Cc'ing Arnd
> > > >
> > > > On Thu, 28 Jan 2016, zengtao wrote:
> > > >
> > > > > The structure:
> > > > > struct timeval {
> > > > >       __kernel_time_t         tv_sec;         /* seconds */
> > > > >       __kernel_suseconds_t    tv_usec;        /* microseconds
> */
> > > > > };
> > > > > both __kernel_time_t and __kernel_suseconds_t are short than u64
> > > > > when it is 32bit platform, so force u64 conversion here.
> > > > >
> > > > > Signed-off-by: zengtao <prime.zeng@huawei.com>
> > > >
> > > > Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> > >
> > > This seems to miss timespec_to_cputime(), which has the same problem,
> > > so only setitimer() is fixed, but not nanosleep() or timer_settime().
> > Yes, I have checked the code just now, the timespec_to_cputime() has the
> > same problem.I found the origin issue through setitimer().And I think the
> > timespec_to_cputime() only affects timer_settime(),by which means it
> affects
> > nanosleep?
> 
> Reading that code again, I think it does not affect sys_nanosleep, but
> it does affect sys_clock_nanosleep(CLOCK_PROCESS_CPUTIME_ID, ...)
> along with timer_create/timer_settime with CLOCK_PROCESS_CPUTIME_ID.
> 
Got it, I will fix the timespec_to_cputime and resend the patch later.
> 	Arnd

[toc] | [prev] | [next] | [standalone]

#1322811

Hi Arnd:
	I have got a new idea about the problem: 
In include/linux/time64.h
#define NSEC_PER_SEC	1000000000L 
I think we should change it to
#define NSEC_PER_SEC	1000000000LL 

My reason is :
1.  when it is used in a multiplication, it will easily get overflow.
2.  when it don't get overflow, the change has no side affect.

Thanks.
Zengtao

> -----Original Message-----
> From: Zengtao (B)
> Sent: Saturday, January 30, 2016 10:31 AM
> To: 'Arnd Bergmann'
> Cc: Thomas Gleixner; LKML; Frederic Weisbecker
> Subject: RE: [PATCH] cputime: Fix timeval-->cputime conversion
> 
> > -----Original Message-----
> > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > Sent: Friday, January 29, 2016 4:46 PM
> > To: Zengtao (B)
> > Cc: Thomas Gleixner; LKML; Frederic Weisbecker
> > Subject: Re: [PATCH] cputime: Fix timeval-->cputime conversion
> >
> > On Friday 29 January 2016 03:12:37 Zengtao wrote:
> > > > -----Original Message-----
> > > > From: Arnd Bergmann [mailto:arnd@arndb.de]
> > > > Sent: Thursday, January 28, 2016 7:52 PM
> > > > To: Thomas Gleixner
> > > > Cc: Zengtao (B); LKML; Frederic Weisbecker
> > > > Subject: Re: [PATCH] cputime: Fix timeval-->cputime conversion
> > > >
> > > > On Thursday 28 January 2016 09:22:04 Thomas Gleixner wrote:
> > > > > Cc'ing Arnd
> > > > >
> > > > > On Thu, 28 Jan 2016, zengtao wrote:
> > > > >
> > > > > > The structure:
> > > > > > struct timeval {
> > > > > >       __kernel_time_t         tv_sec;         /* seconds */
> > > > > >       __kernel_suseconds_t    tv_usec;        /* microseconds
> > */
> > > > > > };
> > > > > > both __kernel_time_t and __kernel_suseconds_t are short than u64
> > > > > > when it is 32bit platform, so force u64 conversion here.
> > > > > >
> > > > > > Signed-off-by: zengtao <prime.zeng@huawei.com>
> > > > >
> > > > > Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
> > > >
> > > > This seems to miss timespec_to_cputime(), which has the same problem,
> > > > so only setitimer() is fixed, but not nanosleep() or timer_settime().
> > > Yes, I have checked the code just now, the timespec_to_cputime() has the
> > > same problem.I found the origin issue through setitimer().And I think the
> > > timespec_to_cputime() only affects timer_settime(),by which means it
> > affects
> > > nanosleep?
> >
> > Reading that code again, I think it does not affect sys_nanosleep, but
> > it does affect sys_clock_nanosleep(CLOCK_PROCESS_CPUTIME_ID, ...)
> > along with timer_create/timer_settime with CLOCK_PROCESS_CPUTIME_ID.
> >
> Got it, I will fix the timespec_to_cputime and resend the patch later.
> > 	Arnd

[toc] | [prev] | [next] | [standalone]

#1322909

On Mon, 1 Feb 2016, Zengtao (B) wrote:

> Hi Arnd:
> 	I have got a new idea about the problem: 
> In include/linux/time64.h
> #define NSEC_PER_SEC	1000000000L 
> I think we should change it to
> #define NSEC_PER_SEC	1000000000LL 
> 
> My reason is :
> 1.  when it is used in a multiplication, it will easily get overflow.
> 2.  when it don't get overflow, the change has no side affect.

That's not a good idea. NSEC_PER_SEC is used in lots of places with 32bit
storage. I really don't want to mop up all the fallout.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1323747

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Monday, February 01, 2016 4:43 PM
> To: Zengtao (B)
> Cc: Arnd Bergmann; LKML; Frederic Weisbecker
> Subject: RE: [PATCH] cputime: Fix timeval-->cputime conversion
> 
> On Mon, 1 Feb 2016, Zengtao (B) wrote:
> 
> > Hi Arnd:
> > 	I have got a new idea about the problem:
> > In include/linux/time64.h
> > #define NSEC_PER_SEC	1000000000L
> > I think we should change it to
> > #define NSEC_PER_SEC	1000000000LL
> >
> > My reason is :
> > 1.  when it is used in a multiplication, it will easily get overflow.
> > 2.  when it don't get overflow, the change has no side affect.
> 
> That's not a good idea. NSEC_PER_SEC is used in lots of places with 32bit
> storage. I really don't want to mop up all the fallout.

Yes, agree, a lot of places it has been used as 32bit value. Beyond that, I think 
it is more reasonable to use 64bit. 

> 
> Thanks,
> 
> 	tglx
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web