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

[André Detsch]

Arnd Bergmann wrote:
> 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.

Ok, I'll change to -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.

I agree AFF_HAS_SPU_AFFINITY flag is redundant with 
list_empty(aff_list). However, as I see, AFF_HAS_MEM_AFFINITY and 
AFF_SUBLIST_HEAD are not.

> 
>> +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.

Ok.

>> @@ -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.

Ok, I'll try to adjust this.

>>  	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.

Ok.

>> -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.

Ok.

>> @@ -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.

Right, the check is not actually needed.