From: Shakeel Butt <shakeelb@google.com>
To: Roman Gushchin <roman.gushchin@linux.dev>
Cc: Yosry Ahmed <yosryahmed@google.com>,
linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
Johannes Weiner <hannes@cmpxchg.org>,
Michal Hocko <mhocko@kernel.org>,
Muchun Song <muchun.song@linux.dev>,
linux-kernel@vger.kernel.org,
syzbot+774c29891415ab0fd29d@syzkaller.appspotmail.com,
Dmitry Vyukov <dvyukov@google.com>
Subject: Re: [PATCH v2 1/2] mm: kmem: fix a NULL pointer dereference in obj_stock_flush_required()
Date: Wed, 3 May 2023 17:03:47 +0000 [thread overview]
Message-ID: <20230503170347.ldrrtenh57trfpdy@google.com> (raw)
In-Reply-To: <ZFGCy1d/wM+JC7xS@P9FQF9L96D.corp.robot.car>
On Tue, May 02, 2023 at 02:38:19PM -0700, Roman Gushchin wrote:
[...]
> >
> > I believe all read accesses other than obj_stock_flush_required() are
> > done under the lock, so READ_ONCE() wouldn't be needed AFAICT. Having
> > READ_ONCE() only around the racy read can be useful to document the
> > racy read and differentiate it from others.
> >
> > With that said, it's also inconvenient to keep track moving forward of
> > which reading sites are racy, and it may be simpler to just annotate
> > all readers with READ_ONCE().
> >
> > I am not sure which approach is better, just thinking out loud.
>
> Yeah, I wasn't sure either. I believe that all changes except the original
> READ_ONCE() are not leading to any meaningful asm changes, so it's a matter
> of taste.
>
> The reason why I went with the "change them all" approach:
> reads without READ_ONCE() and subsequent writes with WRITE_ONCE()
> inside a single function looked really weird.
>
Change them all is the right approach. This code will evolve in future
and having partial tagging will cause confusion or might be missed
altogether. Also the automated tools prefer change them all.
next prev parent reply other threads:[~2023-05-03 17:03 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-02 16:08 Roman Gushchin
2023-05-02 16:08 ` [PATCH v2 2/2] mm: memcg: use READ_ONCE()/WRITE_ONCE() to access stock->cached Roman Gushchin
2023-05-02 20:21 ` Yosry Ahmed
2023-05-03 17:06 ` Shakeel Butt
2023-05-02 20:15 ` [PATCH v2 1/2] mm: kmem: fix a NULL pointer dereference in obj_stock_flush_required() Yosry Ahmed
2023-05-02 21:38 ` Roman Gushchin
2023-05-02 22:10 ` Yosry Ahmed
2023-05-03 17:03 ` Shakeel Butt [this message]
2023-05-03 8:05 ` Dmitry Vyukov
2023-05-03 17:04 ` Shakeel Butt
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=20230503170347.ldrrtenh57trfpdy@google.com \
--to=shakeelb@google.com \
--cc=akpm@linux-foundation.org \
--cc=dvyukov@google.com \
--cc=hannes@cmpxchg.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=muchun.song@linux.dev \
--cc=roman.gushchin@linux.dev \
--cc=syzbot+774c29891415ab0fd29d@syzkaller.appspotmail.com \
--cc=yosryahmed@google.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