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
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 */
--
2.20.1On 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 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.
>>
--
Thanks,
Qi
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
--
Thanks,
Qi
© 2016 - 2026 Red Hat, Inc.