[Cbe-oss-dev] [PATCH 5/6] spufs: add placement computation for scheduling of affinity contexts

[Arnd Bergmann]

On Monday 12 February 2007 02:16, Andre Detsch wrote:

> +static struct spu *
> +aff_ref_location(int mem_aff, int group_size, int prio, int lowest_rel_displ)
> +{
> +	struct spu *spu;
> +	int node;
> +
> +	/* TODO: A better algorithm could be used to find a good spu to be
> +	 *       used as reference location for the ctxs chain.
> +	 *       Also, a isolated SPU should never be chosen here.
> +	 */
> +	for (node = 0; node < MAX_NUMNODES; node++) {
> +		list_for_each_entry(spu, &be_spu_info[node].available_spus,
> +							available_list) {
> +			if (!mem_aff || spu->has_mem_affinity)
> +				return spu;
> +		}
> +	}
> +	BUG_ON(1);
> +	return NULL;
> +}

BUG() is rather rude here. What if none of the SPUs on that node are
available?

> +static struct spu* ctx_location(struct spu *ref, int rel_displ)
> +{
> +	struct spu *spu;
> +
> +	/* TODO: skip isolated SPUs */

I guess you don't mean isolated SPUs, but rather nonschedulable ones,
which is technically a difference. You can create contexts that
refuse scheduling even when they are not in isolated mode. Also, they
might be used by another kernel module.

> +	if (rel_displ >= 0) {
> +		list_for_each_entry(spu, ref->aff_list.prev, aff_list) {
> +			if (rel_displ == 0)
> +				return spu;
> +			rel_displ--;
> +		}
> +	} else {
> +		list_for_each_entry_reverse(spu, ref->aff_list.next, aff_list) {
> +			if (rel_displ == 0)
> +				return spu;
> +			rel_displ++;
> +		}
> +	}
> +	return NULL;
> +}

I've grown used to functions that only return from the end, I almost missed
the fact that you return the pointers from three levels of indentation. Maybe
it could be written more like


if (rel_displ >= 0) {
	list_for_each_entry(spu, ref->aff_list.prev, aff_list) {
		if (rel_displ == 0)
			break;
		rel_displ--;
	}
} else {
	list_for_each_entry_reverse(spu, ref->aff_list.next, aff_list) {
		if (rel_displ == 0)
			break;
		rel_displ++;
	}
}
return spu;


> --- linux-2.6.20.orig/arch/powerpc/platforms/cell/spufs/spufs.h
> +++ linux-2.6.20/arch/powerpc/platforms/cell/spufs/spufs.h
> @@ -85,6 +85,7 @@ struct spu_context {
>  
>  	struct list_head aff_list;
>  	int aff_flags;
> +	int aff_rel_displ;
>  };
>  
>  /* Flag bits for spu_context aff_flags */
> @@ -102,6 +103,8 @@ struct spu_gang {
>  	struct list_head aff_list_head;
>  	struct mutex aff_mutex;
>  	int aff_flags;
> +	struct spu *aff_ref_point_location;
> +	atomic_t aff_sched_count;
>  };

The aff_sched_count member seems to be completely unused. Can you
remove it?

Your way off abbrev variab seems strng. I have no idea what
aff_rel_displ even means. aff_ref_point_location sounds like
it could better be called aff_reference_point or just
aff_reference.

	Arnd <><