[PATCH] [3/4 try 2] x86_64: mce_amd support for family 0x10 processors

Andi Kleen ak at suse.de
Mon May 22 17:55:42 CEST 2006


On Monday 22 May 2006 16:53, shin, jacob wrote:

The function would be probably simpler if it used rdmsrl/wrmsrl instead of
hi/lo, but that could be a follow on patch.

> +		} while ((block ? (address)
> +			        : (address = (low & MASK_BLKPTR_LO) >> 21)) &&
> +			 (block ? (address++)
> +			        : (address += MCG_XBLK_ADDR))               &&
> +			 (++block < NR_BLOCKS));

Can you rewrite that in a less bizarre style suitable for normal humans please?

> +		} while ((block ? (address)
> +			        : (address = (low & MASK_BLKPTR_LO) >> 21)) &&
> +			 (block ? (address++)
> +			        : (address += MCG_XBLK_ADDR))               &&
> +			 (++block < NR_BLOCKS));

Similar.


> +	b = kmalloc(sizeof(struct threshold_block), GFP_KERNEL);
> +	if (!b)
> +		return -ENOMEM;
> +	memset(b, 0, sizeof(struct threshold_block));

kzalloc()

> +out_free:
> +	if (b) {

How can b be NULL here? 


> +	b->sibling_count++;

This private sibling detection is scary. Are you sure you can't use the generic
data structures for this?

> +	goto out;
> +
> +out_free:
> +	per_cpu(threshold_banks, cpu)[bank] = NULL;
> +	kfree(b);
>        out:
> +	up(&threshold_lock);

up() and per_cpu? That normally doesn't mix because per_cpu requires
to turn off preemption. Possibly preempt/locking problem here.

>  	return err;
>  }
>  
> @@ -385,23 +537,50 @@ static __cpuinit int threshold_create_de
>   *   of shared sysfs dir/files, and rest of the cores will be symlinked to it.
>   */
>  
> -/* cpu hotplug call removes all symlinks before first core dies */
> +static __cpuinit void deallocate_threshold_block(unsigned int cpu,
> +						 unsigned int bank)
> +{
> +	struct threshold_block *pos = NULL;
> +	struct threshold_block *tmp = NULL;
> +	struct threshold_bank *head = per_cpu(threshold_banks, cpu)[bank];
> +
> +	if (!head)
> +		return;
> +
> +	list_for_each_entry_safe(pos, tmp, &head->blocks->miscj, miscj) {
> +		kobject_unregister(&pos->kobj);
> +		list_del(&pos->miscj);
> +		kfree(pos);
> +	}
> +
> +	kfree(per_cpu(threshold_banks, cpu)[bank]->blocks);
> +	per_cpu(threshold_banks, cpu)[bank]->blocks = NULL;
> +}
> +
>  static __cpuinit void threshold_remove_bank(unsigned int cpu, int bank)
>  {
>  	struct threshold_bank *b;
>  	char name[16];
>  
> +	down(&threshold_lock);
> +
>  	b = per_cpu(threshold_banks, cpu)[bank];

You would need to turn preemption off for per Pcu (get_cpu)

And when get_cpu is done then down is illegal because it can sleep


Again i would prefer if it just checked the general sibling array
instead of trying to keep track on its own.

-Andi



More information about the discuss mailing list