Re: [PATCH v2 17/24] locking/lockdep: Free lock classes that are no longer in use

[Bart Van Assche]

On Tue, 2018-12-04 at 15:27 -0500, Waiman Long wrote:
+AD4 On 12/03/2018 07:28 PM, Bart Van Assche wrote:
+AD4 +AD4 +-/+ACo Must be called with the graph lock held. +ACo-/
+AD4 +AD4 +-static void remove+AF8-class+AF8-from+AF8-lock+AF8-chain(struct lock+AF8-chain +ACo-chain,
+AD4 +AD4 +-					 struct lock+AF8-class +ACo-class)
+AD4 +AD4 +-+AHs
+AD4 +AD4 +-	u64 chain+AF8-key+ADs
+AD4 +AD4 +-	int i+ADs
+AD4 +AD4 +-
+AD4 +AD4 +-+ACM-ifdef CONFIG+AF8-PROVE+AF8-LOCKING
+AD4 +AD4 +-	for (i +AD0 chain-+AD4-base+ADs i +ADw chain-+AD4-base +- chain-+AD4-depth+ADs i+-+-) +AHs
+AD4 +AD4 +-		if (chain+AF8-hlocks+AFs-i+AF0 +ACEAPQ class - lock+AF8-classes)
+AD4 +AD4 +-			continue+ADs
+AD4 +AD4 +-		if (--chain-+AD4-depth +AD0APQ 0)
+AD4 +AD4 +-			break+ADs
+AD4 +AD4 +-		memmove(+ACY-chain+AF8-hlocks+AFs-i+AF0, +ACY-chain+AF8-hlocks+AFs-i +- 1+AF0,
+AD4 +AD4 +-			(chain-+AD4-base +- chain-+AD4-depth - i) +ACo
+AD4 +AD4 +-			sizeof(chain+AF8-hlocks+AFs-0+AF0))+ADs
+AD4 +AD4 +-		/+ACo
+AD4 +AD4 +-		 +ACo Each lock class occurs at most once in a
+AD4 +AD4 +-		 +ACo lock chain so once we found a match we can
+AD4 +AD4 +-		 +ACo break out of this loop.
+AD4 +AD4 +-		 +ACo-/
+AD4 +AD4 +-		break+ADs
+AD4 +AD4 +-	+AH0
+AD4 +AD4 +-	/+ACo
+AD4 +AD4 +-	 +ACo Note: calling hlist+AF8-del+AF8-rcu() from inside a
+AD4 +AD4 +-	 +ACo hlist+AF8-for+AF8-each+AF8-entry+AF8-rcu() loop is safe.
+AD4 +AD4 +-	 +ACo-/
+AD4 +AD4 +-	if (chain-+AD4-depth +AD0APQ 0) +AHs
+AD4 +AD4 +-		/+ACo To do: decrease chain count. See also inc+AF8-chains(). +ACo-/
+AD4 +AD4 +-		hlist+AF8-del+AF8-rcu(+ACY-chain-+AD4-entry)+ADs
+AD4 +AD4 +-		return+ADs
+AD4 +AD4 +-	+AH0
+AD4 +AD4 +-	chain+AF8-key +AD0 0+ADs
+AD4 +AD4 +-	for (i +AD0 chain-+AD4-base+ADs i +ADw chain-+AD4-base +- chain-+AD4-depth+ADs i+-+-)
+AD4 +AD4 +-		chain+AF8-key +AD0 iterate+AF8-chain+AF8-key(chain+AF8-key, chain+AF8-hlocks+AFs-i+AF0 +- 1)+ADs
+AD4 
+AD4 Do you need to recompute the chain+AF8-key if no entry in the chain is removed?

Thanks for having pointed that out. I will modify this function such that the
chain key is only recalculated if necessary.

+AD4 +AD4  
+AD4 +AD4 +AEAAQA -4141,14 +-4253,31 +AEAAQA static void zap+AF8-class(struct lock+AF8-class +ACo-class)
+AD4 +AD4  	for (i +AD0 0, entry +AD0 list+AF8-entries+ADs i +ADw nr+AF8-list+AF8-entries+ADs i+-+-, entry+-+-) +AHs
+AD4 +AD4  		if (entry-+AD4-class +ACEAPQ class +ACYAJg entry-+AD4-links+AF8-to +ACEAPQ class)
+AD4 +AD4  			continue+ADs
+AD4 +AD4 +-		links+AF8-to +AD0 entry-+AD4-links+AF8-to+ADs
+AD4 +AD4 +-		WARN+AF8-ON+AF8-ONCE(entry-+AD4-class +AD0APQ links+AF8-to)+ADs
+AD4 +AD4  		list+AF8-del+AF8-rcu(+ACY-entry-+AD4-entry)+ADs
+AD4 +AD4 +-		check+AF8-free+AF8-class(class)+ADs
+AD4 
+AD4 Is the check+AF8-free+AF8-class() call redundant? You are going to call it near
+AD4 the end below.

