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

[André Detsch]

Jeremy Kerr wrote:
> Andre,
> 
>> +		if (!list_empty(&neighbor->aff_list) &&
>> +		    !(neighbor->aff_flags & AFF_SUBLIST_HEAD) &&
>> +		    !list_entry(neighbor->aff_list.next, struct spu_context,
>> +		    aff_list)->aff_flags & AFF_SUBLIST_HEAD)
>> +			return ERR_PTR(-EEXIST);
> 
> Just trying to follow some of the logic here - why is it OK for 
> neighbour->aff_list.next to be the AFF_SUBLIST_HEAD? In fact, what 
> exactly does the AFF_SUBLIST_HEAD flag signify?

Conceptually, a gang can have multiple affinity lists. From the 
implementation point of view, we end up merging all lists before any 
scheduling decision is made. So, instead of managing different lists, 
its simpler to keep one list and mark the head (|= AFF_SUBLIST_HEAD) of 
each logical list, to avoid mixing them.
If neighbor.next is AFF_SUBLIST_HEAD, neighbor is the last ctx of its 
logical list. So, it is ok to place a context right after him.

>> +		list_for_each_entry(tmp, &gang->aff_list_head, aff_list)
>> +			count++;
>> +		if (list_empty(&neighbor->aff_list))
>> +			count++;
>> +
>> +		for (node = 0; node < MAX_NUMNODES; node++) {
>> +			if ((be_spu_info[node].n_spus - atomic_read(
>> +				&be_spu_info[node].isolated_spus)) > count)
>> +				break;
>> +		}
> 
> This one is tricky, as isolated contexts can come and go at any time. 
> Maybe checking only n_spus here would be better, but would create 
> scheduling problems later, if the isolated context was long-lived.
> 
> However, we'll have the same problem if a new isolated context starts on 
> one of the SPEs in the node that this finds.

Yes, that's a tricky issue that need some care not only in regards of 
affinity, but also in gang scheduling. What we need is to define a 
global strategy, and make it clear when documenting the features.

>> +	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);
>> +	}
> 
> how about:
> 
> 	if (flags & (SPU_CREATE_AFFINITY_MEM | SPU_CREATE_AFFINITY_SPU)) {
> 		if (!ret)
> 			spufs_set_affinity(flags,
> 				SPUFS_I(dentry->d_inode)->i_ctx, neighbor);
> 		mutex_unlock(&gang->aff_mutex);
> 	}
> 
> 	if (ret)
> 		goto out_unlock;
> 
> ?

That's ok, I'll update the code.

--
Andre Detsch