[PATCH][USBATM]: convert heavy init dances to kthread API

[PATCH][USBATM]: convert heavy init dances to kthread API

[Pavel Emelyanov]

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 <xemul@openvz.org>

---

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

Re: [PATCH][USBATM]: convert heavy init dances to kthread API

[Duncan Sands]

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.

Re: [PATCH][USBATM]: convert heavy init dances to kthread API

[Pavel Emelyanov]

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

Re: [PATCH][USBATM]: convert heavy init dances to kthread API

[Duncan Sands]

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.

Re: [PATCH][USBATM]: convert heavy init dances to kthread API

[Pavel Emelyanov]

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

Re: [PATCH][USBATM]: convert heavy init dances to kthread API

[Alan Stern]

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

Re: [PATCH][USBATM]: convert heavy init dances to kthread API

[Pavel Emelyanov]

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

Re: [PATCH][USBATM]: convert heavy init dances to kthread API

[Duncan Sands]

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.

Re: [PATCH][USBATM]: convert heavy init dances to kthread API

[Pavel Emelyanov]

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

Re: [PATCH][USBATM]: convert heavy init dances to kthread API

[Duncan Sands]

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.

Re: [PATCH] Usbatm: convert heavy init dances to kthread API

[Duncan Sands]

> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> Tested-by: Duncan Sands <baldrick.bulk@free.fr>

Signed-off-by: Duncan Sands <baldrick@free.fr>

Thanks again for doing this!

Best wishes,

Duncan.

[PATCH] Usbatm: convert heavy init dances to kthread API

[Pavel Emelyanov]

This is an attempt to kill two birds with one stone.

First, we kill one more user of kernel_thread, which is scheduled
for removal. 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.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
Tested-by: Duncan Sands <baldrick.bulk@free.fr>

---

diff --git a/drivers/usb/atm/usbatm.c b/drivers/usb/atm/usbatm.c
index e717f5b..0722872 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,10 +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 +1024,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,13 +1032,18 @@ 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;
+
+	t = kthread_create(usbatm_do_heavy_init, instance,
+			instance->driver->driver_name);
+	if (IS_ERR(t)) {
+		usb_err(instance, "%s: failed to create kernel_thread (%ld)!\n",
+				__func__, PTR_ERR(t));
+		return PTR_ERR(t);
 	}
 
+	instance->thread = t;
+	wake_up_process(t);
 	wait_for_completion(&instance->thread_started);
 
 	return 0;
@@ -1124,7 +1127,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;
+	instance->thread = NULL;
 	init_completion(&instance->thread_started);
 	init_completion(&instance->thread_exited);
 
@@ -1287,8 +1290,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..d23aca9 100644
--- a/drivers/usb/atm/usbatm.h
+++ b/drivers/usb/atm/usbatm.h
@@ -176,7 +176,7 @@ struct usbatm_data {
 	int disconnected;
 
 	/* heavy init */
-	int thread_pid;
+	struct task_struct *thread;
 	struct completion thread_started;
 	struct completion thread_exited;