Johannes Weiner <hannes@xxxxxxxxxxx> writes:
> On Tue, Nov 26, 2019 at 08:56:00PM +0100, Giuseppe Scrivano wrote:
>> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
>> index 2ac38bdc18a1..5a6b381e9b92 100644
>> --- a/mm/hugetlb_cgroup.c
>> +++ b/mm/hugetlb_cgroup.c
>> @@ -283,10 +283,55 @@ static u64 hugetlb_cgroup_read_u64(struct cgroup_subsys_state *css,
>> }
>> }
>>
>> +static int hugetlb_cgroup_read_u64_max(struct seq_file *seq, void *v)
>> +{
>> + int idx;
>> + u64 val;
>> + bool write_raw = false;
>> + struct cftype *cft = seq_cft(seq);
>> + unsigned long limit;
>> + struct page_counter *counter;
>> + struct hugetlb_cgroup *h_cg = hugetlb_cgroup_from_css(seq_css(seq));
>> +
>> + idx = MEMFILE_IDX(cft->private);
>> + counter = &h_cg->hugepage[idx];
>> +
>> + switch (MEMFILE_ATTR(cft->private)) {
>> + case RES_USAGE:
>> + val = (u64)page_counter_read(counter);
>> + break;
>> + case RES_LIMIT:
>> + val = (u64)counter->max;
>> + break;
>> + case RES_MAX_USAGE:
>> + val = (u64)counter->watermark;
>> + break;
>
> This case is dead code now.
>
>> + case RES_FAILCNT:
>> + val = counter->failcnt;
>> + write_raw = true;
>> + break;
>> + default:
>> + BUG();
>> + }
>> +
>> + limit = round_down(PAGE_COUNTER_MAX,
>> + 1 << huge_page_order(&hstates[idx]));
>> +
>> + if (val == limit && !write_raw)
>> + seq_puts(seq, "max\n");
>
> This branch applies (or should apply!) only to RES_LIMIT, never
> RES_USAGE or RES_FAILCNT.
>
>> + else if (write_raw)
>> + seq_printf(seq, "%llu\n", val);
>
> This applies only to RES_FAILCNT
>
>> + else
>> + seq_printf(seq, "%llu\n", val * PAGE_SIZE);
>
> And this applies to RES_USAGE and RES_LIMIT.
>
> But this seems unnecessarily obscure. Can you just put the
> seq_printf()s directly into the case statements?
yes, I'll fix it. I'll also drop the branch for RES_FAILCNT as now that
is handled by hugetlb_events_show.
Thanks,
Giuseppe
