On Wed, Jul 8, 2020 at 3:38 PM Stephen Smalley <stephen.smalley.work@xxxxxxxxx> wrote: > On Wed, Jul 8, 2020 at 7:24 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > > > Refactor searching and inserting into hashtabs to pave the way for > > converting hashtab_search() and hashtab_insert() to inline functions in > > the next patch. This will avoid indirect calls and allow the compiler to > > better optimize individual callers, leading to a significant performance > > improvement. > > > > In order to avoid the indirect calls, the key hashing and comparison > > callbacks need to be extracted from the hashtab struct and passed > > directly to hashtab_search()/_insert() by the callers so that the > > callback address is always known at compile time. The kernel's > > rhashtable library (<linux/rhashtable*.h>) does the same thing. > > > > This of course makes the hashtab functions slightly easier to misuse by > > passing a wrong callback set, but unfortunately there is no better way > > to implement a hash table that is both generic and efficient in C. This > > patch tries to somewhat mitigate this by only calling the hashtab > > functions in the same file where the corresponding callbacks are > > defined (wrapping them into more specialized functions as needed). > > > > Note that this patch doesn't bring any benefit without also moving the > > definitions of hashtab_search() and -_insert() to the header file, which > > is done in a follow-up patch for easier review of the hashtab.c changes > > in this patch. > > > > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx> > > --- > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > index 02b722c5c189d..ae78f66e85d29 100644 > > --- a/security/selinux/ss/policydb.c > > +++ b/security/selinux/ss/policydb.c > > @@ -1888,7 +1920,7 @@ static int filename_trans_read_helper_compat(struct policydb *p, void *fp) > > otype = le32_to_cpu(buf[3]); > > > > last = NULL; > > - datum = hashtab_search(&p->filename_trans, &key); > > + datum = hashtab_search(&p->filename_trans, &key, filenametr_key_params); > > Why aren't you using the helper/wrapper function here? Oversight on my part, thanks for spotting it! I'll wait for Paul's feedback and send a fixed v4 if needed. -- Ondrej Mosnacek Software Engineer, Platform Security - SELinux kernel Red Hat, Inc.
