Re: [PATCH -mm -v7 4/9] mm, THP, swap: Add get_huge_swap_page()

[Johannes Weiner]

On Thu, Mar 30, 2017 at 12:28:17PM +0800, Huang, Ying wrote:
> Johannes Weiner <hannes@xxxxxxxxxxx> writes:
> > On Tue, Mar 28, 2017 at 01:32:04PM +0800, Huang, Ying wrote:
> >> @@ -527,6 +527,23 @@ static inline swp_entry_t get_swap_page(void)
> >>  
> >>  #endif /* CONFIG_SWAP */
> >>  
> >> +#ifdef CONFIG_THP_SWAP_CLUSTER
> >> +static inline swp_entry_t get_huge_swap_page(void)
> >> +{
> >> +	swp_entry_t entry;
> >> +
> >> +	if (get_swap_pages(1, &entry, true))
> >> +		return entry;
> >> +	else
> >> +		return (swp_entry_t) {0};
> >> +}
> >> +#else
> >> +static inline swp_entry_t get_huge_swap_page(void)
> >> +{
> >> +	return (swp_entry_t) {0};
> >> +}
> >> +#endif
> >
> > Your introducing a function without a user, making it very hard to
> > judge whether the API is well-designed for the callers or not.
> >
> > I pointed this out as a systemic problem with this patch series in v3,
> > along with other stuff, but with the way this series is structured I'm
> > having a hard time seeing whether you implemented my other feedback or
> > whether your counter arguments to them are justified.
> >
> > I cannot review and ack these patches this way.
> 
> Sorry for inconvenience, I will send a new version to combine the
> function definition and usage into one patch at least for you to
> review.

We tried this before. I reviewed the self-contained patch and you
incorporated the feedback into the split-out structure that made it
impossible for me to verify the updates.

I'm not sure why you insist on preserving this series format. It's not
good for review, and it's not good for merging and git history.

> But I think we can continue our discussion in the comments your
> raised so far firstly, what do you think about that?

Yeah, let's finish the discussions before -v8.