Re: Thread flags modified without set_thread_flag() (non atomically)

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 01 Mar 2007 11:59:07 -0800
Mathieu Desnoyers <compudj@google.com> wrote:

> Andrew Morton wrote:
> > On Thu, 1 Mar 2007 10:34:51 +0100 Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> >
> >   
> >>> On Mon, 26 Feb 2007 12:10:37 -0800 Mathieu Desnoyers <compudj@google.com> wrote:
> >>>       
> >>>> avr32/kernel/ptrace.c:          ti->flags |= _TIF_BREAKPOINT;
> >>>>         
> >>> No, I don't immediately see anything in the flush_old_exec() code path
> >>> which tells us that nobody else can look up this thread_info (or be holding
> >>> a ref to it) in this context.
> >>>
> >>>
> >>>       
> >>>> avr32/kernel/ptrace.c:                  ti->flags |= TIF_SINGLE_STEP;
> >>>>         
> >>> heh.  Haarvard, you got a bug.
> >>>       
> >> Heh, yeah. That would indeed explain some strange gdb behaviour. It
> >> will only trigger when single-stepping into an exception or interrupt
> >> handler so thanks for pointing it out; I would have had a hard time
> >> figuring it out on my own...
> >>     
> >
> > yup, tricky.
> >
> > If there's a lesson here, it is "don't provide #defines in the header for
> > both versions".
> >
> >   
> 
> Hrm, but the bitmask version is useful (and correctly used) whenever
> the flag is read and tested.
> 
> The proper way to do this would be to change every use the _TIF_* flag 
> in a flag comparison
> for a call to test_ti_thread_flag(). I wonder if gcc optimizes multiple 
> constant test_bit() applying
> on the same variable linked by logical and/or so it becomes a single 
> read and a small set of comparisons.

Well, it's unusual for code to want to test multiple bitflags in the same
operation.  Perhaps thread_info.flags is unusual in that regard.

But one can still do

	if (foo->flags & (1<<bar)|(1<<zot))

Which can get to be a pain if it happens in a lot of places.  But if it
only happens in a few places, this seems a reasonable price to pay, given
the safety gains.

Plus I'm _forever_ having to go into the header file to remember whether
REQ_RW is the bitmask or the bit offset.