| Message ID | 1314207232-6031-1-git-send-email-obi@linuxtv.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 24, 2011 at 1:33 PM, Andreas Oberritter <obi@linuxtv.org> wrote:
> Signed-off-by: Andreas Oberritter <obi@linuxtv.org>
This may seem like a silly question, but *why* are you making this
change? There is no explanation for what prompted it. Is it in
response to some issue you encountered?
I'm asking because in general dvb_frontend has a fairly complicated
locking model, and unless there is a compelling reason to make changes
I would be against it.
In other words, this is a bad place for arbitrary "cleanup patches".
Devin
On 24.08.2011 19:54, Devin Heitmueller wrote: > On Wed, Aug 24, 2011 at 1:33 PM, Andreas Oberritter <obi@linuxtv.org> wrote: >> Signed-off-by: Andreas Oberritter <obi@linuxtv.org> > > This may seem like a silly question, but *why* are you making this > change? There is no explanation for what prompted it. Is it in > response to some issue you encountered? A semaphore with only one unit is nothing but a mutex. Using a mutex structure decreases memory footprint and improves readability. > I'm asking because in general dvb_frontend has a fairly complicated > locking model, and unless there is a compelling reason to make changes > I would be against it. The lock is part of fepriv, which is local to dvb_frontend.c. The patch is really simple. > In other words, this is a bad place for arbitrary "cleanup patches". It's impossible to clean up dvb_frontend.c, which looks quite unmaintained, without touching it. Regards, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 24, 2011 at 2:02 PM, Andreas Oberritter <obi@linuxtv.org> wrote: > It's impossible to clean up dvb_frontend.c, which looks quite > unmaintained, without touching it. It is quite unmaintained. In fact, it was broken for numerous cards for almost two years before I finally got someone in the Hauppauge UK office to mail me a couple of affected boards to test with. Now that it works, I'm very hesitant to see any chances made unless there is a *very* good reason. It's just too damn easy to introduce subtle bugs in there that work for "your card" but cause breakage for others. Devin
On 24.08.2011 20:06, Devin Heitmueller wrote: > On Wed, Aug 24, 2011 at 2:02 PM, Andreas Oberritter <obi@linuxtv.org> wrote: >> It's impossible to clean up dvb_frontend.c, which looks quite >> unmaintained, without touching it. > > It is quite unmaintained. In fact, it was broken for numerous cards > for almost two years before I finally got someone in the Hauppauge UK > office to mail me a couple of affected boards to test with. > > Now that it works, I'm very hesitant to see any chances made unless > there is a *very* good reason. It's just too damn easy to introduce > subtle bugs in there that work for "your card" but cause breakage for > others. Instead of wasting your time with theory, you could have easily reviewed my patch. It's really *very* simple any anyone having used semphores or mutexes in the kernel should be able to see that. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 24, 2011 at 2:08 PM, Andreas Oberritter <obi@linuxtv.org> wrote: > Instead of wasting your time with theory, you could have easily reviewed > my patch. It's really *very* simple any anyone having used semphores or > mutexes in the kernel should be able to see that. There's no need to resort to belittlement. Both of us have a non-trivial number of commits to the Linux kernel. My concern is that in the kernel a semaphore with a unit of one is *not* necessarily the same as a mutex. In particular you need to take into account the calling context since mutexes do more enforcement of certain conditions that may have been acceptable for a semaphore. From http://www.kernel.org/doc/Documentation/mutex-design.txt : === - 'struct mutex' semantics are well-defined and are enforced if CONFIG_DEBUG_MUTEXES is turned on. Semaphores on the other hand have virtually no debugging code or instrumentation. The mutex subsystem checks and enforces the following rules: * - only one task can hold the mutex at a time * - only the owner can unlock the mutex * - multiple unlocks are not permitted * - recursive locking is not permitted * - a mutex object must be initialized via the API * - a mutex object must not be initialized via memset or copying * - task may not exit with mutex held * - memory areas where held locks reside must not be freed * - held mutexes must not be reinitialized * - mutexes may not be used in hardware or software interrupt * contexts such as tasklets and timers === and: === Disadvantages ------------- The stricter mutex API means you cannot use mutexes the same way you can use semaphores: e.g. they cannot be used from an interrupt context, nor can they be unlocked from a different context that which acquired it. [ I'm not aware of any other (e.g. performance) disadvantages from using mutexes at the moment, please let me know if you find any. ] === In short, you cannot just arbitrarily replace one with the other. You need to look at all the possible call paths and ensure that there aren't any cases for example where the mutex is set in one but cleared in the other. Did you evaluate your change in the context of each of the differences described in the list above? Without any documentation in the patch, we have absolutely no idea what level of due diligence you exercised in ensuring this didn't cause breakage. Devin
On 24.08.2011 20:54, Devin Heitmueller wrote: > On Wed, Aug 24, 2011 at 2:08 PM, Andreas Oberritter <obi@linuxtv.org> wrote: >> Instead of wasting your time with theory, you could have easily reviewed >> my patch. It's really *very* simple any anyone having used semphores or >> mutexes in the kernel should be able to see that. > > There's no need to resort to belittlement. Both of us have a > non-trivial number of commits to the Linux kernel. > > My concern is that in the kernel a semaphore with a unit of one is > *not* necessarily the same as a mutex. In particular you need to take > into account the calling context since mutexes do more enforcement of > certain conditions that may have been acceptable for a semaphore. > > From http://www.kernel.org/doc/Documentation/mutex-design.txt : > > === > - 'struct mutex' semantics are well-defined and are enforced if > CONFIG_DEBUG_MUTEXES is turned on. Semaphores on the other hand have > virtually no debugging code or instrumentation. The mutex subsystem > checks and enforces the following rules: > > * - only one task can hold the mutex at a time > * - only the owner can unlock the mutex > * - multiple unlocks are not permitted > * - recursive locking is not permitted > * - a mutex object must be initialized via the API > * - a mutex object must not be initialized via memset or copying > * - task may not exit with mutex held > * - memory areas where held locks reside must not be freed > * - held mutexes must not be reinitialized > * - mutexes may not be used in hardware or software interrupt > * contexts such as tasklets and timers > === > > and: > > === > Disadvantages > ------------- > > The stricter mutex API means you cannot use mutexes the same way you > can use semaphores: e.g. they cannot be used from an interrupt context, > nor can they be unlocked from a different context that which acquired > it. [ I'm not aware of any other (e.g. performance) disadvantages from > using mutexes at the moment, please let me know if you find any. ] > === > > In short, you cannot just arbitrarily replace one with the other. You > need to look at all the possible call paths and ensure that there > aren't any cases for example where the mutex is set in one but cleared > in the other. Did you evaluate your change in the context of each of > the differences described in the list above? You're right. There's one place where the semaphore is taken in user context and released by the frontend thread. I'm going to investigate whether this complicated locking is required. It might as well be possible to move the initialization steps from the beginning of the thread to dvb_frontend_start(), thus rendering this use of the semaphore unnecessary, and therefore making the code easier to understand and maintain. Unfortunately, I couldn't find any pointers as to why unlocking a mutex in a different context is not allowed. The only drawback seems to be a warning (which doesn't show up if there was any previous warning...), if mutex debugging is enabled. Besides that, I didn't notice any problem during runtime tests (on mips with SMP enabled). Regards, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 26-08-2011 07:10, Andreas Oberritter escreveu: > On 24.08.2011 20:54, Devin Heitmueller wrote: >> On Wed, Aug 24, 2011 at 2:08 PM, Andreas Oberritter <obi@linuxtv.org> wrote: >>> Instead of wasting your time with theory, you could have easily reviewed >>> my patch. It's really *very* simple any anyone having used semphores or >>> mutexes in the kernel should be able to see that. >> >> There's no need to resort to belittlement. Both of us have a >> non-trivial number of commits to the Linux kernel. >> >> My concern is that in the kernel a semaphore with a unit of one is >> *not* necessarily the same as a mutex. In particular you need to take >> into account the calling context since mutexes do more enforcement of >> certain conditions that may have been acceptable for a semaphore. >> >> From http://www.kernel.org/doc/Documentation/mutex-design.txt : >> >> === >> - 'struct mutex' semantics are well-defined and are enforced if >> CONFIG_DEBUG_MUTEXES is turned on. Semaphores on the other hand have >> virtually no debugging code or instrumentation. The mutex subsystem >> checks and enforces the following rules: >> >> * - only one task can hold the mutex at a time >> * - only the owner can unlock the mutex >> * - multiple unlocks are not permitted >> * - recursive locking is not permitted >> * - a mutex object must be initialized via the API >> * - a mutex object must not be initialized via memset or copying >> * - task may not exit with mutex held >> * - memory areas where held locks reside must not be freed >> * - held mutexes must not be reinitialized >> * - mutexes may not be used in hardware or software interrupt >> * contexts such as tasklets and timers >> === >> >> and: >> >> === >> Disadvantages >> ------------- >> >> The stricter mutex API means you cannot use mutexes the same way you >> can use semaphores: e.g. they cannot be used from an interrupt context, >> nor can they be unlocked from a different context that which acquired >> it. [ I'm not aware of any other (e.g. performance) disadvantages from >> using mutexes at the moment, please let me know if you find any. ] >> === >> >> In short, you cannot just arbitrarily replace one with the other. You >> need to look at all the possible call paths and ensure that there >> aren't any cases for example where the mutex is set in one but cleared >> in the other. Did you evaluate your change in the context of each of >> the differences described in the list above? > > You're right. There's one place where the semaphore is taken in user > context and released by the frontend thread. I'm going to investigate > whether this complicated locking is required. It might as well be > possible to move the initialization steps from the beginning of the > thread to dvb_frontend_start(), thus rendering this use of the semaphore > unnecessary, and therefore making the code easier to understand and > maintain. Ok, I'm dropping this patch from my queue. > Unfortunately, I couldn't find any pointers as to why unlocking a mutex > in a different context is not allowed. The only drawback seems to be a > warning (which doesn't show up if there was any previous warning...), if > mutex debugging is enabled. Besides that, I didn't notice any problem > during runtime tests (on mips with SMP enabled). Maybe it affects only certain archs. I suggest you to look into the git history, and see when the mutex calls were added and when most semaphores were converted into mutexes. Probably, the comments there at git will provide you enough background. > > Regards, > Andreas > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c b/drivers/media/dvb/dvb-core/dvb_frontend.c index a716627..f433a88 100644 --- a/drivers/media/dvb/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c @@ -31,7 +31,6 @@ #include <linux/wait.h> #include <linux/slab.h> #include <linux/poll.h> -#include <linux/semaphore.h> #include <linux/module.h> #include <linux/list.h> #include <linux/freezer.h> @@ -108,7 +107,7 @@ struct dvb_frontend_private { struct dvb_frontend_parameters parameters_in; struct dvb_frontend_parameters parameters_out; struct dvb_fe_events events; - struct semaphore sem; + struct mutex lock; struct list_head list_head; wait_queue_head_t wait_queue; struct task_struct *thread; @@ -190,12 +189,12 @@ static int dvb_frontend_get_event(struct dvb_frontend *fe, if (flags & O_NONBLOCK) return -EWOULDBLOCK; - up(&fepriv->sem); + mutex_unlock(&fepriv->lock); ret = wait_event_interruptible (events->wait_queue, events->eventw != events->eventr); - if (down_interruptible (&fepriv->sem)) + if (mutex_lock_interruptible(&fepriv->lock)) return -ERESTARTSYS; if (ret < 0) @@ -556,7 +555,7 @@ static int dvb_frontend_thread(void *data) set_freezable(); while (1) { - up(&fepriv->sem); /* is locked when we enter the thread... */ + mutex_unlock(&fepriv->lock); /* is locked when we enter the thread... */ restart: timeout = wait_event_interruptible_timeout(fepriv->wait_queue, dvb_frontend_should_wakeup(fe) || kthread_should_stop() @@ -572,7 +571,7 @@ restart: if (try_to_freeze()) goto restart; - if (down_interruptible(&fepriv->sem)) + if (mutex_lock_interruptible(&fepriv->lock)) break; if (fepriv->reinitialise) { @@ -704,7 +703,7 @@ static void dvb_frontend_stop(struct dvb_frontend *fe) kthread_stop(fepriv->thread); - sema_init(&fepriv->sem, 1); + mutex_init(&fepriv->lock); fepriv->state = FESTATE_IDLE; /* paranoia check in case a signal arrived */ @@ -773,7 +772,7 @@ static int dvb_frontend_start(struct dvb_frontend *fe) if (signal_pending(current)) return -EINTR; - if (down_interruptible (&fepriv->sem)) + if (mutex_lock_interruptible(&fepriv->lock)) return -EINTR; fepriv->state = FESTATE_IDLE; @@ -786,7 +785,7 @@ static int dvb_frontend_start(struct dvb_frontend *fe) if (IS_ERR(fe_thread)) { ret = PTR_ERR(fe_thread); printk("dvb_frontend_start: failed to start kthread (%d)\n", ret); - up(&fepriv->sem); + mutex_unlock(&fepriv->lock); return ret; } fepriv->thread = fe_thread; @@ -1535,7 +1534,7 @@ static int dvb_frontend_ioctl(struct file *file, cmd == FE_DISEQC_RECV_SLAVE_REPLY)) return -EPERM; - if (down_interruptible (&fepriv->sem)) + if (mutex_lock_interruptible(&fepriv->lock)) return -ERESTARTSYS; if ((cmd == FE_SET_PROPERTY) || (cmd == FE_GET_PROPERTY)) @@ -1545,7 +1544,7 @@ static int dvb_frontend_ioctl(struct file *file, err = dvb_frontend_ioctl_legacy(file, cmd, parg); } - up(&fepriv->sem); + mutex_unlock(&fepriv->lock); return err; } @@ -2115,7 +2114,7 @@ int dvb_register_frontend(struct dvb_adapter* dvb, } fepriv = fe->frontend_priv; - sema_init(&fepriv->sem, 1); + mutex_init(&fepriv->lock); init_waitqueue_head (&fepriv->wait_queue); init_waitqueue_head (&fepriv->events.wait_queue); mutex_init(&fepriv->events.mtx);
Signed-off-by: Andreas Oberritter <obi@linuxtv.org> --- drivers/media/dvb/dvb-core/dvb_frontend.c | 23 +++++++++++------------ 1 files changed, 11 insertions(+), 12 deletions(-)