On Fri, Mar 31, 2017 at 05:53:00PM +0800, Wei Wang wrote:
> On 03/30/2017 01:48 AM, Michael S. Tsirkin wrote:
> > On Fri, Mar 17, 2017 at 02:55:33PM +0800, Wei Wang wrote:
> > > On 03/17/2017 05:28 AM, Andrew Morton wrote:
> > > > On Thu, 16 Mar 2017 15:08:46 +0800 Wei Wang <wei.w.wang@xxxxxxxxx> wrote:
> > > >
> > > > > From: Liang Li <liang.z.li@xxxxxxxxx>
> > > > >
> > > > > This patch adds a function to provides a snapshot of the present system
> > > > > unused pages. An important usage of this function is to provide the
> > > > > unsused pages to the Live migration thread, which skips the transfer of
> > > > > thoses unused pages. Newly used pages can be re-tracked by the dirty
> > > > > page logging mechanisms.
> > > > I don't think this will be useful for anything other than
> > > > virtio-balloon. I guess it would be better to keep this code in the
> > > > virtio-balloon driver if possible, even though that's rather a layering
> > > > violation :( What would have to be done to make that possible? Perhaps
> > > > we can put some *small* helpers into page_alloc.c to prevent things
> > > > from becoming too ugly.
> > > The patch description was too narrowed and may have caused some
> > > confusion, sorry about that. This function is aimed to be generic. I
> > > agree with the description suggested by Michael.
> > >
> > > Since the main body of the function is related to operating on the
> > > free_list. I think it is better to have them located here.
> > > Small helpers may be less efficient and thereby causing some
> > > performance loss as well.
> > > I think one improvement we can make is to remove the "chunk format"
> > > related things from this function. The function can generally offer the
> > > base pfn to the caller's recording buffer. Then it will be the caller's
> > > responsibility to format the pfn if they need.
> > Sounds good at a high level, but we'd have to see the implementation
> > to judge it properly.
> >
> > > > > --- a/mm/page_alloc.c
> > > > > +++ b/mm/page_alloc.c
> > > > > @@ -4498,6 +4498,120 @@ void show_free_areas(unsigned int filter)
> > > > > show_swap_cache_info();
> > > > > }
> > > > > +static int __record_unused_pages(struct zone *zone, int order,
> > > > > + __le64 *buf, unsigned int size,
> > > > > + unsigned int *offset, bool part_fill)
> > > > > +{
> > > > > + unsigned long pfn, flags;
> > > > > + int t, ret = 0;
> > > > > + struct list_head *curr;
> > > > > + __le64 *chunk;
> > > > > +
> > > > > + if (zone_is_empty(zone))
> > > > > + return 0;
> > > > > +
> > > > > + spin_lock_irqsave(&zone->lock, flags);
> > > > > +
> > > > > + if (*offset + zone->free_area[order].nr_free > size && !part_fill) {
> > > > > + ret = -ENOSPC;
> > > > > + goto out;
> > > > > + }
> > > > > + for (t = 0; t < MIGRATE_TYPES; t++) {
> > > > > + list_for_each(curr, &zone->free_area[order].free_list[t]) {
> > > > > + pfn = page_to_pfn(list_entry(curr, struct page, lru));
> > > > > + chunk = buf + *offset;
> > > > > + if (*offset + 2 > size) {
> > > > > + ret = -ENOSPC;
> > > > > + goto out;
> > > > > + }
> > > > > + /* Align to the chunk format used in virtio-balloon */
> > > > > + *chunk = cpu_to_le64(pfn << 12);
> > > > > + *(chunk + 1) = cpu_to_le64((1 << order) << 12);
> > > > > + *offset += 2;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > +out:
> > > > > + spin_unlock_irqrestore(&zone->lock, flags);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > This looks like it could disable interrupts for a long time. Too long?
> > > What do you think if we give "budgets" to the above function?
> > > For example, budget=1000, and there are 2000 nodes on the list.
> > > record() returns with "incomplete" status in the first round, along with the
> > > status info, "*continue_node".
> > >
> > > *continue_node: pointer to the starting node of the leftover. If
> > > *continue_node
> > > has been used at the time of the second call (i.e. continue_node->next ==
> > > NULL),
> > > which implies that the previous 1000 nodes have been used, then the record()
> > > function can simply start from the head of the list.
> > >
> > > It is up to the caller whether it needs to continue the second round
> > > when getting "incomplete".
> > It might be cleaner to add APIs to
> > - start iteration
> > - do one step
> > - end iteration
> >
> > caller can then iterate without too many issues
> >
>
> OK. I will re-implement it with this simple one - get only one node(page
> block) from the list in each call, and check if the time would increase a
> lot in comparison to v8.
>
> Best,
> Wei
Might work though this isn't what was suggested - just an iterator based
approach that allows user to drop the lock periodically.
--
MST
