Re: [RFC PATCH v2 1/3] meminfo_extra: introduce meminfo extra

[Jaewon Kim]

On 2020ë 03ì 24ì 19:11, Greg KH wrote:
> On Tue, Mar 24, 2020 at 06:11:17PM +0900, Jaewon Kim wrote:
>> On 2020ë 03ì 23ì 18:53, Greg KH wrote:
>>>> +int register_meminfo_extra(atomic_long_t *val, int shift, const char *name)
>>>> +{
>>>> +	struct meminfo_extra *meminfo, *memtemp;
>>>> +	int len;
>>>> +	int error = 0;
>>>> +
>>>> +	meminfo = kzalloc(sizeof(*meminfo), GFP_KERNEL);
>>>> +	if (!meminfo) {
>>>> +		error = -ENOMEM;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	meminfo->val = val;
>>>> +	meminfo->shift_for_page = shift;
>>>> +	strncpy(meminfo->name, name, NAME_SIZE);
>>>> +	len = strlen(meminfo->name);
>>>> +	meminfo->name[len] = ':';
>>>> +	strncpy(meminfo->name_pad, meminfo->name, NAME_BUF_SIZE);
>>>> +	while (++len < NAME_BUF_SIZE - 1)
>>>> +		meminfo->name_pad[len] = ' ';
>>>> +
>>>> +	spin_lock(&meminfo_lock);
>>>> +	list_for_each_entry_rcu(memtemp, &meminfo_head, list) {
>>>> +		if (memtemp->val == val) {
>>>> +			error = -EINVAL;
>>>> +			break;
>>>> +		}
>>>> +	}
>>>> +	if (!error)
>>>> +		list_add_tail_rcu(&meminfo->list, &meminfo_head);
>>>> +	spin_unlock(&meminfo_lock);
>>> If you have a lock, why are you needing rcu?
>> I think _rcu should be removed out of list_for_each_entry_rcu.
>> But I'm confused about what you meant.
>> I used rcu_read_lock on __meminfo_extra,
>> and I think spin_lock is also needed for addition and deletion to handle multiple modifiers.
> If that's the case, then that's fine, it just didn't seem like that was
> needed.  Or I might have been reading your rcu logic incorrectly...
>
>>>> +	if (error)
>>>> +		kfree(meminfo);
>>>> +out:
>>>> +
>>>> +	return error;
>>>> +}
>>>> +EXPORT_SYMBOL(register_meminfo_extra);
>>> EXPORT_SYMBOL_GPL()?  I have to ask :)
>> I can use EXPORT_SYMBOL_GPL.
>>> thanks,
>>>
>>> greg k-h
>>>
>>>
>> Hello
>> Thank you for your comment.
>>
>> By the way there was not resolved discussion on v1 patch as I mentioned on cover page.
>> I'd like to hear your opinion on this /proc/meminfo_extra node.
> I think it is the propagation of an old and obsolete interface that you
> will have to support for the next 20+ years and yet not actually be
> useful :)
>
>> Do you think this is meaningful or cannot co-exist with other future
>> sysfs based API.
> What sysfs-based API?
Please refer to mail thread on v1 patch set - https://lkml.org/lkml/fancy/2020/3/10/2102
especially discussion with Leon Romanovsky on https://lkml.org/lkml/fancy/2020/3/16/140

>
> I still don't know _why_ you want this.  The ION stuff is not needed as
> that code is about to be deleted, so who else wants this?  What is the
> use-case for it that is so desperately needed that parsing
> yet-another-proc file is going to solve the problem?
In my Android device, there are graphic driver memory, zsmalloc memory except ION.
I don't know other cases in other platform.
Not desperately needed but I think we need one userspace knob to see overall hidden huge memory.

Additionally I'd like to see all those hidden memory in OutOfMemory log.
This is useful to get clue to find memory hogger.
i.e.) show_mem on oom
<6>[  420.856428]  Mem-Info:
<6>[  420.856433]  IonSystemHeap:32813kB ZsPages:44114kB GraphicDriver::13091kB
<6>[  420.856450]  active_anon:957205 inactive_anon:159383 isolated_anon:0

Thank you
Jaewon Kim
>
> thanks,
>
> greg k-h
>
>