OpenVZ Forum - RDF feed
type: channel
items: [patch -mm] update mq_notify to use a struct pid
items: Re: [patch -mm] update mq_notify to use a struct pid
items: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110
items: Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110
items: Re: Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110

[patch -mm] update mq_notify to use a struct pid
type: item
description: 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@fr.ibm.com> Cc: Eric Biederman <ebiederm@xmission.com> Cc: Andrew Morton <akpm@osdl.org> Cc: containers@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…
creator: Cedric Le Goater
date: 2006-09-08T16:39:31-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: Cedric Le Goater <clg@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@fr.ibm.com> > Cc: Eric Biederman <ebiederm@xmission.com> > Cc: Andrew Morton <akpm@osdl.org> > Cc: containers@lists.osdl.org Signed-off-by: Eric Biederman <ebiederm@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 _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
creator: ebiederm
date: 2006-09-09T02:39:05-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: Eric W. Biederman wrote: > Cedric Le Goater <clg@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@fr.ibm.com> >> Cc: Eric Biederman <ebiederm@xmission.com> >> Cc: Andrew Morton <akpm@osdl.org> >> Cc: containers@lists.osdl.org > > Signed-off-by: Eric Biederman <ebiederm@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. _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
creator: Cedric Le Goater
date: 2006-09-11T10:17:26-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: Cedric Le Goater <clg@fr.ibm.com> writes: > Eric W. Biederman wrote: >> Cedric Le Goater <clg@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@fr.ibm.com> >>> Cc: Eric Biederman <ebiederm@xmission.com> >>> Cc: Andrew Morton <akpm@osdl.org> >>> Cc: containers@lists.osdl.org >> >> Signed-off-by: Eric Biederman <ebiederm@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-rese…
creator: ebiederm
date: 2006-09-11T11:09:19-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: 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…
creator: Cedric Le Goater
date: 2006-09-11T14:05:50-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: On 09/11, Cedric Le Goater wrote: > > Eric W. Biederman wrote: > > Cedric Le Goater <clg@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@fr.ibm.com> > >> Cc: Eric Biederman <ebiederm@xmission.com> > >> Cc: Andrew Morton <akpm@osdl.org> > >> Cc: containers@lists.osdl.org > > > > Signed-off-by: Eric Biederman <ebiederm@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. _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
creator: Oleg Nesterov
date: 2006-09-11T15:48:33-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: 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@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 ext…
creator: ebiederm
date: 2006-09-11T19:01:18-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: 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 p…
creator: Cedric Le Goater
date: 2006-09-11T21:53:33-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: Cedric Le Goater <clg@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 b…
creator: ebiederm
date: 2006-09-12T01:22:20-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: 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@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…
creator: Herbert Poetzl
date: 2006-09-12T11:05:59-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: Herbert Poetzl <herbert@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 _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
creator: ebiederm
date: 2006-09-12T15:14:02-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: 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. _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
creator: Cedric Le Goater
date: 2006-09-12T15:37:28-00:00

Re: [patch -mm] update mq_notify to use a struct pid
type: item
description: Cedric Le Goater <clg@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 _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
creator: ebiederm
date: 2006-09-12T16:03:32-00:00

[PATCH/RFC] kthread API conversion for dvb_frontend and av7110
type: item
description: 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@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(&current->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_fronte…
creator: Herbert Poetzl
date: 2006-09-14T20:01:03-00:00

Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110
type: item
description: 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 th…
creator: Cedric Le Goater
date: 2006-09-14T21:07:49-00:00

Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110
type: item
description: 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…
creator: Herbert Poetzl
date: 2006-09-14T22:10:24-00:00

Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110
type: item
description: [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. _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
creator: Andrew de Quincey
date: 2006-11-17T01:50:01-00:00

Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110
type: item
description: Andrew de Quincey <adq_dvb@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 _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
creator: ebiederm
date: 2006-12-12T22:58:16-00:00

Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110
type: item
description: On Tue, Dec 12, 2006 at 03:58:16PM -0700, Eric W. Biederman wrote: > Andrew de Quincey <adq_dvb@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? I can take a look at it in the next few days, but I have no hardware to test that, so it would be good to get in contact with somebody who does best, Herbert > Eric _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
creator: Herbert Poetzl
date: 2006-12-12T23:13:15-00:00

Re: Re: [PATCH/RFC] kthread API conversion for dvb_frontend and av7110
type: item
description: Herbert Poetzl wrote: > On Tue, Dec 12, 2006 at 03:58:16PM -0700, Eric W. Biederman wrote: >> Andrew de Quincey <adq_dvb@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? I said I would but I didn't have much time yet? however, I didn't see any big issue. no semaphore like in dvb_frontend.c. just some kill(0) that looks harmless. > I can take a look at it in the next few days, but > I have no hardware to test that, so it would be > good to get in contact with somebody who does neither do I. below is an updated version of herbert's dvb_frontend.c patch for 2.6.19-mm1 thanks, C. From: Herbert Poetzl <herbert@13thfloor.at> 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@13thfloor.at> --- drivers/media/dvb/dvb-core/dvb_frontend.c | 69 ++++++++++-------------------- drivers/media/dvb/ttpci/av7110.c | 29 +++++------- drivers/media/dvb/ttpci/av7110.h | 1 3 files changed, 37 insertions(+), 62 deletions(-) Index: 2.6.19-mm1/drivers/media/dvb/dvb-core/dvb_frontend.c =================================================================== --- 2.6.19-mm1.orig/drivers/me…
creator: Cedric Le Goater
date: 2006-12-13T15:55:12-00:00