Re: [PATCH] linux-2.5.43_vsyscall_A0

Jeff Dike (jdike@karaya.com)
Fri, 18 Oct 2002 23:49:59 -0500


ak@muc.de said:
> Guess you'll have some problems then with UML on x86-64, which always
> uses vgettimeofday. But it's only used for gettimeofday() currently,
> perhaps it's not that bad when the UML child runs with the host's
> time.

It's not horrible, but it's still broken. There are people who depend
on UML being able to keep its own time separately from the host.

> I guess it would be possible to add some support for UML to map own
> code over the vsyscall reserved locations. UML would need to use the
> syscalls then. But it'll be likely ugly.

Yeah, it would be.

My preferred solution would be for libc to ask the kernel where the vsyscall
area is. That's reasonably clean and virtualizable. Andrea doesn't like it
because it adds a few instructions to the vsyscall address calculation.

Jeff

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


i>> + id--;
> + spin_lock_irq(&id_lock);

Shouldn't this be irqsave ? Otherwise there could be hard to track
down bugs later with this code turning on interrupts for other code
by accident.

Overall I'm not sure all this radix tree stuff is worth the overhead
and code complexity. Is just a bitmap with find_first_bit() and
a last hit cache too slow ? How many timers do you expect to maintain ?

It looks like a similar problem to what kernel/pid.c does. If you're
really determined to have an complicated implementation I would
recommend seeing if you could share code with that.

> +struct k_clock {
> + struct timer_pq pq;
> + int res; /* in nano seconds */

Hopefully it never overflows.

> + default:
> + printk(KERN_WARNING "sending signal failed: %d\n", ret);

printk should be removed or at least rate limited, because it could
make the system unusable

> +/*
> + * For some reason mips/mips64 define the SIGEV constants plus 128.
> + * Here we define a mask to get rid of the common bits. The
> + * optimizer should make this costless to all but mips.
> + */
> +#if (ARCH == mips) || (ARCH == mips64)
> +#define MIPS_SIGEV ~(SIGEV_NONE & \
> + SIGEV_SIGNAL & \
> + SIGEV_THREAD & \
> + SIGEV_THREAD_ID)
> +#else
> +#define MIPS_SIGEV (int)-1
> +#endif

This definitely needs to be cleaned up.

> +{
> + struct task_struct * rtn = current;
> +
> + if (event->sigev_notify & SIGEV_THREAD_ID & MIPS_SIGEV ) {
> + if ( !(rtn =
> + find_task_by_pid(event->sigev_notify_thread_id)) ||

Are you holding any locks or how do you make sure rtn doesn't go away ?

> +
> +
> +static struct k_itimer * alloc_posix_timer(void)
> +{
> + struct k_itimer *tmr;
> + tmr = kmem_cache_alloc(posix_timers_cache, GFP_KERNEL);
> + memset(tmr, 0, sizeof(struct k_itimer));

Needs a NULL check.

Overall your code looks like overkill to me. I think it would
be better to start with a simple implementation and only add
all the complicated data structures if it should really be a
bottle neck in some real world load. It also needs some cleanup
to remove dead code etc.

-Andi
-
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/