RE: Re:[PATCH v14 06/17] Use callback to deal withskb_release_data() specially.

[Xin, Xiaohui]

>-----Original Message-----
>From: Eric Dumazet [mailto:eric.dumazet@xxxxxxxxx]
>Sent: Monday, November 08, 2010 4:25 PM
>To: Xin, Xiaohui
>Cc: netdev@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>mst@xxxxxxxxxx; mingo@xxxxxxx; davem@xxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx;
>jdike@xxxxxxxxxxxxxxx
>Subject: Re: Re:[PATCH v14 06/17] Use callback to deal with skb_release_data() specially.
>
>Le lundi 08 novembre 2010 à 16:03 +0800, xiaohui.xin@xxxxxxxxx a écrit :
>> From: Xin Xiaohui <xiaohui.xin@xxxxxxxxx>
>>
>> >> Hmm, I suggest you read the comment two lines above.
>> >>
>> >> If destructor_arg is now cleared each time we allocate a new skb, then,
>> >> please move it before dataref in shinfo structure, so that the following
>> >> memset() does the job efficiently...
>> >
>> >
>> >Something like :
>> >
>> >diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> >index e6ba898..2dca504 100644
>> >--- a/include/linux/skbuff.h
>> >+++ b/include/linux/skbuff.h
>> >@@ -195,6 +195,9 @@ struct skb_shared_info {
>> > 	__be32          ip6_frag_id;
>> > 	__u8		tx_flags;
>> > 	struct sk_buff	*frag_list;
>> >+	/* Intermediate layers must ensure that destructor_arg
>> >+	 * remains valid until skb destructor */
>> >+	void		*destructor_arg;
>> > 	struct skb_shared_hwtstamps hwtstamps;
>> >
>> > 	/*
>> >@@ -202,9 +205,6 @@ struct skb_shared_info {
>> > 	 */
>> > 	atomic_t	dataref;
>> >
>> >-	/* Intermediate layers must ensure that destructor_arg
>> >-	 * remains valid until skb destructor */
>> >-	void *		destructor_arg;
>> > 	/* must be last field, see pskb_expand_head() */
>> > 	skb_frag_t	frags[MAX_SKB_FRAGS];
>> > };
>> >
>> >
>>
>> Will that affect the cache line?
>
>What do you mean ?
>
>> Or, we can move the line to clear destructor_arg to the end of __alloc_skb().
>> It looks like as the following, which one do you prefer?
>>
>> Thanks
>> Xiaohui
>>
>> ---
>>  net/core/skbuff.c |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index c83b421..df852f2 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -224,6 +224,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
>>
>>  		child->fclone = SKB_FCLONE_UNAVAILABLE;
>>  	}
>> +	shinfo->destructor_arg = NULL;
>>  out:
>>  	return skb;
>>  nodata:
>
>I dont understand why you want to do this.
>
>This adds an instruction, makes code bigger, and no obvious gain for me,
>at memory transactions side.
>
>If integrated in the existing memset(), cost is an extra iteration to
>perform the clear of this field.
>
Ok. Thanks for this explanation and will update with your solution.

Thanks
Xiaohui


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