Re: [Qemu-arm] [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters

[Eduardo Habkost]

On Sun, Nov 25, 2018 at 10:27:04PM +0100, Philippe Mathieu-Daudé wrote:
> Hi Eduardo,
> 
> On 23/11/18 19:10, Eduardo Habkost wrote:
> > Hi,
> > 
> > Sorry for not reviewing this series earlier.  I just stumbled
> > upon this part of the code:
> > 
> > On Fri, Nov 23, 2018 at 10:17:14AM +0100, Luc Michel wrote:
> >> This commit adds the cpu-cluster type. It aims at gathering CPUs from
> >> the same cluster in a machine.
> >>
> >> For now it only has a `cluster-id` property.
> >>
> >> Signed-off-by: Luc Michel <address@hidden>
> >> Reviewed-by: Alistair Francis <address@hidden>
> >> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> >> Tested-by: Philippe Mathieu-Daudé <address@hidden>
> >> Reviewed-by: Edgar E. Iglesias <address@hidden>
> > [...]
> >> +static void cpu_cluster_init(Object *obj)
> >> +{
> >> +    static uint32_t cluster_id_auto_increment;
> >> +    CPUClusterState *cluster = CPU_CLUSTER(obj);
> >> +
> >> +    cluster->cluster_id = cluster_id_auto_increment++;
> > 
> > I see that you implemented this after a suggestion from Philippe,
> > but I'm worried about this kind of side-effect on object/device
> > code.  I'm afraid this will bite us back in the future.  We were
> > bitten by problems caused by automatic cpu_index assignment on
> > CPU instance_init, and we took a while to fix that.
> 
> Oops, thanks for catching this. I'm not aware of the cpu_index past issue.
> 
> > 
> > If you really want to do this and assign cluster_id
> > automatically, please do it on realize, where it won't have
> > unexpected side-effects after a simple `qom-list-properties` QMP
> > command.
> 
> This looks clean enough to me.
> Do you prefer we don't use static auto_increment at all?

I would prefer to avoid the static variable if possible, but I
won't reject it if the alternatives make the API too complex to
use.

In either case, I'd like to ensure the caveats of the cluster_id
field are clearly documented: no guarantees about ordering or
predictability, making it not appropriate for external
interfaces.


> 
> > 
> > I would also add a huge warning above the cluster_id field
> > declaration, mentioning that the field is not supposed to be used
> > for anything except debugging.  I think there's a large risk of
> > people trying to reuse the field incorrectly, just like cpu_index
> > was reused for multiple (conflicting) purposes in the past.
> > 
> > 
> >> +}
> >> +
> >> +static Property cpu_cluster_properties[] = {
> >> +    DEFINE_PROP_UINT32("cluster-id", CPUClusterState, cluster_id, 0),
> >> +    DEFINE_PROP_END_OF_LIST()
> >> +};
> > [...]
> > 

-- 
Eduardo