Summary:	DAHLIN-00039: [patch] safer MMX support for echo canceller
Reporter:	Tzafrir Cohen (tzafrir)	Labels:
Date Opened:	2008-09-17 04:38:12	Date Closed:	2009-11-09 13:51:43.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	dahdi (the module)
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) dahdi_mmx_auto.diff ( 1) dahdi_mmx_fix.diff ( 2) zaptel-mmx_fix.diff
Description:	ZAPTEL_CONFIG_MMX has had a reputation for being a nice performance improvement (e.g: by 50% and more), but OTOH also for causing "strange things". This is because the the floating point context is not properly saved and using MMX (uses the floating point registers) in an interrupt context may result in corrupting them to processes. This patch aims to properly fix that. It does so in an ugly and i386-specific way (will not even work on x86_64), but it should be a good starting point. It replaces not only kernel_fpu_begin but also kernel_fpu_end . There is some legacy 2.4-only reimplementation of kernel_fpu_begin() which I chose not to touch. I also added a compilation warning for the case of using CONFIG_ZAPTEL_MMX without this fix, as I suspect it is not safe. The initial patch is vs. zaptel, as I have no relevant EC to test this with on DAHDI. A dahdi-linux patch is also included, but not actually tested.
Comments:	By: Leif Madsen (lmadsen) 2009-02-27 16:50:40.000-0600 Setting this to confirmed as it is not assigned to anyone, but there are patches. Perhaps it makes sense to put them on reviewboard? By: Tzafrir Cohen (tzafrir) 2009-04-21 04:12:26 If MMX support is safe, enable it by default if supported. Patch dahdi_mmx_auto.diff restores the magic from the zaptel Kbuild file that was used to selectively enable MMX support in some cases. It was unused due to the MMX support being broken and was thus removed for the sake of simplicity. By: Tzafrir Cohen (tzafrir) 2009-05-18 07:41:23 This patch adds a (module-wide) static buffer and allows access to it in interrupt context. This will generally work well assuming all your interrupts are served by a single CPU. Luckily this is often the case on quite a few useful setups. However it is not something one would like to rely on. Thus the single buffer will have to be replaced with a per-CPU buffer. By: Tzafrir Cohen (tzafrir) 2009-05-20 12:44:35 The attached patch replaces the static buffer with a per-CPU one. Seems good. It is genenally something I'd like to merge. I'd like to merge this as soon as 2.2.0 is released. As a side note, it fails to build with x86_64. By: Tzafrir Cohen (tzafrir) 2009-05-25 04:27:05 Re-uploaded the patch without some extra local changes that snuck in. To clarify the last note: "Fails to build with x86_64" is the reason why the patch limits itself to 32bits x86. explicitly. By: Matt Riddell (zx81) 2009-05-26 00:20:17 Is there any possibility of making it work with x86_64? By: Tzafrir Cohen (tzafrir) 2009-05-26 01:55:53 Not sure when exactly. There are currently issues with higher priority (that is to say: patches are always welcomed). The ammount of code needed is not that quite small and contained. Getting it to actually build (with the ifdef limitations lifted) would be a good start. By: Tzafrir Cohen (tzafrir) 2009-06-07 10:39:45 Updated the Zaptel version for those who still need it. By: Digium Subversion (svnbot) 2009-06-29 15:05:44 Repository: dahdi Revision: 6791 U   linux/trunk/drivers/dahdi/dahdi-base.c ------------------------------------------------------------------------ r6791 | kpfleming | 2009-06-29 15:05:43 -0500 (Mon, 29 Jun 2009) | 12 lines Improve MMX safety for DAHDI echo cancellers on 32-bit x86 systems. Replaces the standard kernel FPU save/restore operations with custom written versions for 32-bit x86 CPUs, which have been tested to be reliable and safe to use. (closes issue DAHLIN-39) Reported by: tzafrir Patches:      dahdi_mmx_fix.diff uploaded by tzafrir (license 46) ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=6791 By: Kevin P. Fleming (kpfleming) 2009-06-29 15:08:05 I've committed the primary part of this issue; I did not commit the 'auto MMX' patch, because none of the current DAHDI echocans use MMX, and there's no reason to burden a user's system with saving/restoring registers that won't be clobbered anyway. If you want to pursue that patch open another issue and post a patch that adds a bit to dahdi_echocan_features that indicates whether the echocan uses MMX or not; then it would be safe to automatically enable save/restore iff the echocan is going to clobber the registers. By: Digium Subversion (svnbot) 2009-11-09 13:51:41.000-0600 Repository: dahdi Revision: 7541 _U  linux/branches/2.2/ ------------------------------------------------------------------------ r7541 | sruffell | 2009-11-09 13:51:41 -0600 (Mon, 09 Nov 2009) | 48 lines Blocked revisions 6791,6794,6797,6926,6967 via svnmerge ........  r6791 | kpfleming | 2009-06-29 15:05:43 -0500 (Mon, 29 Jun 2009) | 12 lines    Improve MMX safety for DAHDI echo cancellers on 32-bit x86 systems.    Replaces the standard kernel FPU save/restore operations with custom written  versions for 32-bit x86 CPUs, which have been tested to be reliable and safe  to use.    (closes issue DAHLIN-39)  Reported by: tzafrir  Patches:        dahdi_mmx_fix.diff uploaded by tzafrir (license 46) ........  r6794 | sruffell | 2009-06-29 15:56:07 -0500 (Mon, 29 Jun 2009) | 9 lines    dahdi-base: Enable DAHDI to manage the reference counts for the board drivers.    Adds a struct module 'owner' member to the dahdi_span structure and updates  all the board drivers to set this member before registering the span.  This  allows the core of dahdi to maintain the reference counts on the channels  itself.    (closes issue DAHLIN-9)  Reported by: Matti ........  r6797 | sruffell | 2009-06-29 17:44:54 -0500 (Mon, 29 Jun 2009) | 3 lines    wcte11xp: Set the owner field of the dahdi_span before registration.    (related to issue DAHLIN-9) ........  r6926 | sruffell | 2009-08-04 11:22:17 -0500 (Tue, 04 Aug 2009) | 1 line    wctdm24xxp: We no longer need to keep our own reference count. ........  r6967 | tzafrir | 2009-08-11 23:49:47 -0500 (Tue, 11 Aug 2009) | 7 lines    xpp: Don't check for the 'owner' field too soon    The owner field should only tested after the low-level driver  pre-registration method is called.    xpp rev: 7287 ........ ------------------------------------------------------------------------ http://svn.digium.com/view/dahdi?view=rev&revision=7541