Re: [PATCH v2] uload unused input modules
Hello, trying to send updated patches again. There is not much actual code in common due to the use of the list macros in this new series. Thanks Michal ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2] uload unused input modules
On 11 July 2012 08:46, Peter Hutterer peter.hutte...@who-t.net wrote: On Tue, Jul 03, 2012 at 12:45:21PM +0200, Michal Suchanek wrote: Signed-off-by: Michal Suchanek hramr...@gmail.com --- hw/xfree86/loader/loadmod.c | 19 --- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c index 706b9b3..e94265a 100644 --- a/hw/xfree86/loader/loadmod.c +++ b/hw/xfree86/loader/loadmod.c @@ -1103,10 +1103,23 @@ UnloadModuleOrDriver(ModuleDescPtr mod) LoaderUnload(mod-name, mod-handle); } -if (mod-child) +while (mod-child) UnloadModuleOrDriver(mod-child); -if (mod-sib) -UnloadModuleOrDriver(mod-sib); + +if (mod-parent) { +ModuleDescPtr sib_list = mod-parent-child; +if (sib_list == mod) { +mod-parent-child = mod-sib; +} else { +while (sib_list) { +if (sib_list-sib == mod) { +sib_list-sib = mod-sib; +break; +} +sib_list = sib_list-sib; +} +} we've got a bunch of macros to deal with null-terminated lists, so I've changed the second part to: if (mod-parent) nt_list_del(mod, mod-parent-child, ModuleDesc, sib) which does the same thing as above. read as delete mod from mod-parent-child, mod is of type ModuleDesc, sib is the pointer in mod to point to the next element. applied otherwise, thanks. Too early. There is a RemoveChild that does just this, with another copy of this code. Going to send an updated patchset when I get to looking at the others, too. Thanks Michal ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2] uload unused input modules
On 11 July 2012 08:50, Peter Hutterer peter.hutte...@who-t.net wrote: On Mon, Jul 02, 2012 at 03:30:02PM +0200, Michal Suchanek wrote: urgh, no. why does the caller need to care about the module count at all? the input code should just call unload and let the loader sort it out. urgh, yes. The input code needs to know when to delete the driver from its list and the loader does not return anything. I could possibly make it return something different when this check fails but I am not sure it has any nice semantics. The close on the actual object handle can fail also and then the module is already uninitialized, and unloading child submodules can fail, too. the bits your changing already change the ABI, so you've got (more or less) free range with the API as well. shape it into something that makes sense. would be a novel thing for this bit of code, anyways ;) As I understand from the discussion of this patches and the other one about API/ABI this is actually not a change to either. The ModuleDesc structure is extended but it needs to be allocated using a loader function. The functions that are the supposed outside interface even cast pointer to these to void pointers. The added CanUnloadModule is used in the interface between xf86Helper and the loader, and there is no real API there, it's internal to the X server and very interweawed. if (xf86InputDriverList[drvIndex] xf86InputDriverList[drvIndex]-module) UnloadModule(xf86InputDriverList[drvIndex]-module); free(xf86InputDriverList[drvIndex]); xf86InputDriverList[drvIndex] = NULL; +if (drvIndex + 1 xf86NumInputDrivers) +memmove(xf86InputDriverList[drvIndex], xf86InputDriverList[drvIndex+1], +sizeof(xf86InputDriverList[drvIndex]) * (xf86NumInputDrivers - drvIndex - 1)); +xf86NumInputDrivers--; +xf86InputDriverList[xf86NumInputDrivers] = NULL; this sounds like a prime target for a xorg_list switch to avoid this code. The loader sibling list even more so. Still the input device list is substantially simpler structure than xorg_list with the only downside that deletion is somewhat hairy. Perhaps another one is that xorg_list has tests but the input device list does not. that's my main argument. we've found quite a few bugs with various open-coded lists (or manual list resizing) that just using the macros is much safer. the nt_list_* macros for already existing, null-terminated lists and the xorg_list_* macros for anything new. The xf86InputDriverList is defined in xf86Globals but it seems to be internal to the xf86Helper.c functions so it can be safely changed to something else I guess. It is a list of pointers, and the only function that took index into the list rather than one of the pointers stored in the list was unused. It is mentioned in ddxDesign, though. Supposedly you can build a static X server where these arrays are built at compile time. Is that still true? I don't see the code for it. On 11 July 2012 08:46, Peter Hutterer peter.hutte...@who-t.net wrote: On Tue, Jul 03, 2012 at 12:45:21PM +0200, Michal Suchanek wrote: Hello, since the patches got lost somewhere without even generating an error sending as plain email. Changes since v1: - split out module 'query' functions - rename some variables - fix some whitespace and braces Thanks the patches got mangled again :( I could copy/paste the first one into git am directly, number 3 is definitely busted though. I can send a link to a git branch along with the patches in the future but sending bulk emails obviously does not work, likely due to spam prevention measures. looks a lot better, thanks. I even think actually understand most of it, though the loader is a bit obtuse :) I'm still not a big fan of the API, especially the one exposed to bits outside the loader. I think there's still a bit of room for improvement. What we do need for this patch series is a set of simple tests that make sure module loading and unloading works as before and doesn't mangle any pointers or other bits. I _think_ this should be possible to add by declaring a couple of structs and passing them through the various LoadModule/UnloadModule. should help us to find potential memory leaks too. The thing is the patch only formalizes an internal api between xf86Helper and the loader. The input driver list only contains the original modules (in the past never unloaded) and devices only get duplicates (unloaded on device teardown, at least recently). Some tests with a dummy loader might be nice but I am not proficient with gtest or what the x server uses and writing a dummy loader would take some time. It should probably include tests with module version info, the version check code is very weird. if you're changing the module ABI anyway, we can change the InputDriverRec too and add a struct list so we don't need to do the memmove/drvIndex dance in the
Re: [PATCH v2] uload unused input modules
On Tue, Jul 03, 2012 at 12:45:21PM +0200, Michal Suchanek wrote: Hello, since the patches got lost somewhere without even generating an error sending as plain email. Changes since v1: - split out module 'query' functions - rename some variables - fix some whitespace and braces Thanks the patches got mangled again :( I could copy/paste the first one into git am directly, number 3 is definitely busted though. looks a lot better, thanks. I even think actually understand most of it, though the loader is a bit obtuse :) I'm still not a big fan of the API, especially the one exposed to bits outside the loader. I think there's still a bit of room for improvement. What we do need for this patch series is a set of simple tests that make sure module loading and unloading works as before and doesn't mangle any pointers or other bits. I _think_ this should be possible to add by declaring a couple of structs and passing them through the various LoadModule/UnloadModule. should help us to find potential memory leaks too. From a86d41ee113f76776a9410205ef6f90c78e41d22 Mon Sep 17 00:00:00 2001 From: Michal Suchanek hramr...@gmail.com Date: Mon, 4 Jun 2012 14:22:14 +0200 Subject: [PATCH 1/4] xfree86/loader: Do not unload sibling modules. To: xorg-devel@lists.x.org Ordering of sibling modules is internal to the loader implementation. Unloading siblings following the requested module in the sibling list leads to unexpected unloading of seemingly random modules. Signed-off-by: Michal Suchanek hramr...@gmail.com --- hw/xfree86/loader/loadmod.c | 19 --- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c index 706b9b3..e94265a 100644 --- a/hw/xfree86/loader/loadmod.c +++ b/hw/xfree86/loader/loadmod.c @@ -1103,10 +1103,23 @@ UnloadModuleOrDriver(ModuleDescPtr mod) LoaderUnload(mod-name, mod-handle); } -if (mod-child) +while (mod-child) UnloadModuleOrDriver(mod-child); -if (mod-sib) -UnloadModuleOrDriver(mod-sib); + +if (mod-parent) { +ModuleDescPtr sib_list = mod-parent-child; +if (sib_list == mod) { +mod-parent-child = mod-sib; +} else { +while (sib_list) { +if (sib_list-sib == mod) { +sib_list-sib = mod-sib; +break; +} +sib_list = sib_list-sib; +} +} we've got a bunch of macros to deal with null-terminated lists, so I've changed the second part to: if (mod-parent) nt_list_del(mod, mod-parent-child, ModuleDesc, sib) which does the same thing as above. read as delete mod from mod-parent-child, mod is of type ModuleDesc, sib is the pointer in mod to point to the next element. applied otherwise, thanks. +} free(mod-path); free(mod-name); free(mod); -- 1.7.10 From 04aee58b6b497613a9b0c2c9253cfbcbc3f7e249 Mon Sep 17 00:00:00 2001 From: Michal Suchanek hramr...@gmail.com Date: Mon, 2 Jul 2012 10:54:53 +0200 Subject: [PATCH 2/4] xfree86/loader: Split off some loader code into separate query functions. To: xorg-devel@lists.x.org * Add CanUnloadModule Signed-off-by: Michal Suchanek hramr...@gmail.com --- hw/xfree86/loader/loaderProcs.h |5 + hw/xfree86/loader/loadmod.c | 36 ++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/hw/xfree86/loader/loaderProcs.h b/hw/xfree86/loader/loaderProcs.h index a7b752b..c33039a 100644 --- a/hw/xfree86/loader/loaderProcs.h +++ b/hw/xfree86/loader/loaderProcs.h @@ -88,6 +88,11 @@ unsigned long LoaderGetModuleVersion(ModuleDescPtr mod); void LoaderResetOptions(void); void LoaderSetOptions(unsigned long); +int IsBuiltinModule(ModuleDescPtr mod); +int IsDuplicated(ModuleDescPtr mod); +int IsDuplicateModule(ModuleDescPtr mod); +int CanUnloadModule(ModuleDescPtr mod); + /* Options for LoaderSetOptions */ #define LDR_OPT_ABI_MISMATCH_NONFATAL0x0001 diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c index e94265a..4844e18 100644 --- a/hw/xfree86/loader/loadmod.c +++ b/hw/xfree86/loader/loadmod.c @@ -803,6 +803,19 @@ NewModuleDesc(const char *name) return mdp; } +/* An advisory function. When true is returned unloading the module should be + * safe. Uload can be attempted nonetheless. */ +int CanUnloadModule(ModuleDescPtr mod) +{ +if (IsBuiltinModule(mod)) +return 0; +if (IsDuplicated(mod)) +return 0; +/* Neither of the above can be true for modules for which IsDuplicateModule + * is true. */ +return 1; +} + ModuleDescPtr DuplicateModule(ModuleDescPtr mod, ModuleDescPtr parent) { @@ -1083,10 +1096,29 @@ UnloadModule(pointer mod) UnloadModuleOrDriver((ModuleDescPtr) mod); } +/* Is