Re: [PATCH 1/7] mm: introduce common struct mm_slot

[Qi Zheng]

On 2022/8/31 01:03, Yang Shi wrote:
> On Mon, Aug 29, 2022 at 12:51 PM Andrew Morton
> <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> > On Mon, 29 Aug 2022 22:30:49 +0800 Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx> wrote:
> >
> > > At present, both THP and KSM module have similar structures
> > > mm_slot for organizing and recording the information required
> > > for scanning mm, and each defines the following exactly the
> > > same operation functions:
> > >
> > >   - alloc_mm_slot
> > >   - free_mm_slot
> > >   - get_mm_slot
> > >   - insert_to_mm_slots_hash
> > >
> > > In order to de-duplicate these codes, this patch introduces a
> > > common struct mm_slot, and subsequent patches will let THP and
> > > KSM to use it.
> >
> > Seems like a good idea.
> >
> > > --- /dev/null
> > > +++ b/mm/mm_slot.h
> > > @@ -0,0 +1,55 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#ifndef _LINUX_MM_SLOT_H
> > > +#define _LINUX_MM_SLOT_H
> > > +
> > > +#include <linux/hashtable.h>
> > > +#include <linux/slab.h>
> > > +
> > > +/*
> > > + * struct mm_slot - hash lookup from mm to mm_slot
> > > + * @hash: link to the mm_slots hash list
> > > + * @mm_node: link into the mm_slots list
> > > + * @mm: the mm that this information is valid for
> > > + */
> > > +struct mm_slot {
> > > +     struct hlist_node hash;
> > > +     struct list_head mm_node;
> > > +     struct mm_struct *mm;
> > > +};
> >
> > It appears that the presence of an mm_struct in the hash list does not
> > contribute to the mm_struct's refcount?  That's somewhat unexpected.
>
> I didn't find time to look into the series yet, but when the
> mm/mm_slot was added to the list, mmgrab() was definitely called if
> this was not changed by the series.

Yeah, and this series does not change that.

> > It would be helpful to add some words here describing the means by
> > which a user of mm_slot would prevent the mm_struct from getting freed
> > while on the list.  I assume "caller must maintain a reference on the
> > mm_struct while it remains on an mm_slot hash list"?
> >
> > > +#define mm_slot_entry(ptr, type, member) \
> > > +     container_of(ptr, type, member)
> > > +
> > > +static inline void *alloc_mm_slot(struct kmem_cache *cache)
> > > +{
> > > +     if (!cache)     /* initialization failed */
> > > +             return NULL;
> > > +     return kmem_cache_zalloc(cache, GFP_KERNEL);
> > > +}
> > > +
> > > +static inline void free_mm_slot(struct kmem_cache *cache, void *objp)
> > > +{
> > > +     kmem_cache_free(cache, objp);
> > > +}
> > > +
> > > +#define get_mm_slot(_hashtable, _mm)                                        \
> > > +({                                                                          \
> > > +     struct mm_slot *tmp_slot, *mm_slot = NULL;                             \
> > > +                                                                            \
> > > +     hash_for_each_possible(_hashtable, tmp_slot, hash, (unsigned long)_mm) \
> > > +             if (_mm == tmp_slot->mm) {                                     \
> > > +                     mm_slot = tmp_slot;                                    \
> > > +                     break;                                                 \
> > > +             }                                                              \
> > > +                                                                            \
> > > +     mm_slot;                                                               \
> > > +})
> >
> > Is there a reason why this must be implemented as a macro?  That's
> > preferable, although this may be overly large for inlining.  mm/util.c
> > might suit.
> >
> > > +#define insert_to_mm_slots_hash(_hashtable, _mm, _mm_slot)                  \
> > > +({                                                                          \
> > > +     _mm_slot->mm = _mm;                                                    \
> > > +     hash_add(_hashtable, &_mm_slot->hash, (unsigned long)_mm);             \
> > > +})
> >
> > Does this need to be a macro?
> >
> >
> > And the naming.  Can we please have
> >
> > mm_slot_entry
> > mm_slot_alloc
> > mm_slot_free
> > mm_slot_get
> > mm_slot_insert
> >
> > Also, "get" usually implies that a refcout is taken on the obtained
> > object, so mm_slot_lookup() would be more appropriate.

--
Thanks,
Qi