From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bob Liu Subject: Re: [PATCH v2 3/3] xen: use idle vcpus to scrub pages Date: Fri, 25 Jul 2014 15:28:52 +0800 Message-ID: <53D20734.8020902@oracle.com> References: <1404135584-29206-1-git-send-email-bob.liu@oracle.com> <1404135584-29206-3-git-send-email-bob.liu@oracle.com> <53B2979C020000780001EE97@mail.emea.novell.com> <53B2A8C7.9040601@oracle.com> <53B2CCD1020000780001F027@mail.emea.novell.com> <53C4F171.8060807@oracle.com> <53CF80400200007800024F2B@mail.emea.novell.com> <53D06A8B.7010804@oracle.com> <53D0C2D1020000780002556F@mail.emea.novell.com> <53D1A80F.9040509@oracle.com> <53D21AA40200007800025CA7@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1XAZwF-00049o-0a for xen-devel@lists.xenproject.org; Fri, 25 Jul 2014 07:29:11 +0000 In-Reply-To: <53D21AA40200007800025CA7@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Bob Liu , keir@xen.org, ian.campbell@citrix.com, George.Dunlap@eu.citrix.com, andrew.cooper3@citrix.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 07/25/2014 02:51 PM, Jan Beulich wrote: >>>> On 25.07.14 at 02:42, wrote: > >> On 07/24/2014 02:24 PM, Jan Beulich wrote: >>>>>> On 24.07.14 at 04:08, wrote: >>> >>>> On 07/23/2014 03:28 PM, Jan Beulich wrote: >>>>>>>> On 15.07.14 at 11:16, wrote: >>>>>> After so many days I haven't make a workable solution if don't remove >>>>>> pages temporarily. The hardest part is iterating the heap free list >>>>>> without holding heap_lock because if holding the lock it might be heavy >>>>>> lock contention. >>>>>> So do you think it's acceptable if fixed all other concerns about this >>>>>> patch? >>>>> >>>>> No, I don't think so. Instead I'm of the opinion that you may have >>>>> worked in the wrong direction: Rather than not taking the heap lock >>>>> at all, it may also be sufficient to shrink the lock holding time (i.e. >>>>> avoid long loops with the lock held). >>>>> >>>> >>>> But I still think have to drop pages from heap list temporarily else >>>> heap lock must be taken for a long time to get rid of E.g. below race >>>> condition. >>>> >>>> A: alloc path B: idle loop >>>> >>>> spin_lock(&heap_lock) >>>> page_list_for_each( pg, &heap(node, zone, order) ) >>>> if _PGC_need_scrub is set, break; >>>> spin_unlock(&heap_lock) >>>> >>>> if ( test_bit(_PGC_need_scrub, pg) >>>> >>>> ^^^^ >>>> spin_lock(&heap_lock) >>>> delist page >>>> spin_unlock(&heap_lock) >>>> >>>> write data to this page >>>> >>>> scrub_one_page(pg) >>>> ^^^ will clean useful data >>> >>> No (and I'm sure I said so before): The only problem is with the >>> linked list itself; the page contents are not a problem - the >>> allocation path can simply wait for the already suggested >>> _PGC_scrubbing flag to clear before returning. And as already >> >> The page contents are a problem if the race condition I mentioned in >> previous email happen. >> >> Because there is a time window between checking the PGC_need_scrub flag >> and doing the real scrub in idle thread, the idle thread will still >> scrub a page after that page have been allocated by allocation path and >> been used(and have been written some useful data). > > Did you really read all of my previous reply? > Sure, may be I misunderstood your reply. If the allocation path can wait for the flag there is no problem, but I remember you suggested to do the scrubbing also in allocation path in which case I think this race condition will happen. -- Regards, -Bob