[PATCH v2] uload unused input modules

[Michal Suchanek]

On 11 July 2012 08:50, Peter Hutterer <peter.hutterer at 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.hutterer at 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 previous patch.

It's not like anybody sees the input driver list, anyway.

>> 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.

yes, only a bisection point really, should not change anything,
including the inability to unload input driver modules.

>
>> +
>> +    /* 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.

That would be a possible API/ABI change, though.
Overall, the caller that wants to know has to check one way or another.

>
>> +    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;

Yes, not very nice functions.

Should get better with using xorg list I guess.

>>
>>      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

You need the refcounting to verify that xf86Helper is doing the right
thing. You can refcount here or there but no way around that. Given
the way the actual loader works you have to ensure only one handle is
open for a module no matter how many times it is used. Otherwise you
can get a mess of different modules in different states in the X
server as you open, close, and reopen without checking what module you
are getting.

>
>>      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.

The duplicates can be unloaded at any time, they are just description
structure which is freed on 'unload'.

The original has to be kept around so long as duplicates exist.

>
>>  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.

No, it wouldn't, and it would be a bug if it were.

You could hide the original module in the loder and only hand out
duplicates if you wish to straighten the interface a bit but then the
input driver list relying on original modules would have to ask the
loader how many duplicates there are when trying to remove from list.

>
>>
>>      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..

No, that is totally not the way it works.
A module with actual teardown data cannot be unrefed, but the teardown
cannot be done when duplicates do exist (as in this initialization of
the module is still referenced).

If you want to straighten the interface even more specify that module
initialization is supposed to be re-entrant and return unique teardown
data when teardown actually does something. Just perform full
open/probe/init on duplication then. And audit duplication failure
handling in all of Xorg and drivers, currently it can basically fail
only when allocating a module description fails, very unlikely ;-)

Thanks

Michal