Re: [PATCH] mm/mm_slot.h: add a helper function mm_slot_remove

[xu.xin16]

> > No functional change is intended.
> > 
> > Signed-off-by: xu xin <xu.xin16@xxxxxxxxxx>
> 
> In general it's fine but I want more :) see below.
> 
> >
> >
> > diff --git a/mm/mm_slot.h b/mm/mm_slot.h
> > index 83f18ed1c4bd..bc3b9ae8fdbc 100644
> > --- a/mm/mm_slot.h
> > +++ b/mm/mm_slot.h
> > @@ -52,4 +52,11 @@ static inline void mm_slot_free(struct kmem_cache *cache, void *objp)
> >  	hash_add(_hashtable, &_mm_slot->hash, (unsigned long)_mm);	       \
> >  })
> >  
> 
> mm_slot.h also inexplicably implements mm_slot_lookup() and mm_slot_insert() as
> macros. Can you change these to static inline functions in the respin?

Sorry, I looked into turning mm_slot_lookup() and mm_slot_insert() into
static inline functions, but unfortunately it’s not possible because
they rely on hash_for_each_possible() and hash_add(), which expect an
actual array instead of pointer so that sizeof() can determine the
hash table size. Passing a struct hlist_head * would make sizeof() return
the pointer size instead of the array size, leading to incorrect bucket
indexing and memory corruption.

> 
> Also rename the horrible names from _foo -> foo too please.
> 
> And we don't really need 'mm_slot' in the var name for a slot just 'slot' works
> fine.

Ok, I can clean up these stuff.

> 
> > +static inline void mm_slot_remove(struct mm_slot *_mm_slot)
> 
> Yeah this name is gross just 'slot' please.
> 
> > +{
> > +	hash_del(&_mm_slot->hash);
> > +	list_del(&_mm_slot->mm_node);
> > +}
> > +
> > +
> >  #endif /* _LINUX_MM_SLOT_H */
> 
> Thanks, Lorenzo
>