Re: [PATCH RFC] selinux: policydb - convert filename trans hash to rhashtable

[Stephen Smalley <sds@tycho.nsa.gov>]

On 2/4/20 10:01 AM, Ondrej Mosnacek wrote:
> On Fri, Jan 17, 2020 at 8:11 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> On 1/16/20 4:39 PM, Lucas Stach wrote:
>>> The current hash is too small for current usages in, e.g. the Fedora standard
>>> policy. On file creates a considerable amount of CPU time is spent walking the
>>> the hash chains. Increasing the number of hash buckets somewhat mitigates the
>>> issue, but doesn't completely get rid of the long hash chains.
>>>
>>> This patch does take the bit more invasive route by converting the filename
>>> trans hash to a rhashtable to allow this hash to scale with load.
>>>
>>> fs_mark create benchmark on a SSD device, no ramdisk:
>>> Count          Size       Files/sec     App Overhead
>>> before:
>>> 10000          512        512.3           147715
>>> after:
>>> 10000          512        572.3            75141
>>>
>>> filenametr_cmp(), which was the topmost function in the CPU cycle trace before
>>> at ~5% of the overall CPU time, is now down in the noise.
>>
>> Thank you for working on this.  IMHO, Fedora overuses name-based type
>> transitions but that's another topic. I haven't yet investigated the
>> root cause but with your patch applied, I see some test failures related
>> to name-based transitions:
>>
>> [...]
>> #   Failed test at overlay/test line 439.
>> overlay/test ................ 114/119 # Looks like you failed 1 test of 119.
>> [...]
>> filesystem/test ............. 3/70 File context error, expected:
>>          test_filesystem_filenametranscon1_t
>> got:
>>          test_filesystem_filetranscon_t
>>
>> #   Failed test at filesystem/test line 279.
>> File context error, expected:
>>          test_filesystem_filenametranscon2_t
>> got:
>>          test_filesystem_filetranscon_t
>>
>> #   Failed test at filesystem/test line 286.
>> filesystem/test ............. 68/70 # Looks like you failed 2 tests of 70.
>>
>> You can reproduce by cloning the selinux-testsuite from
>> https://github.com/SELinuxProject/selinux-testsuite, applying the
>> filesystem tests patch from
>> https://patchwork.kernel.org/patch/11337659/,
>> and following the README.md instructions.
> 
> I think I figured out what's wrong - see below.
> <snip>
>>> @@ -441,6 +442,39 @@ static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2)
>>>
>>>    }
>>>
>>> +static const struct rhashtable_params filename_trans_hashparams = {
>>> +     .nelem_hint             = 1024,
>>> +     .head_offset            = offsetof(struct filename_trans, hash_head),
> 
> You need to add:
> 
> +.hashfn = filenametr_hash,
> 
> here so that the key is correctly hashed on lookup. After applying
> this fix, the selinux-testuite passes for me with this patch.

Hmm..does that then make the .obj_hashfn assignment below unnecessary or 
is that required too?  I'm unclear on the difference.

> 
>>> +     .obj_hashfn             = filenametr_hash,
>>> +     .obj_cmpfn              = filenametr_cmp,
>>> +};