| Message ID | 1528200262-11834-1-git-send-email-laoar.shao@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,1/2] ipv4: replace ip_hdr() with skb->data for optimization | expand |
On Tue, 2018-06-05 at 08:04 -0400, Yafang Shao wrote: > In ip receive path, when ip header hasn't been pulled yet, ip_hdr() and > skb->data are pointing to the same byte. > > In ip output path, when ip header is just pushed, ip_hdr() and skb->data > are pointing to the same byte. > > As ip_hdr() is more expensive than using skb->data, so replace ip_hdr() > with skb->data in these situations for optimization. IMHO this makes the code less readable and more error prone. Which kind of performance improvement do you measure here? Thanks, Paolo
On Tue, Jun 5, 2018 at 8:20 PM, Paolo Abeni <pabeni@redhat.com> wrote: > On Tue, 2018-06-05 at 08:04 -0400, Yafang Shao wrote: >> In ip receive path, when ip header hasn't been pulled yet, ip_hdr() and >> skb->data are pointing to the same byte. >> >> In ip output path, when ip header is just pushed, ip_hdr() and skb->data >> are pointing to the same byte. >> >> As ip_hdr() is more expensive than using skb->data, so replace ip_hdr() >> with skb->data in these situations for optimization. > > IMHO this makes the code less readable and more error prone. Which kind > of performance improvement do you measure here? > Correct the cc list. Hi Paolo, There's a "+" opertaion in ip_hdr(), using skb->data and avoid this operation. Thanks Yafang
From: Yafang Shao <laoar.shao@gmail.com> Date: Tue, 5 Jun 2018 20:29:05 +0800 > On Tue, Jun 5, 2018 at 8:20 PM, Paolo Abeni <pabeni@redhat.com> wrote: >> On Tue, 2018-06-05 at 08:04 -0400, Yafang Shao wrote: >>> In ip receive path, when ip header hasn't been pulled yet, ip_hdr() and >>> skb->data are pointing to the same byte. >>> >>> In ip output path, when ip header is just pushed, ip_hdr() and skb->data >>> are pointing to the same byte. >>> >>> As ip_hdr() is more expensive than using skb->data, so replace ip_hdr() >>> with skb->data in these situations for optimization. >> >> IMHO this makes the code less readable and more error prone. Which kind >> of performance improvement do you measure here? >> > > Correct the cc list. > > Hi Paolo, > > There's a "+" opertaion in ip_hdr(), using skb->data and avoid this operation. Paolo is asking what performance improvement did you "measure". I don't think this can possibly show up in a benchmark at all, and I agree the code becomes less readable, so I am not applying this, sorry.
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c index 7582713..7a03e8c 100644 --- a/net/ipv4/ip_input.c +++ b/net/ipv4/ip_input.c @@ -309,7 +309,7 @@ static inline bool ip_rcv_options(struct sk_buff *skb) static int ip_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb) { - const struct iphdr *iph = ip_hdr(skb); + const struct iphdr *iph = (const struct iphdr *)skb->data; int (*edemux)(struct sk_buff *skb); struct net_device *dev = skb->dev; struct rtable *rt; @@ -335,7 +335,7 @@ static int ip_rcv_finish(struct net *net, struct sock *sk, struct sk_buff *skb) if (unlikely(err)) goto drop_error; /* must reload iph, skb->head might have changed */ - iph = ip_hdr(skb); + iph = (const struct iphdr *)skb->data; } } @@ -433,7 +433,7 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt, if (!pskb_may_pull(skb, sizeof(struct iphdr))) goto inhdr_error; - iph = ip_hdr(skb); + iph = (const struct iphdr *)skb->data; /* * RFC1122: 3.2.1.2 MUST silently discard any IP frame that fails the checksum. @@ -459,7 +459,7 @@ int ip_rcv(struct sk_buff *skb, struct net_device *dev, struct packet_type *pt, if (!pskb_may_pull(skb, iph->ihl*4)) goto inhdr_error; - iph = ip_hdr(skb); + iph = (const struct iphdr *)skb->data; if (unlikely(ip_fast_csum((u8 *)iph, iph->ihl))) goto csum_error; diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index af5a830..f5014cd 100644 --- a/net/ipv4/ip_output.c +++ b/net/ipv4/ip_output.c @@ -96,7 +96,7 @@ void ip_send_check(struct iphdr *iph) int __ip_local_out(struct net *net, struct sock *sk, struct sk_buff *skb) { - struct iphdr *iph = ip_hdr(skb); + struct iphdr *iph = (struct iphdr *)skb->data; iph->tot_len = htons(skb->len); ip_send_check(iph); @@ -151,7 +151,7 @@ int ip_build_and_send_pkt(struct sk_buff *skb, const struct sock *sk, /* Build the IP header. */ skb_push(skb, sizeof(struct iphdr) + (opt ? opt->opt.optlen : 0)); skb_reset_network_header(skb); - iph = ip_hdr(skb); + iph = (struct iphdr *)skb->data; iph->version = 4; iph->ihl = 5; iph->tos = inet->tos; @@ -477,7 +477,7 @@ int ip_queue_xmit(struct sock *sk, struct sk_buff *skb, struct flowi *fl) /* OK, we know where to send it, allocate and build IP header. */ skb_push(skb, sizeof(struct iphdr) + (inet_opt ? inet_opt->opt.optlen : 0)); skb_reset_network_header(skb); - iph = ip_hdr(skb); + iph = (struct iphdr *)skb->data; *((__be16 *)iph) = htons((4 << 12) | (5 << 8) | (inet->tos & 0xff)); if (ip_dont_fragment(sk, &rt->dst) && !skb->ignore_df) iph->frag_off = htons(IP_DF); @@ -659,7 +659,7 @@ int ip_do_fragment(struct net *net, struct sock *sk, struct sk_buff *skb, __skb_push(frag, hlen); skb_reset_network_header(frag); memcpy(skb_network_header(frag), iph, hlen); - iph = ip_hdr(frag); + iph = (struct iphdr *)skb->data; iph->tot_len = htons(frag->len); ip_copy_metadata(frag, skb); if (offset == 0)
In ip receive path, when ip header hasn't been pulled yet, ip_hdr() and skb->data are pointing to the same byte. In ip output path, when ip header is just pushed, ip_hdr() and skb->data are pointing to the same byte. As ip_hdr() is more expensive than using skb->data, so replace ip_hdr() with skb->data in these situations for optimization. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- net/ipv4/ip_input.c | 8 ++++---- net/ipv4/ip_output.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)