This is an attempt to kill two birds with one stone.
First, we kill one more user of kernel_thread. Second - we kill
one of the last users of kill_proc - the function which is also
to be removed, because it uses a pid_t which is not safe now.
The problem is that I couldn't find the maintainer for the code
in drivers/usb/atm/. Besides, I don't have a proper hardware to
test this.
So, who should I send this patch for review/testing?
Signed-off-by: Pavel Emelyanov <[email protected]>
---
diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
index e717f5b..a99469a 100644
--- a/drivers/usb/atm/usbatm.c
+++ b/drivers/usb/atm/usbatm.c
@@ -80,6 +80,7 @@
#include <linux/stat.h>
#include <linux/timer.h>
#include <linux/wait.h>
+#include <linux/kthread.h>
#ifdef VERBOSE_DEBUG
static int usbatm_print_packet(const unsigned char *data, int len);
@@ -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);
ret = instance->driver->heavy_init(instance, instance->usb_intf);
@@ -1026,7 +1023,7 @@ static int usbatm_do_heavy_init(void *arg)
ret = usbatm_atm_init(instance);
mutex_lock(&instance->serialize);
- instance->thread_pid = -1;
+ instance->thread = NULL;
mutex_unlock(&instance->serialize);
complete_and_exit(&instance->thread_exited, ret);
@@ -1034,15 +1031,15 @@ static int usbatm_do_heavy_init(void *arg)
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);
- 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);
return 0;
}
@@ -1124,8 +1121,7 @@ int usbatm_usb_probe(struct usb_interface *intf, const struct usb_device_id *id,
kref_init(&instance->refcount); /* dropped in usbatm_usb_disconnect */
mutex_init(&instance->serialize);
- instance->thread_pid = -1;
- init_completion(&instance->thread_started);
+ instance->thread = NULL;
init_completion(&instance->thread_exited);
INIT_LIST_HEAD(&instance->vcc_list);
@@ -1287,8 +1283,8 @@ void usbatm_usb_disconnect(struct usb_interface *intf)
mutex_lock(&instance->serialize);
instance->disconnected = 1;
- if (instance->thread_pid >= 0)
- kill_proc(instance->thread_pid, SIGTERM, 1);
+ if (instance->thread != NULL)
+ send_sig(SIGTERM, instance->thread, 1);
mutex_unlock(&instance->serialize);
wait_for_completion(&instance->thread_exited);
diff --git a/drivers/usb/atm/usbatm.h b/drivers/usb/atm/usbatm.h
index ff8551e..62a46fc 100644
--- a/drivers/usb/atm/usbatm.h
+++ b/drivers/usb/atm/usbatm.h
@@ -176,8 +176,7 @@ struct usbatm_data {
int disconnected;
/* heavy init */
- int thread_pid;
- struct completion thread_started;
+ struct task_struct *thread;
struct completion thread_exited;
/* ATM device */
Hi,
> The problem is that I couldn't find the maintainer for the code
> in drivers/usb/atm/.
that would be me (though since I haven't used this modem in years I would
be more than happy to hand it off to someone else).
> Besides, I don't have a proper hardware to test this.
I will try to find where I put my old modem and test your patch this weekend.
> @@ -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.
> 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
wake_up_process? Because if not then what is to stop the kthread finishing before
this thread does "instance->thread = t", resulting in an attempt to send a signal
to a dead process later on in disconnect?
Otherwise it looks fine - thanks!
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...
Best wishes,
Duncan.
Duncan Sands wrote:
> Hi,
>
>> The problem is that I couldn't find the maintainer for the code
>> in drivers/usb/atm/.
>
> that would be me (though since I haven't used this modem in years I would
> be more than happy to hand it off to someone else).
>
>> Besides, I don't have a proper hardware to test this.
>
> I will try to find where I put my old modem and test your patch this weekend.
Oh, that would be great :)
>> @@ -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.
> wake_up_process? Because if not then what is to stop the kthread finishing before
> this thread does "instance->thread = t", resulting in an attempt to send a signal
> to a dead process later on in disconnect?
>
> Otherwise it looks fine - thanks!
>
> 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.
> Best wishes,
>
> Duncan.
>
Thanks,
Pavel
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.
> > 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.
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.
>
>
On Thu, 7 Feb 2008, Pavel Emelyanov wrote:
> > 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.
That is wrong. The API guarantees only that the kthread is initially
created in a non-running state. It cannot guarantee that no other
task will wake up the kthread before the caller does.
Alan Stern
Alan Stern wrote:
> On Thu, 7 Feb 2008, Pavel Emelyanov wrote:
>
>>> 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.
>
> That is wrong. The API guarantees only that the kthread is initially
> created in a non-running state. It cannot guarantee that no other
> task will wake up the kthread before the caller does.
Other tasks cannot wake this thread up since they do not
know the struct task_struct pointer.
> Alan Stern
>
>
Hi Pavel,
> Oh, I see. You're right - this race is possible... I'll fix that up
> if this patch works.
it seems to work fine. Thanks again for doing this!
Best wishes,
Duncan.
Duncan Sands wrote:
> Hi Pavel,
>
>> Oh, I see. You're right - this race is possible... I'll fix that up
>> if this patch works.
>
> it seems to work fine. Thanks again for doing this!
Oh, thanks for testing. What should I do next to get this into
mainline? Send it to Andrew with you in Cc, or you have you own
tree for merging?
> Best wishes,
>
> Duncan.
>
Thanks,
Pavel
On Monday 11 February 2008 12:22:27 Pavel Emelyanov wrote:
> Duncan Sands wrote:
> > Hi Pavel,
> >
> >> Oh, I see. You're right - this race is possible... I'll fix that up
> >> if this patch works.
> >
> > it seems to work fine. Thanks again for doing this!
>
> Oh, thanks for testing. What should I do next to get this into
> mainline? Send it to Andrew with you in Cc, or you have you own
> tree for merging?
Please fix the race (see previous emails) and send it to Andew with Cc.
Thanks,
Duncan.