Re: [Xen-devel] [PATCH] xenpaging:add a new array to speed up page-in in xenpaging

[Hongkaixing <hongkaixing@xxxxxxxxxx>]

> -----邮件原件-----
> 发件人: Ian Jackson [mailto:Ian.Jackson@xxxxxxxxxxxxx]
> 发送时间: 2012年1月6日 2:31
> 收件人: hongkaixing@xxxxxxxxxx
> 抄送: Olaf Hering; xen-devel@xxxxxxxxxxxxxxxxxxx
> 主题: Re: [Xen-devel] [PATCH] xenpaging:add a new array to speed up
page-in
> in xenpaging
> 
> hongkaixing@xxxxxxxxxx writes ("[Xen-devel] [PATCH] xenpaging:add a new
> array to speed up page-in in xenpaging"):
> > xenpaging:add a new array to speed up page-in in xenpaging
> 
> Oh, and a couple of style points.  You should keep to the style of the
> code you're editing.  So:
> 
> > +    if (NULL == victims)
> 
> In the xenpaging code this is written like this:
> 
>        if (victims == NULL)
> 

Oh, This is our coding style, I will modify it according your opinion.

> You should do the same, throughout.
> 
> > +    page_out_index = (victim_to_i_t
> *)calloc(paging->domain_info->max_pages, sizeof(victim_to_i_t));
> 
> Do not cast the return value from malloc et al.
> 
> > +                i = page_out_index[req.gfn].index ;
> 
> The space before the semicolon is not the conventional style.
> 
> > +                    if( victims[i].gfn !=INVALID_MFN )
> 
> > -    free(victims);
> > +    if ( NULL != victims )
> > +    {
> > +        free(victims);
> > +    }
> 
> This is simply pointless.  free(NULL) is legal.

I see...

> 
> > +typedef struct victim_to_i {
> > +    /* the index of victim array to read from */
> > +    int index;
> > +} victim_to_i_t;
> 
> Why wrap this up in a struct ?

We want to keep the same style with

typedef struct xenpaging_victim {
    /* the gfn of the page to evict */
    unsigned long gfn;
} xenpaging_victim_t;

> 
> Use of types with names ending in _t is reserved to the C
> implementation (compiler and runtime).  I know xenpaging is full of
> these but we shouldn't introduce any more.
> 
> Ian.


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