Re: [PATCH 1/2] DVB: dvb_frontend: convert semaphore to mutex
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 o...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] DVB: dvb_frontend: convert semaphore to mutex
On 24.08.2011 20:54, Devin Heitmueller wrote: On Wed, Aug 24, 2011 at 2:08 PM, Andreas Oberritter o...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] DVB: dvb_frontend: convert semaphore to mutex
On Wed, Aug 24, 2011 at 1:33 PM, Andreas Oberritter o...@linuxtv.org wrote: Signed-off-by: Andreas Oberritter o...@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 -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] DVB: dvb_frontend: convert semaphore to mutex
On Wed, Aug 24, 2011 at 2:02 PM, Andreas Oberritter o...@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 -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] DVB: dvb_frontend: convert semaphore to mutex
On 24.08.2011 20:06, Devin Heitmueller wrote: On Wed, Aug 24, 2011 at 2:02 PM, Andreas Oberritter o...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] DVB: dvb_frontend: convert semaphore to mutex
On 24.08.2011 19:54, Devin Heitmueller wrote: On Wed, Aug 24, 2011 at 1:33 PM, Andreas Oberritter o...@linuxtv.org wrote: Signed-off-by: Andreas Oberritter o...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] DVB: dvb_frontend: convert semaphore to mutex
On Wed, Aug 24, 2011 at 2:08 PM, Andreas Oberritter o...@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 -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
