On Tue, Apr 28, 2020 at 8:55 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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.
This commit description describes the next patch in the series, and
some of your motivation, but doesn't really tell me much about this
patch other than it is a "refactoring". I need more info here,
especially considering my comment below.
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> ---
> security/selinux/ss/conditional.c | 4 +-
> security/selinux/ss/conditional.h | 2 +-
> security/selinux/ss/hashtab.c | 44 +++++-----
> security/selinux/ss/hashtab.h | 22 ++---
> security/selinux/ss/mls.c | 23 +++---
> security/selinux/ss/policydb.c | 128 +++++++++++++++++++-----------
> security/selinux/ss/policydb.h | 9 +++
> security/selinux/ss/services.c | 38 ++++-----
> security/selinux/ss/symtab.c | 22 ++++-
> security/selinux/ss/symtab.h | 3 +
> 10 files changed, 178 insertions(+), 117 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 don't like how you've split the hashing and comparison functions out
of the hashtab struct and into their own data structure with no
explicit linkage between the two. This is a bad design decision in my
opinion, and something we should try to avoid.
> };
>
> struct hashtab_info {
--
paul moore
www.paul-moore.com
