Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

Hello,

sorry, was mostly offline for the past few days, now catching up.

On (01/10/17 08:41), Minchan Kim wrote:
> > the idea is that without doing more calculations we extend zero pages
> > to same element pages for zram. zero page is special case of
> > same element page with zero element.
> > 

interesting idea.

[..]
> >  	flush_dcache_page(page);
> > @@ -431,7 +479,7 @@ static ssize_t mm_stat_show(struct device *dev,
> >  			mem_used << PAGE_SHIFT,
> >  			zram->limit_pages << PAGE_SHIFT,
> >  			max_used << PAGE_SHIFT,
> > -			(u64)atomic64_read(&zram->stats.zero_pages),
> > +			(u64)atomic64_read(&zram->stats.same_pages),
> 
> Unfortunately, we cannot replace zero pages stat with same pages's one right
> now due to compatibility problem. Please add same_pages to tail of the stat
> and we should warn deprecated zero_pages stat so we finally will remove it
> two year later. Please reference Documentation/ABI/obsolete/sysfs-block-zram
> And add zero-pages to the document.
> 
> For example,
> 
> ... mm_stat_show()
> {
>         pr_warn_once("zero pages was deprecated so it will be removed at 2019 Jan");
> }
> 
> Sergey, what's your opinion?

oh, I was going to ask you whether you have any work in progress at
the moment or not. because deprecated attrs are scheduled to be removed
in 4.11. IOW, we must send the clean up patch, well, right now. so I can
prepare the patch, but it can conflict with someone's 'more serious/relevant'
work.

we also have zram hot/addd sysfs attr, which must be deprecated and
converted to a char device. per Greg KH.

> Please add same_pages to tail of the stat

sounds ok to me. and yes, can deprecate zero_pages.

seems that with that patch the concept of ZRAM_ZERO disappears. both
ZERO and SAME_ELEMENT pages are considered to be the same thing now.
which is fine and makes sense to me, I think. and if ->.same_pages will
replace ->.zero_pages in mm_stat() then I'm also OK. yes, we will see
increased number in the last column of mm_stat file, but I don't tend
to see any issues here. Minchan, what do you think?


> -ZRAM_ATTR_RO(zero_pages);
> +ZRAM_ATTR_RO(same_pages);

this part is a no-no-no-no :)  we can't simply rename the user space
visible attrs.

	-ss