by Joey Pabalinas

[permalink] [raw]

Subject: Re: [PATCH v2 5/7] zram: support idle/huge page writeback

On Mon, Nov 26, 2018 at 05:28:11PM +0900, Minchan Kim wrote:
> + strlcpy(mode_buf, buf, sizeof(mode_buf));
> + /* ignore trailing newline */
> + sz = strlen(mode_buf);

One possible idea would be to use strscpy() instead and directly assign
the return value to sz, avoiding an extra strlen() call (though you would
have to check if `sz == -E2BIG` and do `sz = sizeof(mode_buf) - 1` in that
case).

> + if (!strcmp(mode_buf, "idle"))
> + mode = IDLE_WRITEBACK;
> + if (!strcmp(mode_buf, "huge"))
> + mode = HUGE_WRITEBACK;

Maybe using `else if (!strcmp(mode_buf, "huge"))` would be slightly
better here, avoiding a second strcmp() if mode_buf has already
matched "idle".

> + if ((mode & IDLE_WRITEBACK &&
> + !zram_test_flag(zram, index, ZRAM_IDLE)) &&
> + (mode & HUGE_WRITEBACK &&
> + !zram_test_flag(zram, index, ZRAM_HUGE)))
> + goto next;

Wouldn't writing this as `mode & (IDLE_WRITEBACK | HUGE_WRITEBACK)`
be a bit easier to read as well as slightly more compact?

> + ret = len;
> + __free_page(page);
> +release_init_lock:
> + up_read(&zram->init_lock);
> + return ret;

Hm, I noticed that this function either returns an error or just the passed
in len on success, and I'm left wondering if there might be other useful
information which could be passed back to the caller instead. I can't
immediately think of any such information, though, so it's possible I'm
just daydreaming :)

--
Cheers,
Joey Pabalinas

Attachments:

(No filename) (1.45 kB)
signature.asc (849.00 B)
Download all attachments

2018-11-26 13:46:39

On Tue, Nov 27, 2018 at 11:13:27AM +0900, Minchan Kim wrote:
> On Sun, Nov 25, 2018 at 11:47:37PM -1000, Joey Pabalinas wrote:
> > On Mon, Nov 26, 2018 at 05:28:11PM +0900, Minchan Kim wrote:
> > > + strlcpy(mode_buf, buf, sizeof(mode_buf));
> > > + /* ignore trailing newline */
> > > + sz = strlen(mode_buf);
> >
> > One possible idea would be to use strscpy() instead and directly assign
> > the return value to sz, avoiding an extra strlen() call (though you would
> > have to check if `sz == -E2BIG` and do `sz = sizeof(mode_buf) - 1` in that
> > case).
>
> Thanks for the suggstion.
> If I limit destination buffer smaller, I couldn't meet -E2BIG?

-E2BIG return from strscpy() just means the string was longer than would
fit in the length passed as the third argument. This means only
`sizeof(mode_buf) - 1` bytes were copied into the dest, with the last
byte being the \0 terminator.

You can find the function source in lib/string.c.

> > > + ret = len;
> > > + __free_page(page);
> > > +release_init_lock:
> > > + up_read(&zram->init_lock);
> > > + return ret;
> >
> > Hm, I noticed that this function either returns an error or just the passed
> > in len on success, and I'm left wondering if there might be other useful
> > information which could be passed back to the caller instead. I can't
> > immediately think of any such information, though, so it's possible I'm
> > just daydreaming :)
>
> It is write syscall semantic of sysfs so not sure it's doable to pass
> other value to user.

Well, with the write system call you can have partial writes, in which
case it would return the (short) number of bytes written. But in this
function, even if there was some sort of partial write possible, this
function still only every returns len or error.

Neither of these are that important though, so Ack from me with or
without these two suggestions.

Reviewed-by: Joey Pabalinas <[email protected]>

--
Cheers,
Joey Pabalinas

Attachments:

(No filename) (1.95 kB)
signature.asc (849.00 B)
Download all attachments

2018-11-28 23:31:18

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] zram: add bd_stat statistics

On Tue, 27 Nov 2018 11:07:54 +0900 Minchan Kim <[email protected]> wrote:

> On Mon, Nov 26, 2018 at 12:58:33PM -0800, Andrew Morton wrote:
> > On Mon, 26 Nov 2018 17:28:12 +0900 Minchan Kim <[email protected]> wrote:
> >
> > > +File /sys/block/zram<id>/bd_stat
> > > +
> > > +The stat file represents device's backing device statistics. It consists of
> > > +a single line of text and contains the following stats separated by whitespace:
> > > + bd_count size of data written in backing device.
> > > + Unit: pages
> > > + bd_reads the number of reads from backing device
> > > + Unit: pages
> > > + bd_writes the number of writes to backing device
> > > + Unit: pages
> >
> > Using `pages' is a bad choice. And I assume this means that
> > writeback_limit is in pages as well, which is worse.
> >
> > Page sizes are not constant! We want userspace which was developed on
> > 4k pagesize to work the same on 64k pagesize.
> >
> > Arguably, we could require that well-written userspace remember to use
> > getpagesize(). However we have traditionally tried to avoid that by
> > performing the pagesize normalization within the kernel.
>
> zram works based on page so I used that term but I agree it's rather
> vague. If there is no objection, I will use (Unit: 4K) instead of
> (Unit: pages).

Is that still true if PAGE_SIZE=64k?

2018-11-29 01:46:33

by Minchan Kim

[permalink] [raw]

Subject: Re: [PATCH v2 6/7] zram: add bd_stat statistics

On Wed, Nov 28, 2018 at 03:30:21PM -0800, Andrew Morton wrote:
> On Tue, 27 Nov 2018 11:07:54 +0900 Minchan Kim <[email protected]> wrote:
>
> > On Mon, Nov 26, 2018 at 12:58:33PM -0800, Andrew Morton wrote:
> > > On Mon, 26 Nov 2018 17:28:12 +0900 Minchan Kim <[email protected]> wrote:
> > >
> > > > +File /sys/block/zram<id>/bd_stat
> > > > +
> > > > +The stat file represents device's backing device statistics. It consists of
> > > > +a single line of text and contains the following stats separated by whitespace:
> > > > + bd_count size of data written in backing device.
> > > > + Unit: pages
> > > > + bd_reads the number of reads from backing device
> > > > + Unit: pages
> > > > + bd_writes the number of writes to backing device
> > > > + Unit: pages
> > >
> > > Using `pages' is a bad choice. And I assume this means that
> > > writeback_limit is in pages as well, which is worse.
> > >
> > > Page sizes are not constant! We want userspace which was developed on
> > > 4k pagesize to work the same on 64k pagesize.
> > >
> > > Arguably, we could require that well-written userspace remember to use
> > > getpagesize(). However we have traditionally tried to avoid that by
> > > performing the pagesize normalization within the kernel.
> >
> > zram works based on page so I used that term but I agree it's rather
> > vague. If there is no objection, I will use (Unit: 4K) instead of
> > (Unit: pages).
>
> Is that still true if PAGE_SIZE=64k?

Oops, it will fix.