Duncan Sands wrote: > Hi Pavel, > >>>> @@ -1014,11 +1015,7 @@ static int usbatm_do_heavy_init(void *arg) >>>> struct usbatm_data *instance = arg; >>>> int ret; >>>> >>>> - daemonize(instance->driver->driver_name); >>>> allow_signal(SIGTERM); >>>> - instance->thread_pid = current->pid; >>>> - >>>> - complete(&instance->thread_started); >>> One reason the completion existed to make sure that the thread was not >>> sent SIGTERM before the above call to allow_signal(SIGTERM). So I think >>> you have opened up a (tiny) race by deleting it. >> Nope. See my answer below :) >> >>>> static int usbatm_heavy_init(struct usbatm_data *instance) >>>> { >>>> - int ret = kernel_thread(usbatm_do_heavy_init, instance, CLONE_FS | >>>> CLONE_FILES); >>>> - >>>> - if (ret < 0) { >>>> - usb_err(instance, "%s: failed to create kernel_thread (%d)!\n", >>>> __func__, ret); >>> Please don't delete this message. >>> >>>> - return ret; >>>> - } >>>> + struct task_struct *t; >>>> >>>> - wait_for_completion(&instance->thread_started); >>>> + t = kthread_create(usbatm_do_heavy_init, instance, >>>> + instance->driver->driver_name); >>>> + if (IS_ERR(t)) >>>> + return PTR_ERR(t); >>>> >>>> + instance->thread = t; >>>> + wake_up_process(t); >>> Does the kthread API guarantee that the kthread is not running until you >>> call >> It does. That's why the race, you mentioned above is impossible. > > I don't see why it helps. The race I mentioned occurs when the kthread > creating thread > runs too fast compared to the kthread. Let C (creator) be the thread running > usbatm_heavy_init, and K (kthread) be the created kthread. When C calls > wake_up_process, > thread K starts running, however on an SMP system C may also be running. Now > suppose > that for some reason K takes a long time to execute the command > "allow_signal(SIGTERM);", > but that C runs very fast and immediately executes the disconnect callback, > and sends the > signal to K before K manages to execute allow_signal. This is the race, and > it can only > be fixed by making C run slower (thus the completion). Of course this is > fantastically > unlikely which is why I described it as tiny, but as far as I can see it is a > theoretical > possibility. I don't see that wake_up_process fixes it, it just makes it > even less likely.
Oh, I see. You're right - this race is possible... I'll fix that up if this patch works. >>> By the way, the right thing to do is (I think) to replace the thread with >>> a workqueue and have users of usbatm register a "shut_down" callback >>> rather than using signals: the disconnect method would call shut_down >>> rathering than trying to kill the thread. That way all this mucking >>> around with pids etc wouldn't be needed. All users of usbatm would need >>> to be modified. I managed to convince myself once that they could all be >>> fixed up in a fairly simple manner thanks to a few tricks and a >>> completion or two, but I don't recall the details... >> Well, that would be also great, since kill_proc will be gone - that's what >> I'm trying to achieve. > > I think your patch should go in, since I'm not likely to ever implement the > scheme I suggested - I don't use this hardware anymore and have lost interest > in the driver. :) > Best wishes, > > Duncan. > > -- 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/