I think so. I will remove the check+AF8-free+AF8-class() that is inside the for-loop.

+AD4 +AD4 +-static void reinit+AF8-class(struct lock+AF8-class +ACo-class)
+AD4 +AD4 +-+AHs
+AD4 +AD4 +-	void +ACo-const p +AD0 class+ADs
+AD4 +AD4 +-	const unsigned int offset +AD0 offsetof(struct lock+AF8-class, key)+ADs
+AD4 +AD4 +-
+AD4 +AD4 +-	WARN+AF8-ON+AF8-ONCE(+ACE-class-+AD4-lock+AF8-entry.next)+ADs
+AD4 +AD4 +-	WARN+AF8-ON+AF8-ONCE(+ACE-list+AF8-empty(+ACY-class-+AD4-locks+AF8-after))+ADs
+AD4 +AD4 +-	WARN+AF8-ON+AF8-ONCE(+ACE-list+AF8-empty(+ACY-class-+AD4-locks+AF8-before))+ADs
+AD4 +AD4 +-	memset(p +- offset, 0, sizeof(+ACo-class) - offset)+ADs
+AD4 +AD4 +-	WARN+AF8-ON+AF8-ONCE(+ACE-class-+AD4-lock+AF8-entry.next)+ADs
+AD4 +AD4 +-	WARN+AF8-ON+AF8-ONCE(+ACE-list+AF8-empty(+ACY-class-+AD4-locks+AF8-after))+ADs
+AD4 +AD4 +-	WARN+AF8-ON+AF8-ONCE(+ACE-list+AF8-empty(+ACY-class-+AD4-locks+AF8-before))+ADs
+AD4 +AD4  +AH0
+AD4 
+AD4 Is it safer to just reinit those fields before +ACI-key+ACI instead of using
+AD4 memset()? Lockdep is slow anyway, doing that individually won't
+AD4 introduce any noticeable slowdown.

The warning statements will only be hit if the order of the struct lock+AF8-class members
would be modified. I don't think that we need to change the approach of this function.

+AD4 +AD4 +AEAAQA -4193,18 +-4355,14 +AEAAQA void lockdep+AF8-free+AF8-key+AF8-range(void +ACo-start, unsigned long size)
+AD4 +AD4  	raw+AF8-local+AF8-irq+AF8-restore(flags)+ADs
+AD4 +AD4  
+AD4 +AD4  	/+ACo
+AD4 +AD4 -	 +ACo Wait for any possible iterators from look+AF8-up+AF8-lock+AF8-class() to pass
+AD4 +AD4 -	 +ACo before continuing to free the memory they refer to.
+AD4 +AD4 -	 +ACo
+AD4 +AD4 -	 +ACo sync+AF8-sched() is sufficient because the read-side is IRQ disable.
+AD4 +AD4 +-	 +ACo Do not wait for concurrent look+AF8-up+AF8-lock+AF8-class() calls. If any such
+AD4 +AD4 +-	 +ACo concurrent call would return a pointer to one of the lock classes
+AD4 +AD4 +-	 +ACo freed by this function then that means that there is a race in the
+AD4 +AD4 +-	 +ACo code that calls look+AF8-up+AF8-lock+AF8-class(), namely concurrently accessing
+AD4 +AD4 +-	 +ACo and freeing a synchronization object.
+AD4 +AD4  	 +ACo-/
+AD4 +AD4 -	synchronize+AF8-sched()+ADs
+AD4 +AD4  
+AD4 +AD4 -	/+ACo
+AD4 +AD4 -	 +ACo XXX at this point we could return the resources to the pool+ADs
+AD4 +AD4 -	 +ACo instead we leak them. We would need to change to bitmap allocators
+AD4 +AD4 -	 +ACo instead of the linear allocators we have now.
+AD4 +AD4 -	 +ACo-/
+AD4 +AD4 +-	schedule+AF8-free+AF8-zapped+AF8-classes()+ADs
+AD4 
+AD4 Should you move the graph+AF8-unlock() and raw+AF8-lock+AF8-irq+AF8-restore() down to
+AD4 after this? The schedule+AF8-free+AF8-zapped+AF8-classes must be called with
+AD4 graph+AF8-lock held. Right?

I will modify this and other patches such that all schedule+AF8-free+AF8-zapped+AF8-classes()
calls are protected by the graph lock.

Bart.