Re: [Xen-devel] [PATCH 0/2] MCA support with page offlining

[SUZUKI Kazuhiro <kaz@xxxxxxxxxxxxxx>]

Hi Wang,

Thank you very much for your comments.
I will post an updated patch later.

Thanks,
KAZ

From: "Wang, Shane" <shane.wang@xxxxxxxxx>
Subject: RE: [Xen-devel] [PATCH 0/2] MCA support with page offlining
Date: Mon, 22 Dec 2008 09:52:28 +0800

> Hi KAZ,
> 
> For your latest patch, I am thinking of the following code.
> 
> +     /* check whether a page number have been already registered or not */
> +     list_for_each_entry(page, &page_offlining, list) {
> +             if (page == pg)
> +                     goto out;
> +     }
> 
> x86_page_offlining_t e is put into the list. Here, is it "if (e->page == pg)"?
> 
> +
> +     /* check whether already having attribute 'reserved' */
> +     if (pg->count_info & PGC_reserved) {
> +             printk(XENLOG_DEBUG "Page offlining: ( %lx ) failure.\n",
> +                    maddr);
> +             return 1;
> +     }
> +
> +     /* add attribute 'reserved' and register the page */
> +     get_page(pg, d);
> +     pg->count_info |= PGC_reserved;
> +
> +     e = xmalloc(x86_page_offlining_t);
> +     BUG_ON(e == NULL);
> +     e->page = pg;
> +     list_add(&e->list, &page_offlining);
> 
> Since page struct is also a list entry (containing "struct list_head list"). 
> You can pick off from domain page list and put it onto your page_offlining 
> list.
> Certainly, the domain may be using it now. You can mark it by using your flag 
> PGC_reserved first and then do this in the free function. I think this is 
> what Jiang Yunhong suggested:)
> What do you think on those suggestions? PS: in that case, 
> alloc_bitmap[]/heap[][][]/avail[] should be considered.
> 
> For the code piece
> 
> +                if ( !list_empty(&heap(node, zone, j)) ) {
> +                    pg = list_entry(heap(node, zone, j).next, struct 
> page_info, list);
> +                    if (!(pg->count_info & PGC_reserved))
> +                        goto found;
> +                    else
> +                        printk(XENLOG_DEBUG "Page %p(%lx) is not to be 
> allocated.\n",
> +                               pg, page_to_maddr(pg));
> +                
> 
> It seems this mechanism is not efficient enough since the first page is 
> marked PGC_reserved (but the rest pages in heap(node, zone, j).next are not, 
> the rest pages can't be used and allocated any more although they are not 
> offlined), and it didn't solve the potential bug I mentioned.
> Moreover, since the original code (i.e. without your patch) just tries to 
> find a space with an enough order, it just gets the first finding. So it is 
> enough. However, I am wondering why you don't try to enumerate heap(node, 
> zone, j) to get the second one ... but directly return "not to be allocated", 
> when the code finds the first list entry can't meet the requirement?
> 
> Thanks.
> Shane
> 
> -----Original Message-----
> From: SUZUKI Kazuhiro [mailto:kaz@xxxxxxxxxxxxxx] 
> Sent: 2008年12月22日 8:42
> To: Jiang, Yunhong
> Cc: Wang, Shane; xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH 0/2] MCA support with page offlining
> 
> Hi,
> 
> > But I'm a bit confused some changes in this patch.
> 
> Sorry, I attached a wrong patch, this is a correct one.
> 
> Thanks,
> KAZ
> 
> Signed-off-by: Kazuhiro Suzuki <kaz@xxxxxxxxxxxxxx>
> 
> 
> From: "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>
> Subject: RE: [Xen-devel] [PATCH 0/2] MCA support with page offlining
> Date: Fri, 19 Dec 2008 18:48:11 +0800
> 
> > SUZUKI Kazuhiro <mailto:kaz@xxxxxxxxxxxxxx> wrote:
> > > Hi Yunhong and Shane,
> > > 
> > > Thank you for your comments and reviews.
> > > 
> > >> For page offlining, It may not be a good way to put the page into an 
> > >> array
> > >> but a list. 
> > > 
> > > I modified to register offlined pages to a list instead of an array.
> > > 
> > >> I guess the code targets for offlining domain pages only,
> > > right? How about free pages and xen pages?
> > >> If so, no need to check in the following when allocating
> > > free pages, since the offlined pages will not be freed into heap()()().
> > >> If not, the following may have a bug.
> > > 
> > > Yes, I assumed that offlining page was needed for domain pages. If xen
> > > pages are impacted, then it is enough to crash the Hypervisor, in current
> > > implementation. 
> > 
> > We have a internal patch for the similar purpose, for  page offline caused 
> > by both #MC or other purpose. We can base our work on your patch if needed.
> > 
> > But I'm a bit confused some changes in this patch.
> > In your previous version, if a page is marked as PGC_reserved, it will not 
> > be allocated anymore. However, in this version, when a page is marked as 
> > PGC_reserved, it is just not freed, so does that mean the page will not be 
> > removed from current list? That seems a bit hack for me.
> > 
> > What I think to do is for page offline is:
> > a) mark a page as PGC_reserved (or other name like PGC_broken )
> > b) If it is free, to step c, otherwise, wait till it is freed by the owner 
> > and to step c.
> > c) Remove the page from the buddy system, motve it to a special and 
> > seperated list (i.e. not in the heap[][][] anymore), and return other page 
> > to the buddy allocator.
> > 
> > Some argument is in step b, that if the page is owned by a guest, we can 
> > replace it with a new page through p2m table, and don't need wait till it 
> > is freed, we didn't do that currently because it is a bit complex to 
> > achieve that.
> > 
> > How is your idea on this?
> > 
> > Thanks
> > Yunhong Jiang
> > 
> > > 
> > > I attach an updated patch for xen part which also includes some bug fixes.
> > > 
> > > Thanks,
> > > KAZ
> > > 
> > > Signed-off-by: Kazuhiro Suzuki <kaz@xxxxxxxxxxxxxx>
> > > 
> > > 
> > > From: "Wang, Shane" <shane.wang@xxxxxxxxx>
> > > Subject: RE: [Xen-devel] [PATCH 0/2] MCA support with page offlining
> > > Date: Tue, 16 Dec 2008 19:10:00 +0800
> > > 
> > >> For page offlining, It may not be a good way to put the page into an 
> > >> array
> > >> but a list. 
> > >> 
> > >> +        pg->count_info |= PGC_reserved;
> > >> +        page_offlining[num_page_offlining++] = pg;
> > >> 
> > >> I guess the code targets for offlining domain pages only,
> > > right? How about free pages and xen pages?
> > >> If so, no need to check in the following when allocating
> > > free pages, since the offlined pages will not be freed into heap()()().
> > >> If not, the following may have a bug.
> > >> 
> > >> +                if ( !list_empty(&heap(node, zone, j)) ) {
> > >> +                    pg = list_entry(heap(node, zone,
> > > j).next, struct page_info, list);
> > >> +                    if (!(pg->count_info & PGC_reserved))
> > >> +                        goto found;
> > >> +                    else
> > >> +                        printk(XENLOG_DEBUG "Page %p(%lx) is not to be
> > >> allocated.\n", +                               pg, page_to_maddr(pg)); + 
> > >>  
> > >> } 
> > >> 
> > >> If one free page (not pg) within pg and pg+(1U<<j) is
> > > offlined, the range pg~pg+(1U<<j) has the risk to be allocated
> > > with that page.
> > >> 
> > >> Shane
> > >> 
> > >> Jiang, Yunhong wrote:
> > >>> xen-devel-bounces@xxxxxxxxxxxxxxxxxxx <> wrote:
> > >>>> Hi all,
> > >>>> 
> > >>>> I had posted about MCA support for Intel64 before. It had only a
> > >>>> function to log the MCA error data received from hypervisor.
> > >>>> 
> > >>>> http://lists.xensource.com/archives/html/xen-devel/2008-09/msg0 
> > >>>> 0876.html
> > >>>> 
> > >>>> I attach patches that support not only error logging but also Page
> > >>>> Offlining function. The page where an MCA occurs will offline and not
> > >>>> reuse. A new flag 'PGC_reserved' was added in page count_info to mark
> > >>>> the impacted page. 
> > >>>> 
> > >>>> I know that it is better to implement the page offlining for general
> > >>>> purpose, but I implemented for MCA specialized in this first step.
> > >>> 
> > >>> Maybe the MCA page offline is a bit different to normal page offline
> > >>> requirement, so take it as first step maybe a good choice :)
> > >>> 
> > >>> As for your current page_offlining, I'm not sure why the PGC_reserved
> > >>> page should not be freed? Also, for following code, will that make
> > >>> the heap(node, zone, j) can't be allocated anymore? Maybe we can
> > >>> creat a special list to hold all those pages and remove them from the
> > >>> heap list? 
> > >>> 
> > >>> +                if ( !list_empty(&heap(node, zone, j)) ) {
> > >>> +                    pg = list_entry(heap(node, zone, j).next, struct
> > >>> page_info, list); +                    if (!(pg->count_info &
> > >>> PGC_reserved)) +                        goto found; +                   
> > >>> else +                        printk(XENLOG_DEBUG "Page %p(%lx) is not 
> > >>> to
> > >>> be allocated.\n", +                               pg, 
> > >>> page_to_maddr(pg));
> > >>> + 
> > >>> 
> > >>> 
> > >>>> 
> > >>>> And I also implement the MCA handler of Dom0 which support to
> > >>>> shutdown the remote domain where a MCA occurred. If the MCA occurred
> > >>>> on a DomU, Dom0 notifies it to the DomU. When the notify is failed,
> > >>>> Dom0 calls SCHEDOP_remote_shutdown hypercall.
> > >>>> 
> > >>>> [1/2] xen part: mca-support-with-page-offlining-xen.patch
> > >>> 
> > >>> We are not sure we really need pass all #MC information to dom0
> > >>> firstly, and let dom0 to notify domU. Xen should knows about
> > >>> everything, so it may have knowledge to decide inject virtual #MC to
> > >>> guest or not. Of course, this does not impact your patch.
> > >>> 
> > >>>> [2/2] linux/x86_64 part:
> > > mca-support-with-page-offlining-linux.patch
> > >>> 
> > >>> As for how to inject virtual #MC to guest (including dom0), I think
> > >>> we need consider following point:
> > >>> 
> > >>> a) Benefit from reusing guest #MC handler's . #MC handler is well
> > >>> known difficult to test, and the native guest handler may have been
> > >>> tested more widely. Also #MC handler improves as time going-on, reuse
> > >>> guest's MCA handler share us those improvement.
> > >>> b) Maintain the PV handler to different OS version may not so easy,
> > >>> especially as hardware improves, and kernel may have better support
> > >>> for error handling/containment.
> > >>> c) #MC handler may need some model specific information to decide the
> > >>> action, while guest (not dom0) has virtualized CPUID information.
> > >>> d) Guest's MCA handler may requires the physical information when the
> > >>> #MC hapen, like the CPU number the #MC happens.
> > >>> e) For HVM domain, PV handler will be difficult (considering Windows
> > >>> guest). 
> > >>> 
> > >>> And we have several option to support virtual #MC to guest:
> > >>> 
> > >>> Option 1 is what currently implemented. A PV #MC handler is
> > >>> implemented in guest. This PV handler gets MCA information from Xen
> > >>> HV through hypercall, including MCA MSR value, also some additional
> > >>> information, like which physical CPU the MCA happened. Option 1 will
> > >>> help us on issue d), but we need main a PV handler, and can't get
> > >>> benifit from native handler. Also it does not resolve issue c) quite 
> > >>> well.
> > >>> 
> > >>> option 2, Xen will provide MCA MSR virtualization so that guest's
> > >>> native #MC handler can run without changes. It can benifit from guest
> > >>> #MC handler, but it will be difficult to get model specific
> > >>> information, and has no physical information.
> > >>> 
> > >>> Option 3 uses a PV #MC handler for guest as option 1, but interface
> > >>> between Xen/guest is abstract event, like offline offending page,
> > >>> terminate current execution context etc. This should be straight
> > >>> forward for Linux, but may be difficult to Windows and other OS.
> > >>> 
> > >>> Currently we are considering option 2 to provide MCA MSR
> > >>> virtualization to guest, and dom0 can also benifit from such support
> > >>> (if guest has different CPUID as native, we will either keep guest
> > >>> running, or kill guest based on error code). Of course, current
> > >>> mechanism of passing MCA information from xen to dom0 will still be
> > >>> useful, but that will be used for logging purpose or for Correcatable
> > >>> Error. How do you think about this?
> > >>> 
> > >>> Thanks
> > >>> Yunhong Jiang
> > >>> 
> > >>>> Signed-off-by: Kazuhiro Suzuki <kaz@xxxxxxxxxxxxxx>
> > >>>> 
> > >>>> Thanks,
> > >>>> KAZ
> > >>>> 
> > >>>> _______________________________________________
> > >>>> Xen-devel mailing list
> > >>>> Xen-devel@xxxxxxxxxxxxxxxxxxx
> > >>>> http://lists.xensource.com/xen-devel
> > >>> _______________________________________________
> > >>> Xen-devel mailing list
> > >>> Xen-devel@xxxxxxxxxxxxxxxxxxx
> > >>> http://lists.xensource.com/xen-devel
> > >> 
> > >> 
> > >> _______________________________________________
> > >> Xen-devel mailing list
> > >> Xen-devel@xxxxxxxxxxxxxxxxxxx
> > >> http://lists.xensource.com/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel