Re: [DRIVER SUBMISSION] DRBD wants to go mainline

[Satyam Sharma]

On 7/23/07, Lars Ellenberg <lars.ellenberg@xxxxxxxxxx> wrote:
> On Sun, Jul 22, 2007 at 09:32:02PM -0400, Kyle Moffett wrote:
> [...]
> > Don't use signals between kernel threads, use proper primitives like
> > notifiers and waitqueues, which means you should also probably switch away
> > from kernel_thread() to the kthread_*() APIs.  Also you should fix this
> > FIXME or remove it if it no longer applies:-D.
>
> right.
> but how to I tell a network thread in tcp_recvmsg to stop early,
> without using signals?


Yup, kthreads API cannot handle (properly stop) kernel threads that want
to sleep on possibly-blocking-forever-till-signalled-functions such as
tcp_recvmsg or skb_recv_datagram etc etc.

There are two workarounds:
1. Use sk_rcvtimeo and related while-continue logic
2. force_sig(SIGKILL) to your kernel thread just before kthread_stop
  (note that you don't need to allow / write code to handle / etc signals
  in your kthread code -- force_sig will work automatically)

> > +/* THINK maybe we actually want to use the default "event/%s" worker threads
> > + * or similar in linux 2.6, which uses per cpu data and threads.
> > + *
> > + * To be general, this might need a spin_lock member.
> > + * For now, please use the mdev->req_lock to protect list_head,
> > + * see drbd_queue_work below.
> > + */
> > +struct drbd_work_queue {
> > +       struct list_head q;
> > +       struct semaphore s; /* producers up it, worker down()s it */
> > +       spinlock_t q_lock;  /* to protect the list. */
> > +};
> >
> > Umm, how about fixing this to actually use proper workqueues or something
> > instead of this open-coded mess?
>
> unlikely to happen "right now".
> but it is on our todo list...


It should be easier to do it now (if you defer it for later, the code will
only grow more and more complex). Also, removing this gunk from
your driver will clearly make it smaller, and easier for us to review :-)

> > +#define peer_mask role_mask
> > +#define pdsk_mask disk_mask
> > +#define susp_mask 1
> > +#define user_isp_mask 1
> > +#define aftr_isp_mask 1
> > +#define NS(T, S) \
> > +#define NS2(T1, S1, T2, S2) \
> > +#define NS3(T1, S1, T2, S2, T3, S3) \
> > +#define _NS(D, T, S) \
> > +#define _NS2(D, T1, S1, T2, S2) \
> > +#define _NS3(D, T1, S1, T2, S2, T3, S3) \
> >
> > Grumble.  When I earlier said I thought I was in macro hell, well, I was
> > wrong.  *THIS* is macro hell.  What the fsck is that supposed to do?  And it
> > doesn't even include a *SINGLE* comment!!!
>
> uhm. basically you are right.
> Phil will take over to explain why it is done that way...


The simplistic problems I initially thought your driver suffered from turned
out to be not so true, actually -- like Kyle mentioned, your driver is in
proper macro hell. Please get rid of as many as possible, and replace
with functions, as applicable.


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