| Message ID | 20220829143055.41201-2-zhengqi.arch@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add common struct mm_slot and use it in THP and KSM | expand |
On Mon, 29 Aug 2022 22:30:49 +0800 Qi Zheng <zhengqi.arch@bytedance.com> 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. 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.
On 2022/8/30 03:51, Andrew Morton wrote: > On Mon, 29 Aug 2022 22:30:49 +0800 Qi Zheng <zhengqi.arch@bytedance.com> 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. Hi, The reason is that khugepaged_exit()/ksm_exit() will be called first in __mmput() to remove mm from the linked list. So it is prevented the mm_struct from getting freed while on the list. > > 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 Since _hashtable is an array name, IIUC, this cannot be passed as a function parameter, so I chose to implement it as a macro. > 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? Ditto. > > > 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. These names are better, will modify to it in the next version. Thanks, Qi
On Mon, Aug 29, 2022 at 12:51 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 29 Aug 2022 22:30:49 +0800 Qi Zheng <zhengqi.arch@bytedance.com> 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. > > 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. >
On 2022/8/31 01:03, Yang Shi wrote: > On Mon, Aug 29, 2022 at 12:51 PM Andrew Morton > <akpm@linux-foundation.org> wrote: >> >> On Mon, 29 Aug 2022 22:30:49 +0800 Qi Zheng <zhengqi.arch@bytedance.com> 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. >>
diff --git a/mm/mm_slot.h b/mm/mm_slot.h new file mode 100644 index 000000000000..c8f0d26ef7b0 --- /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; +}; + +#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; \ +}) + +#define insert_to_mm_slots_hash(_hashtable, _mm, _mm_slot) \ +({ \ + _mm_slot->mm = _mm; \ + hash_add(_hashtable, &_mm_slot->hash, (unsigned long)_mm); \ +}) + +#endif /* _LINUX_MM_SLOT_H */
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. Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com> --- mm/mm_slot.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 mm/mm_slot.h