From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC v2 1/4] xen: add real time scheduler rt Date: Thu, 7 Aug 2014 12:48:39 +0100 Message-ID: <53E36797.4080504@citrix.com> References: <1406598782-2237-1-git-send-email-mengxu@cis.upenn.edu> <1406598782-2237-2-git-send-email-mengxu@cis.upenn.edu> <53D77939.6020904@citrix.com> <53DF5F18.20601@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3179392443854466643==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Meng Xu Cc: Ian Campbell , Sisu Xi , Stefano Stabellini , George Dunlap , Ian Jackson , "xen-devel@lists.xen.org" , Meng Xu , Jan Beulich , Chong Li , Dagaen Golomb List-Id: xen-devel@lists.xenproject.org --===============3179392443854466643== Content-Type: multipart/alternative; boundary="------------070409020007010300010408" --------------070409020007010300010408 Content-Type: text/plain; charset="UTF-8" Content-Length: 2874 Content-Transfer-Encoding: quoted-printable On 07/08/14 12:26, Meng Xu wrote: > Hi Andrew, > > =E2=80=8BThank you so much for your advice! They are very useful! :-) > > I have one simple question about your comments: > =E2=80=8B > >> >> > +static void * >> > +rt_alloc_domdata(const struct scheduler *ops, struct domain *dom) >> > +{ >> > + unsigned long flags; >> > + struct rt_dom *sdom; >> >> > + struct rt_private * prv =3D RT_PRIV(ops); >> > + >> > + printtime(); >> > + printk("dom=3D%d\n", dom->domain_id); >> > + >> > + sdom =3D xzalloc(struct rt_dom); >> > + if ( sdom =3D=3D NULL ) >> > + { >> > + printk("%s, xzalloc failed\n", __func__); >> > + return NULL; >> > + } >> > + >> > + INIT_LIST_HEAD(&sdom->vcpu); >> > + INIT_LIST_HEAD(&sdom->sdom_elem); >> > + sdom->dom =3D dom; >> > + >> > + /* spinlock here to insert the dom */ >> > + spin_lock_irqsave(&prv->lock, flags); >> > + list_add_tail(&sdom->sdom_elem, &(prv->sdom)); >> > + spin_unlock_irqrestore(&prv->lock, flags); >> > + >> > + return (void *)sdom; >> >> Bogus cast. >> >> >> =E2=80=8BI think we have to cast it to void * =E2=80=8Bbecause the definition of >> this function asks the return type to be void *. In addition, the >> credit2 scheduler also did the same cast in this _alloc_domdata >> function. So I guess this should be fine=3F >> > > In C, all pointers are implicitly castable to/from void*. This is > not the case in C++. (which suggests to me that George learnt C++ > before C, or is at least more familiar with C++=3F) > > The act of using type punning means that you are trying to do > something which the C type system doesn't like. This in turn > requires more thought from people reading the code to work out > whether it is actually safe or not. As a result, we strive for as > few type overrides as possible. > > Please don't fall into the trap of assuming the credit2 code, or > indeed any of the other code in Xen, is perfect. None of it is, > although most of it is good. > > > =E2=80=8BI can change the return (void *)sdom to return sdom. > =E2=80=8BBut I'm not sure what else should I modify=3F > In sched-if.h, this function is =E2=80=8B"void * (*alloc_domdata) > (const struct scheduler *, struct domain *);" I think I should not > change this declaration, otherwise, the modification will affect the > compilation of other schedulers. > =E2=80=8B=E2=80=8B > =E2=80=8BThank you so much! > > Best, > > Meng=E2=80=8B return sdom; is perfectly fine with the existing function prototype. It is an implicit cast of a pointer to void*, which is perfectly legal in C. ~Andrew --------------070409020007010300010408 Content-Type: text/html; charset="UTF-8" Content-Length: 8082 Content-Transfer-Encoding: quoted-printable
On 07/08/14 12:26, Meng Xu wrote:
Hi Andrew,

=E2=80=8BThank you so much for your advice! They are very useful! :-)

I have one simple question about your comments:=C2=A0
=E2=80=8B

> +static void *
> +rt_alloc_domdata(const struct scheduler *ops, struct domain *dom)
> +{
> + =C2=A0 =C2=A0unsigned long flags;
> + =C2=A0 =C2=A0struct rt_dom *sdom;

> + =C2=A0 =C2=A0struct rt_private * prv =3D RT_PRIV(ops);
> +
> + =C2=A0 =C2=A0printtime();
> + =C2=A0 =C2=A0printk("dom=3D%d\n", dom->domain_id);
> +
> + =C2=A0 =C2=A0sdom =3D xzalloc(struct rt_dom);
> + =C2=A0 =C2=A0if ( sdom =3D=3D NULL )
> + =C2=A0 =C2=A0{
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0printk("%s, xzalloc failed\n", __func__);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return NULL;
> + =C2=A0 =C2=A0}
> +
> + =C2=A0 =C2=A0INIT_LIST_HEAD(&sdom->vcpu);
> + =C2=A0 =C2=A0INIT_LIST_HEAD(&sdom->sdom_elem);
> + =C2=A0 =C2=A0sdom->dom =3D dom;
> +
> + =C2=A0 =C2=A0/* spinlock here to insert the dom */
> + =C2=A0 =C2=A0spin_lock_irqsave(&prv->lock, flags);
> + =C2=A0 =C2=A0list_add_tail(&sdom->sdom_elem, &(prv->sdom));
> + =C2=A0 =C2=A0spin_unlock_irqrestore(&prv->lock, flags);
> +
> + =C2=A0 =C2=A0return (void *)sdom;

Bogus cast.

=E2=80=8BI think we have to cast it to void * =E2=80=8Bbecause the definition of this function asks the return type to be void *. In addition, the credit2 scheduler also did the same cast in =C2=A0this _alloc_domdata function. So I guess this should be fine=3F


In C, all pointers are implicitly castable to/from void*.=C2=A0 This is not the case in C++.=C2=A0 (which suggests to me that George learnt C++ before C, or is at least more familiar with C++=3F)

The act of using type punning means that you are trying to do something which the C type system doesn't like.=C2=A0 This in turn requires more thought from people reading the code to work out whether it is actually safe or not.=C2=A0 As a result, we strive for as few type overrides as possible.

Please don't fall into the trap of assuming the credit2 code, or indeed any of the other code in Xen, is perfect.=C2=A0 None of it is, although most of it is good.

=C2=A0
=E2=80=8BI can change the return (void *)sdom to return sdom.=C2=A0
=E2=80=8BBut I'm not sure what else should I modify=3F=C2=A0
In sched-if.h, =C2=A0this function is =E2=80=8B"void * =C2=A0 =C2=A0 =C2=A0 (*alloc_domdata) =C2=A0(const struct scheduler *, struct domain *);" I think I should not change this declaration, otherwise, the modification will affect the compilation of other schedulers.
=E2=80=8B=E2=80=8B
=E2=80=8BThank you so much!

Best,

Meng=E2=80=8B

return sdom; is perfectly fine with the existing function prototype.=C2=A0 It is an implicit cast of a pointer to void*, which is perfectly legal in C.

~Andrew
--------------070409020007010300010408-- --===============3179392443854466643== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============3179392443854466643==--