[ovs-dev] [PATCH 2/2] datapath: Allow minimum headroom to be set when copying buffers.

Ben Pfaff blp at nicira.com
Wed Nov 18 21:22:50 UTC 2009


Jesse Gross <jesse at nicira.com> writes:

> If we need to copy an sk_buff in order to make it writable, allow
> the minimum headroom to be specified.  This ensures that if we
> need to add additional data, such as a VLAN tag, we will not have
> to make a second copy.

Good catch.

>  	if (skb_shared(skb) || skb_cloned(skb)) {
> -		struct sk_buff *nskb = skb_copy(skb, gfp);
> +		struct sk_buff *nskb;
> +		int headroom = min_headroom > skb_headroom(skb) ? min_headroom
> +				: skb_headroom(skb);

I would write this as 
    unsigned headroom = max(min_headroom, skb_headroom(skb));
and change "min_headroom" to an unsigned int.

(Outside the Linux kernel this might not be a good idea.  But the
Linux kernel definition of "max" only evaluates its arguments
once.)

> +#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,24)
> +			/* Before 2.6.24 these fields were not copied when
> +			 * doing an skb_copy_expand. */
> +			nskb->ip_summed = skb->ip_summed;
> +			nskb->csum = skb->csum;
> +#endif

Wow, that's real attention to detail, great.

> diff --git a/datapath/actions.h b/datapath/actions.h
> index bda6d55..f6eec29 100644
> --- a/datapath/actions.h
> +++ b/datapath/actions.h
> @@ -16,7 +16,7 @@ struct sk_buff;
>  struct odp_flow_key;
>  union odp_action;
>  
> -struct sk_buff *make_writable(struct sk_buff *, gfp_t gfp);
> +struct sk_buff *make_writable(struct sk_buff *, int min_headroom, gfp_t gfp);
>  int dp_xmit_skb(struct sk_buff *);
>  int execute_actions(struct datapath *dp, struct sk_buff *skb,
>  		    struct odp_flow_key *key,

I can't see any users of this function outside actions.c, could
we just make it static?




More information about the dev mailing list