Kairui Song <[email protected]> writes:
> From: Kairui Song <[email protected]>
>
> Since all callers of swapin_entry need to check the swap cache first, we
> can merge this common routine into swapin_entry, so it can be shared and
> optimized later.
>
> Also introduce a enum to better represent possible swap cache usage, and
> add some comments about it, make the usage of swap cache easier to
> understand.
I don't find any benefit to do this. The code line number isn't
reduced. The concept of swap cache isn't hided either.
--
Best Regards,
Huang, Ying
Huang, Ying <[email protected]> 于2024年1月8日周一 16:28写道：
>
> Kairui Song <[email protected]> writes:
>
> > From: Kairui Song <[email protected]>
> >
> > Since all callers of swapin_entry need to check the swap cache first, we
> > can merge this common routine into swapin_entry, so it can be shared and
> > optimized later.
> >
> > Also introduce a enum to better represent possible swap cache usage, and
> > add some comments about it, make the usage of swap cache easier to
> > understand.
>
> I don't find any benefit to do this. The code line number isn't
> reduced. The concept of swap cache isn't hided either.
Hi Ying
Thanks for the comments.
Maybe I should squash this with the following commit? The following
commit want to do cache lookup in swapin_entry to avoid a redundant
shadow lookup, it can help to improve the performance by about 4% for
swapin path. So it need to return a enum to represent cache status.
Further more, note the comments added here:
+/*
+ * Caller of swapin_entry may need to know the cache lookup result:
+ *
+ * SWAP_CACHE_HIT: cache hit, cached folio is retured.
+ * SWAP_CACHE_MISS: cache miss, folio is allocated, read from swap device
+ * and adde to swap cache, but still may return a cached
+ * folio if raced (check __read_swap_cache_async).
+ * SWAP_CACHE_BYPASS: cache miss, folio is new allocated and read
+ * from swap device bypassing the cache.
+ */
SWAP_CACHE_MISS might be inaccurate, this is not an issue introduced
by this commit, but better exposed. May worth a fix later. So far I
can see two benefits fixing it:
- More accurate maj/min page fault count.
- Note the PageHWPoison check in do_swap_page, it ignored the race
case, if a page getting poisoned is raced with swapcache then it may
not work as expected.
These are all minor issue indeed, some other optimization might also
be doable, but at least a comment might be helpful.
> --
> Best Regards,
> Huang, Ying
>
Kairui Song <[email protected]> writes:
> Huang, Ying <[email protected]> 于2024年1月8日周一 16:28写道：
>>
>> Kairui Song <[email protected]> writes:
>>
>> > From: Kairui Song <[email protected]>
>> >
>> > Since all callers of swapin_entry need to check the swap cache first, we
>> > can merge this common routine into swapin_entry, so it can be shared and
>> > optimized later.
>> >
>> > Also introduce a enum to better represent possible swap cache usage, and
>> > add some comments about it, make the usage of swap cache easier to
>> > understand.
>>
>> I don't find any benefit to do this. The code line number isn't
>> reduced. The concept of swap cache isn't hided either.
>
> Hi Ying
>
> Thanks for the comments.
>
> Maybe I should squash this with the following commit? The following
> commit want to do cache lookup in swapin_entry to avoid a redundant
> shadow lookup, it can help to improve the performance by about 4% for
> swapin path.
It's good to improve performance. But I don't think we must put
swap_cache_get_folio() in swapin_entry() to do that. We can just get
"shadow" from swap_cache_get_folio() and pass it to swapin_entry().
> So it need to return a enum to represent cache status.
I don't think we are talking about the same thing here.
> Further more, note the comments added here:
>
> +/*
> + * Caller of swapin_entry may need to know the cache lookup result:
> + *
> + * SWAP_CACHE_HIT: cache hit, cached folio is retured.
> + * SWAP_CACHE_MISS: cache miss, folio is allocated, read from swap device
> + * and adde to swap cache, but still may return a cached
> + * folio if raced (check __read_swap_cache_async).
> + * SWAP_CACHE_BYPASS: cache miss, folio is new allocated and read
> + * from swap device bypassing the cache.
> + */
>
> SWAP_CACHE_MISS might be inaccurate, this is not an issue introduced
> by this commit, but better exposed. May worth a fix later. So far I
> can see two benefits fixing it:
> - More accurate maj/min page fault count.
> - Note the PageHWPoison check in do_swap_page, it ignored the race
> case, if a page getting poisoned is raced with swapcache then it may
> not work as expected.
>
> These are all minor issue indeed, some other optimization might also
> be doable, but at least a comment might be helpful.
>
--
Best Regards,
Huang, Ying
Huang, Ying <[email protected]> 于2024年1月15日周一 09:47写道：
>
> Kairui Song <[email protected]> writes:
>
> > Huang, Ying <[email protected]> 于2024年1月8日周一 16:28写道：
> >>
> >> Kairui Song <[email protected]> writes:
> >>
> >> > From: Kairui Song <[email protected]>
> >> >
> >> > Since all callers of swapin_entry need to check the swap cache first, we
> >> > can merge this common routine into swapin_entry, so it can be shared and
> >> > optimized later.
> >> >
> >> > Also introduce a enum to better represent possible swap cache usage, and
> >> > add some comments about it, make the usage of swap cache easier to
> >> > understand.
> >>
> >> I don't find any benefit to do this. The code line number isn't
> >> reduced. The concept of swap cache isn't hided either.
> >
> > Hi Ying
> >
> > Thanks for the comments.
> >
> > Maybe I should squash this with the following commit? The following
> > commit want to do cache lookup in swapin_entry to avoid a redundant
> > shadow lookup, it can help to improve the performance by about 4% for
> > swapin path.
>
> It's good to improve performance. But I don't think we must put
> swap_cache_get_folio() in swapin_entry() to do that. We can just get
> "shadow" from swap_cache_get_folio() and pass it to swapin_entry().
Good idea. My only concern is, if the argument list is getting too
long (7 args if we added all mpol, ilx, shadow here), will that cause
issue for readability or performance?