On Mon, Dec 10, 2012 at 9:58 PM, Dmitry Kravkov <dmi...@broadcom.com> wrote: >> -----Original Message----- >> From: saeed bishara [mailto:saeed.bish...@gmail.com] >> Sent: Monday, December 10, 2012 12:04 PM >> To: Joseph Gasparakis >> Cc: da...@davemloft.net; shemmin...@vyatta.com; chr...@sous-sol.org; >> go...@redhat.com; net...@vger.kernel.org; linux-kernel@vger.kernel.org; >> Dmitry Kravkov; bhutchi...@solarflare.com; Peter P Waskiewicz Jr; Alexander >> Duyck >> Subject: Re: [PATCH v4 1/5] net: Add support for hardware-offloaded >> encapsulation >> >> > +static inline struct iphdr *inner_ip_hdr(const struct sk_buff *skb) >> > +{ >> > + return (struct iphdr *)skb_inner_network_header(skb); >> > +} >> >> Hi, >> I'm a little bit bothered because of those inner_ functions, what >> about the following approach: >> 1. the skb will have a new state, that state can be outer (normal >> mode) and inner. >> 2. when you change the state to inner, all the helper functions such >> as ip_hdr will return the innter header. >> >> that's ofcourse the API side. the implementation may still use the >> fields you added to the skb. >> >> what you think? >> saeed > > Some drivers will probably need both inner_ and other_ in same flow, > switching between two states will consume cpu cycles. from performance perspective, I'm not sure the switching is worse, it may be better as it reduces code size. please have a look at patch 2/5, with switching you can avoid doing the following change -> less code, less if-else. - skb_set_transport_header(skb, - skb_checksum_start_offset(skb)); + if (skb->encapsulation) + skb_set_inner_transport_header(skb, + skb_checksum_start_offset(skb)); + else + skb_set_transport_header(skb, + skb_checksum_start_offset(skb)); if (!(features & NETIF_F_ALL_CSUM) &&
I think also that from (stack) maintenance perspective, less code is better. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
