Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 25 Nov 2018 23:47:37 -1000
From:   Joey Pabalinas <joeypabalinas@gmail.com>
To:     Minchan Kim <minchan@kernel.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        Joey Pabalinas <joeypabalinas@gmail.com>
Subject: Re: [PATCH v2 5/7] zram: support idle/huge page writeback
Message-ID: <20181126094737.sfudpeoigra2vir7@gmail.com>
Mail-Followup-To: Joey Pabalinas <joeypabalinas@gmail.com>,
        Minchan Kim <minchan@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
References: <20181126082813.81977-1-minchan@kernel.org>
 <20181126082813.81977-6-minchan@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="c5mdpm6lxzx5f4kq"
Content-Disposition: inline
In-Reply-To: <20181126082813.81977-6-minchan@kernel.org>
User-Agent: NeoMutt/20180716
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--c5mdpm6lxzx5f4kq
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Nov 26, 2018 at 05:28:11PM +0900, Minchan Kim wrote:
> +	strlcpy(mode_buf, buf, sizeof(mode_buf));
> +	/* ignore trailing newline */
> +	sz =3D 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 =3D=3D -E2BIG` and do `sz =3D sizeof(mode_buf) - 1` in=
 that
case).

> +	if (!strcmp(mode_buf, "idle"))
> +		mode =3D IDLE_WRITEBACK;
> +	if (!strcmp(mode_buf, "huge"))
> +		mode =3D 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 =3D 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 :)

--=20
Cheers,
Joey Pabalinas

--c5mdpm6lxzx5f4kq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEENpTlWU2hUK/KMvHp2rGdfm9DtVIFAlv7wTgACgkQ2rGdfm9D
tVL31BAAztN6cmovy/+sjVssa8XfRFNqmOOBHbmQvoJYUMxHPh17ZPtzsZlOtRgp
J2neB19lBhauatC2mJoi21Re+/kttdUGgpIus3PMgBzc6dIHkqkkcT1QeEc6wCej
AkL0gIu9pw5qDks/ZIeMGYrIVIrxwXQvhQTBeZ/Wsg+SHqTP78sVixLXPTRChXiu
CQE3ZlrJh8NVImsQofeexokn0OmX1N5yieZw4Fhf2KYFTbnp/4YR+qFrinJKkhoh
ccwXxmtgeg+SdUuBVIYIsCSdsTcMYI1yqoibTJ9tP4Ohu9IDPC5pDFpevW2mua0/
4l1UarsMFmDwr/gjqKxe7r5otj8S+iKMOgB7KKNNY3GlcN3e8gCW4Ss5q3re5Q/N
n03oplrdqrkcHIHQgM8FvbaI8Pa8JHF2cmILbryfiZ1SEnofv1key1+17SM6WFeR
bDLVtypq9Jt/Shw7SYK57C7wUMuVazEh/J7Ld+/NHqwo/BqWdd4CJ08wq+6AZ+cJ
M2LWwlBbXPQ0nWfQGtPOAXAHRhMKQR0ie7p8d89M+0OO736vX9KQYJ7oQ7IazB9X
uqBkKNo0MiXHQzfOKLS1PSJFsdj2tHMzk0/LmUEyLLnCS77M4dowLh0RukQe2jni
y5naS0ykf4wokXc0YhyonGWPrpCIBVulzczhhVFijL+MTPtJSVM=
=n7eU
-----END PGP SIGNATURE-----

--c5mdpm6lxzx5f4kq--