[Cbe-oss-dev] [PATCH 4/6] spufs: extension of spu_create to support affinity definition

[Arnd Bergmann]

On Monday 12 February 2007 02:14, Andre Detsch wrote:
> Subject: spufs: extension of spu_create to support affinity definition
> From: Andre Detsch <adetsch at br.ibm.com>
> 
> This patch adds support for additional flags at spu_create, which relate
> to the establishment of affinity between contexts and contexts to memory.
> A fourth, optional, parameter is supported. This parameter represent
> a affinity neighbor of the context being created, and is used when defining
> SPU-SPU affinity.
> Affinity is represented as a doubly linked list of spu_contexts.

I couldn't see a place where you get a reference count for the contexts
you link to each other. Have you checked that this is safe to do?
Did I miss something there?

> @@ -307,11 +307,102 @@ out:
>  	return ret;
>  }
>  
> -static int spufs_create_context(struct inode *inode,
> -			struct dentry *dentry,
> -			struct vfsmount *mnt, int flags, int mode)
> +static struct spu_context *
> +spufs_assert_affinity(unsigned int flags, struct spu_gang *gang,
> +						struct file *filp)
> +{
> +	struct spu_context *tmp, *neighbor = NULL;
> +	int count, node;
> +	int aff_supp;
> +
> +	aff_supp = !list_empty(&(list_entry(be_spu_info[0].available_spus.next,
> +					struct spu, available_list))->aff_list);
> +
> +	if (!aff_supp)
> +		return ERR_PTR(-ENOTSUPP);

ENOTSUPP is an NFS specific error code, you shouldn't use it here, especially
if this is passed back to user space. I'd say it should either be silently
ignored so that programs with affinity definitions still run on the PS3
without changes, or return -EINVAL.

> +static void
> +spufs_set_affinity(unsigned int flags, struct spu_context *ctx,
> +					struct spu_context *neighbor)
> +{
> +	if (flags & SPU_CREATE_AFFINITY_MEM) {
> +		ctx->gang->aff_ref_point = ctx;
> +		ctx->aff_flags |= AFF_HAS_MEM_AFFINITY;
> +	}
> +	if (flags & SPU_CREATE_AFFINITY_SPU) {
> +		if (list_empty(&neighbor->aff_list)) {
> +			list_add_tail(&neighbor->aff_list,
> +				&ctx->gang->aff_list_head);
> +			neighbor->aff_flags |= AFF_SUBLIST_HEAD;
> +		}

Why do we need the extra aff_flags property? The information seems
a little redundant considering we already have ctx->flags and we
can look into list_empty(aff_list) etc.

> +static int
> +spufs_create_context(struct inode *inode, struct dentry *dentry,
> +			struct vfsmount *mnt, int flags, int mode,
> +			struct file *aff_filp)
>  {
>  	int ret;
> +	struct spu_gang *gang = NULL;
> +	struct spu_context *neighbor = NULL;
>  
>  	ret = -EPERM;
>  	if ((flags & SPU_CREATE_NOSCHED) &&

style: don't initialize local variables in their definition.

> @@ -326,10 +417,29 @@ static int spufs_create_context(struct i
>  	ret = -ENODEV;
>  	if ((flags & SPU_CREATE_ISOLATE) && !isolated_loader)
>  		goto out_unlock;
> +	if (flags & (SPU_CREATE_AFFINITY_MEM | SPU_CREATE_AFFINITY_SPU)) {
> +		gang = SPUFS_I(inode)->i_gang;
> +		mutex_lock(&gang->aff_mutex);
> +		neighbor = spufs_assert_affinity(flags, gang, aff_filp);
> +		if (IS_ERR(neighbor)) {
> +			ret = PTR_ERR(neighbor);
> +			mutex_unlock(&gang->aff_mutex);
> +			goto out_unlock;
> +		}
> +	}

this looks wrong, you shouldn't release a mutex in the error path
before the goto, better jump to a target where you unlock.

It's too easy to introduce a bug here if you add another failure
path and forget the unlock there.

>  	ret = spufs_mkdir(inode, dentry, flags, mode & S_IRWXUGO);
> -	if (ret)
> +	if (ret) {
> +		if (flags & (SPU_CREATE_AFFINITY_MEM | SPU_CREATE_AFFINITY_SPU))
> +			mutex_unlock(&gang->aff_mutex);
>  		goto out_unlock;
> +	}
> +
> +	if (flags & (SPU_CREATE_AFFINITY_MEM | SPU_CREATE_AFFINITY_SPU)) {
> +		spufs_set_affinity(flags, SPUFS_I(dentry->d_inode)->i_ctx,
> +					neighbor);
> +		mutex_unlock(&gang->aff_mutex);
> +	}

same for these two.

> -asmlinkage long sys_spu_create(const char __user *pathname,
> -					unsigned int flags, mode_t mode)
> +asmlinkage long do_spu_create(const char __user *pathname, unsigned int 
> flags,
> +				mode_t mode, struct file *filp)
>  {
>  	char *tmp;
>  	int ret;

calling the fourth argument filp in all these functions is a little
misleading, since it's not the filp for the spu itself. Better call
it 'neighbor' or something like that.

> @@ -90,7 +90,10 @@ asmlinkage long sys_spu_create(const cha
>  		ret = path_lookup(tmp, LOOKUP_PARENT|
>  				LOOKUP_OPEN|LOOKUP_CREATE, &nd);
>  		if (!ret) {
> -			ret = spufs_create(&nd, flags, mode);
> +			if (flags & SPU_CREATE_AFFINITY_SPU)
> +				ret = spufs_create(&nd, flags, mode, filp);
> +			else
> +				ret = spufs_create(&nd, flags, mode, NULL);
>  			path_release(&nd);
>  		}
>  		putname(tmp);

Do you need that check again here? It looks like filp is already
NULL when not SPU_CREATE_AFFINITY_SPU.

	Arnd <><