Re: [PATCH V8 08/13] posix clocks: cleanup the CLOCK_DISPTACHmacro

[Thomas Gleixner]

On Wed, 12 Jan 2011, Richard Cochran wrote:

> On Tue, Jan 11, 2011 at 01:57:23PM +0100, Thomas Gleixner wrote:
> > I wonder whether we should be a bit more clever here and create an
> > extra entry for posix_cpu_timers in the posix_clocks array and do the
> > following:
> ...
> > That reduces the code significantly and the MAX_CLOCKS check in
> > clock_get_array_id() replaces the invalid_clock() check in the
> > syscalls as well. It does not matter whether we check this before or
> > after copying stuff from user.
> 
> Well, this does reduce the number of LOC, but the number of
> comparisons is the same. I think the code size would be also no
> different.

Right, It's about lines of code and ever repeated if / else constructs
in the dispatch functions. The number of comparisons is probably the
same, but I'm sure that the binary size will be smaller.

We probably can remove the dispatch inlines that way completely and do
the call directly from the syscall function.
 
> > Adding your new stuff requires just another entry in the array, the
> > setup of the function pointers and the CLOCKFD check in
> > clock_get_array_id(). Everything else just falls in place.
> 
> For me, I am not sure if either version is really very pretty or easy
> to understand.

Well, if we document clock_get_array_id() proper, then it's easier to
follow than 10 dispatch functions which have all the same checks and
comparisons inside.
 
> My instinct is to keep the posix_cpu_X and pc_X functions out of the
> array of static clock id functions, if only to make the distinction
> between the legacy static ids and new dynamic ids clear.
> 
> The conclusion from the "dynamic clock as file" discussion was that we
> don't want to add any more static clock ids, and new clocks should use
> the new, dynamic clock API. So, we should discourage any future
> attempt to add to that function array!

These IDs are not public, they are helpers to make the code simpler,
nothing else. I agree, that we don't want to extend the static ids for
public consumption, but implementation wise we can do that confined to
posix-timers.c.

> Having said that, if you insist on it, I won't mind reworking the
> dispatch code as you suggested.

I'm not insisting. I just saw the repeated if/else constructs and
added the clockfd check mentally :) That's where I started to wonder
about a way to do the same thing with way less lines of code.

> > >  clock_nanosleep_restart(struct restart_block *restart_block)
> > >  {
> > > -	clockid_t which_clock = restart_block->arg0;
> > > -
> > 
> > How does that compile ?
> 
> Because the CLOCK_DISPATCH macro, above, makes no use of the first

Missed that, sorry.

> argument! I have removed the macro for the next round.

Cool.

Thanks,

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