From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Hemminger Subject: Re: [patch net-next-2.6] net: vlan: make non-hw-accel rx path similar to hw-accel Date: Sat, 2 Apr 2011 08:55:24 -0700 Message-ID: <20110402085524.6692131a@nehalam> References: <1301739966-7604-1-git-send-email-jpirko@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, davem@davemloft.net, kaber@trash.net, fubar@us.ibm.com, eric.dumazet@gmail.com, nicolas.2p.debian@gmail.com, andy@greyhouse.net, xiaosuo@gmail.com, jesse@nicira.com To: Jiri Pirko Return-path: Received: from smtp1.linux-foundation.org ([140.211.169.13]:37528 "EHLO smtp1.linux-foundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753163Ab1DBPz6 (ORCPT ); Sat, 2 Apr 2011 11:55:58 -0400 In-Reply-To: <1301739966-7604-1-git-send-email-jpirko@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On Sat, 2 Apr 2011 12:26:06 +0200 Jiri Pirko wrote: > Now there are 2 paths for rx vlan frames. When rx-vlan-hw-accel is > enabled, skb is untagged by NIC, vlan_tci is set and the skb gets into > vlan code in __netif_receive_skb - vlan_hwaccel_do_receive. > > For non-rx-vlan-hw-accel however, tagged skb goes thru whole > __netif_receive_skb, it's untagged in ptype_base hander and reinjected > > This incosistency is fixed by this patch. Vlan untagging happens early in > __netif_receive_skb so the rest of code (ptype_all handlers, rx_handlers) > see the skb like it was untagged by hw. > > Signed-off-by: Jiri Pirko > --- > include/linux/if_vlan.h | 10 ++- > net/8021q/vlan.c | 8 -- > net/8021q/vlan.h | 2 - > net/8021q/vlan_core.c | 86 +++++++++++++++++++++++- > net/8021q/vlan_dev.c | 173 ----------------------------------------------- > net/core/dev.c | 8 ++- > 6 files changed, 100 insertions(+), 187 deletions(-) > > diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h > index 635e1fa..998b299 100644 > --- a/include/linux/if_vlan.h > +++ b/include/linux/if_vlan.h > @@ -132,7 +132,8 @@ extern u16 vlan_dev_vlan_id(const struct net_device *dev); > > extern int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp, > u16 vlan_tci, int polling); > -extern bool vlan_hwaccel_do_receive(struct sk_buff **skb); > +extern bool vlan_do_receive(struct sk_buff **skb); > +extern struct sk_buff *vlan_untag(struct sk_buff *skb); > extern gro_result_t > vlan_gro_receive(struct napi_struct *napi, struct vlan_group *grp, > unsigned int vlan_tci, struct sk_buff *skb); > @@ -166,13 +167,18 @@ static inline int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp, > return NET_XMIT_SUCCESS; > } > > -static inline bool vlan_hwaccel_do_receive(struct sk_buff **skb) > +static inline bool vlan_do_receive(struct sk_buff **skb) > { > if ((*skb)->vlan_tci & VLAN_VID_MASK) > (*skb)->pkt_type = PACKET_OTHERHOST; > return false; > } Why the added unnecessary indirection > +inline struct sk_buff *vlan_untag(struct sk_buff *skb) > +{ > + return skb; > +} This adds no value. > static inline gro_result_t > vlan_gro_receive(struct napi_struct *napi, struct vlan_group *grp, > unsigned int vlan_tci, struct sk_buff *skb) > diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c > index 7850412..59f0a9d 100644 > --- a/net/8021q/vlan.c > +++ b/net/8021q/vlan.c > @@ -49,11 +49,6 @@ const char vlan_version[] = DRV_VERSION; > static const char vlan_copyright[] = "Ben Greear "; > static const char vlan_buggyright[] = "David S. Miller "; > > -static struct packet_type vlan_packet_type __read_mostly = { > - .type = cpu_to_be16(ETH_P_8021Q), > - .func = vlan_skb_recv, /* VLAN receive method */ > -}; > - > /* End of global variables definitions. */ > > static void vlan_group_free(struct vlan_group *grp) > @@ -688,7 +683,6 @@ static int __init vlan_proto_init(void) > if (err < 0) > goto err4; > > - dev_add_pack(&vlan_packet_type); > vlan_ioctl_set(vlan_ioctl_handler); > return 0; > > @@ -709,8 +703,6 @@ static void __exit vlan_cleanup_module(void) > > unregister_netdevice_notifier(&vlan_notifier_block); > > - dev_remove_pack(&vlan_packet_type); > - > unregister_pernet_subsys(&vlan_net_ops); > rcu_barrier(); /* Wait for completion of call_rcu()'s */ > > diff --git a/net/8021q/vlan.h b/net/8021q/vlan.h > index 5687c9b..c3408de 100644 > --- a/net/8021q/vlan.h > +++ b/net/8021q/vlan.h > @@ -75,8 +75,6 @@ static inline struct vlan_dev_info *vlan_dev_info(const struct net_device *dev) > } > > /* found in vlan_dev.c */ > -int vlan_skb_recv(struct sk_buff *skb, struct net_device *dev, > - struct packet_type *ptype, struct net_device *orig_dev); > void vlan_dev_set_ingress_priority(const struct net_device *dev, > u32 skb_prio, u16 vlan_prio); > int vlan_dev_set_egress_priority(const struct net_device *dev, > diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c > index ce8e3ab..bc83ecc 100644 > --- a/net/8021q/vlan_core.c > +++ b/net/8021q/vlan_core.c > @@ -4,7 +4,7 @@ > #include > #include "vlan.h" > > -bool vlan_hwaccel_do_receive(struct sk_buff **skbp) > +bool vlan_do_receive(struct sk_buff **skbp) > { > struct sk_buff *skb = *skbp; > u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK; > @@ -88,3 +88,87 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp, > return napi_gro_frags(napi); > } > EXPORT_SYMBOL(vlan_gro_frags); > + > +static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb) > +{ > + if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) { > + if (skb_cow(skb, skb_headroom(skb)) < 0) > + skb = NULL; > + if (skb) { > + /* Lifted from Gleb's VLAN code... */ > + memmove(skb->data - ETH_HLEN, > + skb->data - VLAN_ETH_HLEN, 12); > + skb->mac_header += VLAN_HLEN; > + } > + } > + return skb; > +} Do not mark code as 'static inline' let compiler decide. > +static inline void vlan_set_encap_proto(struct sk_buff *skb, > + struct vlan_hdr *vhdr) > +{ > + __be16 proto; > + unsigned char *rawp; > + > + /* > + * Was a VLAN packet, grab the encapsulated protocol, which the layer > + * three protocols care about. > + */ > + > + proto = vhdr->h_vlan_encapsulated_proto; > + if (ntohs(proto) >= 1536) { > + skb->protocol = proto; > + return; > + } > + > + rawp = skb->data; > + if (*(unsigned short *) rawp == 0xFFFF) > + /* > + * This is a magic hack to spot IPX packets. Older Novell > + * breaks the protocol design and runs IPX over 802.3 without > + * an 802.2 LLC layer. We look for FFFF which isn't a used > + * 802.2 SSAP/DSAP. This won't work for fault tolerant netware > + * but does for the rest. > + */ > + skb->protocol = htons(ETH_P_802_3); > + else > + /* > + * Real 802.2 LLC > + */ > + skb->protocol = htons(ETH_P_802_2); > +} What about doublely tagged packets? > +struct sk_buff *vlan_untag(struct sk_buff *skb) > +{ > + struct vlan_hdr *vhdr; > + u16 vlan_tci; > + > + if (unlikely(vlan_tx_tag_present(skb))) { > + /* vlan_tci is already set-up so leave this for another time */ > + return skb; > + } > + > + skb = skb_share_check(skb, GFP_ATOMIC); > + if (unlikely(!skb)) > + goto err_free; > + > + if (unlikely(!pskb_may_pull(skb, VLAN_HLEN))) > + goto err_free; > + > + vhdr = (struct vlan_hdr *) skb->data; > + vlan_tci = ntohs(vhdr->h_vlan_TCI); > + __vlan_hwaccel_put_tag(skb, vlan_tci); > + > + skb_pull_rcsum(skb, VLAN_HLEN); > + vlan_set_encap_proto(skb, vhdr); > + > + skb = vlan_check_reorder_header(skb); > + if (unlikely(!skb)) > + goto err_free; > + > + return skb; > + > +err_free: > + kfree_skb(skb); > + return NULL; > +} > diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c > index e34ea9e..b4f061f 100644 > --- a/net/8021q/vlan_dev.c > +++ b/net/8021q/vlan_dev.c > @@ -65,179 +65,6 @@ static int vlan_dev_rebuild_header(struct sk_buff *skb) > return 0; > } > > -static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb) > -{ > - if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) { > - if (skb_cow(skb, skb_headroom(skb)) < 0) > - skb = NULL; > - if (skb) { > - /* Lifted from Gleb's VLAN code... */ > - memmove(skb->data - ETH_HLEN, > - skb->data - VLAN_ETH_HLEN, 12); > - skb->mac_header += VLAN_HLEN; > - } > - } > - > - return skb; > -} > - > -static inline void vlan_set_encap_proto(struct sk_buff *skb, > - struct vlan_hdr *vhdr) > -{ > - __be16 proto; > - unsigned char *rawp; > - > - /* > - * Was a VLAN packet, grab the encapsulated protocol, which the layer > - * three protocols care about. > - */ > - > - proto = vhdr->h_vlan_encapsulated_proto; > - if (ntohs(proto) >= 1536) { > - skb->protocol = proto; > - return; > - } > - > - rawp = skb->data; > - if (*(unsigned short *)rawp == 0xFFFF) > - /* > - * This is a magic hack to spot IPX packets. Older Novell > - * breaks the protocol design and runs IPX over 802.3 without > - * an 802.2 LLC layer. We look for FFFF which isn't a used > - * 802.2 SSAP/DSAP. This won't work for fault tolerant netware > - * but does for the rest. > - */ > - skb->protocol = htons(ETH_P_802_3); > - else > - /* > - * Real 802.2 LLC > - */ > - skb->protocol = htons(ETH_P_802_2); > -} > - > -/* > - * Determine the packet's protocol ID. The rule here is that we > - * assume 802.3 if the type field is short enough to be a length. > - * This is normal practice and works for any 'now in use' protocol. > - * > - * Also, at this point we assume that we ARE dealing exclusively with > - * VLAN packets, or packets that should be made into VLAN packets based > - * on a default VLAN ID. > - * > - * NOTE: Should be similar to ethernet/eth.c. > - * > - * SANITY NOTE: This method is called when a packet is moving up the stack > - * towards userland. To get here, it would have already passed > - * through the ethernet/eth.c eth_type_trans() method. > - * SANITY NOTE 2: We are referencing to the VLAN_HDR frields, which MAY be > - * stored UNALIGNED in the memory. RISC systems don't like > - * such cases very much... > - * SANITY NOTE 2a: According to Dave Miller & Alexey, it will always be > - * aligned, so there doesn't need to be any of the unaligned > - * stuff. It has been commented out now... --Ben > - * > - */ > -int vlan_skb_recv(struct sk_buff *skb, struct net_device *dev, > - struct packet_type *ptype, struct net_device *orig_dev) > -{ > - struct vlan_hdr *vhdr; > - struct vlan_pcpu_stats *rx_stats; > - struct net_device *vlan_dev; > - u16 vlan_id; > - u16 vlan_tci; > - > - skb = skb_share_check(skb, GFP_ATOMIC); > - if (skb == NULL) > - goto err_free; > - > - if (unlikely(!pskb_may_pull(skb, VLAN_HLEN))) > - goto err_free; > - > - vhdr = (struct vlan_hdr *)skb->data; > - vlan_tci = ntohs(vhdr->h_vlan_TCI); > - vlan_id = vlan_tci & VLAN_VID_MASK; > - > - rcu_read_lock(); > - vlan_dev = vlan_find_dev(dev, vlan_id); > - > - /* If the VLAN device is defined, we use it. > - * If not, and the VID is 0, it is a 802.1p packet (not > - * really a VLAN), so we will just netif_rx it later to the > - * original interface, but with the skb->proto set to the > - * wrapped proto: we do nothing here. > - */ > - > - if (!vlan_dev) { > - if (vlan_id) { > - pr_debug("%s: ERROR: No net_device for VID: %u on dev: %s\n", > - __func__, vlan_id, dev->name); > - goto err_unlock; > - } > - rx_stats = NULL; > - } else { > - skb->dev = vlan_dev; > - > - rx_stats = this_cpu_ptr(vlan_dev_info(skb->dev)->vlan_pcpu_stats); > - > - u64_stats_update_begin(&rx_stats->syncp); > - rx_stats->rx_packets++; > - rx_stats->rx_bytes += skb->len; > - > - skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tci); > - > - pr_debug("%s: priority: %u for TCI: %hu\n", > - __func__, skb->priority, vlan_tci); > - > - switch (skb->pkt_type) { > - case PACKET_BROADCAST: > - /* Yeah, stats collect these together.. */ > - /* stats->broadcast ++; // no such counter :-( */ > - break; > - > - case PACKET_MULTICAST: > - rx_stats->rx_multicast++; > - break; > - > - case PACKET_OTHERHOST: > - /* Our lower layer thinks this is not local, let's make > - * sure. > - * This allows the VLAN to have a different MAC than the > - * underlying device, and still route correctly. > - */ > - if (!compare_ether_addr(eth_hdr(skb)->h_dest, > - skb->dev->dev_addr)) > - skb->pkt_type = PACKET_HOST; > - break; > - default: > - break; > - } > - u64_stats_update_end(&rx_stats->syncp); > - } > - > - skb_pull_rcsum(skb, VLAN_HLEN); > - vlan_set_encap_proto(skb, vhdr); > - > - if (vlan_dev) { > - skb = vlan_check_reorder_header(skb); > - if (!skb) { > - rx_stats->rx_errors++; > - goto err_unlock; > - } > - } > - > - netif_rx(skb); > - > - rcu_read_unlock(); > - return NET_RX_SUCCESS; > - > -err_unlock: > - rcu_read_unlock(); > -err_free: > - atomic_long_inc(&dev->rx_dropped); > - kfree_skb(skb); > - return NET_RX_DROP; > -} > - > static inline u16 > vlan_dev_get_egress_qos_mask(struct net_device *dev, struct sk_buff *skb) > { > diff --git a/net/core/dev.c b/net/core/dev.c > index 3da9fb0..bfe9fce 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -3130,6 +3130,12 @@ another_round: > > __this_cpu_inc(softnet_data.processed); > > + if (skb->protocol == cpu_to_be16(ETH_P_8021Q)) { > + skb = vlan_untag(skb); > + if (unlikely(!skb)) > + goto out; > + } This becomes a NOP, why is it here? > #ifdef CONFIG_NET_CLS_ACT > if (skb->tc_verd & TC_NCLS) { > skb->tc_verd = CLR_TC_NCLS(skb->tc_verd); > @@ -3177,7 +3183,7 @@ ncls: > ret = deliver_skb(skb, pt_prev, orig_dev); > pt_prev = NULL; > } > - if (vlan_hwaccel_do_receive(&skb)) { > + if (vlan_do_receive(&skb)) { > ret = __netif_receive_skb(skb); > goto out; > } else if (unlikely(!skb)) Why rename the function?