[PATCH v2] uload unused input modules
Peter Hutterer
peter.hutterer at who-t.net
Tue Jul 10 23:46:23 PDT 2012
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 <hramrach at 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 at 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 <hramrach at 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 <hramrach at 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 at lists.x.org
>
> * Add CanUnloadModule
>
> Signed-off-by: Michal Suchanek <hramrach at 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_NONFATAL 0x0001
>
> 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 given module builtin? */
> +int IsBuiltinModule(ModuleDescPtr mod)
> +{
> + return (mod == (ModuleDescPtr) 1);
> +}
> +
> +/* Is given module duplicate of another? */
> +int IsDuplicateModule(ModuleDescPtr mod)
> +{
> + return (mod->TearDownData == ModuleDuplicated);
> +}
> +
> +/* Do duplicates of given module exist? */
> +int IsDuplicated(ModuleDescPtr mod)
> +{
> + /* cannot tell */
> + return 1;
> +}
> +
this really needs a comment that it will be fixed in a later patch (and have
this in the commit message). I was staring hard at this, wondering why you'd
submit a obviously broken patch like this before it came to me that this was
probably fixed in a later patch in this series.
> static void
> UnloadModuleOrDriver(ModuleDescPtr mod)
> {
> - if (mod == (ModuleDescPtr) 1)
> + if (IsBuiltinModule(mod))
> return;
>
> if (mod == NULL || mod->name == NULL)
> @@ -1097,7 +1129,7 @@ UnloadModuleOrDriver(ModuleDescPtr mod)
> else
> xf86MsgVerb(X_INFO, 3, "UnloadModule: \"%s\"\n", mod->name);
>
> - if (mod->TearDownData != ModuleDuplicated) {
> + if (!IsDuplicateModule(mod)) {
> if ((mod->TearDownProc) && (mod->TearDownData))
> mod->TearDownProc(mod->TearDownData);
> LoaderUnload(mod->name, mod->handle);
> --
> 1.7.10
>
> From a2bdabbe9e5610b6c0ad80fb3659039da8800970 Mon Sep 17 00:00:00 2001
> From: Michal Suchanek <hramrach at gmail.com>
> Date: Mon, 4 Jun 2012 17:29:37 +0200
> Subject: [PATCH 3/4] xfree86: unload unused input drivers
> To: xorg-devel at lists.x.org
>
> Signed-off-by: Michal Suchanek <hramrach at gmail.com>
> ---
> hw/xfree86/common/xf86Helper.c | 22 +++++++++++++++++++++-
> hw/xfree86/common/xf86Xinput.c | 4 ++++
> hw/xfree86/common/xf86Xinput.h | 2 +-
> 3 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/hw/xfree86/common/xf86Helper.c b/hw/xfree86/common/xf86Helper.c
> index 5ef1dab..7bf48bf 100644
> --- a/hw/xfree86/common/xf86Helper.c
> +++ b/hw/xfree86/common/xf86Helper.c
> @@ -127,12 +127,32 @@ xf86AddInputDriver(InputDriverPtr driver,
> pointer module, int flags)
> }
>
> void
> -xf86DeleteInputDriver(int drvIndex)
> +xf86DeleteInputDriver(InputDriverPtr drv)
> {
> + int drvIndex;
> + for (drvIndex = 0; drvIndex < xf86NumInputDrivers; drvIndex ++)
> + if (xf86InputDriverList[drvIndex] == drv) break;
linebreaks before break statements please.
> + if (drvIndex >= xf86NumInputDrivers)
> + return; /* not found */
can we actually unload all modules before removing a device?
this seems unlikely and should probably be flagged with BUG_WARN
> +
> + /* The loader does not return error from UnloadModule so check that the
> + * driver can be unloaded. */
can't we chance UnloadModule to return true/false? if it's void at the
moment, we don't even need to worry about the existing callers too much.
plus, it would make the CanUnloadModule() internal to the loader, making it
nicer for the caller.
> + if (xf86InputDriverList[drvIndex] && xf86InputDriverList[drvIndex]->module
> + &&
> !CanUnloadModule((ModuleDescPtr)xf86InputDriverList[drvIndex]->module))
> + return; /* cannot unload (yet) */
we can just use drv->module or a temporary variable? the first part cannot
be true either except if drv is NULL which should probably be checked
separately. so this condition can be simplified to
if (drv->module && !CanUnloadModule(drv->module))
return;
> +
> + /* Remove the driver */
> if (xf86InputDriverList[drvIndex] && xf86InputDriverList[drvIndex]->module)
> UnloadModule(xf86InputDriverList[drvIndex]->module);
> free(xf86InputDriverList[drvIndex]);
same as above, we can use drv->module here.
> xf86InputDriverList[drvIndex] = NULL;
superfluous statement since we always move it up unless it's the last, in
which case we set it to NULL anyway
> +
> + /* Compact the list of input drivers */
> + if (drvIndex + 1 < xf86NumInputDrivers)
> + memmove(xf86InputDriverList[drvIndex], xf86InputDriverList[drvIndex+1],
> + sizeof(xf86InputDriverList[drvIndex]) *
> (xf86NumInputDrivers - drvIndex - 1));
> + xf86NumInputDrivers--;
> + xf86InputDriverList[xf86NumInputDrivers] = NULL;
> }
>
> InputDriverPtr
> diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
> index bee407b..ff3b5e9 100644
> --- a/hw/xfree86/common/xf86Xinput.c
> +++ b/hw/xfree86/common/xf86Xinput.c
> @@ -753,6 +753,10 @@ xf86DeleteInput(InputInfoPtr pInp, int flags)
> if (pInp->module)
> UnloadModule(pInp->module);
>
> + /* Attempt to unload the driver */
> + if (pInp->drv)
> + xf86DeleteInputDriver(pInp->drv);
> +
> /* This should *really* be handled in drv->UnInit(dev) call instead, but
> * if the driver forgets about it make sure we free it or at least crash
> * with flying colors */
> diff --git a/hw/xfree86/common/xf86Xinput.h b/hw/xfree86/common/xf86Xinput.h
> index 1d4363a..12ce0be 100644
> --- a/hw/xfree86/common/xf86Xinput.h
> +++ b/hw/xfree86/common/xf86Xinput.h
> @@ -180,7 +180,7 @@ InputInfoPtr xf86AllocateInput(void);
> /* xf86Helper.c */
> extern _X_EXPORT void xf86AddInputDriver(InputDriverPtr driver, pointer module,
> int flags);
> -extern _X_EXPORT void xf86DeleteInputDriver(int drvIndex);
> +extern _X_EXPORT void xf86DeleteInputDriver(InputDriverPtr drv);
> extern _X_EXPORT InputDriverPtr xf86LookupInputDriver(const char *name);
> extern _X_EXPORT InputInfoPtr xf86LookupInput(const char *name);
> extern _X_EXPORT void xf86DeleteInput(InputInfoPtr pInp, int flags);
> --
> 1.7.10
>
> From 818dd1317dcd356092b00749085ba1b43743e45a Mon Sep 17 00:00:00 2001
> From: Michal Suchanek <hramrach at gmail.com>
> Date: Mon, 4 Jun 2012 16:46:03 +0200
> Subject: [PATCH 4/4] xfree86/loader: add back silly reference counting. (and
> move it a layer higher)
> To: xorg-devel at lists.x.org
>
> ab7f057ce removes reference counting of loaded objects.
>
> However, DuplicateModule is used extensively in X server to create
> copies of modules. Without the reference counting layer DuplicateModule
> cannot call LoaderOpen since at least glibc will happily realod
> (possibly different) object from disk, and DuplicateModule does not
> perform version checks nor module initialization.
>
> To make it possible to unload module safely a duplicate count should be
> tracked so that duplicated modules that are possibly still in use
> somewhere are not uninitialized. Calling UnloadModule on a duplicated
> module has no effect but no error is returned as the loader interface
> does not allow for that.
>
> Signed-off-by: Michal Suchanek <hramrach at gmail.com>
> ---
> hw/xfree86/loader/loaderProcs.h | 2 ++
> hw/xfree86/loader/loadmod.c | 30 ++++++++++++++++++------------
> 2 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/hw/xfree86/loader/loaderProcs.h b/hw/xfree86/loader/loaderProcs.h
> index c33039a..bdd1328 100644
> --- a/hw/xfree86/loader/loaderProcs.h
> +++ b/hw/xfree86/loader/loaderProcs.h
> @@ -66,6 +66,8 @@ typedef struct module_desc {
> ModuleTearDownProc TearDownProc;
> void *TearDownData; /* returned from SetupProc */
> const XF86ModuleVersionInfo *VersionInfo;
> + struct module_desc *original;
> + int duplicate_count;
> } ModuleDesc, *ModuleDescPtr;
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 previous patch.
> /* External API for the loader */
> diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c
> index 4844e18..d3ab51b 100644
> --- a/hw/xfree86/loader/loadmod.c
> +++ b/hw/xfree86/loader/loadmod.c
> @@ -93,8 +93,6 @@ const ModuleVersions LoaderVersionInfo = {
> ABI_FONT_VERSION
> };
>
> -static int ModuleDuplicated[] = { };
> -
> static void
> FreeStringList(char **paths)
> {
> @@ -803,8 +801,11 @@ NewModuleDesc(const char *name)
> return mdp;
> }
>
> -/* An advisory function. When true is returned unloading the module should be
> - * safe. Uload can be attempted nonetheless. */
> +static ModuleDescPtr ModuleGetOriginal(ModuleDescPtr mod)
> +{
> + return (mod->original ? mod->original : mod);
> +}
> +
> int CanUnloadModule(ModuleDescPtr mod)
> {
> if (IsBuiltinModule(mod))
> @@ -829,10 +830,11 @@ DuplicateModule(ModuleDescPtr mod, ModuleDescPtr parent)
> return NULL;
>
> ret->handle = mod->handle;
> -
> + /* some code may check the procs are the same */
> ret->SetupProc = mod->SetupProc;
> ret->TearDownProc = mod->TearDownProc;
> - ret->TearDownData = ModuleDuplicated;
> + ret->original = ModuleGetOriginal(mod);
> + ret->original->duplicate_count++;
please no manual refcounting. add a helper function to do this for you, but
in my experience refcounting always goes wrong sooner or later.
make it so that the ref/unref functions return the current refcount, so you
can then write code in the form of
if (UnrefModule() == 0)
delete module data
> ret->child = DuplicateModule(mod->child, ret);
> ret->sib = DuplicateModule(mod->sib, parent);
> ret->parent = parent;
> @@ -1105,31 +1107,35 @@ int IsBuiltinModule(ModuleDescPtr mod)
> /* Is given module duplicate of another? */
> int IsDuplicateModule(ModuleDescPtr mod)
> {
> - return (mod->TearDownData == ModuleDuplicated);
> + return (mod->original != NULL);
> }
>
> /* Do duplicates of given module exist? */
> int IsDuplicated(ModuleDescPtr mod)
> {
> - /* cannot tell */
> - return 1;
> + return (mod->duplicate_count > 0);
> }
shouldn't this check mod->original->duplicate_count as well?
afaict UnloadModuleOrDriver can be called with duplicated modules.
> static void
> UnloadModuleOrDriver(ModuleDescPtr mod)
> {
> - if (IsBuiltinModule(mod))
> + if (mod == NULL || mod->name == NULL)
> return;
>
> - if (mod == NULL || mod->name == NULL)
> + if (!CanUnloadModule(mod)) {
> + if (IsDuplicated(mod))
> + xf86MsgVerb(X_WARNING, 3, "Cannot unload duplicated
> module: \"%s\"\n", mod->name);
> return;
> + }
X_INFO is enough? not really an error, this would be called during normal
server operation.
>
> if (mod->parent)
> xf86MsgVerb(X_INFO, 3, "UnloadSubModule: \"%s\"\n", mod->name);
> else
> xf86MsgVerb(X_INFO, 3, "UnloadModule: \"%s\"\n", mod->name);
>
> - if (!IsDuplicateModule(mod)) {
> + if (IsDuplicateModule(mod)) {
> + ModuleGetOriginal(mod)->duplicate_count--;
> + } else {
> if ((mod->TearDownProc) && (mod->TearDownData))
> mod->TearDownProc(mod->TearDownData);
> LoaderUnload(mod->name, mod->handle);
as said above, this would be nicer as'
if (UnrefModule(mod) == 0)
teardown..
Cheers,
Peter
More information about the xorg-devel
mailing list