Re: [PATCH 0/8] i386: bitops: Cleanup, sanitize, optimize

[Satyam Sharma <ssatyam@cse.iitk.ac.in>]

Hi Denis,


On Mon, 30 Jul 2007, Denis Vlasenko wrote:

> Hi Satyam,
> 
> On Monday 23 July 2007 17:05, Satyam Sharma wrote:
> > There was a lot of bogus stuff that include/asm-i386/bitops.h was doing,
> > that was unnecessary and not required for the correctness of those APIs.
> > All that superfluous stuff was also unnecessarily disallowing compiler
> > optimization possibilities, and making gcc generate code that wasn't as
> > beautiful as it could otherwise have been. Hence the following series
> > of cleanups (some trivial, some surprising, in no particular order):
> 
> [I did read entire thread]
> 
> Welcome to the minefield.
> 
> This bitops and barrier stuff is complicated. It's very easy to
> introduce bugs which are hard to trigger, or happen only with some
> specific gcc versions, or only on massively parallel SMP boxes.
> 
> You can also make technically correct changes which relax needlessly
> strict barrier semantics of some bitops and trigger latent bugs
> in code which was unknowingly depending on it.
> 
> How you can proceed:
> 
> Make a change which you believe is right. Recompile allyesconfig
> kernel with and without this change. Find a piece of assembly code
> which become different. Check that new code is correct (and smaller
> and/or faster). Post your patch together with example(s) of code
> fragments that got better. Be as verbose as needed.
> 
> Repeat for each change separately.
> 
> This can be painfully slow, but less likely to be rejected outright
> in fear of introducing difficult bugs.

Thanks for the tips, I'd remember these in the future :-)


> > * Marking "memory" as clobbered for no good reason
> 
> I vaguely remember that "memory" clobbers are needed in some rather
> obscure, non-obvious situations. Google for it - Linus wrote about it
> to lkml (a few years ago IIRC).
> 
> > * Volatile-casting of memory addresses
> >   (wholly unnecessary, makes gcc generate bad code)
> 
> Do you know any code difference resulting from this patch?
> 
> > * Unwarranted use of __asm__ __volatile__ even when those semantics
> >   are not required
> 
> ditto
> 
> > * Unnecessarily harsh definitions of smp_mb__{before, after}_clear_bit()
> >   (again, this was like *asking* gcc to generate bad code)
> 
> ditto

The problem with most of the patches in this series (other than the two
related to vague gcc docs), as you know, turned out to be the fact that
a lot of code/callers out there rely on the bitops as also being
lock/unlock functions -- and hence the need for disallowing both
compiler as well as CPU reordering / optimizations.

There were few valid changes in the patchset too, and probably I'll try
to look at them the way you described above (or probably it also makes
some sense to keep the bitops as-is, "it works" today after all, and
there are other avenues for optimizations/cleanups too).


Thanks again,
Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/