linux-kernel - Re: [PATCH v2] btrfs: zstd: use spin

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]

Message-ID: <09c2a9ce-3b04-ed94-1d62-0e5a072b9dac@suse.com>
Date:   Wed, 13 Apr 2022 17:58:41 +0300
From:   Nikolay Borisov <nborisov@...e.com>
To:     Schspa Shi <schspa@...il.com>, dsterba@...e.cz
Cc:     clm@...com, dsterba@...e.com, josef@...icpanda.com,
        linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        terrelln@...com
Subject: Re: [PATCH v2] btrfs: zstd: use spin_lock in timer callback



On 11.04.22 г. 18:55 ч., Schspa Shi wrote:
> This is an optimization for fix fee13fe96529 ("btrfs:
> correct zstd workspace manager lock to use spin_lock_bh()")
> 
> The critical region for wsm.lock is only accessed by the process context and
> the softirq context.
> 
> Because in the soft interrupt, the critical section will not be preempted by the
> soft interrupt again, there is no need to call spin_lock_bh(&wsm.lock) to turn
> off the soft interrupt, spin_lock(&wsm.lock) is enough for this situation.
> 
> Changelog:
> v1 -> v2:
> 	- Change the commit message to make it more readable.
> 
> [1] https://lore.kernel.org/all/20220408181523.92322-1-schspa@gmail.com/
> 
> Signed-off-by: Schspa Shi <schspa@...il.com>

Has there been any measurable impact by this change? While it's correct it does mean that
  someone looking at the code would see that in one call site we use plain spinlock and in
another a _bh version and this is somewhat inconsistent.

What's more I believe this is a noop since when softirqs are executing preemptible() would
be false due to preempt_count() being non-0 and in the bh-disabling code
in the spinlock we have:

  /* First entry of a task into a BH disabled section? */
     1         if (!current->softirq_disable_cnt) {
   167                 if (preemptible()) {
     1                         local_lock(&softirq_ctrl.lock);
     2                         /* Required to meet the RCU bottomhalf requirements. */
     3                         rcu_read_lock();
     4                 } else {
     5                         DEBUG_LOCKS_WARN_ON(this_cpu_read(softirq_ctrl.cnt));
     6                 }
     7         }


In this case we'd hit the else branch.
> ---
>   fs/btrfs/zstd.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> index fc42dd0badd7..faa74306f0b7 100644
> --- a/fs/btrfs/zstd.c
> +++ b/fs/btrfs/zstd.c
> @@ -105,10 +105,10 @@ static void zstd_reclaim_timer_fn(struct timer_list *timer)
>   	unsigned long reclaim_threshold = jiffies - ZSTD_BTRFS_RECLAIM_JIFFIES;
>   	struct list_head *pos, *next;
>   
> -	spin_lock_bh(&wsm.lock);
> +	spin_lock(&wsm.lock);
>   
>   	if (list_empty(&wsm.lru_list)) {
> -		spin_unlock_bh(&wsm.lock);
> +		spin_unlock(&wsm.lock);
>   		return;
>   	}
>   
> @@ -137,7 +137,7 @@ static void zstd_reclaim_timer_fn(struct timer_list *timer)
>   	if (!list_empty(&wsm.lru_list))
>   		mod_timer(&wsm.timer, jiffies + ZSTD_BTRFS_RECLAIM_JIFFIES);
>   
> -	spin_unlock_bh(&wsm.lock);
> +	spin_unlock(&wsm.lock);
>   }
>   
>   /*