Re: [PATCH 1/5] locking: Introduce range reader/writer lock

[Davidlohr Bueso <dave@stgolabs.net>]

On Wed, 29 Mar 2017, Peter Zijlstra wrote:

>On Mon, Mar 06, 2017 at 09:03:26PM -0800, Davidlohr Bueso wrote:
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 88e01e08e279..e4d9eadd2c47 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -154,7 +154,6 @@ config DRM_RADEON
>>  	select HWMON
>>  	select BACKLIGHT_CLASS_DEVICE
>>  	select BACKLIGHT_LCD_SUPPORT
>> -	select INTERVAL_TREE
>>  	help
>>  	  Choose this option if you have an ATI Radeon graphics card.  There
>>  	  are both PCI and AGP versions.  You don't need to choose this to
>> @@ -174,7 +173,6 @@ config DRM_AMDGPU
>>  	select HWMON
>>  	select BACKLIGHT_CLASS_DEVICE
>>  	select BACKLIGHT_LCD_SUPPORT
>> -	select INTERVAL_TREE
>>  	help
>>  	  Choose this option if you have a recent AMD Radeon graphics card.
>>
>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>> index 183f5dc1c3f2..8a9154550f46 100644
>> --- a/drivers/gpu/drm/i915/Kconfig
>> +++ b/drivers/gpu/drm/i915/Kconfig
>> @@ -3,7 +3,6 @@ config DRM_I915
>>  	depends on DRM
>>  	depends on X86 && PCI
>>  	select INTEL_GTT
>> -	select INTERVAL_TREE
>>  	# we need shmfs for the swappable backing store, and in particular
>>  	# the shmem_readpage() which depends upon tmpfs
>>  	select SHMEM
>
>I presume this is part of making INTERVAL_TREE unconditional; should be
>a separate patch, no?

Ok, I can separate it.

>> +/*
>> + * The largest range will span [0,RANGE_RWLOCK_INFINITY].
>> + */
>> +#define RANGE_RWLOCK_INFINITY  (~0UL - 1)
>
>That's a strange limit, what's wrong with ~0UL ?

Nothing, I was under the impression I had updated that.

>> +
>> +struct range_rwlock {
>> +	struct interval_tree_node node;
>> +	struct task_struct *task;
>> +	/* Number of ranges which are blocking acquisition of the lock */
>> +	unsigned int blocking_ranges;
>> +	bool reader;
>> +};
>
>Hate the name; our rwlock is a spinlock, therefore this thing suggests
>it is too.

Hmmm not sure of a better name, kinda liked it. I'll see what I can come
up with.

>
>Also, no bool in structures.

Ok, but is that because sizeof(bool) can be undefined?  In any case, I'm thinking
we could skip the 'reader' member entirely and tag the task pointer (similar with
what we do for other locks).