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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
