On Thu, May 05, 2022 at 01:42:17AM +0100, Matthew Wilcox wrote:
> On Wed, May 04, 2022 at 04:47:51PM -0700, Minchan Kim wrote:
> > Since this patch introduces a new "contended" field as out param
> > in rmap_walk_control, it's not immutable any longer so remove
> > const keywords on rmap related functions. Since rmap walking
> > is already expensive operation, I doubt the const would help sizable
> > benefit(And we didn't have it until 5.17).
>
> Um? If it's now mutable, it surely can't be static as that means it
> would be shared by all callers, and you might write to the new fields
> in one caller and have them interpreted by the other caller!
>
> Or if it is safe, then the comment needs to not be deleted, but modified
> to explain why it's safe to do so in this instance, and that other
> instances should not copy the approach unless they are similarly safe.
It's safe since rwc.contended is used only when rwc.try_lock is true.
>
> > diff --git a/mm/page_idle.c b/mm/page_idle.c
> > index fc0435abf909..fdff8c6dcd2d 100644
> > --- a/mm/page_idle.c
> > +++ b/mm/page_idle.c
> > @@ -86,11 +86,8 @@ static bool page_idle_clear_pte_refs_one(struct folio *folio,
> > static void page_idle_clear_pte_refs(struct page *page)
> > {
> > struct folio *folio = page_folio(page);
> > - /*
> > - * Since rwc.arg is unused, rwc is effectively immutable, so we
> > - * can make it static const to save some cycles and stack.
> > - */
> > - static const struct rmap_walk_control rwc = {
> > +
> > + static struct rmap_walk_control rwc = {
> > .rmap_one = page_idle_clear_pte_refs_one,
> > .anon_lock = folio_lock_anon_vma_read,
> > };
So, delta is
diff --git a/mm/page_idle.c b/mm/page_idle.c
index fdff8c6dcd2d..bc08332a609c 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -87,6 +87,10 @@ static void page_idle_clear_pte_refs(struct page *page)
{
struct folio *folio = page_folio(page);
+ /*
+ * Since rwc.try_lock is unused, rwc is effectively immutable, so we
+ * can make it static to save some cycles and stack.
+ */
static struct rmap_walk_control rwc = {
.rmap_one = page_idle_clear_pte_refs_one,
.anon_lock = folio_lock_anon_vma_read,
