diff mbox series

[v2,2/3] selinux: prepare for inlining of hashtab functions

Message ID 20200504115923.88828-3-omosnace@redhat.com (mailing list archive)
State Superseded
Headers show
Series Inline some hashtab functions to improve performance | expand

Commit Message

Ondrej Mosnacek May 4, 2020, 11:59 a.m. UTC 
Refactor searching and inserting into hashtabs to pave 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 drastic 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 it is
always known which callbacks we want to call at compile time. Note that
the kernel's rhashtable library (<linux/rhashtable*.h>) also does the
same.

This of course makes the hashtab functions more easy to misuse by
passing a wrong calback 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 defining
hashtab_search() and -_insert() inline, which is done in a follow-up
patch to make it easier to review the hashtab.c changes here.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ss/hashtab.c  | 44 ++++++++++----------
 security/selinux/ss/hashtab.h  | 22 +++++-----
 security/selinux/ss/mls.c      |  2 +-
 security/selinux/ss/policydb.c | 76 ++++++++++++++++++++++++----------
 security/selinux/ss/policydb.h |  9 ++++
 security/selinux/ss/services.c |  4 +-
 security/selinux/ss/symtab.c   | 16 ++++---
 7 files changed, 110 insertions(+), 63 deletions(-)

Comments

Paul Moore May 5, 2020, 11:34 a.m. UTC | #1 
On Mon, May 4, 2020 at 7:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Refactor searching and inserting into hashtabs to pave 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 drastic 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 it is
> always known which callbacks we want to call at compile time. Note that
> the kernel's rhashtable library (<linux/rhashtable*.h>) also does the
> same.
>
> This of course makes the hashtab functions more easy to misuse by
> passing a wrong calback 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 defining
> hashtab_search() and -_insert() inline, which is done in a follow-up
> patch to make it easier to review the hashtab.c changes here.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/ss/hashtab.c  | 44 ++++++++++----------
>  security/selinux/ss/hashtab.h  | 22 +++++-----
>  security/selinux/ss/mls.c      |  2 +-
>  security/selinux/ss/policydb.c | 76 ++++++++++++++++++++++++----------
>  security/selinux/ss/policydb.h |  9 ++++
>  security/selinux/ss/services.c |  4 +-
>  security/selinux/ss/symtab.c   | 16 ++++---
>  7 files changed, 110 insertions(+), 63 deletions(-)

...

