linux-kernel - Re: [PATCH RFC tip/core/rcu 13/20] rcu: increase synchronize_sched

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 19 Dec 2010 10:43:46 +0100
From:	Tejun Heo <tj@...nel.org>
To:	paulmck@...ux.vnet.ibm.com
CC:	linux-kernel@...r.kernel.org, mingo@...e.hu, laijs@...fujitsu.com,
	dipankar@...ibm.com, akpm@...ux-foundation.org,
	mathieu.desnoyers@...ymtl.ca, josh@...htriplett.org,
	niv@...ibm.com, tglx@...utronix.de, peterz@...radead.org,
	rostedt@...dmis.org, Valdis.Kletnieks@...edu, dhowells@...hat.com,
	eric.dumazet@...il.com, darren@...art.com
Subject: Re: [PATCH RFC tip/core/rcu 13/20] rcu: increase synchronize_sched_expedited()
 batching

Hello,

On 12/18/2010 09:14 PM, Paul E. McKenney wrote:
>>> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
>>> index 49e8e16..af56148 100644
>>> --- a/include/linux/rcupdate.h
>>> +++ b/include/linux/rcupdate.h
>>> @@ -47,6 +47,8 @@
>>>  extern int rcutorture_runnable; /* for sysctl */
>>>  #endif /* #ifdef CONFIG_RCU_TORTURE_TEST */
>>>  
>>> +#define UINT_CMP_GE(a, b)	(UINT_MAX / 2 >= (a) - (b))
>>> +#define UINT_CMP_LT(a, b)	(UINT_MAX / 2 < (a) - (b))
>>>  #define ULONG_CMP_GE(a, b)	(ULONG_MAX / 2 >= (a) - (b))
>>>  #define ULONG_CMP_LT(a, b)	(ULONG_MAX / 2 < (a) - (b))
>>
>> I don't think the original comparison had overflow problem.  (a) < (b)
>> gives the wrong result on overflow but (int)((a) - (b)) < 0 is
>> correct.
> 
> You are right that it does give the correct result now, but the C
> standard never has defined overflow for signed integers, as noted in
> Section 6.3.1.3p3 of the N1494 Working Draft of the C standard:
> 
> 	Otherwise, the new type is signed and the value cannot be
> 	represented in it; either the result is implementation-defined
> 	or an implementation-defined signal is raised.
> 
> I have heard too many compiler guys gleefully discussing optimizations
> that they could use if they took full advantage of this clause, so I
> am not comfortable relying on the intuitive semantics for signed
> arithmetic.  (Now atomic_t is another story -- both C and C++ will
> be requiring twos-complement semantics, thankfully.)
>
>> I find the latter approach cleaner and that way the constant in the
>> instruction can be avoided too although if the compiler might generate
>> the same code regardless.
> 
> I would like your way better if it was defined in the C standard.
> But it unfortunately is not.  :-(

I see, then would something like the following work?

 (int)((unsigned)(a) - (unsigned)(b)) < 0

>> Also, I think the names are misleading.  They aren't testing whether
>> one is greater or less than the other.  They're testing whether one is
>> before or after the other where the counters are used as monotonically
>> incrementing (with wrapping) sequence, so wouldn't something like the
>> following be better?
> 
> They are comparing the twos-complement difference between the two
> numbers against zero.

But still GE/LT are way too misleading.  Anyways, so with the above
change the macro now would look like the following.

#define SEQ_TEST(a, b, op)	({					\
	typeof(a) __seq_a = (a);					\
	typeof(b) __seq_b = (b);					\
	bool __ret;							\
	(void)(&__seq_a == &__seq_b);					\
	switch (sizeof(__seq_a)) {					\
		case sizeof(s8):					\
			__ret = (s8)((u8)__seq_a - (u8)__seq_b) op 0;	\
			break;						\
		case sizeof(s16):					\
			__ret = (s16)((u16)__seq_a - (u16)__seq_b) op 0;\
			break;						\
		case sizeof(s32):					\
			__ret = (s32)((u32)__seq_a - (u32)__seq_b) op 0;\
			break;						\
		case sizeof(s64):					\
			__ret = (s64)((u64)__seq_a - (u64)__seq_b) op 0;\
			break;						\
		default:						\
			__make_build_fail;				\
	}								\
	__ret;								\
})

Would the above work?

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/