Re: [PATCH 1/2] timekeeping: Use unsigned int for seqlock sequence

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- Original Message -----
> From: "George Spelvin" <linux@horizon.com>
> To: "john stultz" <john.stultz@linaro.org>, linux@horizon.com
> Cc: linux-kernel@vger.kernel.org, "mathieu desnoyers" <mathieu.desnoyers@efficios.com>
> Sent: Monday, May 12, 2014 10:45:19 PM
> Subject: [PATCH 1/2] timekeeping: Use unsigned int for seqlock sequence
> 
> read_seqlock_begin returns an int, so using "unsigned long" to
> hold the return value is harmless, but a waste.

Did you try comparing this to changing read_seqlock to return
an "unsigned long" instead ? Does it change the executable
size at all ?

Seq locks correctness depends on ensuring the sequence
counter does not overflow. Strictly speaking, a 64-bit
value provides a stronger no-overflow guarantee than
a 32-bit value, even though it is already unlikely that
the 32-bit value will ever overflow in a sane system.
Therefore, I'd be tempted to use "unsigned long" if
possible if it's not significantly more expensive.

Thoughts ?

Thanks,

Mathieu

> 
> Signed-off-by: George Spelvin <linux@horizon.com>
> ---
>  kernel/time/timekeeping.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index f7df8ea217..14e703e5bd 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -300,7 +300,7 @@ static void timekeeping_forward_now(struct timekeeper
> *tk)
>  int __getnstimeofday(struct timespec *ts)
>  {
>  	struct timekeeper *tk = &timekeeper;
> -	unsigned long seq;
> +	unsigned int seq;
>  	s64 nsecs = 0;
>  
>  	do {
> @@ -399,7 +399,7 @@ EXPORT_SYMBOL_GPL(ktime_get_ts);
>  void timekeeping_clocktai(struct timespec *ts)
>  {
>  	struct timekeeper *tk = &timekeeper;
> -	unsigned long seq;
> +	unsigned int seq;
>  	u64 nsecs;
>  
>  	WARN_ON(timekeeping_suspended);
> @@ -447,7 +447,7 @@ EXPORT_SYMBOL(ktime_get_clocktai);
>  void getnstime_raw_and_real(struct timespec *ts_raw, struct timespec
>  *ts_real)
>  {
>  	struct timekeeper *tk = &timekeeper;
> -	unsigned long seq;
> +	unsigned int seq;
>  	s64 nsecs_raw, nsecs_real;
>  
>  	WARN_ON_ONCE(timekeeping_suspended);
> @@ -700,7 +700,7 @@ EXPORT_SYMBOL_GPL(ktime_get_real);
>  void getrawmonotonic(struct timespec *ts)
>  {
>  	struct timekeeper *tk = &timekeeper;
> -	unsigned long seq;
> +	unsigned int seq;
>  	s64 nsecs;
>  
>  	do {
> @@ -720,7 +720,7 @@ EXPORT_SYMBOL(getrawmonotonic);
>  int timekeeping_valid_for_hres(void)
>  {
>  	struct timekeeper *tk = &timekeeper;
> -	unsigned long seq;
> +	unsigned int seq;
>  	int ret;
>  
>  	do {
> @@ -739,7 +739,7 @@ int timekeeping_valid_for_hres(void)
>  u64 timekeeping_max_deferment(void)
>  {
>  	struct timekeeper *tk = &timekeeper;
> -	unsigned long seq;
> +	unsigned int seq;
>  	u64 ret;
>  
>  	do {
> @@ -1546,7 +1546,7 @@ struct timespec current_kernel_time(void)
>  {
>  	struct timekeeper *tk = &timekeeper;
>  	struct timespec now;
> -	unsigned long seq;
> +	unsigned int seq;
>  
>  	do {
>  		seq = read_seqcount_begin(&timekeeper_seq);
> @@ -1562,7 +1562,7 @@ struct timespec get_monotonic_coarse(void)
>  {
>  	struct timekeeper *tk = &timekeeper;
>  	struct timespec now, mono;
> -	unsigned long seq;
> +	unsigned int seq;
>  
>  	do {
>  		seq = read_seqcount_begin(&timekeeper_seq);
> @@ -1596,7 +1596,7 @@ void get_xtime_and_monotonic_and_sleep_offset(struct
> timespec *xtim,
>  				struct timespec *wtom, struct timespec *sleep)
>  {
>  	struct timekeeper *tk = &timekeeper;
> -	unsigned long seq;
> +	unsigned int seq;
>  
>  	do {
>  		seq = read_seqcount_begin(&timekeeper_seq);
> @@ -1647,7 +1647,7 @@ ktime_t ktime_get_update_offsets(ktime_t *offs_real,
> ktime_t *offs_boot,
>  ktime_t ktime_get_monotonic_offset(void)
>  {
>  	struct timekeeper *tk = &timekeeper;
> -	unsigned long seq;
> +	unsigned int seq;
>  	struct timespec wtom;
>  
>  	do {
> --
> 1.9.2
> 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com