diff mbox series

[6/7] bcache: optimize barrier usage for Rmw atomic bitops

Message ID 20200322060305.70637-7-colyli@suse.de (mailing list archive)
State New, archived
Headers show
Series bcache patches for Linux v5.7-rc1 | expand

Commit Message

Coly Li March 22, 2020, 6:03 a.m. UTC 
From: Davidlohr Bueso <dave@stgolabs.net>

We can avoid the unnecessary barrier on non LL/SC architectures,
such as x86. Instead, use the smp_mb__after_atomic().

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Signed-off-by: Coly Li <colyli@suse.de>
---
 drivers/md/bcache/writeback.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Hannes Reinecke March 23, 2020, 7:08 a.m. UTC | #1 
On 3/22/20 7:03 AM, Coly Li wrote:
> From: Davidlohr Bueso <dave@stgolabs.net>
> 
> We can avoid the unnecessary barrier on non LL/SC architectures,
> such as x86. Instead, use the smp_mb__after_atomic().
> 
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> Signed-off-by: Coly Li <colyli@suse.de>
> ---
>   drivers/md/bcache/writeback.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index 6673a37c8bd2..72ba6d015786 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -183,7 +183,7 @@ static void update_writeback_rate(struct work_struct *work)
>   	 */
>   	set_bit(BCACHE_DEV_RATE_DW_RUNNING, &dc->disk.flags);
>   	/* paired with where BCACHE_DEV_RATE_DW_RUNNING is tested */
> -	smp_mb();
> +	smp_mb__after_atomic();
>   
>   	/*
>   	 * CACHE_SET_IO_DISABLE might be set via sysfs interface,
> @@ -193,7 +193,7 @@ static void update_writeback_rate(struct work_struct *work)
>   	    test_bit(CACHE_SET_IO_DISABLE, &c->flags)) {
>   		clear_bit(BCACHE_DEV_RATE_DW_RUNNING, &dc->disk.flags);
>   		/* paired with where BCACHE_DEV_RATE_DW_RUNNING is tested */
> -		smp_mb();
> +		smp_mb__after_atomic();
>   		return;
>   	}
>   
> @@ -229,7 +229,7 @@ static void update_writeback_rate(struct work_struct *work)
>   	 */
>   	clear_bit(BCACHE_DEV_RATE_DW_RUNNING, &dc->disk.flags);
>   	/* paired with where BCACHE_DEV_RATE_DW_RUNNING is tested */
> -	smp_mb();
> +	smp_mb__after_atomic();
>   }
>   
>   static unsigned int writeback_delay(struct cached_dev *dc,
> 
Personally, I'd typically tend to use 'test_and_set_bit', as this not 
only implies a barrier, but also captures any errors; if you just use
'set_bit' you'll never know if the bit had been set already.

Cheers,

Hannes
Coly Li March 23, 2020, 8:45 a.m. UTC | #2 
On 2020/3/23 3:08 下午, Hannes Reinecke wrote:
> On 3/22/20 7:03 AM, Coly Li wrote:
>> From: Davidlohr Bueso <dave@stgolabs.net>
>>
>> We can avoid the unnecessary barrier on non LL/SC architectures,
>> such as x86. Instead, use the smp_mb__after_atomic().
>>
>> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
>> Signed-off-by: Coly Li <colyli@suse.de>
>> ---
>>   drivers/md/bcache/writeback.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/md/bcache/writeback.c
>> b/drivers/md/bcache/writeback.c
>> index 6673a37c8bd2..72ba6d015786 100644
>> --- a/drivers/md/bcache/writeback.c
>> +++ b/drivers/md/bcache/writeback.c
>> @@ -183,7 +183,7 @@ static void update_writeback_rate(struct
>> work_struct *work)
>>        */
>>       set_bit(BCACHE_DEV_RATE_DW_RUNNING, &dc->disk.flags);
>>       /* paired with where BCACHE_DEV_RATE_DW_RUNNING is tested */
>> -    smp_mb();
>> +    smp_mb__after_atomic();
>>         /*
>>        * CACHE_SET_IO_DISABLE might be set via sysfs interface,
>> @@ -193,7 +193,7 @@ static void update_writeback_rate(struct
>> work_struct *work)
>>           test_bit(CACHE_SET_IO_DISABLE, &c->flags)) {
>>           clear_bit(BCACHE_DEV_RATE_DW_RUNNING, &dc->disk.flags);
>>           /* paired with where BCACHE_DEV_RATE_DW_RUNNING is tested */
>> -        smp_mb();
>> +        smp_mb__after_atomic();
>>           return;
>>       }
>>   @@ -229,7 +229,7 @@ static void update_writeback_rate(struct
>> work_struct *work)
>>        */
>>       clear_bit(BCACHE_DEV_RATE_DW_RUNNING, &dc->disk.flags);
>>       /* paired with where BCACHE_DEV_RATE_DW_RUNNING is tested */
>> -    smp_mb();
>> +    smp_mb__after_atomic();
>>   }
>>     static unsigned int writeback_delay(struct cached_dev *dc,
>>
> Personally, I'd typically tend to use 'test_and_set_bit', as this not
> only implies a barrier, but also captures any errors; if you just use
> 'set_bit' you'll never know if the bit had been set already.

Copied. There are other locations may optimize with test_and_set_bit(),
for example the cannibalize_lock stuffs. I will post all the
test_and_set_bit()/test_and_clear_bit() optimization as another series
later.

Thanks.
diff mbox series

Patch

diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 6673a37c8bd2..72ba6d015786 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -183,7 +183,7 @@  static void update_writeback_rate(struct work_struct *work)
 	 */
 	set_bit(BCACHE_DEV_RATE_DW_RUNNING, &dc->disk.flags);
 	/* paired with where BCACHE_DEV_RATE_DW_RUNNING is tested */
-	smp_mb();
+	smp_mb__after_atomic();
 
 	/*
 	 * CACHE_SET_IO_DISABLE might be set via sysfs interface,
@@ -193,7 +193,7 @@  static void update_writeback_rate(struct work_struct *work)
 	    test_bit(CACHE_SET_IO_DISABLE, &c->flags)) {
 		clear_bit(BCACHE_DEV_RATE_DW_RUNNING, &dc->disk.flags);
 		/* paired with where BCACHE_DEV_RATE_DW_RUNNING is tested */
-		smp_mb();
+		smp_mb__after_atomic();
 		return;
 	}
 
@@ -229,7 +229,7 @@  static void update_writeback_rate(struct work_struct *work)
 	 */
 	clear_bit(BCACHE_DEV_RATE_DW_RUNNING, &dc->disk.flags);
 	/* paired with where BCACHE_DEV_RATE_DW_RUNNING is tested */
-	smp_mb();
+	smp_mb__after_atomic();
 }
 
 static unsigned int writeback_delay(struct cached_dev *dc,