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

[Stephen Smalley]

On 2/4/20 10:01 AM, Ondrej Mosnacek wrote:
> On Fri, Jan 17, 2020 at 8:11 PM Stephen Smalley <sds@xxxxxxxxxxxxx> 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,
> > > +};