linux-kernel - Re: [RFC] seqlock,lockdep: Add lock primitives to read

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1301579984.4859.284.camel@twins>
Date:	Thu, 31 Mar 2011 15:59:44 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:	rostedt@...dmis.org, mingo@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC] seqlock,lockdep: Add lock primitives to read_seqbegin().

On Wed, 2011-03-30 at 21:17 +0900, Tetsuo Handa wrote:
> Peter Zijlstra wrote:
> > >  Also, I assume you meant to call
> > > spin_acquire() before entering the spin state (as with
> > > 
> > >   static inline void __raw_spin_lock(raw_spinlock_t *lock)
> > >   {
> > >         preempt_disable();
> > >         spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
> > >         LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> > >   }
> > > 
> > > . Otherwise, lockdep cannot report it when hit this bug upon the first call to
> > > this function). 
> > 
> > Huh no, of course not, a seqlock read side cannot contend in the classic
> > sense.
> 
> I couldn't understand what 'contend' means. I think
> 
>   static __always_inline unsigned read_seqbegin(const seqlock_t *sl)
>   {
>   	unsigned ret;
>   repeat:
>   	ret = sl->sequence;
>   	smp_rmb();
>   	if (unlikely(ret & 1)) {
>   		cpu_relax();
>   		goto repeat;
>   	}
>   	return ret;
>   }
> 
> is equivalent (except that above one will not write to any kernel memory) to
> 
>   static __always_inline unsigned read_seqbegin(seqlock_t *sl)
>   {
>   	unsigned ret;
>   	unsigned long flags;
>   	spin_lock_irqsave(&sl->lock, flags);
>   	ret = sl->sequence;
>   	spin_unlock_irqrestore(&sl->lock, flags);
>   	return ret;
>   }
> 
> because read_seqbegin() cannot return to the reader until the writer (if there
> is one) calls write_sequnlock().

It more or less it, but conceptually the seqlock read-side is a
non-blocking algorithm and thus doesn't block or contend. The above
initial wait is merely an optimization to avoid having to retry, which
could be more expensive than simply waiting there.

Anyway, all the lockdep contention crap is purely about lockstat and
doesn't matter for dependency tracking.

> Don't we call this situation (a reader thread temporarily behaves like a writer
> thread who writes nothing) as 'contended'?
> 
> Anyway, could you show me read_seqbegin2()/read_seqretry2() for testing with
> locktest module?

Like I wrote before:

> @@ -94,6 +94,7 @@ static __always_inline unsigned read_seqbegin(const seqlock_t *sl)
>                 cpu_relax();
>                 goto repeat;
>         }
> +       rwlock_acquire_read(&sl->lock->dep_map, 0, 0, _RET_IP_);
>  
>         return ret;
>  }
> @@ -107,6 +108,8 @@ static __always_inline int read_seqretry(const seqlock_t *sl, unsigned start)
>  {
>         smp_rmb();
>  
> +       rwlock_release(&sl->lock->dep_map, 1, _RET_IP_);
> +
>         return unlikely(sl->sequence != start);
>  }

Should do, except that lockdep doesn't properly work for read-recursive
locks, which needs to get fixed.
--
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/