Re: [PATCH 1/3] plist: pass the real plist_head to plist_del()

[Lai Jiangshan]

On 12/17/2010 05:19 AM, Darren Hart wrote:
> On 12/16/2010 02:23 AM, Lai Jiangshan wrote:
>> These patches shrink the struct plist_head. After it is shrinked
>> plist_del() required a real plist_head passed into.
>>
>> My tests did not cover all paths.
>>
>> Subject: plist: pass the real plist_head to plist_del()
>>
>>
>> Some plist_del()s in kernel/futex.c are passed a faked plist_head.
> 
> Can you explain what you mean by a "faked plist_head"? I'm looking at
> the existing source and q.list.plist is a "struct plist_head". Is it the
> required knowledge of the implementation of a plist_node that the
> current implementation depends on that you object to? Effectively a lack
> of encapsulation?


What I said is "a faked head of a priority list", I am sorry that
"faked plist_head" misled you.

/**
 * plist_del - Remove a @node from plist.
 *
 * @node:	&struct plist_node pointer - entry to be removed
 * @head:	&struct plist_head pointer - list head
 */

The document said that @head is "list head" the head of the priority list.
But the code of plist_del() just uses the head for checking, so it will
not cause bad result even when we use a faked head.

In "plist_del(&q->list, &q->list.plist);", &q->list.plist is indeed a
"struct plist_head", but it is not the real head of the priority list.

I think this is bad code. We should pass the real head OR just remove
this parameter since it is not required real one. But my next patch will use
this "list head", so this patch fixes futex.c and pass the real head
to plist_del().


> 
>>
>> It can work because current code does not require real plist_head
>> in plist_del(). But it is an undocumented usage, it is not good.
> 
> 
> Please compare the results of the futextest performance tests before and
> after your applied patch and report the results with the patch:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/dvhart/futextest.git
> 

I thought it is a clean up patch. But where are the testcases?
Are they included in LTP?

> 
>>
>> Signed-off-by:  Lai Jiangshan<laijs@xxxxxxxxxxxxxx>
>> ---
>> diff --git a/kernel/futex.c b/kernel/futex.c
>> index 6c683b3..6c4f67a 100644
>> --- a/kernel/futex.c
>> +++ b/kernel/futex.c
>> @@ -158,6 +158,15 @@ static inline int match_futex(union futex_key
>> *key1, union futex_key *key2)
>>   }
>>
>>   /*
>> + * find the bucket of a futex entry.
>> + * the same as hash_futex(&q->key) but a little more effcient
> 
> efficient.
> 
> Please capitalize the first word in each sentence.
> 
>> + */
> 
> 
> Please use proper kernel doc formatting for any new functions added to
> futex.c.

Will fix it.


> 
> 
>> +static struct futex_hash_bucket *futex_bucket(struct futex_q *q)
>> +{
>> +    return container_of(q->lock_ptr, struct futex_hash_bucket, lock);
>> +}
> 
> 
> Have you found the jhash2 to be a bottleneck in a particular path? Or
> was it an optimization to reduce the impact of the changes below (I'm
> assuming the latter).
> 

jhash2() need tens of instructions after compiled.
container_of() is just a ADD/SUB operation instruction.

It is OK to use hash_futex(&q->key) instead.

> 
>> +
>> +/*
>>    * Take a reference to the resource addressed by a key.
>>    * Can be called while holding spinlocks.
>>    *
>> @@ -744,7 +753,7 @@ static void wake_futex(struct futex_q *q)
>>        */
>>       get_task_struct(p);
>>
>> -    plist_del(&q->list,&q->list.plist);
>> +    plist_del(&q->list,&futex_bucket(q)->chain);
> 
> 
> Space after the comma while you're at it.
> 

Space is existed in the patch mail I received from LKML. It is strange.

Thanks,
Lai. 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/