Re: SoC: linuxolator update: first patch

On Tuesday 15 August 2006 10:55, Divacky Roman wrote:
> > - You tsleep() in linux_schedtail() while holding one lock and do the
> >   wakeup() in linux_proc_init() while holding no locks.  You've got lost
> >   wakeup races that way that you work around by having to use a goto and
> >   a sleep with a timeout.
> 
> 7) I dont understand this. can you explain more? thnx

Suppose you have two threads like so:

	thread A		thread B
	FOO_LOCK();		...
	if (foo)		...
		FOO_UNLOCK()	FOO_LOCK()
		...		foo = 1
		...		wakeup(&foo);
		tsleep(&foo);	...
		...		FOO_UNLOCK()

thread A isn't going to get the wakeup from B.  The solution is an interlocked 
sleep where you release the lock and put the thread to sleep as an 'atomic 
operation'.  If you are familiar with pthreads, this is why pthread condition 
variables take a mutex argument to their wait functions (and why in the 
kernel cv_wait* and msleep() both take a mutex arg), they use internal 
locking to atomically drop the lock and block to avoid that race.  
interlocked sleeps aren't likely to be implemented for sx locks, but I will 
need to add them for rwlocks before too long.  For now, if you stick with 
mutexes you can use either condition variables or msleep() to avoid this.
	
> > - You should include the appropriate header to get the declaration of 'hz'
> >   rather than just adding an 'extern' directly in your code.
> 
> 8) I didnt find such header. 

[11:41:39] (ttyp7) jhb_at_mutex:~/work/cvs/sys                                     
> grep hz *
...
kernel.h:extern int tick;                       /* usec per tick (1000000 / 
hz) */
kernel.h:extern int hz;                         /* system clock's frequency */
kernel.h:extern int stathz;                     /* statistics clock's 
frequency */
kernel.h:extern int profhz;                     /* profiling clock's frequency 
*/
...

#include <sys/kernel.h>

> > - In futex_get() you don't handle the race of two concurrent creates.
> 
> 9) hopefully fixed.

See my replies to your p4 submits..

> > - Should come up with a better name than 'schedtail' for the eventhandler
> >   in fork_exit().  Maybe 'process_fork_exit'?
> 
> 14) linux uses this name. I might change it but I think its ok to be in sync
> with linux.

Yes, but this is FreeBSD, and all the other process related eventhandlers are 
called process_<foo>. :)  A generic eventhandler might be used by more than 
just the Linux ABI.

> > - You should probably ask Peter to review the link_elf_obj change.  An
> >   alternative is to use linker_lookup_set() in your module load routine to
> >   lookup your linker set details.
> 
> 15) yes... when I start working on amd64 I will definitely do something 
about it

What I did for my debug kernel modules is something like this:

struct crash_event {
        const char *ev_name;
        void (*ev_handler)(void);
};

#define CRASH_EVENT(name, function)                                     \
        static struct crash_event function ## _crash_event =            \
            { name, function };                                         \
        DATA_SET(crash_event_set, function ## _crash_event)

#define MAX_EVENT       event_max

static int event_max;
static struct crash_event **event_start, **event_stop;

Then in my module event handler:

        switch (cmd) {
        case MOD_LOAD:
                error = linker_file_lookup_set(module_file(module),
                    "crash_event_set", &event_start, &event_stop, &event_max);
                if (error)
                        break;

Here's a sample foreach loop:

        for (ev = event_start; ev < event_stop; ev++) {
                /* Skip null event 0. */
                if ((*ev)->ev_name == NULL)
                        continue;
                printf("%4td  %s\n", ev - event_start, (*ev)->ev_name);
        }

and here's a sample index operation:

                if (ev < 0 || ev >= MAX_EVENT) {
                        printf("crash: event %d is not defined!\n", event);
                        continue;
                }
                evp = event_start[ev];
                printf("crash: %s\n", evp->ev_name);
                evp->ev_handler();

(If you want to see this specific module more, you can view it 
at //depot/projects/smpng/sys/modules/crash/crash.c.  There's a 
crash2/crash2.c that is similar as well.)

> > - Regarding the locking, I'm not sure why you have the two separate sx 
locks,
> >   and why you don't just merge them into a single lock instead.
> 
> 16) it makes the locking easier and they cover different things. I like it 
this way :)

I think it might be prone to races though.  If you are almost always acquiring 
the same two locks in lockstep, then all you are doing is adding overhead.

> thnx for exhausting review! I commited some fixes to p4, you might want to 
check them

I am. :)

-- 
John Baldwin