Re: [Xen-devel] [PATCH 2/3] Handle GNTST_eagain in kernel drivers

[Olaf Hering <olaf@xxxxxxxxx>]

On Sat, Dec 17, Konrad Rzeszutek Wilk wrote:

> > +#define gnttab_check_GNTST_eagain_while(__HCop, __HCarg_p)                 
> >                 \
> 
> So why does this have to be a macro? What is the advantage of that
> versus making this a function?

I dont remember why I turned this into a macro instead of a function.

> > +do {                                                                       
> >                                                                         \
> > +   u8 __hc_delay = 1;                                                      
> >                                                         \
> > +   int __ret;                                                              
> >                                                                 \
> > +   while (unlikely((__HCarg_p)->status == GNTST_eagain && __hc_delay)) {   
> > \
> > +           msleep(__hc_delay++);                                           
> >                                                 \
> 
> Ugh. Not sure what happend to this, but there are tons of '\' at the
> end.

A multiline macro needs backslashes at the end.

> So why msleep? Why not go for a proper yield? Call the safe_halt()
> function?

It needs some interuptible sleep, whatever is best in this context.

> > +           __ret = HYPERVISOR_grant_table_op(__HCop, (__HCarg_p), 1);      
> >                 \
> > +           BUG_ON(__ret);                                                  
> >                                                         \
> > +   }                                                                       
> >                                                                         \
> > +   if (__hc_delay == 0) {                                                  
> >                                                 \
> 
> So this would happen if we rolled over __hc_delay, so did this more than
> 255 times? Presumarily this can happen if the swapper in dom0 crashes..

Or if something in the paging paths goes wrong.

> I would recommend you use 'WARN' here and include tons of details.
> This is a pretty serious issues, is it not?

Either the host is really busy and cant page-in quick enough even after
so-many seconds. Or something in the pager/hypervisor does not work
right. In either case, a backtrace wont help much as it does only cause
noise. The printk below prints the function name (I think thats the
reason why it is a macro) to give some hint. 

> > +           printk(KERN_ERR "%s: gnt busy\n", __func__,);                   
> >                         \
> > +           (__HCarg_p)->status = GNTST_bad_page;                           
> >                                 \
> > +   }                                                                       
> >                                                                         \
> > +   if ((__HCarg_p)->status != GNTST_okay)                                  
> >                                 \
> > +           printk(KERN_ERR "%s: gnt status %x\n",                          
> >                                 \
> > +                   __func__, (__HCarg_p)->status);                         
> >                                         \
> 
> Use GNTTABOP_error_msgs. Also should we continue? What is the
> impact if we do continue? The times this is hit is if the status
> is not GNTS_okay and if it is not GNTS_eagain - so what are the
> situations in which this happens and what can the domain do
> to recover? Should there be some helpfull message instead of
> just "gnt status: X" ?

The caller has to deal with the various !GNTST_okay states anyway, this
patch wont change that fact.

Olaf

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