lkml.org 
[lkml]   [2018]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 2/8] rhashtable: remove nulls_base and related code.
    On Mon, May 07 2018, Herbert Xu wrote:

    > On Sun, May 06, 2018 at 07:37:49AM +1000, NeilBrown wrote:
    >>
    >> I can see no evidence that this is required for anything, as it isn't
    >> use and I'm fairly sure that in it's current form - it cannot be used.
    >
    > Search for nulls in net/ipv4. This is definitely used throughout
    > the network stack. As the aim is to convert as many existing hash
    > tables to rhashtable as possible, we want to keep this.

    Yes, I'm sure that nulls lists are very useful and I can see them used
    in net/ipv4 (and I would like to use them myself). I don't want to
    remove the nulls list concept from rhashtable, and my patch doesn't do
    that.

    It just removes nulls_base (as the subject says) and stops setting any
    particular value in the nulls pointer, because the value is currently
    never tested.

    The point of nulls lists is, as I'm sure you know, to detect if an
    object was moved to a different chain and to restart the search in that
    case.
    This means that on an unsuccessful search, we need to test
    get_nulls_value() of the tail pointer.
    Several times in net/ipv4/inet_hashtables.c (for example) we see code
    like:

    /*
    * if the nulls value we got at the end of this lookup is
    * not the expected one, we must restart lookup.
    * We probably met an item that was moved to another chain.
    */
    if (get_nulls_value(node) != slot)
    goto begin;

    which does exactly that. This test-and-restart cannot be done by a
    caller to rhashtable_lookup_fast() as the caller doesn't even get to
    the tail nulls.
    It would need to be done in rhashtable.[ch].
    If you like, I can make this functionality work rather than just
    removing the useless parts.

    I would change INIT_RHT_NULLS_HEAD() to return to be
    #define INIT_RHT_NULLS_HEAD(ptr, ht, hash) \
    ((ptr) = (void*)NULLS_MARKER(((unsigned long)ptr)>>1)

    Then when __rhashtable_lookup() comes to the end of the chain
    without finding anything it would do something like
    if (get_nulls_value(he)<<1 == (unsigned long)head)
    goto restart;

    where 'head' is the head of the list that we would have saved at the
    start.

    i.e. instead of using a nulls_base and limiting the size of the hash, we
    store the address of the bucket in the nulls.

    In my mind that should be a separate patch. First remove the unused,
    harmful code. Then add code to provide useful functionality. But
    I can put the two together in one patch if you would prefer that.

    Thanks,
    NeilBrown
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2018-05-08 03:14    [W:2.279 / U:0.224 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site