Summary:	DAHLIN-00229: [patch] fails to compile against kernel 2.6.37 without CONFIG_BKL
Reporter:	Jaco Kroon (jkroon)	Labels:
Date Opened:	2011-01-12 07:28:56.000-0600	Date Closed:	2012-04-04 12:42:31
Priority:	Blocker	Regression?	No
Status:	Closed/Complete	Components:	dahdi (the module)
Versions:	2.4.0	Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 0001-dahdi-Experimentally-remove-dependency-on-the-Big-Ke.patch ( 1) dahdi-2.4.0-linux-2.6.37-locked.patch ( 2) dahdi-2.4.0-linux-2.6.37-unlocked.patch ( 3) dahdi-2.4.0-linux-2.6.37-unlocked-global_dialparams_lock.patch ( 4) dahdi-2.4.0-semaphores.patch ( 5) dahdi-no-bkl.patch
Description:	dahdi-linux refuses to compile against the 2.6.37 kernel unless CONFIG_BKL is set.  CONFIG_BKL is set to die very soon.
Comments:	By: Shaun Ruffell (sruffell) 2011-01-12 10:38:05.000-0600 There has been some movement already on this issue.  For example: http://svn.asterisk.org/view/dahdi?view=revision&revision=9592 http://svn.asterisk.org/view/dahdi?view=revision&revision=9596 and http://svn.asterisk.org/view/dahdi?view=revision&revision=9619 I believe as we move towards a 2.5 release, the lock_kernel (BKL) dependency will naturally drop away, but someone will need to do a careful audit before those calls are removed. By: Jaco Kroon (jkroon) 2011-01-12 12:52:17.000-0600 Attached both my patches anyway.  I haven't had any obvious issues on 2.4.0 on a range of BRI, PRI and TDM400, 800 or 2400 cards without CONFIG_BKL and got a number of machines where it's working properly.  For that reason I believe the -unlocked variant of the patch should be OK, alternatively the -locked version should be good enough to drop the dependency on the BKL from the code. I believe Digium has a battery of regression tests that it can throw at the modules and I myself have what I call the monkey test I can use that'll do a few million call setups + teardowns in relatively short order (using multiple PRI and BRI loops). The "movement" you mention seems to be mostly documentation indicating that it'll be annoying on >2.6.37 kernels, this provides actual code patches which I hope will be useful. Please note that without CONFIG_BKL in pre 2.6.37, lock_kernel and unlock_kernel seems to be no-op operations anyway, so on all my systems (quite a few with kernels ranging from 2.6.30 through to 2.6.37) it seems to be OK.  Obviously I could just have been insanely lucky so far. By: Shaun Ruffell (sruffell) 2011-01-12 13:25:54.000-0600 Well...ok "movement" was a little strong. :)    Since CONFIG_BKL will probably still remain the default for a few more kernel cycles, if you want to modify your "unlocked" patch to only remove the lock_kernel if CONFIG_BKL is not defined, and add some protection around at least the global dialparams, then I don't have a problem committing it right now.  Then a full audit, etc.. can be done when all remnants of "lock_kernel()" is removed. What do you think about that?  Also, if you use git, feel free to attach the patch with git then that way you can write the commit message (and your email address can go into the commit).  If you don't use git....no sweat. By: Jaco Kroon (jkroon) 2011-01-12 14:15:34.000-0600 Which tree do you want me to clone? You'll note I also cleaned up the mess with regards to the function declaration - is this change OK with you?  (IE, can I keep this change in the new patch) CONFIG_BKL is as far as I can tell already not the default anymore since at least 2.6.37 (check both configs in arch/x86/configs, neither of which has CONFIG_BKL set). Essentially it sounds like you want the -locked variant of the patch though which just replaces the BKL with a local mutex which should achieve the same thing as using the BKL, except that it has a local effect only instead of locking the entire kernel. Ok, protecting the global_dialparams doesn't look too difficult, rwlock or a simple spinlock?  I read somewhere today that rwlocks are being deprecated - what's the replacement anyway? I've also found some issues with respect to the "mutexes" used in some of the other modules, eg, voicebus, attaching a patch.  If you're OK with the actual changes I'll roll a version that has any required #ifdef protection for that too.  It would seem previously the mutexes was just aliases for semaphores anyway, so I just went to using the semaphore functions directly.  I think there may be issues with DEFINE_SEMAPHORE vs DECLARE_SEMAPHORE though but I'm not sure, which is why the offer to roll a new patch on this too that takes that into consideration. The struct mutex code I used in my -locked patch is at least available on 2.6.36 - if someone can indicate to me at which kernel version that became available that would be useful information too. By: Shaun Ruffell (sruffell) 2011-01-12 15:05:50.000-0600 First, regarding the mutexes and semaphores.  I believe those issues are all resolved in the current trunk: http://svn.asterisk.org/view/dahdi?view=revision&revision=9464 http://svn.asterisk.org/view/dahdi?view=revision&revision=9579 You should prepare your patches against the current trunk if you are not already Second, you do not need to clone any particular git repo if you don't want to.  You can: git svn clone -r HEAD http://svn.asterisk.org/dahdi/linux/trunk for-dahdi then in for-dahdi, make your edits, and get the commit(s) looking like you want, then ]# git format-patch git-svn..HEAD to make the patches that you want to attach to this issue. However, I currently keep https://github.com/sruffell/dahdi-linux updated if you want to clone that, but it's nothing official. Third, CONFIG_BKL is the default since it's pulled in from lib/Kconfig.debug.  If you run "make defconfig" in your kernel tree and then look at your .config file, you'll see CONFIG_BKL is defined. Fourth, it's the "unlocked" patch I think we want here, not the locked patch.  And for the dialparams it's not in the hotpath so the most general lock is a mutex, unless there really aren't any allocations or function calls in the code path, in which case I would use a spinlock. Finally, I didn't see what you were referring to as the mess with the function declarations in you unlocked ioctl patch.  It looked good to me (again with the exception that it should still call lock_kernel if CONFIG_BKL is defined, and add some protection for the global dial params). By: Jaco Kroon (jkroon) 2011-01-24 02:58:44.000-0600 Hi, sorry for the delay.  Ran into some other issues. I'm unable to clone the svn HEAD (something regarding OPTIONS request failing), and I'm unsure of how to configure git for proxy use to get to github, so I'm afraid I'll just have to hope the patch I'm about to attach will apply to trunk.  Sorry. Also, the semaphore patches looks good, thanks for pointing those out. I've opted to use a mutex over a spinlock specifically due to calls to printk and kfree in ioctl_load_zone - as far as I understand there are up to four updates to global_dialparams that we want to happen "atomically" so I need to lock over the for(;;) loop, that loop contains printk and kfree calls, whilst there seems to be indications that kfree is in fact OK under spinlock use I don't think the same can be said for the printk. As it stands I've tried to keep the code to NOT use {un,}lock_kernel if HAVE_UNLOCKED_IOCTL doesn't exist, but to use it if we're using the unlocked ioctl and CONFIG_BKL is set (if both HAVE_UNLOCKED_IOCTL and CONFIG_BKL is set).  Additionally, the mutex on global_dialparams is unconditional.  I've used a mutex instead of a spinlock as per the explanation above. By: Shaun Ruffell (sruffell) 2011-01-24 12:28:29.000-0600 No luck on applying to trunk: $ wget 'https://issues.asterisk.org/file_download.php?file_id=28327&type=bug' -O - | patch -p1 patching file drivers/dahdi/dahdi-base.c Hunk #1 FAILED at 47. Hunk #2 succeeded at 289 with fuzz 2 (offset -94 lines). Hunk #3 succeeded at 3160 with fuzz 2 (offset 93 lines). Hunk #4 succeeded at 3286 (offset 89 lines). Hunk ASTERISK-1 FAILED at 4479. Hunk ASTERISK-2 FAILED at 4520. Hunk ASTERISK-3 FAILED at 5841. Hunk ASTERISK-4 succeeded at 6332 with fuzz 1 (offset 433 lines). Hunk ASTERISK-5 succeeded at 6353 (offset 433 lines). Hunk ASTERISK-6 succeeded at 9182 (offset 568 lines). You are only able to grab the sources via tarballs?  "svn co http://svn.asterisk.org/svn/dahdi/linux/trunk dahdi-linux" gives you the OPTIONS request failing error? By: Jaco Kroon (jkroon) 2011-01-25 11:20:45.000-0600 It did ... works now. I've also included some goto fixes in dahdi_ioctl_loadzone that removes a few calls to kfree and instead leaves the free'ing after the error_exit label.  I also noticed three goto's out of the for() loop around which I'm locking, as such I've added a unlock_error_exit which first unlocks the mutex before returning. The single return that I've updated wouldn't cause a memory leak (both z and work was being freed and was as a result of z->name = kasprintf() failure, but my feeling is rather safe than sorry. I also don't see a cleaner way for ensuring the mutex is unlocked than adding the extra unlock_error_exit label. In dahdi_ioctl_set_dialparams I only lock after copy_from_user().  I mention this because gdp = &global_dialparams _before_ I take the lock.  It should be noted that taking the address of global_dialparams doesn't access the struct, thus this is safe. I can confirm that the code compiles both with and without CONFIG_BKL. By: Shaun Ruffell (sruffell) 2011-01-28 13:28:01.000-0600 jkroon: I slightly edited your patch (removed some whitespace errors, added a warning, and made the new mutex static) and uploaded the complete patch (with commit message) If you give me your "Name <email@address.com>" I can make sure that shows up in the first "Signed-off-block" so you will get picked up as the original author in any git-aware clones of the svn repository. Otherwise, if everything looks good to you, I'll just drop that line and commit it. By: Jaco Kroon (jkroon) 2011-01-31 00:53:31.000-0600 Looks good. You can use "Jaco Kroon" <jaco at uls dot co dot za> - highly appreciated. Jaco By: Digium Subversion (svnbot) 2011-01-31 09:53:33.000-0600 Repository: dahdi Revision: 9721 U   linux/trunk/drivers/dahdi/dahdi-base.c ------------------------------------------------------------------------ r9721 | sruffell | 2011-01-31 09:53:32 -0600 (Mon, 31 Jan 2011) | 19 lines dahdi: Experimentally remove dependency on the Big Kernel Lock. With the release of Linux 2.6.37, the Big Kernel Lock is now a compile time option.  This change adds a mutex around the one place in the code that we already knew was dependent on the lock_kernel/unlock_kernel calls for serialization and drops the other calls to lock_kernel/unlock_kernel if CONFIG_BKL is not defined. This is *mostly* the dahdi-no-bkl.patch with a few minor whitespace changes, the global_dialparmslock made static, and a warning added to let people know they are running an experimental configuration. (issue DAHLIN-229) Reported by: jkroon Patches:      dahdi-no-bkl.patch uploaded by jkroon (license 714) Signed-off-by: Jaco Kroon <jaco@uls.co.za> Signed-off-by: Shaun Ruffell <sruffell@digium.com> ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=9721 By: Digium Subversion (svnbot) 2011-01-31 11:59:02.000-0600 Repository: dahdi Revision: 9725 U   linux/branches/2.4/drivers/dahdi/dahdi-base.c ------------------------------------------------------------------------ r9725 | sruffell | 2011-01-31 11:59:02 -0600 (Mon, 31 Jan 2011) | 21 lines dahdi: Experimentally remove dependency on the Big Kernel Lock. With the release of Linux 2.6.37, the Big Kernel Lock is now a compile time option.  This change adds a mutex around the one place in the code that we already knew was dependent on the lock_kernel/unlock_kernel calls for serialization and drops the other calls to lock_kernel/unlock_kernel if CONFIG_BKL is not defined. This is *mostly* the dahdi-no-bkl.patch with a few minor whitespace changes, the global_dialparmslock made static, and a warning added to let people know they are running an experimental configuration. (issue DAHLIN-229) Reported by: jkroon Patches:      dahdi-no-bkl.patch uploaded by jkroon (license 714) Signed-off-by: Jaco Kroon <jaco@uls.co.za> Signed-off-by: Shaun Ruffell <sruffell@digium.com> Origin: http://svnview.digium.com/svn/dahdi?view=rev&rev=9721 ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=9725 By: Shaun Ruffell (sruffell) 2012-04-04 12:42:31.638-0500 Closing this out since this was fixed in 2.4.1 and later releases.