Messages in this thread | | | From | "Huang\, Ying" <> | Subject | Re: [PATCH -mm -V3 03/21] mm, THP, swap: Support PMD swap mapping in swap_duplicate() | Date | Tue, 12 Jun 2018 09:23:19 +0800 |
| |
Hi, Daniel,
Thanks for your effort to review this series.
Daniel Jordan <daniel.m.jordan@oracle.com> writes:
> Hi, > > The series up to and including this patch doesn't build. For this patch we > need: > > diff --git a/mm/swap_state.c b/mm/swap_state.c > index c6b3eab73fde..2f2d07627113 100644 > --- a/mm/swap_state.c > +++ b/mm/swap_state.c > @@ -433,7 +433,7 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask, > /* > * Swap entry may have been freed since our caller observed it. > */ > - err = swapcache_prepare(entry); > + err = swapcache_prepare(entry, false); > if (err == -EEXIST) { > radix_tree_preload_end(); > /*
Thanks for pointing this out! Will change in the next version.
> > On Wed, May 23, 2018 at 04:26:07PM +0800, Huang, Ying wrote: >> @@ -3516,11 +3512,39 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage) > > Two comments about this part of __swap_duplicate as long as you're moving it to > another function: > > } else if (count || has_cache) { > > if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX) /* #1 */ > count += usage; > else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) /* #2 */ > err = -EINVAL; > > #1: __swap_duplicate_locked might use > > VM_BUG_ON(usage != SWAP_HAS_CACHE && usage != 1); > > to document the unstated assumption that usage is 1 (otherwise count could > overflow).
Sounds good. Will do this.
> #2: We've masked off SWAP_HAS_CACHE and COUNT_CONTINUED, and already checked > for SWAP_MAP_BAD, so I think condition #2 always fails and can just be removed.
I think this is used to check some software bug. For example, SWAP_MAP_SHMEM will yield true here.
>> +#ifdef CONFIG_THP_SWAP >> +static int __swap_duplicate_cluster(swp_entry_t *entry, unsigned char usage) > ... >> + } else { >> + for (i = 0; i < SWAPFILE_CLUSTER; i++) { >> +retry: >> + err = __swap_duplicate_locked(si, offset + i, 1); > > I guess usage is assumed to be 1 at this point (__swap_duplicate_locked makes > the same assumption). Maybe make this explicit with > > err = __swap_duplicate_locked(si, offset + i, usage); > > , use 'usage' in cluster_set_count and __swap_entry_free too, and then > earlier have a > > VM_BUG_ON(usage != SWAP_HAS_CACHE && usage != 1); > > ?
Yes. I will fix this. And we can just check it in __swap_duplicate_locked() and all these will be covered.
>> +#else >> +static inline int __swap_duplicate_cluster(swp_entry_t *entry, > > This doesn't need inline.
Why not? This is just a one line stub.
> Not related to your changes, but while we're here, the comment with > SWAP_HAS_CONT in swap_count() could be deleted: I don't think there ever was a > SWAP_HAS_CONT.
Yes. We should correct this. Because this should go to a separate patch, would you mind to submit a patch to fix it?
> The rest looks ok up to this point.
Thanks!
Best Regards, Huang, Ying
| |