On Sun, Apr 29, 2007 at 09:02:13PM -0700, Roland McGrath wrote:
> I'm sorry I've taken so long to reply to your review comments.
> I won't dwell on that, and just dive into the discussion.
>
> > --- linux-2.6/include/asm-i386/thread_info.h.utrace-ptrace-compat
> > +++ linux-2.6/include/asm-i386/thread_info.h
> > @@ -135,13 +135,13 @@ static inline struct thread_info *curren
> > #define TIF_NEED_RESCHED 3 /* rescheduling necessary */
> > #define TIF_SINGLESTEP 4 /* restore singlestep on return
> > to user mode */
> > #define TIF_IRET 5 /* return with iret */
> > -#define TIF_SYSCALL_EMU 6 /* syscall emulation active */
> > #define TIF_SYSCALL_AUDIT 7 /* syscall auditing active */
> > #define TIF_SECCOMP 8 /* secure computing */
> > #define TIF_RESTORE_SIGMASK 9 /* restore signal mask in
> > do_signal() */
> > #define TIF_MEMDIE 16
> > #define TIF_DEBUG 17 /* uses debug registers */
> > #define TIF_IO_BITMAP 18 /* uses I/O bitmap */
> > +#define TIF_FORCED_TF 19 /* true if TF in eflags
> > artificially */
> >
> > I think it would make a lot of sense if you simply reused the
> > existing flag number instead of leaving a hole.
>
> You mean renumber 7,8,9 down to 6,7,8? I guess there's no reason not to
> do that, I was just not changing values I didn't need to. AFAIK the
> only thing that matters about these value is the bits < 16 that there
> and the bits >= 16 likewise, for _TIF_ALLWORK_MASK to work right.
No need to renumber. You remove TIF_SYSCALL_EMU which is six,
so the newly added TIF_FORCED_TF could reuse that bit.
> > +/***
> > + *** These are the exported entry points for tracing engines to use.
> > + ***/
> >
> > This is not a standard comment format. It should be:
>
> Ok. The intention was to clearly set off those declarations from others.
> It appears the thing other linux/*.h files do for this is:
>
> /**************************************************************************
> * These are the exported entry points for tracing engines to use.
> **************************************************************************/
Not really that much. Prefered would be just a normal comment which
would stick out if the actual comments were kerneldoc in the source files.
> > Please don't put such long comments in headerfiles. They should be
> > part of the kerneldoc comments near the actual function body so
> > we can extract them and autogenerate real documentation for it.
> > A big thanks for the huge amount of comments, btw - they're just
> > in the wrong place ;-)
>
> Ok. The kerneldoc stuff is new and strange to me, and I've never
> personally experienced its benefical features. But I've read
> Documentation/kernel-doc-nano-HOWTO.txt now and I will try to convert all
> my documentary comments to that style.
Thanks. If you need some help ping me or Randy.
>
> > Please try consolidate all the compat code in a single #ifdef block,
> > preferably at the end of the file.
>
> I did it this way to put each compat_foo near foo so it makes sense in
> context, and it's easy to remember to update it in parallel if you change
> foo. Is it really better to move them all to an undifferentiated cluster
> at the end?
That's the normal approach. If it's big enough it might even make
sense to give it a file of it's own to have the compat code totally
separate. But IIRC there wasn't a whole lot of compat code in utrace.
> > Please don't do this kind of conditional mess. It leads to really
> > ugly code like this (later in the patch):
> >
> > #ifdef ARCH_HAS_SINGLE_STEP
> > if (! ARCH_HAS_SINGLE_STEP)
> > #endif
> > WARN_ON(flags & UTRACE_ACTION_SINGLESTEP);
> > #ifdef ARCH_HAS_BLOCK_STEP
> > if (! ARCH_HAS_BLOCK_STEP)
> > #endif
> > WARN_ON(flags & UTRACE_ACTION_BLOCKSTEP);
> >
> > Instead make it a
> >
> > arch_has_single_step()
> >
> > which must be defined by every architecture as a boolean value, with
> > fixes like that the code above will become a lot more readable:
> >
> > WARN_ON(!arch_has_single_step() && (flags & UTRACE_ACTION_SINGLESTEP));
> > WARN_ON(!arch_has_block_step() && (flags & UTRACE_ACTION_BLOCKSTEP));
>
> I agree it's come out ugly. The reason I complicated it was an attempt
> to make the answer testable in #if when it's constant at compile time.
> My thinking is there will be situations where you want to avoid building
> in some useless dead code and just "if (0)" is not sufficient. But
> primarily I was thinking of avoiding building extra fancy code in lieu
> of single-step on machines/configurations where single-step is always
> available, and the current #if's don't help with that. How about if an
> asm-arch/tracehook.h either declares arch_has_single_step or else it
> #define's one of two things and in a single place I'll add:
>
> #ifdef ARCH_ALWAYS_HAS_SINGLE_STEP
> #define arch_has_single_step() 1
> #elif defined ARCH_NEVER_HAS_SINGLE_STEP
> #define arch_has_single_step() 0
> #endif
Current gcc (since 3.<something>) can optimize away code under if-branches
that are determinable at compile time, so having each architecture
define a (possible trivial) arch_has_single_step() should be fine.
Btw, is there a fundamental reason why an architecture would not support
single-stepping except for a transition period of porting, i.e. are there
real hardware limitation in any of our ports?
> > The coding style here is wrong . The else should be on the line
> > of the closing brace.
>
> I can ordinarily ignore syntax, but this is an abomination in the sight
> of the Lord and always will be. Fortunately, it's far from being 100%
> consistently used in the kernel already. People are welcome to change
> the code after I submit it, but I just can't make myself write it that
> way, sorry.
It's used pretty consistant in core code not written by you, which
looks similarly horrible. Please try to fit a consistant style.
>
> [wrt utrace_regset_copy*]
> > This function is far too large to inline it. Please move it out
> > of line.
>
> The reason these functions are inlines is that some of their arguments
> are always constants, so they get inlined into not a whole lot more than
> the inlined memcpy. Having these interfaces makes it a whole lot easier
> to write the machine regset code, which needs pretty simple code, but
> with constant arithmetic that's easy to louse up doing it by hand. As
> real functions they would just be more code that always had some runtime
> arithmetic and tests it didn't need.
Needs comments in the code explaining it.
> > if ((tsk->utrace_flags & UTRACE_EVENT_SIGNAL_ALL) &&
> > (tsk->utrace_flags & (UTRACE_ACTION_SINGLESTEP |
> > UTRACE_ACTION_BLOCKSTEP)))
> >
> > Also I'm not sure why you're doing this kind of test,
> > as you could do a
> >
> > if (current->utrace_flags & (UTRACE_EVENT_SIGNAL_ALL|
> > UTRACE_ACTION_SINGLESTEP|UTRACE_ACTION_BLOCKSTEP))
>
> No, that's not equivalent. UTRACE_EVENT_SIGNAL_ALL is not just one bit,
> but a mask of several bits.
> > +#define REPORT(callback, ...) do { \
> > + u32 ret = (*rcu_dereference(engine->ops)->callback) \
> > + (engine, tsk, ##__VA_ARGS__); \
> > + action = update_action(tsk, utrace, engine, ret); \
> > + } while (0)
> >
> > I don not think these kinds of macros are a very good idea.
> > Just opencode the two lines of code instead of a single
> > macro.
>
> I don't agree that duplicating intricate boilerplate code is a good
> idea. I will see if I can formulate macros more similar to those common
> in the kernel that don't increase error-prone manual duplication much.
It's just a few uses and expands to two lines of code, so avoiding
the macro probably is a good idea.
> > +const struct utrace_regset_view utrace_ppc32_view = {
> > + .name = "ppc", .e_machine = EM_PPC,
> > + .regsets = ppc32_regsets,
> > + .n = sizeof ppc32_regsets / sizeof ppc32_regsets[0],
> > +};
> > +EXPORT_SYMBOL_GPL(utrace_ppc32_view);
> > +#endif
> >
> > Who would be using this from modular code?
> > I see this is for all views, but I'd rather move the
> > accessor out of line than exporting all them if we
> > really have legitimate modular users.
>
> utrace_native_view is expected to be the only user of the *_view symbols.
> I inlined it because on single-arch platforms it's just a constant return
> and I didn't want to add another useless call frame. I admit this is undue
> microoptimization. Perhaps utrace_native_view should just be non-inlined
> and exported.
Yeah, the latter probably makes sense.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
