[Regression] dix: Fix overzealous caching of ResourceClientBits()

Commit c7311654 cached the value of ResourceClientBits(), but that value depends on the MaxClients value set either from the command line or from the configuration file.

For the latter, a call to ResourceClientBits() is issued before the configuration file is read, meaning that the cached value is from the default, not from the maximum number of clients set in the configuration file.

That obviously causes all sort of issues, including memory corruption and crashes of the Xserver when reaching the default limit value.

To avoid that issue, also keep the LimitClient value, and recompute the ilog2() value if that changes, as on startup when the value is set from the the xorg.conf ServerFlags section.

Fixes: c7311654 - dix: cache ResourceClientBits() value
Closes: #1310 (closed)
Signed-off-by: Olivier Fourdan ofourdan@redhat.com

assigned to @ofourdan

added 1 commit

• c7c431d5 - dix: Fix overzealous caching of ResourceClientBits()

Compare with previous version

changed title from [Regression] Revert "dix: cache ResourceClientBits() value" to [Regression] dix: Fix overzealous caching of ResourceClientBits()

changed the description

v2: Do not revert, but try to be smarter instead, and cache the limit value so that when it changes, we recompute the cached value

added dix label

mentioned in issue #1310 (closed)

Haven't tested yet, but isn't ilog2 a single instruction on modern CPUs? Is there actually any benefit to caching it? (I would wonder if the cached version ends up being slower in practice...)

At the very least, the check for cached==0 is unnecessary now.

Looks like the ilog2 in resource.c is exceedingly inefficient. This will do it better with GCC (maybe want to guard it with a configure check?):

unsigned int
ilog2(int val)
{
    if (val <= 0)
        return 0;
    static const int offset = (sizeof(val) * 8) - 1;
    return offset - __builtin_clz(val);
}

Note that ResourceClientBits appears to be the only consumer of ilog2, so you could inline it and drop the val <= 0 check too. Then it's a single instruction (x86) or two (PPC). No need to cache.

Or if you really don't want to calculate it here, maybe just add a LimitClientsBits in parallel to LimitClients, and calculate it once in configServerFlags after the value is initialised?

added 1 commit

• 64358260 - dix: Fix overzealous caching of ResourceClientBits()

Compare with previous version

added 3 commits

• 64358260...6907b6ea - 2 commits from branch xorg:master
• 2645ec46 - dix: Fix overzealous caching of ResourceClientBits()

Compare with previous version

cc @ajax and @p12tic for review - fwiw, I reproduced the issue (affecting Xorg) and the patch fixes the regression.

resolved all threads

If you're only trying to recompute this on server reset then using serverGeneration for the trigger is maybe a little clearer (there's plenty of examples in-tree of how it's used). Which is probably the right thing, here, because changing the client limit dynamically would mean the XID space per client is no longer fixed among clients, which absolutely nothing is prepared for, but since there's no actual mechanism to make that change at runtime, meh.

With or without any of that addressed,

Reviewed-by: Adam Jackson <ajax@redhat.com>

added 6 commits

• 2645ec46...24d7d93f - 5 commits from branch xorg:master
• 2efa6d65 - dix: Fix overzealous caching of ResourceClientBits()

Compare with previous version

resolved all threads

enabled an automatic merge when the pipeline for 2efa6d65 succeeds

merged

mentioned in merge request !181