On Wed, Jul 8, 2020 at 10:37 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote: > > 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. I remain not overly excited about these patches as I fear they make the code more prone to error, but they are in a reasonably important code path so I'll turn a deaf ear to my objections once you make the changes Stephen is requesting. -- paul moore www.paul-moore.com
