Messages in this thread | | | Subject | Re: [PATCH v2] mm: prototype: rid swapoff of quadratic complexity | From | Vineeth Remanan Pillai <> | Date | Mon, 26 Nov 2018 13:35:30 -0500 |
| |
Hi Mathew,
Thanks for your response!
On 11/26/18 12:22 PM, Matthew Wilcox wrote: > On Mon, Nov 26, 2018 at 04:55:21PM +0000, Vineeth Remanan Pillai wrote: >> + do { >> + XA_STATE(xas, &mapping->i_pages, start); >> + int i; >> + int entries = 0; >> + struct page *page; >> + pgoff_t indices[PAGEVEC_SIZE]; >> + unsigned long end = start + PAGEVEC_SIZE; >> >> + rcu_read_lock(); >> + xas_for_each(&xas, page, end) { > I think this is a mistake. You should probably specify ULONG_MAX for the > end. Otherwise if there are no swap entries in the first 60kB of the file, > you'll just exit. That does mean you'll need to check 'entries' for > hitting PAGEVEC_SIZE.
Thanks for pointing this out. I shall fix this in the next version.
> This seems terribly complicated. You run through i_pages, record the > indices of the swap entries, then go back and look them up again by > calling shmem_getpage() which calls the incredibly complex 300 line > shmem_getpage_gfp(). > > Can we refactor shmem_getpage_gfp() to skip some of the checks which > aren't necessary when called from this path, and turn this into a nice > simple xas_for_each() loop which works one entry at a time?
I shall investigate this and make this simpler as you suggested.
>> + list_for_each_safe(p, next, &shmem_swaplist) { >> + info = list_entry(p, struct shmem_inode_info, swaplist); > This could use list_for_each_entry_safe(), right?
Yes, you are right. Will fix.
Thanks,
Vineeth
>
| |