On Wed, Jul 20, 2022 at 08:06:42AM +0530, Anshuman Khandual wrote:
> On 7/20/22 07:16, Barry Song wrote:
> > On Tue, Jul 19, 2022 at 4:04 PM Anshuman Khandual
> > <anshuman.khandual@xxxxxxx> wrote:
> >> On 7/19/22 09:29, Barry Song wrote:
> >>> On Tue, Jul 19, 2022 at 3:35 PM Anshuman Khandual
> >>> <anshuman.khandual@xxxxxxx> wrote:
> >>>> On 7/19/22 08:58, Huang, Ying wrote:
> >>>>> Anshuman Khandual <anshuman.khandual@xxxxxxx> writes:
> >>>>>>>> How about the following?
> >>>>>>>>
> >>>>>>>> static inline bool arch_wants_thp_swap(void)
> >>>>>>>> {
> >>>>>>>> return IS_ENABLED(ARCH_WANTS_THP_SWAP);
> >>>>>>>> }
> >>>>>>>
> >>>>>>> This looks good. then i'll need to change arm64 to
> >>>>>>>
> >>>>>>> +static inline bool arch_thp_swp_supported(void)
> >>>>>>> +{
> >>>>>>> + return IS_ENABLED(ARCH_WANTS_THP_SWAP) && !system_supports_mte();
> >>>>>>> +}
> >>>>>>
> >>>>>> Why ? CONFIG_THP_SWAP depends on ARCH_WANTS_THP_SWAP. In folio_alloc_swap(),
> >>>>>> IS_ENABLED(CONFIG_THP_SWAP) enabled, will also imply ARCH_WANTS_THP_SWAP too
> >>>>>> is enabled. Hence checking for ARCH_WANTS_THP_SWAP again does not make sense
> >>>>>> either in the generic fallback stub, or in arm64 platform override. Because
> >>>>>> without ARCH_WANTS_THP_SWAP enabled, arch_thp_swp_supported() should never
> >>>>>> be called in the first place.
> >>>>>
> >>>>> For the only caller now, the checking looks redundant. But the original
> >>>>> proposed implementation as follows,
> >>>>>
> >>>>> static inline bool arch_thp_swp_supported(void)
> >>>>> {
> >>>>> return true;
> >>>>> }
> >>>>>
> >>>>> will return true even on architectures that don't support/want THP swap.
> >>>>
> >>>> But the function will never be called on for those platforms.
> >>>>
> >>>>> That will confuse people too.
> >>>>
> >>>> I dont see how.
> >>>>
> >>>>>
> >>>>> And the "redundant" checking has no run time overhead, because compiler
> >>>>> will do the trick.
> >>>> I understand that, but dont think this indirection is necessary.
> >>>
> >>> Hi Anshuman, Hi Ying,
> >>> Thanks for the comments of both of you. Does the below look ok?
> >>>
> >>> generic,
> >>>
> >>> static inline bool arch_wants_thp_swap(void)
> >>> {
> >>> return IS_ENABLED(CONFIG_THP_SWAP);
> >>> }
> >>>
> >>> arm64,
> >>>
> >>> static inline bool arch_thp_swp_supported(void)
> >>> {
> >>> return IS_ENABLED(CONFIG_THP_SWAP) && !system_supports_mte();
> >>> }
> >>>
> >>> caller,
> >>>
> >>> folio_alloc_swap(struct folio *folio)
> >>> {
> >>>
> >>> if (folio_test_large(folio)) {
> >>> - if (IS_ENABLED(CONFIG_THP_SWAP))
> >>> + if (arch_thp_swp_supported())
> >>> get_swap_pages(1, &entry, folio_nr_pages(folio));
> >>> goto out;
> >>> }
> >>
> >> Current proposal in this patch LGTM, I dont see any reason for these changes.
> >
> > OK, thanks, Anshuman. Can I collect this as a Reviewed-by?
>
> Yes please.
>
> Reviewed-by: Anshuman Khandual <anshuman.khandual@xxxxxxx>
I've lost track of exactly what the outcome here is, so Barry, please can
you send a final version of the agreed-upon patch?
Thanks,
Will
