Message-ID: <56527394.70602@huawei.com>
Date: Mon, 23 Nov 2015 10:01:56 +0800
From: "Wangnan (F)" <wangnan0@huawei.com>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: Arnaldo Carvalho de Melo <acme@kernel.org>
CC: <ast@plumgrid.com>, <brendan.d.gregg@gmail.com>, <a.p.zijlstra@chello.nl>,
        <daniel@iogearbox.net>, <dsahern@gmail.com>, <hekuang@huawei.com>,
        <jolsa@kernel.org>, <lizefan@huawei.com>,
        Ingo Molnar <mingo@kernel.org>, <masami.hiramatsu.pt@hitachi.com>,
        <namhyung@kernel.org>, <paulus@samba.org>,
        <linux-kernel@vger.kernel.org>, <pi3orama@163.com>,
        <xiakaixu@huawei.com>
Subject: Re: [RFC PATCH 5/7] perf tools: Support setting different slots in
 a BPF map separately
References: <1445078910-73699-1-git-send-email-wangnan0@huawei.com> <1445078910-73699-6-git-send-email-wangnan0@huawei.com> <564F1F50.9000807@huawei.com> <20151120153434.GR29361@kernel.org>
In-Reply-To: <20151120153434.GR29361@kernel.org>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2589
Lines: 70


On 2015/11/20 23:34, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 20, 2015 at 09:25:36PM +0800, Wangnan (F) escreveu:
>>> +		case BPF_MAP_PRIV_KEY_INDICS:
>>> +			for (i = 0; i < priv->key.indics.nr_indics; i++) {
>>> +				u64 _idx = priv->key.indics.indics[i];
>>> +				unsigned int idx = (unsigned int)(_idx);
>>> +
>>> +				err = (*func)(name, map_fd, &def,
>>> +					      priv, &idx, arg);
>>> +				if (err) {
>>> +					pr_debug("ERROR: failed to insert value to %s[%u]\n",
>>> +						 name, idx);
>>> +					return err;
>>> +				}
>>> +			}
>> This for-loop has a potential problem that, if perf's user want to
>> set a very big array using indices, for example:
>>
>>   # perf record -e
>> mybpf.c/maps:mymap:values[1,2,3,10-100000,200000-400000]=3/
>> mybpf.c/maps:mymap:values[100000-200000]=3/ ...
>>
>> Perf would alloc nearly 300000 slots for indices array, consume too much
>> memory.
>>
>> I will fix this problem by reinterprete indices array, makes negative
>> value represent range start and use next slot to store range size. For
>> example, the above perf cmdline can be converted to:
>>
>> {1,2,3,-10, 99991,-200000,200001} and {-100000,100001}.
> Why is that changing the way you specify what entries should be set to
> a value will make it not allocate too much memory?

It is actually a problem in the next patch, in which it expand all range
into a series of indices. If user wants 1-10000, it creates an array as
[1,2,3,4,...10000], so user is possible to use a simple cmdline to consume
all of available memory.

However, the method I described above is not the best way to solve this 
probelm.
I thought yesterday that we should not insist on indices array. We can
make parser always return ranges. For example, [1,2,3-5] can be represent
using [(1,1), (2,1), (3,3)], so we don't need the above ugly negative
indicators.

> I found the first form of representing ( start-end ) to be better than (
> -start, size ), but I would use what the C language uses for expressing
> ranges in switch case ranges, which is familiar and doesn't reuses the
> minus arithmetic operator to express a range, i.e.:
>
>   # perf record -e \
>     mybpf.c/maps:mymap:values[1,2,3,10..100000,200000..400000]=3/
>
>   # perf record -e \
>     mybpf.c/maps:mymap:values[100000..200000]=3/ ...

'..' is better.

Thank you.

> - Arnaldo


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/