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

["Denis Vlasenko" <vda.linux@googlemail.com>]

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.

> * 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

> My testbox boots/works fine with all these patches (uptime half an hour)

For this kind of things, you really need something more stressing.
Try to find big SMP people who is willing to give it a whirl.

> and the compressed bzImage is smaller by about ~2 KB for my .config --

At least it proves that _something_ changed.
--
vda