Re: [PATCH 2/2] Introduce __cond_lock_err

[Matthew Wilcox <willy@infradead.org>]

On Wed, Dec 27, 2017 at 03:28:54PM +0100, Luc Van Oostenryck wrote:
> On Fri, Dec 22, 2017 at 04:31:12AM -0800, Matthew Wilcox wrote:
> > On Thu, Dec 21, 2017 at 08:21:20PM -0800, Josh Triplett wrote:
> > 
> > While I've got you, I've been looking at some other sparse warnings from
> > this file.  There are several caused by sparse being unable to handle
> > the following construct:
> > 
> > 	if (foo)
> > 		x = NULL;
> > 	else {
> > 		x = bar;
> > 		__acquire(bar);
> > 	}
> > 	if (!x)
> > 		return -ENOMEM;
> > 
> > Writing it as:
> > 
> > 	if (foo)
> > 		return -ENOMEM;
> > 	else {
> > 		x = bar;
> > 		__acquire(bar);
> > 	}
> > 
> > works just fine.  ie this removes the warning:
> 
> It must be noted that these two versions are not equivalent
> (in the first version, it also returns with -ENOMEM if bar
> is NULL/zero).

They happen to be equivalent in the original; I was providing a simplified
version.  Here's the construct sparse can't understand:

        dst_pte = pte_alloc_map_lock(dst_mm, dst_pmd, addr, &dst_ptl);
        if (!dst_pte)
                return -ENOMEM;

with:

#define pte_alloc(mm, pmd, address)                     \
        (unlikely(pmd_none(*(pmd))) && __pte_alloc(mm, pmd, address))

#define pte_offset_map_lock(mm, pmd, address, ptlp)     \
({                                                      \
        spinlock_t *__ptl = pte_lockptr(mm, pmd);       \
        pte_t *__pte = pte_offset_map(pmd, address);    \
        *(ptlp) = __ptl;                                \
        spin_lock(__ptl);                               \
        __pte;                                          \
})

#define pte_alloc_map_lock(mm, pmd, address, ptlp)      \
        (pte_alloc(mm, pmd, address) ?                  \
                 NULL : pte_offset_map_lock(mm, pmd, address, ptlp))

If pte_alloc() succeeds, pte_offset_map_lock() will return non-NULL.
Manually inlining pte_alloc_map_lock() into the caller like so:

        if (pte_alloc(dst_mm, dst_pmd, addr)
		return -ENOMEM;
        dst_pte = pte_offset_map_lock(dst_mm, dst_pmd, addr, ptlp);

causes sparse to not warn.

> > Is there any chance sparse's dataflow analysis will be improved in the
> > near future?
> 
> A lot of functions in the kernel have this context imbalance,
> really a lot. For example, any function doing conditional locking
> is a problem here. Happily when these functions are inlined,
> sparse, thanks to its optimizations, can remove some paths and
> merge some others. 
> So yes, by adding some smartness to sparse, some of the false
> warnings will be removed, however:
> 1) some __must_hold()/__acquires()/__releases() annotations are
>    missing, making sparse's job impossible.

Partly there's a documentation problem here.  I'd really like to see a
document explaining how to add sparse annotations to a function which
intentionally does conditional locking.  For example, should we be
annotating the function as __acquires, and then marking the exits which
don't acquire the lock with __acquire(), or should we not annotate
the function, and annotate the exits which _do_ acquire the lock as
__release() with a comment like /* Caller will release */

> 2) a lot of the 'false warnings' are not so false because there is
>    indeed two possible paths with different lock state
> 3) it has its limits (at the end, giving the correct warning is
>    equivalent to the halting problem).
> 
> Now, to answer to your question, I'm not aware of any effort that would
> make a significant differences (it would need, IMO, code hoisting & 
> value range propagation).

That's fair.  I wonder if we were starting from scratch whether we'd
choose to make sparse a GCC plugin today.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>