> diff --git a/security/selinux/ss/hashtab.h b/security/selinux/ss/hashtab.h
> index 31c11511fe10..4885234257d4 100644
> --- a/security/selinux/ss/hashtab.h
> +++ b/security/selinux/ss/hashtab.h
> @@ -13,6 +13,12 @@
>
>  #define HASHTAB_MAX_NODES      0xffffffff
>
> +struct hashtab_key_params {
> +       u32 (*hash)(const void *key);   /* hash function */
> +       int (*cmp)(const void *key1, const void *key2);
> +                                       /* key comparison function */
> +};
> +
>  struct hashtab_node {
>         void *key;
>         void *datum;
> @@ -23,10 +29,6 @@ struct hashtab {
>         struct hashtab_node **htable;   /* hash table */
>         u32 size;                       /* number of slots in hash table */
>         u32 nel;                        /* number of elements in hash table */
> -       u32 (*hash_value)(struct hashtab *h, const void *key);
> -                                       /* hash function */
> -       int (*keycmp)(struct hashtab *h, const void *key1, const void *key2);
> -                                       /* key comparison function */
>  };

I remain concerned that in the process of chasing performance this
patchset makes the code worse and more fragile.

What if we took a slightly different approach such that instead of
breaking apart hashtab_node we moved the variable portions (htable,
size, nel) into a separate struct which we could reference in
hashtab_node?  For example:

  struct hashtab_var {
    struct hashtab_node **htable;
    u32 size;
    u32 nel;
  }

  struct hashtab {
    struct hashtab_var *table; // adds an extra ptr deref
    u32 (*hash_value)(...);
    int (*keycmp)(...);
  }

I might be mistaken, but I believe this should still allow you to
implement the inlining/pass-by-value tricks for search and insert
while maintaining a binding between the hashtable data and
hash/compare functions.  Yes?  Or does the entire struct need to be
declared as a static const for the compiler optimizations to work?  It
is not clear to me from the commit descriptions or comments in the
code.

As far as testing is concerned, whenever possible it is better to show
performance improvements in the context of some sort of user workload
and not an artificial stress test or micro benchmark.  I understand
that such focused tests can be attractive both in terms of their
results and ease of use, but they can also be very misleading.
Performance improvements for things like policy loads, kernel
compiles, etc. are much more interesting to me than results from
running something like stress-ng.
Ondrej Mosnacek May 7, 2020, 9:19 a.m. UTC | #2 
On Tue, May 5, 2020 at 1:34 PM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, May 4, 2020 at 7:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Refactor searching and inserting into hashtabs to pave 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 drastic 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 it is
> > always known which callbacks we want to call at compile time. Note that
> > the kernel's rhashtable library (<linux/rhashtable*.h>) also does the
> > same.
> >
> > This of course makes the hashtab functions more easy to misuse by
> > passing a wrong calback 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 defining
> > hashtab_search() and -_insert() inline, which is done in a follow-up
> > patch to make it easier to review the hashtab.c changes here.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  security/selinux/ss/hashtab.c  | 44 ++++++++++----------
> >  security/selinux/ss/hashtab.h  | 22 +++++-----
> >  security/selinux/ss/mls.c      |  2 +-
> >  security/selinux/ss/policydb.c | 76 ++++++++++++++++++++++++----------
> >  security/selinux/ss/policydb.h |  9 ++++
> >  security/selinux/ss/services.c |  4 +-
> >  security/selinux/ss/symtab.c   | 16 ++++---
> >  7 files changed, 110 insertions(+), 63 deletions(-)
>
> ...
>
> > diff --git a/security/selinux/ss/hashtab.h b/security/selinux/ss/hashtab.h
> > index 31c11511fe10..4885234257d4 100644
> > --- a/security/selinux/ss/hashtab.h
> > +++ b/security/selinux/ss/hashtab.h
> > @@ -13,6 +13,12 @@
> >
> >  #define HASHTAB_MAX_NODES      0xffffffff
> >
> > +struct hashtab_key_params {
> > +       u32 (*hash)(const void *key);   /* hash function */
> > +       int (*cmp)(const void *key1, const void *key2);
> > +                                       /* key comparison function */
> > +};
> > +
> >  struct hashtab_node {
> >         void *key;
> >         void *datum;
> > @@ -23,10 +29,6 @@ struct hashtab {
> >         struct hashtab_node **htable;   /* hash table */
> >         u32 size;                       /* number of slots in hash table */
> >         u32 nel;                        /* number of elements in hash table */
> > -       u32 (*hash_value)(struct hashtab *h, const void *key);
> > -                                       /* hash function */
> > -       int (*keycmp)(struct hashtab *h, const void *key1, const void *key2);
> > -                                       /* key comparison function */
> >  };
>
> I remain concerned that in the process of chasing performance this
> patchset makes the code worse and more fragile.
>
> What if we took a slightly different approach such that instead of
> breaking apart hashtab_node we moved the variable portions (htable,
> size, nel) into a separate struct which we could reference in
> hashtab_node?  For example:
>
>   struct hashtab_var {
>     struct hashtab_node **htable;
>     u32 size;
>     u32 nel;
>   }
>
>   struct hashtab {
>     struct hashtab_var *table; // adds an extra ptr deref
>     u32 (*hash_value)(...);
>     int (*keycmp)(...);
>   }
>
> I might be mistaken, but I believe this should still allow you to
> implement the inlining/pass-by-value tricks for search and insert
> while maintaining a binding between the hashtable data and
> hash/compare functions.  Yes?  Or does the entire struct need to be
> declared as a static const for the compiler optimizations to work?  It
> is not clear to me from the commit descriptions or comments in the
> code.

No, that wouldn't work. For example: when compiling a
hashtab_search(&policydb->some_hashtab, ...) call (let's assume it's
defined inline already) inside a function that takes policydb as an
argument, the compiler has no way of knowing what functions you
stashed into hash_value and keycmp in some different function, so it
has to load the pointers from memory and call them. It needs to know
what the functions are right there if it is to generate a direct
constant-address call (or inline them). In C you simply can't
associate functions (data) to types. In C++ this would be easily done
with templates, but the kernel isn't written in C++, so you simply
have to get (somewhat) dirty to get efficient code.

How about if I moved all the 4 hashtab types into a common compilation
unit (policydb_tables.[hc]?) and hid the unsafe generic hashtab
building blocks inside the C file? The header would then only expose
struct hashtab and provide specific functions for each type of
hashtab. And each hashtab "subclass" could be a separate type (which
would just wrap struct hashtab in another struct), which would be even
more type safe than the current state. Let me try to put it into a
patch, where it will be easier to see what I mean...

>
> As far as testing is concerned, whenever possible it is better to show
> performance improvements in the context of some sort of user workload
> and not an artificial stress test or micro benchmark.  I understand
> that such focused tests can be attractive both in terms of their
> results and ease of use, but they can also be very misleading.
> Performance improvements for things like policy loads, kernel
> compiles, etc. are much more interesting to me than results from
> running something like stress-ng.

OK, I'll try to make my case on some more realistic benchmark.

--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
diff mbox series

Patch

diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
index 5ee868116d70..8126b909a757 100644
--- a/security/selinux/ss/hashtab.c
+++ b/security/selinux/ss/hashtab.c
@@ -29,16 +29,10 @@  static u32 hashtab_compute_size(u32 nel)
 	return nel == 0 ? 0 : roundup_pow_of_two(nel);
 }
 
-int hashtab_init(struct hashtab *h,
-		 u32 (*hash_value)(struct hashtab *h, const void *key),
-		 int (*keycmp)(struct hashtab *h, const void *key1,
-			       const void *key2),
-		 u32 nel_hint)
+int hashtab_init(struct hashtab *h, u32 nel_hint)
 {
 	h->size = hashtab_compute_size(nel_hint);
 	h->nel = 0;
-	h->hash_value = hash_value;
-	h->keycmp = keycmp;
 	if (!h->size)
 		return 0;
 
@@ -46,7 +40,8 @@  int hashtab_init(struct hashtab *h,
 	return h->htable ? 0 : -ENOMEM;
 }
 
-int hashtab_insert(struct hashtab *h, void *key, void *datum)
+int hashtab_insert(struct hashtab *h, void *key, void *datum,
+		   struct hashtab_key_params key_params)
 {
 	u32 hvalue;
 	struct hashtab_node *prev, *cur, *newnode;
@@ -56,17 +51,20 @@  int hashtab_insert(struct hashtab *h, void *key, void *datum)
 	if (!h->size || h->nel == HASHTAB_MAX_NODES)
 		return -EINVAL;
 
-	hvalue = h->hash_value(h, key);
+	hvalue = key_params.hash(key) & (h->size - 1);
 	prev = NULL;
 	cur = h->htable[hvalue];
-	while (cur && h->keycmp(h, key, cur->key) > 0) {
+	while (cur) {
+		int cmp = key_params.cmp(key, cur->key);
+
+		if (cmp == 0)
+			return -EEXIST;
+		if (cmp < 0)
+			break;
 		prev = cur;
 		cur = cur->next;
 	}
 
-	if (cur && (h->keycmp(h, key, cur->key) == 0))
-		return -EEXIST;
-
 	newnode = kmem_cache_zalloc(hashtab_node_cachep, GFP_KERNEL);
 	if (!newnode)
 		return -ENOMEM;
@@ -84,7 +82,8 @@  int hashtab_insert(struct hashtab *h, void *key, void *datum)
 	return 0;
 }
 
-void *hashtab_search(struct hashtab *h, const void *key)
+void *hashtab_search(struct hashtab *h, const void *key,
+		     struct hashtab_key_params key_params)
 {
 	u32 hvalue;
 	struct hashtab_node *cur;
@@ -92,15 +91,18 @@  void *hashtab_search(struct hashtab *h, const void *key)
 	if (!h->size)
 		return NULL;
 
-	hvalue = h->hash_value(h, key);
+	hvalue = key_params.hash(key) & (h->size - 1);
 	cur = h->htable[hvalue];
-	while (cur && h->keycmp(h, key, cur->key) > 0)
-		cur = cur->next;
+	while (cur) {
+		int cmp = key_params.cmp(key, cur->key);
 
-	if (!cur || (h->keycmp(h, key, cur->key) != 0))
-		return NULL;
-
-	return cur->datum;
+		if (cmp == 0)
+			return cur->datum;
+		if (cmp < 0)
+			break;
+		cur = cur->next;
+	}
+	return NULL;
 }
 
 void hashtab_destroy(struct hashtab *h)
diff --git a/security/selinux/ss/hashtab.h b/security/selinux/ss/hashtab.h
index 31c11511fe10..4885234257d4 100644
--- a/security/selinux/ss/hashtab.h
+++ b/security/selinux/ss/hashtab.h
@@ -13,6 +13,12 @@ 
 
 #define HASHTAB_MAX_NODES	0xffffffff
 
+struct hashtab_key_params {
+	u32 (*hash)(const void *key);	/* hash function */
+	int (*cmp)(const void *key1, const void *key2);
+					/* key comparison function */
+};
+
 struct hashtab_node {
 	void *key;
 	void *datum;
@@ -23,10 +29,6 @@  struct hashtab {
 	struct hashtab_node **htable;	/* hash table */
 	u32 size;			/* number of slots in hash table */
 	u32 nel;			/* number of elements in hash table */
-	u32 (*hash_value)(struct hashtab *h, const void *key);
-					/* hash function */
-	int (*keycmp)(struct hashtab *h, const void *key1, const void *key2);
-					/* key comparison function */
 };
 
 struct hashtab_info {
@@ -39,11 +41,7 @@  struct hashtab_info {
  *
  * Returns -ENOMEM if insufficient space is available or 0 otherwise.
  */
-int hashtab_init(struct hashtab *h,
-		 u32 (*hash_value)(struct hashtab *h, const void *key),
-		 int (*keycmp)(struct hashtab *h, const void *key1,
-			       const void *key2),
-		 u32 nel_hint);
+int hashtab_init(struct hashtab *h, u32 nel_hint);
 
 /*
  * Inserts the specified (key, datum) pair into the specified hash table.
@@ -53,7 +51,8 @@  int hashtab_init(struct hashtab *h,
  * -EINVAL for general errors or
   0 otherwise.
  */
-int hashtab_insert(struct hashtab *h, void *k, void *d);
+int hashtab_insert(struct hashtab *h, void *k, void *d,
+		   struct hashtab_key_params key_params);
 
 /*
  * Searches for the entry with the specified key in the hash table.
@@ -61,7 +60,8 @@  int hashtab_insert(struct hashtab *h, void *k, void *d);
  * Returns NULL if no entry has the specified key or
  * the datum of the entry otherwise.
  */
-void *hashtab_search(struct hashtab *h, const void *k);
+void *hashtab_search(struct hashtab *h, const void *k,
+		     struct hashtab_key_params key_params);
 
 /*
  * Destroys the specified hash table.
diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 5be241b6b190..408d306895f8 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -507,7 +507,7 @@  int mls_compute_sid(struct policydb *p,
 		rtr.source_type = scontext->type;
 		rtr.target_type = tcontext->type;
 		rtr.target_class = tclass;
-		r = hashtab_search(&p->range_tr, &rtr);
+		r = policydb_rangetr_search(p, &rtr);
 		if (r)
 			return mls_range_set(newcontext, r);
 
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 043c7b111399..ee870f1b1010 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -411,7 +411,7 @@  out:
 	return rc;
 }
 
-static u32 filenametr_hash(struct hashtab *h, const void *k)
+static u32 filenametr_hash(const void *k)
 {
 	const struct filename_trans_key *ft = k;
 	unsigned long hash;
@@ -423,10 +423,10 @@  static u32 filenametr_hash(struct hashtab *h, const void *k)
 	byte_num = 0;
 	while ((focus = ft->name[byte_num++]))
 		hash = partial_name_hash(focus, hash);
-	return hash & (h->size - 1);
+	return hash;
 }
 
-static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2)
+static int filenametr_cmp(const void *k1, const void *k2)
 {
 	const struct filename_trans_key *ft1 = k1;
 	const struct filename_trans_key *ft2 = k2;
@@ -444,15 +444,26 @@  static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2)
 
 }
 
-static u32 rangetr_hash(struct hashtab *h, const void *k)
+static const struct hashtab_key_params filenametr_key_params = {
+	.hash = filenametr_hash,
+	.cmp = filenametr_cmp,
+};
+
+struct filename_trans_datum *policydb_filenametr_search(
+	struct policydb *p, struct filename_trans_key *key)
+{
+	return hashtab_search(&p->filename_trans, key, filenametr_key_params);
+}
+
+static u32 rangetr_hash(const void *k)
 {
 	const struct range_trans *key = k;
 
-	return (key->source_type + (key->target_type << 3) +
-		(key->target_class << 5)) & (h->size - 1);
+	return key->source_type + (key->target_type << 3) +
+		(key->target_class << 5);
 }
 
-static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
+static int rangetr_cmp(const void *k1, const void *k2)
 {
 	const struct range_trans *key1 = k1, *key2 = k2;
 	int v;
@@ -470,15 +481,25 @@  static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2)
 	return v;
 }
 
-static u32 role_trans_hash(struct hashtab *h, const void *k)
+static const struct hashtab_key_params rangetr_key_params = {
+	.hash = rangetr_hash,
+	.cmp = rangetr_cmp,
+};
+
+struct mls_range *policydb_rangetr_search(struct policydb *p,
+					  struct range_trans *key)
+{
+	return hashtab_search(&p->range_tr, key, rangetr_key_params);
+}
+
+static u32 role_trans_hash(const void *k)
 {
 	const struct role_trans_key *key = k;
 
-	return (key->role + (key->type << 3) + (key->tclass << 5)) &
-		(h->size - 1);
+	return key->role + (key->type << 3) + (key->tclass << 5);
 }
 
-static int role_trans_cmp(struct hashtab *h, const void *k1, const void *k2)
+static int role_trans_cmp(const void *k1, const void *k2)
 {
 	const struct role_trans_key *key1 = k1, *key2 = k2;
 	int v;
@@ -494,6 +515,17 @@  static int role_trans_cmp(struct hashtab *h, const void *k1, const void *k2)
 	return key1->tclass - key2->tclass;
 }
 
+static const struct hashtab_key_params roletr_key_params = {
+	.hash = role_trans_hash,
+	.cmp = role_trans_cmp,
+};
+
+struct role_trans_datum *policydb_roletr_search(struct policydb *p,
+						struct role_trans_key *key)
+{
+	return hashtab_search(&p->role_tr, key, roletr_key_params);
+}
+
 /*
  * Initialize a policy database structure.
  */
@@ -1796,7 +1828,7 @@  static int range_read(struct policydb *p, void *fp)
 
 	nel = le32_to_cpu(buf[0]);
 
-	rc = hashtab_init(&p->range_tr, rangetr_hash, rangetr_cmp, nel);
+	rc = hashtab_init(&p->range_tr, nel);
 	if (rc)
 		return rc;
 
@@ -1841,7 +1873,7 @@  static int range_read(struct policydb *p, void *fp)
 			goto out;
 		}
 
-		rc = hashtab_insert(&p->range_tr, rt, r);
+		rc = hashtab_insert(&p->range_tr, rt, r, rangetr_key_params);
 		if (rc)
 			goto out;
 
@@ -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);
 	while (datum) {
 		if (unlikely(ebitmap_get_bit(&datum->stypes, stype - 1))) {
 			/* conflicting/duplicate rules are ignored */
@@ -1918,7 +1950,8 @@  static int filename_trans_read_helper_compat(struct policydb *p, void *fp)
 			if (!ft)
 				goto out;
 
-			rc = hashtab_insert(&p->filename_trans, ft, datum);
+			rc = hashtab_insert(&p->filename_trans, ft, datum,
+					    filenametr_key_params);
 			if (rc)
 				goto out;
 			name = NULL;
@@ -2006,7 +2039,8 @@  static int filename_trans_read_helper(struct policydb *p, void *fp)
 	ft->tclass = tclass;
 	ft->name = name;
 
-	rc = hashtab_insert(&p->filename_trans, ft, first);
+	rc = hashtab_insert(&p->filename_trans, ft, first,
+			    filenametr_key_params);
 	if (rc == -EEXIST)
 		pr_err("SELinux:  Duplicate filename transition key\n");
 	if (rc)
@@ -2044,8 +2078,7 @@  static int filename_trans_read(struct policydb *p, void *fp)
 	if (p->policyvers < POLICYDB_VERSION_COMP_FTRANS) {
 		p->compat_filename_trans_count = nel;
 
-		rc = hashtab_init(&p->filename_trans, filenametr_hash,
-				  filenametr_cmp, (1 << 11));
+		rc = hashtab_init(&p->filename_trans, (1 << 11));
 		if (rc)
 			return rc;
 
@@ -2055,8 +2088,7 @@  static int filename_trans_read(struct policydb *p, void *fp)
 				return rc;
 		}
 	} else {
-		rc = hashtab_init(&p->filename_trans, filenametr_hash,
-				  filenametr_cmp, nel);
+		rc = hashtab_init(&p->filename_trans, nel);
 		if (rc)
 			return rc;
 
@@ -2537,7 +2569,7 @@  int policydb_read(struct policydb *p, void *fp)
 		goto bad;
 	nel = le32_to_cpu(buf[0]);
 
-	rc = hashtab_init(&p->role_tr, role_trans_hash, role_trans_cmp, nel);
+	rc = hashtab_init(&p->role_tr, nel);
 	if (rc)
 		goto bad;
 	for (i = 0; i < nel; i++) {
@@ -2574,7 +2606,7 @@  int policydb_read(struct policydb *p, void *fp)
 		    !policydb_role_isvalid(p, rtd->new_role))
 			goto bad;
 
-		rc = hashtab_insert(&p->role_tr, rtk, rtd);
+		rc = hashtab_insert(&p->role_tr, rtk, rtd, roletr_key_params);
 		if (rc)
 			goto bad;
 
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 9591c9587cb6..c24d4e1063ea 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -324,6 +324,15 @@  extern int policydb_role_isvalid(struct policydb *p, unsigned int role);
 extern int policydb_read(struct policydb *p, void *fp);
 extern int policydb_write(struct policydb *p, void *fp);
 
+extern struct filename_trans_datum *policydb_filenametr_search(
+	struct policydb *p, struct filename_trans_key *key);
+
+extern struct mls_range *policydb_rangetr_search(
+	struct policydb *p, struct range_trans *key);
+
+extern struct role_trans_datum *policydb_roletr_search(
+	struct policydb *p, struct role_trans_key *key);
+
 #define POLICYDB_CONFIG_MLS    1
 
 /* the config flags related to unknown classes/perms are bits 2 and 3 */
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 1d12bb9ff3dd..9e76a80db6e1 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1671,7 +1671,7 @@  static void filename_compute_type(struct policydb *policydb,
 	ft.tclass = tclass;
 	ft.name = objname;
 
-	datum = hashtab_search(&policydb->filename_trans, &ft);
+	datum = policydb_filenametr_search(policydb, &ft);
 	while (datum) {
 		if (ebitmap_get_bit(&datum->stypes, stype - 1)) {
 			newcontext->type = datum->otype;
@@ -1834,7 +1834,7 @@  static int security_compute_sid(struct selinux_state *state,
 			.tclass = tclass,
 		};
 
-		rtd = hashtab_search(&policydb->role_tr, &rtk);
+		rtd = policydb_roletr_search(policydb, &rtk);
 		if (rtd)
 			newcontext.role = rtd->new_role;
 	}
diff --git a/security/selinux/ss/symtab.c b/security/selinux/ss/symtab.c
index 48f523ef93aa..c42a6648a07d 100644
--- a/security/selinux/ss/symtab.c
+++ b/security/selinux/ss/symtab.c
@@ -9,7 +9,7 @@ 
 #include <linux/errno.h>
 #include "symtab.h"
 
-static unsigned int symhash(struct hashtab *h, const void *key)
+static unsigned int symhash(const void *key)
 {
 	const char *p, *keyp;
 	unsigned int size;
@@ -20,10 +20,10 @@  static unsigned int symhash(struct hashtab *h, const void *key)
 	size = strlen(keyp);
 	for (p = keyp; (p - keyp) < size; p++)
 		val = (val << 4 | (val >> (8*sizeof(unsigned int)-4))) ^ (*p);
-	return val & (h->size - 1);
+	return val;
 }
 
-static int symcmp(struct hashtab *h, const void *key1, const void *key2)
+static int symcmp(const void *key1, const void *key2)
 {
 	const char *keyp1, *keyp2;
 
@@ -32,19 +32,23 @@  static int symcmp(struct hashtab *h, const void *key1, const void *key2)
 	return strcmp(keyp1, keyp2);
 }
 
+static const struct hashtab_key_params symtab_key_params = {
+	.hash = symhash,
+	.cmp = symcmp,
+};
 
 int symtab_init(struct symtab *s, unsigned int size)
 {
 	s->nprim = 0;
-	return hashtab_init(&s->table, symhash, symcmp, size);
+	return hashtab_init(&s->table, size);
 }
 
 int symtab_insert(struct symtab *s, char *name, void *datum)
 {
-	return hashtab_insert(&s->table, name, datum);
+	return hashtab_insert(&s->table, name, datum, symtab_key_params);
 }
 
 void *symtab_search(struct symtab *s, const char *name)
 {
-	return hashtab_search(&s->table, name);
+	return hashtab_search(&s->table, name, symtab_key_params);
 }