From: Sergey Senozhatsky <senozhatsky@chromium.org>
To: Vasily Averin <vasily.averin@linux.dev>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>,
Minchan Kim <minchan@kernel.org>,
linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
zhouxianrong <zhouxianrong@huawei.com>
Subject: Re: [PATCH] zram: extra zram_get_element call in zram_read_from_zspool()
Date: Wed, 8 Nov 2023 12:32:44 +0900 [thread overview]
Message-ID: <20231108033244.GH11577@google.com> (raw)
In-Reply-To: <b87ff5e2-156f-4bf8-9001-9cfbb79871ae@linux.dev>
On (23/11/08 06:16), Vasily Averin wrote:
> On 11/8/23 05:49, Sergey Senozhatsky wrote:
> > On (23/11/06 22:55), Vasily Averin wrote:
> >>
> >> 'element' and 'handle' are union in struct zram_table_entry.
> >>
> >> Fixes: 8e19d540d107 ("zram: extend zero pages to same element pages")
> >
> > Sorry, what exactly does it fix?
>
> It removes unneeded call of zram_get_element() and unneeded variable 'value'.
Yes, what the patch does is pretty clear. It doesn't *fix* anything per se.
> zram_get_element() == zram_get_handle(), they both access the same field of the same struct zram_table_entry,
> no need to read it 2nd time.
> 'value' variable is not required, 'handle' can be used instead.
>
> I hope this explain why element/handle union should be removed: it confuses reviewers.
I do not agree with "union should be removed" part.
In this particular case - using handle as the page pattern (element)
is in fact quite confusing. The visual separation of `handle` and `element`
is helpful.
next prev parent reply other threads:[~2023-11-08 3:32 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-06 19:55 [PATCH] zram: extra zram_get_element call in zram_read_from_zspool() Vasily Averin
2023-11-06 20:03 ` Vasily Averin
2023-11-08 2:48 ` Sergey Senozhatsky
2023-11-08 2:49 ` Sergey Senozhatsky
2023-11-08 3:16 ` Vasily Averin
2023-11-08 3:32 ` Sergey Senozhatsky [this message]
2023-11-08 4:40 ` Vasily Averin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231108033244.GH11577@google.com \
--to=senozhatsky@chromium.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=minchan@kernel.org \
--cc=vasily.averin@linux.dev \
--cc=zhouxianrong@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).