Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <aa628023-c6ad-8970-ec44-28d560864174@bytedance.com>
Date:   Wed, 31 Aug 2022 10:51:45 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.1.2
Subject: Re: [PATCH 1/7] mm: introduce common struct mm_slot
Content-Language: en-US
To:     Yang Shi <shy828301@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     willy@infradead.org, vbabka@suse.cz, hannes@cmpxchg.org,
        minchan@kernel.org, rppt@kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20220829143055.41201-1-zhengqi.arch@bytedance.com>
 <20220829143055.41201-2-zhengqi.arch@bytedance.com>
 <20220829125134.9b05f9b8caf5da4bec8f31e8@linux-foundation.org>
 <CAHbLzkrP7Ce7iTKQHvZQ-s3NMs1mhzjkNs8XMF_iOtbgJuJDUQ@mail.gmail.com>
From:   Qi Zheng <zhengqi.arch@bytedance.com>
In-Reply-To: <CAHbLzkrP7Ce7iTKQHvZQ-s3NMs1mhzjkNs8XMF_iOtbgJuJDUQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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