Re: [patch 06/10] cpu topology support for s390.

Andrew Morton
2008-03-12T23:12:27Z

On Wed, 12 Mar 2008 18:32:01 +0100
Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> From: Heiko Carstens <heiko.carstens@de.ibm.com>
>
> Add s390 backend so we can give the scheduler some hints about the
> cpu topology.
>
> ===================================================================
> --- /dev/null
> +++ quilt-2.6/arch/s390/kernel/topology.c
> @@ -0,0 +1,271 @@
> +/*
> + * arch/s390/kernel/topology.c
> + *
> + * Copyright IBM Corp. 2007
> + * Author(s): Heiko Carstens <heiko.carstens@de.ibm.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/bootmem.h>
> +#include <linux/sched.h>
> +#include <linux/workqueue.h>
> +#include <linux/cpu.h>
> +#include <linux/smp.h>
> +#include <asm/delay.h>
> +#include <asm/s390_ext.h>
> +
> +#define CPU_BITS 64
> +
> +struct tl_cpu {
> + unsigned char reserved[6];
> + unsigned short origin;
> + unsigned long mask[CPU_BITS / BITS_PER_LONG];
> +};

mask[] will be too small for CPU_BITS=65 ;)

> ...
>
> +static union tl_entry *next_tle(union tl_entry *tle)
> +{
> + if (tle->nl)
> + return (union tl_entry *)((struct tl_container *)tle + 1);
> + else
> + return (union tl_entry *)((struct tl_cpu *)tle + 1);
> +}

omg.

> +static void tl_to_cores(struct tl_info *info)
> +{
> + union tl_entry *tle, *end;
> + struct core_info *core = &core_info;
> +
> + mutex_lock(&smp_cpu_state_mutex);
> + clear_cores();
> + tle = (union tl_entry *)&info->tle;

and this cast was unneeded!

> + end = (union tl_entry *)((unsigned long)info + info->length);

I'd suggest that you take a look at all the pointer arith games which are
being played in this code and see if it can be done better with a more
appropriate use of the C type system. Before someone dies.