|
|
| Next: [PATCH 1/3] FRV: Improve FRV's use of generic IRQ.. |
| Author |
Message |
Cedric Le Goater External

Since: Jun 01, 2006 Posts: 129
|
Posted: Fri Sep 08, 2006 6:50 pm Post subject: [patch -mm] update mq_notify to use a struct pid Archived from groups: linux>kernel (more info?) |
|
|
message queues can signal a process waiting for a message.
this patch replaces the pid_t value with a struct pid to avoid pid wrap
around problems.
Signed-off-by: Cedric Le Goater <clg RemoveThis @fr.ibm.com>
Cc: Eric Biederman <ebiederm RemoveThis @xmission.com>
Cc: Andrew Morton <akpm RemoveThis @osdl.org>
Cc: containers RemoveThis @lists.osdl.org
---
ipc/mqueue.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
Index: 2.6.18-rc6-mm1/ipc/mqueue.c
===================================================================
--- 2.6.18-rc6-mm1.orig/ipc/mqueue.c
+++ 2.6.18-rc6-mm1/ipc/mqueue.c
@@ -73,7 +73,7 @@ struct mqueue_inode_info {
struct mq_attr attr;
struct sigevent notify;
- pid_t notify_owner;
+ struct pid* notify_owner;
struct user_struct *user; /* user who created, for accounting */
struct sock *notify_sock;
struct sk_buff *notify_cookie;
@@ -134,7 +134,7 @@ static struct inode *mqueue_get_inode(st
INIT_LIST_HEAD(&info->e_wait_q[0].list);
INIT_LIST_HEAD(&info->e_wait_q[1].list);
info->messages = NULL;
- info->notify_owner = 0;
+ info->notify_owner = NULL;
info->qsize = 0;
info->user = NULL; /* set when all is ok */
memset(&info->attr, 0, sizeof(info->attr));
@@ -338,7 +338,7 @@ static ssize_t mqueue_read_file(struct f
(info->notify_owner &&
info->notify.sigev_notify == SIGEV_SIGNAL) ?
info->notify.sigev_signo : 0,
- info->notify_owner);
+ pid_nr(info->notify_owner));
spin_unlock(&info->lock);
buffer[sizeof(buffer)-1] = '\0';
slen = strlen(buffer)+1;
@@ -363,7 +363,7 @@ static int mqueue_flush_file(struct file
struct mqueue_inode_info *info = MQUEUE_I(filp->f_dentry->d_inode);
spin_lock(&info->lock);
- if (current->tgid == info->notify_owner)
+ if (task_tgid(current) == info->notify_owner)
remove_notification(info);
spin_unlock(&info->lock);
@@ -518,8 +518,8 @@ static void __do_notify(struct mqueue_in
sig_i.si_pid = current->tgid;
sig_i.si_uid = current->uid;
- kill_proc_info(info->notify.sigev_signo,
- &sig_i, info->notify_owner);
+ kill_pid_info(info->notify.sigev_signo,
+ &sig_i, info->notify_owner);
break;
case SIGEV_THREAD:
set_cookie(info->notify_cookie, NOTIFY_WOKENUP);
@@ -528,7 +528,8 @@ static void __do_notify(struct mqueue_in
break;
}
/* after notification unregisters process */
- info->notify_owner = 0;
+ put_pid(info->notify_owner);
+ info->notify_owner = NULL;
}
wake_up(&info->wait_q);
}
@@ -566,12 +567,13 @@ static long prepare_timeout(const struct
static void remove_notification(struct mqueue_inode_info *info)
{
- if (info->notify_owner != 0 &&
+ if (info->notify_owner != NULL &&
info->notify.sigev_notify == SIGEV_THREAD) {
set_cookie(info->notify_cookie, NOTIFY_REMOVED);
netlink_sendskb(info->notify_sock, info->notify_cookie, 0);
}
- info->notify_owner = 0;
+ put_pid(info->notify_owner);
+ info->notify_owner = NULL;
}
static int mq_attr_ok(struct mq_attr *attr)
@@ -1062,11 +1064,11 @@ retry:
ret = 0;
spin_lock(&info->lock);
if (u_notification == NULL) {
- if (info->notify_owner == current->tgid) {
+ if (info->notify_owner == task_tgid(current)) {
remove_notification(info);
inode->i_atime = inode->i_ctime = CURRENT_TIME;
}
- } else if (info->notify_owner != 0) {
+ } else if (info->notify_owner != NULL) {
ret = -EBUSY;
} else {
switch (notification.sigev_notify) {
@@ -1086,7 +1088,8 @@ retry:
info->notify.sigev_notify = SIGEV_SIGNAL;
break;
}
- info->notify_owner = current->tgid;
+
+ info->notify_owner = get_pid(task_tgid(current));
inode->i_atime = inode->i_ctime = CURRENT_TIME;
}
spin_unlock(&info->lock);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Eric W. Biederman External

Since: May 19, 2006 Posts: 1134
|
Posted: Sat Sep 09, 2006 4:50 am Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Cedric Le Goater <clg.DeleteThis@fr.ibm.com> writes:
> message queues can signal a process waiting for a message.
>
> this patch replaces the pid_t value with a struct pid to avoid pid wrap
> around problems.
>
> Signed-off-by: Cedric Le Goater <clg.DeleteThis@fr.ibm.com>
> Cc: Eric Biederman <ebiederm.DeleteThis@xmission.com>
> Cc: Andrew Morton <akpm.DeleteThis@osdl.org>
> Cc: containers.DeleteThis@lists.osdl.org
Signed-off-by: Eric Biederman <ebiederm.DeleteThis@xmission.com>
I was just about to send out this patch in a couple more hours.
So expect the fact we wrote the same code is a good sign
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Cedric Le Goater External

Since: Jun 01, 2006 Posts: 129
|
Posted: Mon Sep 11, 2006 12:20 pm Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Eric W. Biederman wrote:
> Cedric Le Goater <clg.TakeThisOut@fr.ibm.com> writes:
>
>> message queues can signal a process waiting for a message.
>>
>> this patch replaces the pid_t value with a struct pid to avoid pid wrap
>> around problems.
>>
>> Signed-off-by: Cedric Le Goater <clg.TakeThisOut@fr.ibm.com>
>> Cc: Eric Biederman <ebiederm.TakeThisOut@xmission.com>
>> Cc: Andrew Morton <akpm.TakeThisOut@osdl.org>
>> Cc: containers.TakeThisOut@lists.osdl.org
>
> Signed-off-by: Eric Biederman <ebiederm.TakeThisOut@xmission.com>
>
> I was just about to send out this patch in a couple more hours.
Well, you did the same with the usb/devio.c and friends
> So expect the fact we wrote the same code is a good sign
How does oleg feel about it ? I've seen some long thread on possible race
conditions with put_pid() and solutions with rcu. I didn't quite get all of
it ... it will need another run for me.
On the "pid_t to struct pid*" topic:
* I started smbfs and realized it was useless.
* in the following, the init process is being killed directly using 1. I'm
not sure how useful it would be to use a struct pid. To begin with, may be
they could use a :
kill_init(int signum, int priv)
../arch/mips/sgi-ip32/ip32-reset.c
../arch/powerpc/platforms/iseries/mf.c
../drivers/parisc/power.c
../drivers/char/snsc_event.c
../kernel/sys.c
../kernel/sysctl.c
../drivers/char/nwbutton.c
../drivers/s390/s390mach.c
* some more drivers,
* some more kthread to convert
C.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Eric W. Biederman External

Since: May 19, 2006 Posts: 1134
|
Posted: Mon Sep 11, 2006 1:20 pm Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Cedric Le Goater <clg.TakeThisOut@fr.ibm.com> writes:
> Eric W. Biederman wrote:
>> Cedric Le Goater <clg.TakeThisOut@fr.ibm.com> writes:
>>
>>> message queues can signal a process waiting for a message.
>>>
>>> this patch replaces the pid_t value with a struct pid to avoid pid wrap
>>> around problems.
>>>
>>> Signed-off-by: Cedric Le Goater <clg.TakeThisOut@fr.ibm.com>
>>> Cc: Eric Biederman <ebiederm.TakeThisOut@xmission.com>
>>> Cc: Andrew Morton <akpm.TakeThisOut@osdl.org>
>>> Cc: containers.TakeThisOut@lists.osdl.org
>>
>> Signed-off-by: Eric Biederman <ebiederm.TakeThisOut@xmission.com>
>>
>> I was just about to send out this patch in a couple more hours.
>
> Well, you did the same with the usb/devio.c and friends
Good. The you should be familiar enough with it to review my patch
and make certain I didn't do anything stupid
>> So expect the fact we wrote the same code is a good sign
>
> How does oleg feel about it ? I've seen some long thread on possible race
> conditions with put_pid() and solutions with rcu. I didn't quite get all of
> it ... it will need another run for me.
Short. Oleg felt it was a shame that locking was needed to use a
struct pid.
While parsing that I realized my second vt patch that deals with
vt_pid (the pid for console switching) has a subtle race, and
that patch needs to be reworked.
We confused each other.
> On the "pid_t to struct pid*" topic:
>
> * I started smbfs and realized it was useless.
Killing the user space part is useless?
I thought that is what I saw happening.
Of course I don't frequently mount smbfs.
> * in the following, the init process is being killed directly using 1. I'm
> not sure how useful it would be to use a struct pid. To begin with, may be
> they could use a :
>
> kill_init(int signum, int priv)
An interesting notion. The other half of them use cad_pid.
Converting that is going to need some sysctl work, so I have been
ignoring it temporarily.
Filling in a struct pid through sysctl is extremely ugly at the
moment, plus cad_pid needs some locking.
> ./arch/mips/sgi-ip32/ip32-reset.c
> ./arch/powerpc/platforms/iseries/mf.c
> ./drivers/parisc/power.c
> ./drivers/char/snsc_event.c
> ./kernel/sys.c
> ./kernel/sysctl.c
> ./drivers/char/nwbutton.c
> ./drivers/s390/s390mach.c
>
> * some more drivers,
> * some more kthread to convert
Ok. Time to exchange some status information, before I roll over and
go back to sleep.
My patch todo list (almost a series file) currently looks like:
> n_r396r
> fs3270-Change-to-use-struct-pid.txt
> smbfs-Make-conn_pid-a-struct-pid.txt
> ncpfs-Use-struct-pid-to-track-the-userspace-watchdog-process.txt
>
> Don-t-use-kill_pg-in-the-sunos-compatibility-code.txt
>
> usbatm-use-kthread-api (I think I have this one)
I did usbatm mostly to figure out why kthread conversions seem
to be so hard, and got lucky this one wasn't too ugly.
> The-dvb_core-needs-to-use-the-kthread-api-not-kernel-threads.txt
> nfs-Note-we-need-to-start-using-the-kthreads-api.txt
dvb-core I have only started looking at.
nfs I noticed it is the svc stuff that matters.
usbatm, dvb-core, and nfs are the 3 kernel_thread users
that also use kill_proc, and thus are high on my immediate hit list.
> pid-Replace-session_of_pgrp-with-pgrp_in_current_session.txt
> pid-Use-struct-pid-for-talking-about-process-groups-in-exit.c.txt
> pid-Replace-is_orphaned_pgrp-with-is_current_pgrp_orphaned.txt
>
> tty-Update-the-tty-layer-to-work-with-struct-pid.txt
I need to ensure I don't have a race with task->signal->tty_old_pgrp.
tty_old_pgrp is a weird notion that I haven't fully wrapped my head
around yet.
> pid-Remove-use-of-old-do_each_task_pid-while_each_task_pid.txt
>
> Rewrite-kill_something_info-so-it-uses-newer-helpers.txt
>
> pid-Remove-now-unused-do_each_task_pid-and-while_each_task_pid.txt
> Remove-the-now-unused-kill_pg-kill_pg_info-and-__kill_pg_info.txt
>
>
> pid-Better-tests-for-same-thread-group-membership.txt
> pid-Cleanup-the-pid-equality-tests.txt
> pid-Track-the-sending-pid-of-a-queued-signal.txt
> proc-Use-pid_nr-in-array.c-so-the-code-is-foobar-safe.txt
>
> sysctl-Implement-get_data-put_data.txt
>
> cad-pid (killing init)
Once the above list is processed none of the old none of the signal
functions that take a pid_t is needed anymore.
i.e. kill_proc, kill_pg, and do_each_task_pid will be removable.
I have at least a first draft of everything on my list except for the
kthread conversions which I just started messing with yesterday. But
don't worry about beating me to something if you feel you have it
complete. That just means I will have enough of a clue to review your
code
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Cedric Le Goater External

Since: Jun 01, 2006 Posts: 129
|
Posted: Mon Sep 11, 2006 4:10 pm Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Eric W. Biederman wrote:
>>> I was just about to send out this patch in a couple more hours.
>> Well, you did the same with the usb/devio.c and friends
>
> Good. The you should be familiar enough with it to review my patch
> and make certain I didn't do anything stupid
well, the least i can try ...
>>> So expect the fact we wrote the same code is a good sign
>> How does oleg feel about it ? I've seen some long thread on possible race
>> conditions with put_pid() and solutions with rcu. I didn't quite get all of
>> it ... it will need another run for me.
>
> Short. Oleg felt it was a shame that locking was needed to use a
> struct pid.
>
> While parsing that I realized my second vt patch that deals with
> vt_pid (the pid for console switching) has a subtle race, and
> that patch needs to be reworked.
>
> We confused each other.
>
>> On the "pid_t to struct pid*" topic:
>>
>> * I started smbfs and realized it was useless.
>
> Killing the user space part is useless?
> I thought that is what I saw happening.
smb_fill_super() says :
if (warn_count < 5) {
warn_count++;
printk(KERN_EMERG "smbfs is deprecated and will be removed in"
" December, 2006. Please migrate to cifs\n");
}
So, i guess we should forget about it and spend our time on the cifs
kthread instead.
> Of course I don't frequently mount smbfs.
>
>> * in the following, the init process is being killed directly using 1. I'm
>> not sure how useful it would be to use a struct pid. To begin with, may be
>> they could use a :
>>
>> kill_init(int signum, int priv)
>
> An interesting notion. The other half of them use cad_pid.
yes.
> Converting that is going to need some sysctl work, so I have been
> ignoring it temporarily.
>
> Filling in a struct pid through sysctl is extremely ugly at the
> moment, plus cad_pid needs some locking.
Which distros use /proc/sys/kernel/cad_pid and why ? I can image the need
but i didn't find much on the topic.
> My patch todo list (almost a series file) currently looks like:
>> n_r396r
>> fs3270-Change-to-use-struct-pid.txt
done that. will send to martin for review.
>> smbfs-Make-conn_pid-a-struct-pid.txt
deprecated in december. so we could just forget about it.
>> ncpfs-Use-struct-pid-to-track-the-userspace-watchdog-process.txt
>>
>> Don-t-use-kill_pg-in-the-sunos-compatibility-code.txt
>>
>> usbatm-use-kthread-api (I think I have this one)
> I did usbatm mostly to figure out why kthread conversions seem
> to be so hard, and got lucky this one wasn't too ugly.
argh. i've done also and i just send my second version of the patch to the
maintainer Duncan Sands.
This one might just be useless also because greg kh has a patch in -mm to
enable multithread probing of USB devices.
>> The-dvb_core-needs-to-use-the-kthread-api-not-kernel-threads.txt
>> nfs-Note-we-need-to-start-using-the-kthreads-api.txt
>
> dvb-core I have only started looking at.
suka and i have sent patches to fix :
drivers/media/video/tvaudio.c
drivers/media/video/saa7134/saa7134-tvaudio.c
we are no waiting for the maintainer feedback.
> nfs I noticed it is the svc stuff that matters.
>
> usbatm, dvb-core, and nfs are the 3 kernel_thread users
> that also use kill_proc, and thus are high on my immediate hit list.
nfs is also full of signal_pending() ...
>> pid-Replace-session_of_pgrp-with-pgrp_in_current_session.txt
>> pid-Use-struct-pid-for-talking-about-process-groups-in-exit.c.txt
>> pid-Replace-is_orphaned_pgrp-with-is_current_pgrp_orphaned.txt
>>
>> tty-Update-the-tty-layer-to-work-with-struct-pid.txt
> I need to ensure I don't have a race with task->signal->tty_old_pgrp.
> tty_old_pgrp is a weird notion that I haven't fully wrapped my head
> around yet.
>
>> pid-Remove-use-of-old-do_each_task_pid-while_each_task_pid.txt
>>
>> Rewrite-kill_something_info-so-it-uses-newer-helpers.txt
>>
>> pid-Remove-now-unused-do_each_task_pid-and-while_each_task_pid.txt
>> Remove-the-now-unused-kill_pg-kill_pg_info-and-__kill_pg_info.txt
>>
>>
>> pid-Better-tests-for-same-thread-group-membership.txt
>> pid-Cleanup-the-pid-equality-tests.txt
>> pid-Track-the-sending-pid-of-a-queued-signal.txt
is that about updating the siginfos in collect_signal() to hold the right
pid value depending on the pid namespace they are being received ?
>> proc-Use-pid_nr-in-array.c-so-the-code-is-foobar-safe.txt
>>
>> sysctl-Implement-get_data-put_data.txt
>>
>> cad-pid (killing init)
>
> Once the above list is processed none of the old none of the signal
> functions that take a pid_t is needed anymore.
> i.e. kill_proc, kill_pg, and do_each_task_pid will be removable.
>
> I have at least a first draft of everything on my list except for the
> kthread conversions which I just started messing with yesterday. But
> don't worry about beating me to something if you feel you have it
> complete. That just means I will have enough of a clue to review your
> code
good list ! I look at it in details.
thanks,
C.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Oleg Nesterov External

Since: May 14, 2006 Posts: 614
|
Posted: Mon Sep 11, 2006 5:50 pm Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
On 09/11, Cedric Le Goater wrote:
>
> Eric W. Biederman wrote:
> > Cedric Le Goater <clg.TakeThisOut@fr.ibm.com> writes:
> >
> >> message queues can signal a process waiting for a message.
> >>
> >> this patch replaces the pid_t value with a struct pid to avoid pid wrap
> >> around problems.
> >>
> >> Signed-off-by: Cedric Le Goater <clg.TakeThisOut@fr.ibm.com>
> >> Cc: Eric Biederman <ebiederm.TakeThisOut@xmission.com>
> >> Cc: Andrew Morton <akpm.TakeThisOut@osdl.org>
> >> Cc: containers.TakeThisOut@lists.osdl.org
> >
> > Signed-off-by: Eric Biederman <ebiederm.TakeThisOut@xmission.com>
> >
> > I was just about to send out this patch in a couple more hours.
>
> Well, you did the same with the usb/devio.c and friends
>
> > So expect the fact we wrote the same code is a good sign
>
> How does oleg feel about it ? I've seen some long thread on possible race
> conditions with put_pid() and solutions with rcu. I didn't quite get all of
> it ... it will need another run for me.
I assume you are talking about this patch:
http://marc.theaimsgroup.com/?l=linux-mm-commits&m=115773820415171
I think it's ok, info->notify_owner is always used under info->lock.
This is simple. If, for example, mqueue_read_file() didn't take info->lock,
then we have a problem: pid_nr() may read a freed memory in case when
__do_notify()->put_pid() happens at the same time.
In this context info->notify_owner is a usual refcounted object, no special
attention is needed.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Eric W. Biederman External

Since: May 19, 2006 Posts: 1134
|
Posted: Mon Sep 11, 2006 9:10 pm Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Cedric you mentioned a couple of other patches that are in flight.
In the future could you please Cc: the containers list so independent
efforts are less likely to duplicate work, and we are more likely
to review each others patches instead?
Cedric Le Goater <clg DeleteThis @fr.ibm.com> writes:
> Eric W. Biederman wrote:
>
>>>> I was just about to send out this patch in a couple more hours.
>>> Well, you did the same with the usb/devio.c and friends
>>
>> Good. The you should be familiar enough with it to review my patch
>> and make certain I didn't do anything stupid
>
> well, the least i can try ...
>
>>> * I started smbfs and realized it was useless.
>>
>> Killing the user space part is useless?
>> I thought that is what I saw happening.
>
> smb_fill_super() says :
>
> if (warn_count < 5) {
> warn_count++;
> printk(KERN_EMERG "smbfs is deprecated and will be removed in"
> " December, 2006. Please migrate to cifs\n");
> }
>
> So, i guess we should forget about it and spend our time on the cifs
> kthread instead.
Sure. Although in this instance the changes are simple enough I will probably
send the patch anyway That at least explains why you figured it was
useless work.
>> Of course I don't frequently mount smbfs.
>>
>>> * in the following, the init process is being killed directly using 1. I'm
>>> not sure how useful it would be to use a struct pid. To begin with, may be
>>> they could use a :
>>>
>>> kill_init(int signum, int priv)
>>
>> An interesting notion. The other half of them use cad_pid.
>
> yes.
>
>> Converting that is going to need some sysctl work, so I have been
>> ignoring it temporarily.
>>
>> Filling in a struct pid through sysctl is extremely ugly at the
>> moment, plus cad_pid needs some locking.
>
> Which distros use /proc/sys/kernel/cad_pid and why ? I can image the need
> but i didn't find much on the topic.
I'm not at all certain, and I'm not even certain I care. The concept
is there in the code so it needs to be dealt with. Although if I we
extend the cad_pid concept it may make a difference.
>> My patch todo list (almost a series file) currently looks like:
>>> n_r396r
>>> fs3270-Change-to-use-struct-pid.txt
>
> done that. will send to martin for review.
Added to my queue of pending patches to look at review.
>>> ncpfs-Use-struct-pid-to-track-the-userspace-watchdog-process.txt
>>>
>>> Don-t-use-kill_pg-in-the-sunos-compatibility-code.txt
>>>
>>> usbatm-use-kthread-api (I think I have this one)
>> I did usbatm mostly to figure out why kthread conversions seem
>> to be so hard, and got lucky this one wasn't too ugly.
>
> argh. i've done also and i just send my second version of the patch to the
> maintainer Duncan Sands.
>
> This one might just be useless also because greg kh has a patch in -mm to
> enable multithread probing of USB devices.
Added to my queue of pending patches to track down and reivew.
>>> The-dvb_core-needs-to-use-the-kthread-api-not-kernel-threads.txt
>>> nfs-Note-we-need-to-start-using-the-kthreads-api.txt
>>
>> dvb-core I have only started looking at.
>
> suka and i have sent patches to fix :
>
> drivers/media/video/tvaudio.c
> drivers/media/video/saa7134/saa7134-tvaudio.c
>
> we are no waiting for the maintainer feedback.
Ok. I think I saw a little of that.
>> nfs I noticed it is the svc stuff that matters.
>>
>> usbatm, dvb-core, and nfs are the 3 kernel_thread users
>> that also use kill_proc, and thus are high on my immediate hit list.
>
> nfs is also full of signal_pending() ...
Yes, there is a lot to read and understand before I can confidently
do much with nfs.
>>> pid-Better-tests-for-same-thread-group-membership.txt
>>> pid-Cleanup-the-pid-equality-tests.txt
>>> pid-Track-the-sending-pid-of-a-queued-signal.txt
>
> is that about updating the siginfos in collect_signal() to hold the right
> pid value depending on the pid namespace they are being received ?
Yes in send_signal, and in collect signal. To make it work easily I needed
to add a struct pid to struct sigqueue. So in send_signal I generate
the struct pid from the pid_t value and in collect signal I regenerate
the numeric value.
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Cedric Le Goater External

Since: Jun 01, 2006 Posts: 129
|
Posted: Tue Sep 12, 2006 12:00 am Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Eric W. Biederman wrote:
> Cedric you mentioned a couple of other patches that are in flight.
> In the future could you please Cc: the containers list so independent
> efforts are less likely to duplicate work, and we are more likely
> to review each others patches instead?
yes sure, i was relying on the openvz wiki to avoid duplicated efforts on
this topic but i guess email is just the one and only tool for this kind of
development
[ ... ]
>>> Converting that is going to need some sysctl work, so I have been
>>> ignoring it temporarily.
>>>
>>> Filling in a struct pid through sysctl is extremely ugly at the
>>> moment, plus cad_pid needs some locking.
>> Which distros use /proc/sys/kernel/cad_pid and why ? I can image the need
>> but i didn't find much on the topic.
>
> I'm not at all certain, and I'm not even certain I care. The concept
> is there in the code so it needs to be dealt with.
OK. It would be nice to make sure this is still in use before trying to
deal with /proc/sys/kernel/cad_pid.
> Although if I we extend the cad_pid concept it may make a difference.
what do you mean by extending cad_pid ? kill_init() ?
[ ... ]
>> is that about updating the siginfos in collect_signal() to hold the right
>> pid value depending on the pid namespace they are being received ?
>
> Yes in send_signal, and in collect signal. To make it work easily I needed
> to add a struct pid to struct sigqueue. So in send_signal I generate
> the struct pid from the pid_t value and in collect signal I regenerate
> the numeric value.
OK. That's what i imagined also but we need a bit more of the pid namespace
to regenerate the numerical value. So, how will you convert this 'struct
pid*' in a pid value using the current pid namespace ?
thinking aloud :
* if the pid namespace of the sending struct pid and current match,
use nr.
* if they don't,
if the sending pid namespace is the ancestor of the current pid
namespace
use 0
else
it's a bug.
struct pid* needs a pid namespace attribute and pid namespace needs to know
its parent.
C.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Eric W. Biederman External

Since: May 19, 2006 Posts: 1134
|
Posted: Tue Sep 12, 2006 3:30 am Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Cedric Le Goater <clg RemoveThis @fr.ibm.com> writes:
> Eric W. Biederman wrote:
>
>> Cedric you mentioned a couple of other patches that are in flight.
>> In the future could you please Cc: the containers list so independent
>> efforts are less likely to duplicate work, and we are more likely
>> to review each others patches instead?
>
> yes sure, i was relying on the openvz wiki to avoid duplicated efforts on
> this topic but i guess email is just the one and only tool for this kind of
> development
Sure. Especially when it comes to helping review each others code
Not duplicating work is not really my goal, not submitting a patch
after a patch has been reviewed and accepted is.
Plus we need patch review.
Several people working on a patch in parallel if it is difficult
can frequently find a solution that a single person would miss.
>>>> Filling in a struct pid through sysctl is extremely ugly at the
>>>> moment, plus cad_pid needs some locking.
>>> Which distros use /proc/sys/kernel/cad_pid and why ? I can image the need
>>> but i didn't find much on the topic.
>>
>> I'm not at all certain, and I'm not even certain I care. The concept
>> is there in the code so it needs to be dealt with.
>
> OK. It would be nice to make sure this is still in use before trying to
> deal with /proc/sys/kernel/cad_pid.
>
>> Although if I we extend the cad_pid concept it may make a difference.
>
> what do you mean by extending cad_pid ? kill_init() ?
My meaning was every time we are sending a signal to init. It is quite
possible we should be using cad_pid instead.
>>> is that about updating the siginfos in collect_signal() to hold the right
>>> pid value depending on the pid namespace they are being received ?
>>
>> Yes in send_signal, and in collect signal. To make it work easily I needed
>> to add a struct pid to struct sigqueue. So in send_signal I generate
>> the struct pid from the pid_t value and in collect signal I regenerate
>> the numeric value.
>
> OK. That's what i imagined also but we need a bit more of the pid namespace
> to regenerate the numerical value. So, how will you convert this 'struct
> pid*' in a pid value using the current pid namespace ?
By calling pid_nr The question I guess is how will pid_nr be implemented.
> thinking aloud :
>
> * if the pid namespace of the sending struct pid and current match,
> use nr.
> * if they don't,
> if the sending pid namespace is the ancestor of the current pid
> namespace
> use 0
> else
> it's a bug.
>
> struct pid* needs a pid namespace attribute and pid namespace needs to know
> its parent.
Yes, that sounds correct.
There is also the case that should not come up with signals where
we have a pid from a child namespace, that we should also be able to
compute the pid for.
In essence I intend to have a list of pid_namespace, pid_t pairs connected
to a struct pid that we can look through to find the appropriate pid.
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Herbert Poetzl External

Since: May 19, 2006 Posts: 109
|
Posted: Tue Sep 12, 2006 1:10 pm Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
On Mon, Sep 11, 2006 at 01:01:18PM -0600, Eric W. Biederman wrote:
>
> Cedric you mentioned a couple of other patches that are in flight.
> In the future could you please Cc: the containers list so independent
> efforts are less likely to duplicate work, and we are more likely
> to review each others patches instead?
>
> Cedric Le Goater <clg.DeleteThis@fr.ibm.com> writes:
>
> > Eric W. Biederman wrote:
> >
> >>>> I was just about to send out this patch in a couple more hours.
> >>> Well, you did the same with the usb/devio.c and friends
> >>
> >> Good. The you should be familiar enough with it to review my patch
> >> and make certain I didn't do anything stupid
> >
> > well, the least i can try ...
> >
>
> >>> * I started smbfs and realized it was useless.
> >>
> >> Killing the user space part is useless?
> >> I thought that is what I saw happening.
> >
> > smb_fill_super() says :
> >
> > if (warn_count < 5) {
> > warn_count++;
> > printk(KERN_EMERG "smbfs is deprecated and will be removed in"
> > " December, 2006. Please migrate to cifs\n");
> > }
> >
> > So, i guess we should forget about it and spend our time on the cifs
> > kthread instead.
>
> Sure. Although in this instance the changes are simple enough I will
> probably send the patch anyway That at least explains why you
> figured it was useless work.
>
>
> >> Of course I don't frequently mount smbfs.
> >>
> >>> * in the following, the init process is being killed directly
> >>> using 1. I'm not sure how useful it would be to use a struct pid.
> >>> To begin with, may be they could use a :
> >>>
> >>> kill_init(int signum, int priv)
> >>
> >> An interesting notion. The other half of them use cad_pid.
> >
> > yes.
> >
> >> Converting that is going to need some sysctl work, so I have been
> >> ignoring it temporarily.
> >>
> >> Filling in a struct pid through sysctl is extremely ugly at the
> >> moment, plus cad_pid needs some locking.
> >
> > Which distros use /proc/sys/kernel/cad_pid and why ? I can image the
> > need but i didn't find much on the topic.
>
> I'm not at all certain, and I'm not even certain I care. The concept
> is there in the code so it needs to be dealt with. Although if I we
> extend the cad_pid concept it may make a difference.
>
> >> My patch todo list (almost a series file) currently looks like:
> >>
> >>> n_r396r fs3270-Change-to-use-struct-pid.txt
> >
> > done that. will send to martin for review.
>
> Added to my queue of pending patches to look at review.
>
> >>> ncpfs-Use-struct-pid-to-track-the-userspace-watchdog-process.txt
> >>>
> >>> Don-t-use-kill_pg-in-the-sunos-compatibility-code.txt
> >>>
> >>> usbatm-use-kthread-api (I think I have this one)
> >>
> >> I did usbatm mostly to figure out why kthread conversions seem to
> >> be so hard, and got lucky this one wasn't too ugly.
> >
> > argh. i've done also and i just send my second version of the patch
> > to the maintainer Duncan Sands.
> >
> > This one might just be useless also because greg kh has a patch in
> > -mm to enable multithread probing of USB devices.
>
> Added to my queue of pending patches to track down and reivew.
>
>
> >>> The-dvb_core-needs-to-use-the-kthread-api-not-kernel-threads.txt
> >>> nfs-Note-we-need-to-start-using-the-kthreads-api.txt
> >>
> >> dvb-core I have only started looking at.
> >
> > suka and i have sent patches to fix :
> >
> > drivers/media/video/tvaudio.c
> > drivers/media/video/saa7134/saa7134-tvaudio.c
> >
> > we are no waiting for the maintainer feedback.
>
> Ok. I think I saw a little of that.
>
> >> nfs I noticed it is the svc stuff that matters.
> >>
> >> usbatm, dvb-core, and nfs are the 3 kernel_thread users
> >> that also use kill_proc, and thus are high on my immediate hit list.
> > nfs is also full of signal_pending() ...
> Yes, there is a lot to read and understand before I can confidently
> do much with nfs.
I already did a lot of adjustments to the nfs system, and
I poked aound in dvb-core before, so I will take a look
at this in the next few days, at least the switch to the
kthread api should not be a big deal ...
HTH,
Herbert
> >>> pid-Better-tests-for-same-thread-group-membership.txt
> >>> pid-Cleanup-the-pid-equality-tests.txt
> >>> pid-Track-the-sending-pid-of-a-queued-signal.txt
> >
> > is that about updating the siginfos in collect_signal() to hold the right
> > pid value depending on the pid namespace they are being received ?
>
> Yes in send_signal, and in collect signal. To make it work easily I
> needed to add a struct pid to struct sigqueue. So in send_signal I
> generate the struct pid from the pid_t value and in collect signal I
> regenerate the numeric value.
>
>
> Eric
>
> _______________________________________________
> Containers mailing list
> Containers.DeleteThis@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/containers
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Eric W. Biederman External

Since: May 19, 2006 Posts: 1134
|
Posted: Tue Sep 12, 2006 5:20 pm Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Herbert Poetzl <herbert.TakeThisOut@13thfloor.at> writes:
> I already did a lot of adjustments to the nfs system, and
> I poked aound in dvb-core before, so I will take a look
> at this in the next few days, at least the switch to the
> kthread api should not be a big deal ...
Ok. If you can get this it would be great.
To some extent the last holdouts on the kernel_thread api seem to
be the ones that are not trivial to convert
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.TakeThisOut@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Cedric Le Goater External

Since: Jun 01, 2006 Posts: 129
|
Posted: Tue Sep 12, 2006 5:40 pm Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Eric W. Biederman wrote:
[ ... ]
> There is also the case that should not come up with signals where
> we have a pid from a child namespace, that we should also be able to
> compute the pid for.
I don't understand how a signal can come from a child pid namespace ?
> In essence I intend to have a list of pid_namespace, pid_t pairs connected
> to a struct pid that we can look through to find the appropriate pid.
yes, that's the purpose of pid_nr() I guess.
This list would contain in nearly all cases a single pair (current pid
namespace, pid value). It will contain 2 pairs for a task that has unshared
its pid namespace : a pair for the current pid namespace, that needs to
allocated when unshare() is called, and one pair for the ancestor pid
namespace which is already allocated.
Do you see more ?
C.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Eric W. Biederman External

Since: May 19, 2006 Posts: 1134
|
Posted: Tue Sep 12, 2006 6:10 pm Post subject: Re: [patch -mm] update mq_notify to use a struct pid [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Cedric Le Goater <clg DeleteThis @fr.ibm.com> writes:
> Eric W. Biederman wrote:
>
> [ ... ]
>
>> There is also the case that should not come up with signals where
>> we have a pid from a child namespace, that we should also be able to
>> compute the pid for.
>
> I don't understand how a signal can come from a child pid namespace ?
SIG_CHLD is the only case where I think we will be sending a signal
from the child pid namespace.
Reading pids from the status files in /proc, from a parent namespace,
is another example where we need to deal with the pid of children.
>> In essence I intend to have a list of pid_namespace, pid_t pairs connected
>> to a struct pid that we can look through to find the appropriate pid.
>
> yes, that's the purpose of pid_nr() I guess.
>
> This list would contain in nearly all cases a single pair (current pid
> namespace, pid value). It will contain 2 pairs for a task that has unshared
> its pid namespace : a pair for the current pid namespace, that needs to
> allocated when unshare() is called, and one pair for the ancestor pid
> namespace which is already allocated.
>
> Do you see more ?
I don't see the list getting longer until we get into a nested pid namespaces.
As long as the interface is well defined for the container in a container
case I don't mind having additional restrictions.
I will note that you can get some extremely weird interactions if
you do things like open a file descriptor in the parent pid namespace.
Fork two children each child in a different pid_namespaces.
fcntl(F_SETOWN) is called in one child, and fcntl(F_GETOWN) is called
in the other child.
So we can't just call BUG_ON, if we have can't find the namespace.
But returning 0 from pid_nr should be fine.
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Herbert Poetzl External

Since: May 19, 2006 Posts: 109
|
Posted: Thu Sep 14, 2006 10:10 pm Post subject: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110 [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Okay, as I promised, I had a first shot at the
dvb kernel_thread to kthread API port, and here
is the result, which is running fine here since
yesterday, including module load/unload and
software suspend (which doesn't work as expected
with or without this patch , I didn't convert
the dvb_ca_en50221 as I do not have such an
interface, but if the conversion process is fine
with the v4l-dvb maintainers, it should not be
a problem to send a patch for that too ...
best,
Herbert
Signed-off-by: Herbert Poetzl <herbert RemoveThis @13thfloor.at>
diff -NurpP linux-2.6.18-rc6/drivers/media/dvb/dvb-core/dvb_frontend.c linux-2.6.18-rc6-kthread.v02.3/drivers/media/dvb/dvb-core/dvb_frontend.c
--- linux-2.6.18-rc6/drivers/media/dvb/dvb-core/dvb_frontend.c 2006-09-12 18:16:12 +0200
+++ linux-2.6.18-rc6-kthread.v02.3/drivers/media/dvb/dvb-core/dvb_frontend.c 2006-09-14 21:23:37 +0200
@@ -36,6 +36,7 @@
#include <linux/list.h>
#include <linux/suspend.h>
#include <linux/jiffies.h>
+#include <linux/kthread.h>
#include <asm/processor.h>
#include "dvb_frontend.h"
@@ -100,7 +101,7 @@ struct dvb_frontend_private {
struct semaphore sem;
struct list_head list_head;
wait_queue_head_t wait_queue;
- pid_t thread_pid;
+ struct task_struct *thread;
unsigned long release_jiffies;
unsigned int exit;
unsigned int wakeup;
@@ -508,19 +509,11 @@ static int dvb_frontend_thread(void *dat
struct dvb_frontend *fe = data;
struct dvb_frontend_private *fepriv = fe->frontend_priv;
unsigned long timeout;
- char name [15];
fe_status_t s;
struct dvb_frontend_parameters *params;
dprintk("%s\n", __FUNCTION__);
- snprintf (name, sizeof(name), "kdvb-fe-%i", fe->dvb->num);
-
- lock_kernel();
- daemonize(name);
- sigfillset(¤t->blocked);
- unlock_kernel();
-
fepriv->check_wrapped = 0;
fepriv->quality = 0;
fepriv->delay = 3*HZ;
@@ -534,14 +527,16 @@ static int dvb_frontend_thread(void *dat
up(&fepriv->sem); /* is locked when we enter the thread... */
timeout = wait_event_interruptible_timeout(fepriv->wait_queue,
- dvb_frontend_should_wakeup(fe),
- fepriv->delay);
- if (0 != dvb_frontend_is_exiting(fe)) {
+ dvb_frontend_should_wakeup(fe) || kthread_should_stop(),
+ fepriv->delay);
+
+ if (kthread_should_stop() || dvb_frontend_is_exiting(fe)) {
/* got signal or quitting */
break;
}
- try_to_freeze();
+ if (try_to_freeze())
+ continue;
if (down_interruptible(&fepriv->sem))
break;
@@ -591,7 +586,7 @@ static int dvb_frontend_thread(void *dat
fe->ops.sleep(fe);
}
- fepriv->thread_pid = 0;
+ fepriv->thread = NULL;
mb();
dvb_frontend_wakeup(fe);
@@ -600,7 +595,6 @@ static int dvb_frontend_thread(void *dat
static void dvb_frontend_stop(struct dvb_frontend *fe)
{
- unsigned long ret;
struct dvb_frontend_private *fepriv = fe->frontend_priv;
dprintk ("%s\n", __FUNCTION__);
@@ -608,33 +602,17 @@ static void dvb_frontend_stop(struct dvb
fepriv->exit = 1;
mb();
- if (!fepriv->thread_pid)
- return;
-
- /* check if the thread is really alive */
- if (kill_proc(fepriv->thread_pid, 0, 1) == -ESRCH) {
- printk("dvb_frontend_stop: thread PID %d already died\n",
- fepriv->thread_pid);
- /* make sure the mutex was not held by the thread */
- init_MUTEX (&fepriv->sem);
+ if (!fepriv->thread)
return;
- }
-
- /* wake up the frontend thread, so it notices that fe->exit == 1 */
- dvb_frontend_wakeup(fe);
- /* wait until the frontend thread has exited */
- ret = wait_event_interruptible(fepriv->wait_queue,0 == fepriv->thread_pid);
- if (-ERESTARTSYS != ret) {
- fepriv->state = FESTATE_IDLE;
- return;
- }
+ kthread_stop(fepriv->thread);
+ init_MUTEX (&fepriv->sem);
fepriv->state = FESTATE_IDLE;
/* paranoia check in case a signal arrived */
- if (fepriv->thread_pid)
- printk("dvb_frontend_stop: warning: thread PID %d won't exit\n",
- fepriv->thread_pid);
+ if (fepriv->thread)
+ printk("dvb_frontend_stop: warning: thread %p won't exit\n",
+ fepriv->thread);
}
s32 timeval_usec_diff(struct timeval lasttime, struct timeval curtime)
@@ -684,10 +662,11 @@ static int dvb_frontend_start(struct dvb
{
int ret;
struct dvb_frontend_private *fepriv = fe->frontend_priv;
+ struct task_struct *fe_thread;
dprintk ("%s\n", __FUNCTION__);
- if (fepriv->thread_pid) {
+ if (fepriv->thread) {
if (!fepriv->exit)
return 0;
else
@@ -701,18 +680,18 @@ static int dvb_frontend_start(struct dvb
fepriv->state = FESTATE_IDLE;
fepriv->exit = 0;
- fepriv->thread_pid = 0;
+ fepriv->thread = NULL;
mb();
- ret = kernel_thread (dvb_frontend_thread, fe, 0);
-
- if (ret < 0) {
- printk("dvb_frontend_start: failed to start kernel_thread (%d)\n", ret);
+ fe_thread = kthread_run(dvb_frontend_thread, fe,
+ "kdvb-fe-%i", fe->dvb->num);
+ if (IS_ERR(fe_thread)) {
+ ret = PTR_ERR(fe_thread);
+ printk("dvb_frontend_start: failed to start kthread (%d)\n", ret);
up(&fepriv->sem);
return ret;
}
- fepriv->thread_pid = ret;
-
+ fepriv->thread = fe_thread;
return 0;
}
diff -NurpP linux-2.6.18-rc6/drivers/media/dvb/ttpci/av7110.c linux-2.6.18-rc6-kthread.v02.3/drivers/media/dvb/ttpci/av7110.c
--- linux-2.6.18-rc6/drivers/media/dvb/ttpci/av7110.c 2006-09-12 18:16:13 +0200
+++ linux-2.6.18-rc6-kthread.v02.3/drivers/media/dvb/ttpci/av7110.c 2006-09-14 21:21:03 +0200
@@ -51,6 +51,7 @@
#include <linux/firmware.h>
#include <linux/crc32.h>
#include <linux/i2c.h>
+#include <linux/kthread.h>
#include <asm/system.h>
@@ -223,11 +224,10 @@ static void recover_arm(struct av7110 *a
static void av7110_arm_sync(struct av7110 *av7110)
{
- av7110->arm_rmmod = 1;
- wake_up_interruptible(&av7110->arm_wait);
+ if (av7110->arm_thread)
+ kthread_stop(av7110->arm_thread);
- while (av7110->arm_thread)
- msleep(1);
+ av7110->arm_thread = NULL;
}
static int arm_thread(void *data)
@@ -238,17 +238,11 @@ static int arm_thread(void *data)
dprintk(4, "%p\n",av7110);
- lock_kernel();
- daemonize("arm_mon");
- sigfillset(¤t->blocked);
- unlock_kernel();
-
- av7110->arm_thread = current;
-
for (; {
timeout = wait_event_interruptible_timeout(av7110->arm_wait,
- av7110->arm_rmmod, 5 * HZ);
- if (-ERESTARTSYS == timeout || av7110->arm_rmmod) {
+ kthread_should_stop(), 5 * HZ);
+
+ if (-ERESTARTSYS == timeout || kthread_should_stop()) {
/* got signal or told to quit*/
break;
}
@@ -276,7 +270,6 @@ static int arm_thread(void *data)
av7110->arm_errors = 0;
}
- av7110->arm_thread = NULL;
return 0;
}
@@ -2334,6 +2327,7 @@ static int __devinit av7110_attach(struc
const int length = TS_WIDTH * TS_HEIGHT;
struct pci_dev *pdev = dev->pci;
struct av7110 *av7110;
+ struct task_struct *thread;
int ret, count = 0;
dprintk(4, "dev: %p\n", dev);
@@ -2618,9 +2612,12 @@ static int __devinit av7110_attach(struc
printk ("dvb-ttpci: Warning, firmware version 0x%04x is too old. "
"System might be unstable!\n", FW_VERSION(av7110->arm_app));
- ret = kernel_thread(arm_thread, (void *) av7110, 0);
- if (ret < 0)
+ thread = kthread_run(arm_thread, (void *) av7110, "arm_mon");
+ if (IS_ERR(thread)) {
+ ret = PTR_ERR(thread);
goto err_stop_arm_9;
+ }
+ av7110->arm_thread = thread;
/* set initial volume in mixer struct */
av7110->mixer.volume_left = volume;
diff -NurpP linux-2.6.18-rc6/drivers/media/dvb/ttpci/av7110.h linux-2.6.18-rc6-kthread.v02.3/drivers/media/dvb/ttpci/av7110.h
--- linux-2.6.18-rc6/drivers/media/dvb/ttpci/av7110.h 2006-09-12 18:16:13 +0200
+++ linux-2.6.18-rc6-kthread.v02.3/drivers/media/dvb/ttpci/av7110.h 2006-09-14 21:21:03 +0200
@@ -205,7 +205,6 @@ struct av7110 {
struct task_struct *arm_thread;
wait_queue_head_t arm_wait;
u16 arm_loops;
- int arm_rmmod;
void *debi_virt;
dma_addr_t debi_bus;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Cedric Le Goater External

Since: Jun 01, 2006 Posts: 129
|
Posted: Thu Sep 14, 2006 11:10 pm Post subject: Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110 [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Herbert Poetzl wrote:
> Okay, as I promised, I had a first shot at the
> dvb kernel_thread to kthread API port, and here
> is the result, which is running fine here since
> yesterday, including module load/unload and
> software suspend (which doesn't work as expected
> with or without this patch ,
So you have such an hardware ?
[ ... ]
> @@ -600,7 +595,6 @@ static int dvb_frontend_thread(void *dat
>
> static void dvb_frontend_stop(struct dvb_frontend *fe)
> {
> - unsigned long ret;
> struct dvb_frontend_private *fepriv = fe->frontend_priv;
>
> dprintk ("%s\n", __FUNCTION__);
> @@ -608,33 +602,17 @@ static void dvb_frontend_stop(struct dvb
> fepriv->exit = 1;
do we still need the ->exit flag now that we are using kthread_stop() ?
same question for ->wakeup ?
> mb();
>
> - if (!fepriv->thread_pid)
> - return;
> -
> - /* check if the thread is really alive */
> - if (kill_proc(fepriv->thread_pid, 0, 1) == -ESRCH) {
> - printk("dvb_frontend_stop: thread PID %d already died\n",
> - fepriv->thread_pid);
> - /* make sure the mutex was not held by the thread */
> - init_MUTEX (&fepriv->sem);
> + if (!fepriv->thread)
> return;
> - }
> -
> - /* wake up the frontend thread, so it notices that fe->exit == 1 */
> - dvb_frontend_wakeup(fe);
>
> - /* wait until the frontend thread has exited */
> - ret = wait_event_interruptible(fepriv->wait_queue,0 == fepriv->thread_pid);
> - if (-ERESTARTSYS != ret) {
> - fepriv->state = FESTATE_IDLE;
> - return;
> - }
> + kthread_stop(fepriv->thread);
> + init_MUTEX (&fepriv->sem);
the use of the semaphore to synchronise the thread is complex. It will
require extra care to avoid deadlocks.
> fepriv->state = FESTATE_IDLE;
>
> /* paranoia check in case a signal arrived */
> - if (fepriv->thread_pid)
> - printk("dvb_frontend_stop: warning: thread PID %d won't exit\n",
> - fepriv->thread_pid);
> + if (fepriv->thread)
> + printk("dvb_frontend_stop: warning: thread %p won't exit\n",
> + fepriv->thread);
kthread_stop uses a completion already. so the above is real paranoia
> }
>
> s32 timeval_usec_diff(struct timeval lasttime, struct timeval curtime)
> @@ -684,10 +662,11 @@ static int dvb_frontend_start(struct dvb
> {
> int ret;
> struct dvb_frontend_private *fepriv = fe->frontend_priv;
> + struct task_struct *fe_thread;
>
> dprintk ("%s\n", __FUNCTION__);
>
> - if (fepriv->thread_pid) {
> + if (fepriv->thread) {
> if (!fepriv->exit)
> return 0;
> else
> @@ -701,18 +680,18 @@ static int dvb_frontend_start(struct dvb
>
> fepriv->state = FESTATE_IDLE;
> fepriv->exit = 0;
> - fepriv->thread_pid = 0;
> + fepriv->thread = NULL;
> mb();
>
> - ret = kernel_thread (dvb_frontend_thread, fe, 0);
> -
> - if (ret < 0) {
> - printk("dvb_frontend_start: failed to start kernel_thread (%d)\n", ret);
> + fe_thread = kthread_run(dvb_frontend_thread, fe,
> + "kdvb-fe-%i", fe->dvb->num);
> + if (IS_ERR(fe_thread)) {
> + ret = PTR_ERR(fe_thread);
ret could be local.
[ ... ]
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Herbert Poetzl External

Since: May 19, 2006 Posts: 109
|
Posted: Fri Sep 15, 2006 12:20 am Post subject: Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110 [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
On Thu, Sep 14, 2006 at 11:07:49PM +0200, Cedric Le Goater wrote:
> Herbert Poetzl wrote:
> > Okay, as I promised, I had a first shot at the
> > dvb kernel_thread to kthread API port, and here
> > is the result, which is running fine here since
> > yesterday, including module load/unload and
> > software suspend (which doesn't work as expected
> > with or without this patch ,
>
> So you have such an hardware ?
yes, I do .. that's how I tested it
> [ ... ]
>
> > @@ -600,7 +595,6 @@ static int dvb_frontend_thread(void *dat
> >
> > static void dvb_frontend_stop(struct dvb_frontend *fe)
> > {
> > - unsigned long ret;
> > struct dvb_frontend_private *fepriv = fe->frontend_priv;
> >
> > dprintk ("%s\n", __FUNCTION__);
> > @@ -608,33 +602,17 @@ static void dvb_frontend_stop(struct dvb
> > fepriv->exit = 1;
>
> do we still need the ->exit flag now that we are using kthread_stop() ?
> same question for ->wakeup ?
probably not, but I didn't want to change too
much on the first try, especially I'd appreciate
some feedback to this from the maintainer(s)
> > mb();
> >
> > - if (!fepriv->thread_pid)
> > - return;
> > -
> > - /* check if the thread is really alive */
> > - if (kill_proc(fepriv->thread_pid, 0, 1) == -ESRCH) {
> > - printk("dvb_frontend_stop: thread PID %d already died\n",
> > - fepriv->thread_pid);
> > - /* make sure the mutex was not held by the thread */
> > - init_MUTEX (&fepriv->sem);
> > + if (!fepriv->thread)
> > return;
> > - }
> > -
> > - /* wake up the frontend thread, so it notices that fe->exit == 1 */
> > - dvb_frontend_wakeup(fe);
> >
> > - /* wait until the frontend thread has exited */
> > - ret = wait_event_interruptible(fepriv->wait_queue,0 == fepriv->thread_pid);
> > - if (-ERESTARTSYS != ret) {
> > - fepriv->state = FESTATE_IDLE;
> > - return;
> > - }
> > + kthread_stop(fepriv->thread);
> > + init_MUTEX (&fepriv->sem);
>
> the use of the semaphore to synchronise the thread is complex. It will
> require extra care to avoid deadlocks.
well, it 'works' quite fine for now, but yeah, I
thought about completely removing the additional
synchronization and 'jsut' go with the kthread
one, if that is sufficient ...
> > fepriv->state = FESTATE_IDLE;
> >
> > /* paranoia check in case a signal arrived */
> > - if (fepriv->thread_pid)
> > - printk("dvb_frontend_stop: warning: thread PID %d won't exit\n",
> > - fepriv->thread_pid);
> > + if (fepriv->thread)
> > + printk("dvb_frontend_stop: warning: thread %p won't exit\n",
> > + fepriv->thread);
>
> kthread_stop uses a completion already. so the above is real paranoia
again, I think this will go away soon
> > }
> >
> > s32 timeval_usec_diff(struct timeval lasttime, struct timeval curtime)
> > @@ -684,10 +662,11 @@ static int dvb_frontend_start(struct dvb
> > {
> > int ret;
> > struct dvb_frontend_private *fepriv = fe->frontend_priv;
> > + struct task_struct *fe_thread;
> >
> > dprintk ("%s\n", __FUNCTION__);
> >
> > - if (fepriv->thread_pid) {
> > + if (fepriv->thread) {
> > if (!fepriv->exit)
> > return 0;
> > else
> > @@ -701,18 +680,18 @@ static int dvb_frontend_start(struct dvb
> >
> > fepriv->state = FESTATE_IDLE;
> > fepriv->exit = 0;
> > - fepriv->thread_pid = 0;
> > + fepriv->thread = NULL;
> > mb();
> >
> > - ret = kernel_thread (dvb_frontend_thread, fe, 0);
> > -
> > - if (ret < 0) {
> > - printk("dvb_frontend_start: failed to start kernel_thread (%d)\n", ret);
> > + fe_thread = kthread_run(dvb_frontend_thread, fe,
> > + "kdvb-fe-%i", fe->dvb->num);
> > + if (IS_ERR(fe_thread)) {
> > + ret = PTR_ERR(fe_thread);
>
> ret could be local.
correct, will fix that up in the next round
thanks for the feedback,
Herbert
> [ ... ]
>
> _______________________________________________
> Containers mailing list
> Containers.RemoveThis@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/containers
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Andrew de Quincey External

Since: Jul 25, 2006 Posts: 9
|
Posted: Fri Nov 17, 2006 3:00 am Post subject: Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110 [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
[snip]
> correct, will fix that up in the next round
>
> thanks for the feedback,
> Herbert
Hi - the conversion looks good to me.. I can't really offer any more
constructive suggestions beyond what Cedric has already said.
Theres another thread in dvb_ca_en50221.c that could be converted as well
though, hint hint
Apologies for the delay in this reply - I've been hibernating for a bit.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.DeleteThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Cedric Le Goater External

Since: Jun 01, 2006 Posts: 129
|
Posted: Wed Nov 22, 2006 4:00 pm Post subject: Re: [Devel] Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110 [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Andrew de Quincey wrote:
> Hi - the conversion looks good to me.. I can't really offer any more
> constructive suggestions beyond what Cedric has already said.
ok. so, should we just resend a refreshed version of the patch when 2.6.19
comes out ?
> Theres another thread in dvb_ca_en50221.c that could be converted as well
> though, hint hint
ok ok i'll look at it ...
> Apologies for the delay in this reply - I've been hibernating for a bit.
np.
thanks,
C.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo RemoveThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Andrew de Quincey External

Since: Jul 25, 2006 Posts: 9
|
Posted: Wed Nov 22, 2006 10:40 pm Post subject: Re: [v4l-dvb-maintainer] Re: [Devel] Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110 [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
On Wednesday 22 November 2006 14:56, Cedric Le Goater wrote:
> Andrew de Quincey wrote:
> > Hi - the conversion looks good to me.. I can't really offer any more
> > constructive suggestions beyond what Cedric has already said.
>
> ok. so, should we just resend a refreshed version of the patch when 2.6.19
> comes out ?
Yeah - sounds good.
> > Theres another thread in dvb_ca_en50221.c that could be converted as well
> > though, hint hint
>
> ok ok i'll look at it ...
heh
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo.RemoveThis@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
Eric W. Biederman External

Since: May 19, 2006 Posts: 1134
|
Posted: Wed Dec 13, 2006 12:10 am Post subject: Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110 [Login to view extended thread Info.] Archived from groups: per prev. post (more info?) |
|
|
Andrew de Quincey <adq_dvb DeleteThis @lidskialf.net> writes:
> [snip]
>
>> correct, will fix that up in the next round
>>
>> thanks for the feedback,
>> Herbert
>
> Hi - the conversion looks good to me.. I can't really offer any more
> constructive suggestions beyond what Cedric has already said.
>
> Theres another thread in dvb_ca_en50221.c that could be converted as well
> though, hint hint
>
> Apologies for the delay in this reply - I've been hibernating for a bit.
Guys where are we at on this conversion?
Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo DeleteThis @vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/ |
|
| Back to top |
|
 |
|
|
|
You can post new topics in this forum You can reply to topics in this forum You cannot edit your posts in this forum You cannot delete your posts in this forum You cannot vote in polls in this forum
|
| |
|
|