* [patch net-next v4 1/9] openvswitch: actions: use skb_postpull_rcsum when possible
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
@ 2014-11-19 13:04 ` Jiri Pirko
2014-11-20 1:52 ` Simon Horman
2014-11-19 13:04 ` [patch net-next v4 2/9] vlan: make __vlan_hwaccel_put_tag return void Jiri Pirko
` (9 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:04 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
Replace duplicated code by calling skb_postpull_rcsum
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
---
net/openvswitch/actions.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 394efa6..749a301 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -175,10 +175,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
if (unlikely(err))
return err;
- if (skb->ip_summed == CHECKSUM_COMPLETE)
- skb->csum = csum_sub(skb->csum,
- csum_partial(skb_mpls_header(skb),
- MPLS_HLEN, 0));
+ skb_postpull_rcsum(skb, skb_mpls_header(skb), MPLS_HLEN);
memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
skb->mac_len);
@@ -230,9 +227,7 @@ static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
if (unlikely(err))
return err;
- if (skb->ip_summed == CHECKSUM_COMPLETE)
- skb->csum = csum_sub(skb->csum, csum_partial(skb->data
- + (2 * ETH_ALEN), VLAN_HLEN, 0));
+ skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
*current_tci = vhdr->h_vlan_TCI;
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [patch net-next v4 1/9] openvswitch: actions: use skb_postpull_rcsum when possible
2014-11-19 13:04 ` [patch net-next v4 1/9] openvswitch: actions: use skb_postpull_rcsum when possible Jiri Pirko
@ 2014-11-20 1:52 ` Simon Horman
0 siblings, 0 replies; 24+ messages in thread
From: Simon Horman @ 2014-11-20 1:52 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, pshelar, therbert, edumazet, willemb,
dborkman, mst, fw, Paul.Durrant, tgraf, cwang
On Wed, Nov 19, 2014 at 02:04:55PM +0100, Jiri Pirko wrote:
> Replace duplicated code by calling skb_postpull_rcsum
>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> Acked-by: Pravin B Shelar <pshelar@nicira.com>
I believe this may have originally been my handiwork:
Acked-by: Simon Horman <simon.horman@netronome.com>
> ---
> net/openvswitch/actions.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 394efa6..749a301 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -175,10 +175,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> if (unlikely(err))
> return err;
>
> - if (skb->ip_summed == CHECKSUM_COMPLETE)
> - skb->csum = csum_sub(skb->csum,
> - csum_partial(skb_mpls_header(skb),
> - MPLS_HLEN, 0));
> + skb_postpull_rcsum(skb, skb_mpls_header(skb), MPLS_HLEN);
>
> memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
> skb->mac_len);
> @@ -230,9 +227,7 @@ static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
> if (unlikely(err))
> return err;
>
> - if (skb->ip_summed == CHECKSUM_COMPLETE)
> - skb->csum = csum_sub(skb->csum, csum_partial(skb->data
> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>
> vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> *current_tci = vhdr->h_vlan_TCI;
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [patch net-next v4 2/9] vlan: make __vlan_hwaccel_put_tag return void
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
2014-11-19 13:04 ` [patch net-next v4 1/9] openvswitch: actions: use skb_postpull_rcsum when possible Jiri Pirko
@ 2014-11-19 13:04 ` Jiri Pirko
2014-11-19 13:04 ` [patch net-next v4 3/9] vlan: kill vlan_put_tag helper Jiri Pirko
` (8 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:04 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
Always returns the same skb it gets, so change to void.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
---
drivers/scsi/fcoe/fcoe.c | 6 ++----
include/linux/if_vlan.h | 9 ++++-----
net/8021q/vlan_dev.c | 2 +-
3 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 4a8ac7d..73a8cc4 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -1669,10 +1669,8 @@ static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) {
/* must set skb->dev before calling vlan_put_tag */
skb->dev = fcoe->realdev;
- skb = __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
- vlan_dev_vlan_id(fcoe->netdev));
- if (!skb)
- return -ENOMEM;
+ __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
+ vlan_dev_vlan_id(fcoe->netdev));
} else
skb->dev = fcoe->netdev;
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index d69f057..1b5dbc2 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -347,13 +347,11 @@ static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb,
*
* Puts the VLAN TCI in @skb->vlan_tci and lets the device do the rest
*/
-static inline struct sk_buff *__vlan_hwaccel_put_tag(struct sk_buff *skb,
- __be16 vlan_proto,
- u16 vlan_tci)
+static inline void __vlan_hwaccel_put_tag(struct sk_buff *skb,
+ __be16 vlan_proto, u16 vlan_tci)
{
skb->vlan_proto = vlan_proto;
skb->vlan_tci = VLAN_TAG_PRESENT | vlan_tci;
- return skb;
}
/**
@@ -368,7 +366,8 @@ static inline struct sk_buff *vlan_put_tag(struct sk_buff *skb,
__be16 vlan_proto, u16 vlan_tci)
{
if (vlan_hw_offload_capable(skb->dev->features, vlan_proto)) {
- return __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
+ __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
+ return skb;
} else {
return __vlan_put_tag(skb, vlan_proto, vlan_tci);
}
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 0d441ec..d6524b2 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -150,7 +150,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
u16 vlan_tci;
vlan_tci = vlan->vlan_id;
vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
- skb = __vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);
+ __vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);
}
skb->dev = vlan->real_dev;
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [patch net-next v4 3/9] vlan: kill vlan_put_tag helper
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
2014-11-19 13:04 ` [patch net-next v4 1/9] openvswitch: actions: use skb_postpull_rcsum when possible Jiri Pirko
2014-11-19 13:04 ` [patch net-next v4 2/9] vlan: make __vlan_hwaccel_put_tag return void Jiri Pirko
@ 2014-11-19 13:04 ` Jiri Pirko
2014-11-19 13:04 ` [patch net-next v4 4/9] vlan: rename __vlan_put_tag to vlan_insert_tag_set_proto Jiri Pirko
` (7 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:04 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
Since both tx and rx paths work with skb->vlan_tci, there's no need for
this function anymore. Switch users directly to __vlan_hwaccel_put_tag.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
drivers/net/bonding/bond_alb.c | 17 ++++-------------
drivers/net/bonding/bond_main.c | 8 ++------
drivers/net/usb/cdc_mbim.c | 2 +-
include/linux/if_vlan.h | 19 -------------------
4 files changed, 7 insertions(+), 39 deletions(-)
diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index e1f1a00..bb9e9fc 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -475,12 +475,8 @@ static void rlb_update_client(struct rlb_client_info *client_info)
skb->dev = client_info->slave->dev;
if (client_info->vlan_id) {
- skb = vlan_put_tag(skb, htons(ETH_P_8021Q), client_info->vlan_id);
- if (!skb) {
- netdev_err(client_info->slave->bond->dev,
- "failed to insert VLAN tag\n");
- continue;
- }
+ __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
+ client_info->vlan_id);
}
arp_xmit(skb);
@@ -951,13 +947,8 @@ static void alb_send_lp_vid(struct slave *slave, u8 mac_addr[],
skb->priority = TC_PRIO_CONTROL;
skb->dev = slave->dev;
- if (vid) {
- skb = vlan_put_tag(skb, vlan_proto, vid);
- if (!skb) {
- netdev_err(slave->bond->dev, "failed to insert VLAN tag\n");
- return;
- }
- }
+ if (vid)
+ __vlan_hwaccel_put_tag(skb, vlan_proto, vid);
dev_queue_xmit(skb);
}
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 8575fee..e26c682 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2159,12 +2159,8 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op,
if (outer_tag->vlan_id) {
netdev_dbg(slave_dev, "outer tag: proto %X vid %X\n",
ntohs(outer_tag->vlan_proto), outer_tag->vlan_id);
- skb = vlan_put_tag(skb, outer_tag->vlan_proto,
- outer_tag->vlan_id);
- if (!skb) {
- net_err_ratelimited("failed to insert outer VLAN tag\n");
- return;
- }
+ __vlan_hwaccel_put_tag(skb, outer_tag->vlan_proto,
+ outer_tag->vlan_id);
}
xmit:
diff --git a/drivers/net/usb/cdc_mbim.c b/drivers/net/usb/cdc_mbim.c
index 5ee7a1d..96fc8a5 100644
--- a/drivers/net/usb/cdc_mbim.c
+++ b/drivers/net/usb/cdc_mbim.c
@@ -402,7 +402,7 @@ static struct sk_buff *cdc_mbim_process_dgram(struct usbnet *dev, u8 *buf, size_
/* map MBIM session to VLAN */
if (tci)
- vlan_put_tag(skb, htons(ETH_P_8021Q), tci);
+ __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), tci);
err:
return skb;
}
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 1b5dbc2..75b70a5 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -355,25 +355,6 @@ static inline void __vlan_hwaccel_put_tag(struct sk_buff *skb,
}
/**
- * vlan_put_tag - inserts VLAN tag according to device features
- * @skb: skbuff to tag
- * @vlan_tci: VLAN TCI to insert
- *
- * Assumes skb->dev is the target that will xmit this frame.
- * Returns a VLAN tagged skb.
- */
-static inline struct sk_buff *vlan_put_tag(struct sk_buff *skb,
- __be16 vlan_proto, u16 vlan_tci)
-{
- if (vlan_hw_offload_capable(skb->dev->features, vlan_proto)) {
- __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
- return skb;
- } else {
- return __vlan_put_tag(skb, vlan_proto, vlan_tci);
- }
-}
-
-/**
* __vlan_get_tag - get the VLAN ID that is part of the payload
* @skb: skbuff to query
* @vlan_tci: buffer to store vlaue
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [patch net-next v4 4/9] vlan: rename __vlan_put_tag to vlan_insert_tag_set_proto
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
` (2 preceding siblings ...)
2014-11-19 13:04 ` [patch net-next v4 3/9] vlan: kill vlan_put_tag helper Jiri Pirko
@ 2014-11-19 13:04 ` Jiri Pirko
2014-11-19 13:04 ` [patch net-next v4 5/9] vlan: introduce *vlan_hwaccel_push_inside helpers Jiri Pirko
` (6 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:04 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
Name fits better. Plus there's going to be introduced
__vlan_insert_tag later on.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
---
drivers/net/bonding/bond_main.c | 4 ++--
drivers/net/ethernet/emulex/benet/be_main.c | 6 ++++--
drivers/net/vxlan.c | 12 ++++++------
include/linux/if_vlan.h | 8 +++++---
net/bridge/br_vlan.c | 4 ++--
net/core/dev.c | 4 ++--
net/core/netpoll.c | 4 ++--
net/ipv4/geneve.c | 11 +++++------
net/openvswitch/actions.c | 4 +++-
net/openvswitch/datapath.c | 3 ++-
net/openvswitch/vport-gre.c | 6 +++---
11 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e26c682..c1d7da4 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2146,8 +2146,8 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op,
netdev_dbg(slave_dev, "inner tag: proto %X vid %X\n",
ntohs(outer_tag->vlan_proto), tags->vlan_id);
- skb = __vlan_put_tag(skb, tags->vlan_proto,
- tags->vlan_id);
+ skb = vlan_insert_tag_set_proto(skb, tags->vlan_proto,
+ tags->vlan_id);
if (!skb) {
net_err_ratelimited("failed to insert inner VLAN tag\n");
return;
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 54160cc..d02fbc7 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -887,7 +887,8 @@ static struct sk_buff *be_insert_vlan_in_pkt(struct be_adapter *adapter,
}
if (vlan_tag) {
- skb = __vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_tag);
+ skb = vlan_insert_tag_set_proto(skb, htons(ETH_P_8021Q),
+ vlan_tag);
if (unlikely(!skb))
return skb;
skb->vlan_tci = 0;
@@ -896,7 +897,8 @@ static struct sk_buff *be_insert_vlan_in_pkt(struct be_adapter *adapter,
/* Insert the outer VLAN, if any */
if (adapter->qnq_vid) {
vlan_tag = adapter->qnq_vid;
- skb = __vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_tag);
+ skb = vlan_insert_tag_set_proto(skb, htons(ETH_P_8021Q),
+ vlan_tag);
if (unlikely(!skb))
return skb;
if (skip_hw_vlan)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 23b1e8c..bb8fbab 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1600,9 +1600,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
return err;
if (vlan_tx_tag_present(skb)) {
- if (WARN_ON(!__vlan_put_tag(skb,
- skb->vlan_proto,
- vlan_tx_tag_get(skb))))
+ skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+ vlan_tx_tag_get(skb));
+ if (WARN_ON(!skb))
return -ENOMEM;
skb->vlan_tci = 0;
@@ -1644,9 +1644,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
return err;
if (vlan_tx_tag_present(skb)) {
- if (WARN_ON(!__vlan_put_tag(skb,
- skb->vlan_proto,
- vlan_tx_tag_get(skb))))
+ skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+ vlan_tx_tag_get(skb));
+ if (WARN_ON(!skb))
return -ENOMEM;
skb->vlan_tci = 0;
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 75b70a5..46e4a15 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -320,8 +320,9 @@ static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
}
/**
- * __vlan_put_tag - regular VLAN tag inserting
+ * vlan_insert_tag_set_proto - regular VLAN tag inserting
* @skb: skbuff to tag
+ * @vlan_proto: VLAN encapsulation protocol
* @vlan_tci: VLAN TCI to insert
*
* Inserts the VLAN tag into @skb as part of the payload
@@ -330,8 +331,9 @@ static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
* Following the skb_unshare() example, in case of error, the calling function
* doesn't have to worry about freeing the original skb.
*/
-static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb,
- __be16 vlan_proto, u16 vlan_tci)
+static inline struct sk_buff *vlan_insert_tag_set_proto(struct sk_buff *skb,
+ __be16 vlan_proto,
+ u16 vlan_tci)
{
skb = vlan_insert_tag(skb, vlan_proto, vlan_tci);
if (skb)
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 150048f..97b8ddf 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -199,8 +199,8 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
if (skb->vlan_proto != proto) {
/* Protocol-mismatch, empty out vlan_tci for new tag */
skb_push(skb, ETH_HLEN);
- skb = __vlan_put_tag(skb, skb->vlan_proto,
- vlan_tx_tag_get(skb));
+ skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+ vlan_tx_tag_get(skb));
if (unlikely(!skb))
return false;
diff --git a/net/core/dev.c b/net/core/dev.c
index 1ab168e..3611e60 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2645,8 +2645,8 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
{
if (vlan_tx_tag_present(skb) &&
!vlan_hw_offload_capable(features, skb->vlan_proto)) {
- skb = __vlan_put_tag(skb, skb->vlan_proto,
- vlan_tx_tag_get(skb));
+ skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+ vlan_tx_tag_get(skb));
if (skb)
skb->vlan_tci = 0;
}
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index e6645b4..65d3723 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -79,8 +79,8 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
if (vlan_tx_tag_present(skb) &&
!vlan_hw_offload_capable(features, skb->vlan_proto)) {
- skb = __vlan_put_tag(skb, skb->vlan_proto,
- vlan_tx_tag_get(skb));
+ skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+ vlan_tx_tag_get(skb));
if (unlikely(!skb)) {
/* This is actually a packet drop, but we
* don't want the code that calls this
diff --git a/net/ipv4/geneve.c b/net/ipv4/geneve.c
index 31802af..fd430a6 100644
--- a/net/ipv4/geneve.c
+++ b/net/ipv4/geneve.c
@@ -132,12 +132,11 @@ int geneve_xmit_skb(struct geneve_sock *gs, struct rtable *rt,
return err;
if (vlan_tx_tag_present(skb)) {
- if (unlikely(!__vlan_put_tag(skb,
- skb->vlan_proto,
- vlan_tx_tag_get(skb)))) {
- err = -ENOMEM;
- return err;
- }
+ skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+ vlan_tx_tag_get(skb));
+ if (unlikely(!skb)
+ return -ENOMEM;
+
skb->vlan_tci = 0;
}
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 749a301..426b913 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -287,7 +287,9 @@ static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
/* push down current VLAN tag */
current_tag = vlan_tx_tag_get(skb);
- if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
+ skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+ current_tag);
+ if (!skb)
return -ENOMEM;
/* Update mac_len for subsequent MPLS actions */
skb->mac_len += VLAN_HLEN;
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index ab141d4..c63e60e 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -425,7 +425,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
if (!nskb)
return -ENOMEM;
- nskb = __vlan_put_tag(nskb, nskb->vlan_proto, vlan_tx_tag_get(nskb));
+ nskb = vlan_insert_tag_set_proto(nskb, nskb->vlan_proto,
+ vlan_tx_tag_get(nskb));
if (!nskb)
return -ENOMEM;
diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
index 8e61a5c..777cd8c 100644
--- a/net/openvswitch/vport-gre.c
+++ b/net/openvswitch/vport-gre.c
@@ -176,9 +176,9 @@ static int gre_tnl_send(struct vport *vport, struct sk_buff *skb)
}
if (vlan_tx_tag_present(skb)) {
- if (unlikely(!__vlan_put_tag(skb,
- skb->vlan_proto,
- vlan_tx_tag_get(skb)))) {
+ skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+ vlan_tx_tag_get(skb));
+ if (unlikely(!skb) {
err = -ENOMEM;
goto err_free_rt;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [patch net-next v4 5/9] vlan: introduce *vlan_hwaccel_push_inside helpers
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
` (3 preceding siblings ...)
2014-11-19 13:04 ` [patch net-next v4 4/9] vlan: rename __vlan_put_tag to vlan_insert_tag_set_proto Jiri Pirko
@ 2014-11-19 13:04 ` Jiri Pirko
2014-11-19 20:32 ` Pravin Shelar
2014-11-19 13:05 ` [patch net-next v4 6/9] vlan: introduce __vlan_insert_tag helper which does not free skb Jiri Pirko
` (5 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:04 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
Use them to push skb->vlan_tci into the payload and avoid code
duplication.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
v3->v4:
- fixed error path in ovs datapath
drivers/net/vxlan.c | 22 ++++++----------------
include/linux/if_vlan.h | 34 ++++++++++++++++++++++++++++++++++
net/core/dev.c | 8 ++------
net/core/netpoll.c | 4 +---
net/ipv4/geneve.c | 11 +++--------
net/openvswitch/datapath.c | 4 +---
net/openvswitch/vport-gre.c | 12 ++++--------
7 files changed, 51 insertions(+), 44 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index bb8fbab..64d45fa 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1599,14 +1599,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
if (unlikely(err))
return err;
- if (vlan_tx_tag_present(skb)) {
- skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
- vlan_tx_tag_get(skb));
- if (WARN_ON(!skb))
- return -ENOMEM;
-
- skb->vlan_tci = 0;
- }
+ skb = vlan_hwaccel_push_inside(skb);
+ if (WARN_ON(!skb))
+ return -ENOMEM;
vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
vxh->vx_flags = htonl(VXLAN_FLAGS);
@@ -1643,14 +1638,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
if (unlikely(err))
return err;
- if (vlan_tx_tag_present(skb)) {
- skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
- vlan_tx_tag_get(skb));
- if (WARN_ON(!skb))
- return -ENOMEM;
-
- skb->vlan_tci = 0;
- }
+ skb = vlan_hwaccel_push_inside(skb);
+ if (WARN_ON(!skb))
+ return -ENOMEM;
vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
vxh->vx_flags = htonl(VXLAN_FLAGS);
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 46e4a15..291e670 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -341,6 +341,40 @@ static inline struct sk_buff *vlan_insert_tag_set_proto(struct sk_buff *skb,
return skb;
}
+/*
+ * __vlan_hwaccel_push_inside - pushes vlan tag to the payload
+ * @skb: skbuff to tag
+ *
+ * Pushes the VLAN tag from @skb->vlan_tci inside to the payload.
+ *
+ * Following the skb_unshare() example, in case of error, the calling function
+ * doesn't have to worry about freeing the original skb.
+ */
+static inline struct sk_buff *__vlan_hwaccel_push_inside(struct sk_buff *skb)
+{
+ skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
+ vlan_tx_tag_get(skb));
+ if (likely(skb))
+ skb->vlan_tci = 0;
+ return skb;
+}
+/*
+ * vlan_hwaccel_push_inside - pushes vlan tag to the payload
+ * @skb: skbuff to tag
+ *
+ * Checks is tag is present in @skb->vlan_tci and if it is, it pushes the
+ * VLAN tag from @skb->vlan_tci inside to the payload.
+ *
+ * Following the skb_unshare() example, in case of error, the calling function
+ * doesn't have to worry about freeing the original skb.
+ */
+static inline struct sk_buff *vlan_hwaccel_push_inside(struct sk_buff *skb)
+{
+ if (vlan_tx_tag_present(skb))
+ skb = __vlan_hwaccel_push_inside(skb);
+ return skb;
+}
+
/**
* __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
* @skb: skbuff to tag
diff --git a/net/core/dev.c b/net/core/dev.c
index 3611e60..ac48362 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2644,12 +2644,8 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
netdev_features_t features)
{
if (vlan_tx_tag_present(skb) &&
- !vlan_hw_offload_capable(features, skb->vlan_proto)) {
- skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
- vlan_tx_tag_get(skb));
- if (skb)
- skb->vlan_tci = 0;
- }
+ !vlan_hw_offload_capable(features, skb->vlan_proto))
+ skb = __vlan_hwaccel_push_inside(skb);
return skb;
}
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 65d3723..e0ad5d1 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -79,8 +79,7 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
if (vlan_tx_tag_present(skb) &&
!vlan_hw_offload_capable(features, skb->vlan_proto)) {
- skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
- vlan_tx_tag_get(skb));
+ skb = __vlan_hwaccel_push_inside(skb);
if (unlikely(!skb)) {
/* This is actually a packet drop, but we
* don't want the code that calls this
@@ -88,7 +87,6 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
*/
goto out;
}
- skb->vlan_tci = 0;
}
status = netdev_start_xmit(skb, dev, txq, false);
diff --git a/net/ipv4/geneve.c b/net/ipv4/geneve.c
index fd430a6..a457232 100644
--- a/net/ipv4/geneve.c
+++ b/net/ipv4/geneve.c
@@ -131,14 +131,9 @@ int geneve_xmit_skb(struct geneve_sock *gs, struct rtable *rt,
if (unlikely(err))
return err;
- if (vlan_tx_tag_present(skb)) {
- skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
- vlan_tx_tag_get(skb));
- if (unlikely(!skb)
- return -ENOMEM;
-
- skb->vlan_tci = 0;
- }
+ skb = vlan_hwaccel_push_inside(skb);
+ if (unlikely(!skb))
+ return -ENOMEM;
gnvh = (struct genevehdr *)__skb_push(skb, sizeof(*gnvh) + opt_len);
geneve_build_header(gnvh, tun_flags, vni, opt_len, opt);
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index c63e60e..f37ca3e 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -425,12 +425,10 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
if (!nskb)
return -ENOMEM;
- nskb = vlan_insert_tag_set_proto(nskb, nskb->vlan_proto,
- vlan_tx_tag_get(nskb));
+ nskb = __vlan_hwaccel_push_inside(nskb);
if (!nskb)
return -ENOMEM;
- nskb->vlan_tci = 0;
skb = nskb;
}
diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
index 777cd8c..6b69df5 100644
--- a/net/openvswitch/vport-gre.c
+++ b/net/openvswitch/vport-gre.c
@@ -175,14 +175,10 @@ static int gre_tnl_send(struct vport *vport, struct sk_buff *skb)
goto err_free_rt;
}
- if (vlan_tx_tag_present(skb)) {
- skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
- vlan_tx_tag_get(skb));
- if (unlikely(!skb) {
- err = -ENOMEM;
- goto err_free_rt;
- }
- skb->vlan_tci = 0;
+ skb = vlan_hwaccel_push_inside(skb);
+ if (unlikely(!skb)) {
+ err = -ENOMEM;
+ goto err_free_rt;
}
/* Push Tunnel header. */
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [patch net-next v4 5/9] vlan: introduce *vlan_hwaccel_push_inside helpers
2014-11-19 13:04 ` [patch net-next v4 5/9] vlan: introduce *vlan_hwaccel_push_inside helpers Jiri Pirko
@ 2014-11-19 20:32 ` Pravin Shelar
0 siblings, 0 replies; 24+ messages in thread
From: Pravin Shelar @ 2014-11-19 20:32 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
On Wed, Nov 19, 2014 at 5:04 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Use them to push skb->vlan_tci into the payload and avoid code
> duplication.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
> ---
>
> v3->v4:
> - fixed error path in ovs datapath
>
> drivers/net/vxlan.c | 22 ++++++----------------
> include/linux/if_vlan.h | 34 ++++++++++++++++++++++++++++++++++
> net/core/dev.c | 8 ++------
> net/core/netpoll.c | 4 +---
> net/ipv4/geneve.c | 11 +++--------
> net/openvswitch/datapath.c | 4 +---
> net/openvswitch/vport-gre.c | 12 ++++--------
> 7 files changed, 51 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index bb8fbab..64d45fa 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1599,14 +1599,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
> if (unlikely(err))
> return err;
>
> - if (vlan_tx_tag_present(skb)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (WARN_ON(!skb))
> - return -ENOMEM;
> -
> - skb->vlan_tci = 0;
> - }
> + skb = vlan_hwaccel_push_inside(skb);
> + if (WARN_ON(!skb))
> + return -ENOMEM;
>
> vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> vxh->vx_flags = htonl(VXLAN_FLAGS);
> @@ -1643,14 +1638,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
> if (unlikely(err))
> return err;
>
> - if (vlan_tx_tag_present(skb)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (WARN_ON(!skb))
> - return -ENOMEM;
> -
> - skb->vlan_tci = 0;
> - }
> + skb = vlan_hwaccel_push_inside(skb);
> + if (WARN_ON(!skb))
> + return -ENOMEM;
>
> vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> vxh->vx_flags = htonl(VXLAN_FLAGS);
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 46e4a15..291e670 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -341,6 +341,40 @@ static inline struct sk_buff *vlan_insert_tag_set_proto(struct sk_buff *skb,
> return skb;
> }
>
> +/*
> + * __vlan_hwaccel_push_inside - pushes vlan tag to the payload
> + * @skb: skbuff to tag
> + *
> + * Pushes the VLAN tag from @skb->vlan_tci inside to the payload.
> + *
> + * Following the skb_unshare() example, in case of error, the calling function
> + * doesn't have to worry about freeing the original skb.
> + */
> +static inline struct sk_buff *__vlan_hwaccel_push_inside(struct sk_buff *skb)
> +{
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> + if (likely(skb))
> + skb->vlan_tci = 0;
> + return skb;
> +}
> +/*
> + * vlan_hwaccel_push_inside - pushes vlan tag to the payload
> + * @skb: skbuff to tag
> + *
> + * Checks is tag is present in @skb->vlan_tci and if it is, it pushes the
> + * VLAN tag from @skb->vlan_tci inside to the payload.
> + *
> + * Following the skb_unshare() example, in case of error, the calling function
> + * doesn't have to worry about freeing the original skb.
> + */
> +static inline struct sk_buff *vlan_hwaccel_push_inside(struct sk_buff *skb)
> +{
> + if (vlan_tx_tag_present(skb))
> + skb = __vlan_hwaccel_push_inside(skb);
> + return skb;
> +}
> +
> /**
> * __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
> * @skb: skbuff to tag
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3611e60..ac48362 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2644,12 +2644,8 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
> netdev_features_t features)
> {
> if (vlan_tx_tag_present(skb) &&
> - !vlan_hw_offload_capable(features, skb->vlan_proto)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (skb)
> - skb->vlan_tci = 0;
> - }
> + !vlan_hw_offload_capable(features, skb->vlan_proto))
> + skb = __vlan_hwaccel_push_inside(skb);
> return skb;
> }
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 65d3723..e0ad5d1 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -79,8 +79,7 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
>
> if (vlan_tx_tag_present(skb) &&
> !vlan_hw_offload_capable(features, skb->vlan_proto)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> + skb = __vlan_hwaccel_push_inside(skb);
> if (unlikely(!skb)) {
> /* This is actually a packet drop, but we
> * don't want the code that calls this
> @@ -88,7 +87,6 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
> */
> goto out;
> }
> - skb->vlan_tci = 0;
> }
>
> status = netdev_start_xmit(skb, dev, txq, false);
> diff --git a/net/ipv4/geneve.c b/net/ipv4/geneve.c
> index fd430a6..a457232 100644
> --- a/net/ipv4/geneve.c
> +++ b/net/ipv4/geneve.c
> @@ -131,14 +131,9 @@ int geneve_xmit_skb(struct geneve_sock *gs, struct rtable *rt,
> if (unlikely(err))
> return err;
>
> - if (vlan_tx_tag_present(skb)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (unlikely(!skb)
> - return -ENOMEM;
> -
> - skb->vlan_tci = 0;
> - }
> + skb = vlan_hwaccel_push_inside(skb);
> + if (unlikely(!skb))
> + return -ENOMEM;
>
> gnvh = (struct genevehdr *)__skb_push(skb, sizeof(*gnvh) + opt_len);
> geneve_build_header(gnvh, tun_flags, vni, opt_len, opt);
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index c63e60e..f37ca3e 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -425,12 +425,10 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
> if (!nskb)
> return -ENOMEM;
>
> - nskb = vlan_insert_tag_set_proto(nskb, nskb->vlan_proto,
> - vlan_tx_tag_get(nskb));
> + nskb = __vlan_hwaccel_push_inside(nskb);
> if (!nskb)
> return -ENOMEM;
>
> - nskb->vlan_tci = 0;
> skb = nskb;
> }
>
> diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
> index 777cd8c..6b69df5 100644
> --- a/net/openvswitch/vport-gre.c
> +++ b/net/openvswitch/vport-gre.c
> @@ -175,14 +175,10 @@ static int gre_tnl_send(struct vport *vport, struct sk_buff *skb)
> goto err_free_rt;
> }
>
> - if (vlan_tx_tag_present(skb)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (unlikely(!skb) {
> - err = -ENOMEM;
> - goto err_free_rt;
> - }
> - skb->vlan_tci = 0;
> + skb = vlan_hwaccel_push_inside(skb);
> + if (unlikely(!skb)) {
> + err = -ENOMEM;
> + goto err_free_rt;
> }
>
> /* Push Tunnel header. */
> --
> 1.9.3
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [patch net-next v4 6/9] vlan: introduce __vlan_insert_tag helper which does not free skb
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
` (4 preceding siblings ...)
2014-11-19 13:04 ` [patch net-next v4 5/9] vlan: introduce *vlan_hwaccel_push_inside helpers Jiri Pirko
@ 2014-11-19 13:05 ` Jiri Pirko
2014-11-19 13:05 ` [patch net-next v4 7/9] net: move make_writable helper into common code Jiri Pirko
` (4 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:05 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
There's a need for helper which inserts vlan tag but does not free the
skb in case of an error.
Suggested-by: Pravin Shelar <pshelar@nicira.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
---
include/linux/if_vlan.h | 45 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 291e670..515a35e 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -282,28 +282,24 @@ static inline bool vlan_hw_offload_capable(netdev_features_t features,
}
/**
- * vlan_insert_tag - regular VLAN tag inserting
+ * __vlan_insert_tag - regular VLAN tag inserting
* @skb: skbuff to tag
* @vlan_proto: VLAN encapsulation protocol
* @vlan_tci: VLAN TCI to insert
*
* Inserts the VLAN tag into @skb as part of the payload
- * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
- *
- * Following the skb_unshare() example, in case of error, the calling function
- * doesn't have to worry about freeing the original skb.
+ * Returns error if skb_cow_head failes.
*
* Does not change skb->protocol so this function can be used during receive.
*/
-static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
- __be16 vlan_proto, u16 vlan_tci)
+static inline int __vlan_insert_tag(struct sk_buff *skb,
+ __be16 vlan_proto, u16 vlan_tci)
{
struct vlan_ethhdr *veth;
- if (skb_cow_head(skb, VLAN_HLEN) < 0) {
- dev_kfree_skb_any(skb);
- return NULL;
- }
+ if (skb_cow_head(skb, VLAN_HLEN) < 0)
+ return -ENOMEM;
+
veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
/* Move the mac addresses to the beginning of the new header. */
@@ -316,6 +312,33 @@ static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
/* now, the TCI */
veth->h_vlan_TCI = htons(vlan_tci);
+ return 0;
+}
+
+/**
+ * vlan_insert_tag - regular VLAN tag inserting
+ * @skb: skbuff to tag
+ * @vlan_proto: VLAN encapsulation protocol
+ * @vlan_tci: VLAN TCI to insert
+ *
+ * Inserts the VLAN tag into @skb as part of the payload
+ * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
+ *
+ * Following the skb_unshare() example, in case of error, the calling function
+ * doesn't have to worry about freeing the original skb.
+ *
+ * Does not change skb->protocol so this function can be used during receive.
+ */
+static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
+ __be16 vlan_proto, u16 vlan_tci)
+{
+ int err;
+
+ err = __vlan_insert_tag(skb, vlan_proto, vlan_tci);
+ if (err) {
+ dev_kfree_skb_any(skb);
+ return NULL;
+ }
return skb;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [patch net-next v4 7/9] net: move make_writable helper into common code
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
` (5 preceding siblings ...)
2014-11-19 13:05 ` [patch net-next v4 6/9] vlan: introduce __vlan_insert_tag helper which does not free skb Jiri Pirko
@ 2014-11-19 13:05 ` Jiri Pirko
2014-11-19 13:05 ` [patch net-next v4 8/9] net: move vlan pop/push functions " Jiri Pirko
` (3 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:05 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
note that skb_make_writable already exists in net/netfilter/core.c
but does something slightly different.
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
---
include/linux/skbuff.h | 1 +
net/core/skbuff.c | 12 ++++++++++++
net/openvswitch/actions.c | 39 ++++++++++++++-------------------------
3 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 73c370e..e045516 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2678,6 +2678,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet);
unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
+int skb_ensure_writable(struct sk_buff *skb, int write_len);
struct skb_checksum_ops {
__wsum (*update)(const void *mem, int len, __wsum wsum);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7001896..d11bbe0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4151,6 +4151,18 @@ err_free:
}
EXPORT_SYMBOL(skb_vlan_untag);
+int skb_ensure_writable(struct sk_buff *skb, int write_len)
+{
+ if (!pskb_may_pull(skb, write_len))
+ return -ENOMEM;
+
+ if (!skb_cloned(skb) || skb_clone_writable(skb, write_len))
+ return 0;
+
+ return pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
+}
+EXPORT_SYMBOL(skb_ensure_writable);
+
/**
* alloc_skb_with_frags - allocate skb with page frags
*
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 426b913..7ffa377 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -119,17 +119,6 @@ static bool is_flow_key_valid(const struct sw_flow_key *key)
return !!key->eth.type;
}
-static int make_writable(struct sk_buff *skb, int write_len)
-{
- if (!pskb_may_pull(skb, write_len))
- return -ENOMEM;
-
- if (!skb_cloned(skb) || skb_clone_writable(skb, write_len))
- return 0;
-
- return pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
-}
-
static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
const struct ovs_action_push_mpls *mpls)
{
@@ -171,7 +160,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
struct ethhdr *hdr;
int err;
- err = make_writable(skb, skb->mac_len + MPLS_HLEN);
+ err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN);
if (unlikely(err))
return err;
@@ -201,7 +190,7 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
__be32 *stack;
int err;
- err = make_writable(skb, skb->mac_len + MPLS_HLEN);
+ err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN);
if (unlikely(err))
return err;
@@ -223,7 +212,7 @@ static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
struct vlan_hdr *vhdr;
int err;
- err = make_writable(skb, VLAN_ETH_HLEN);
+ err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
if (unlikely(err))
return err;
@@ -310,7 +299,7 @@ static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
const struct ovs_key_ethernet *eth_key)
{
int err;
- err = make_writable(skb, ETH_HLEN);
+ err = skb_ensure_writable(skb, ETH_HLEN);
if (unlikely(err))
return err;
@@ -412,8 +401,8 @@ static int set_ipv4(struct sk_buff *skb, struct sw_flow_key *key,
struct iphdr *nh;
int err;
- err = make_writable(skb, skb_network_offset(skb) +
- sizeof(struct iphdr));
+ err = skb_ensure_writable(skb, skb_network_offset(skb) +
+ sizeof(struct iphdr));
if (unlikely(err))
return err;
@@ -450,8 +439,8 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *key,
__be32 *saddr;
__be32 *daddr;
- err = make_writable(skb, skb_network_offset(skb) +
- sizeof(struct ipv6hdr));
+ err = skb_ensure_writable(skb, skb_network_offset(skb) +
+ sizeof(struct ipv6hdr));
if (unlikely(err))
return err;
@@ -493,7 +482,7 @@ static int set_ipv6(struct sk_buff *skb, struct sw_flow_key *key,
return 0;
}
-/* Must follow make_writable() since that can move the skb data. */
+/* Must follow skb_ensure_writable() since that can move the skb data. */
static void set_tp_port(struct sk_buff *skb, __be16 *port,
__be16 new_port, __sum16 *check)
{
@@ -523,8 +512,8 @@ static int set_udp(struct sk_buff *skb, struct sw_flow_key *key,
struct udphdr *uh;
int err;
- err = make_writable(skb, skb_transport_offset(skb) +
- sizeof(struct udphdr));
+ err = skb_ensure_writable(skb, skb_transport_offset(skb) +
+ sizeof(struct udphdr));
if (unlikely(err))
return err;
@@ -548,8 +537,8 @@ static int set_tcp(struct sk_buff *skb, struct sw_flow_key *key,
struct tcphdr *th;
int err;
- err = make_writable(skb, skb_transport_offset(skb) +
- sizeof(struct tcphdr));
+ err = skb_ensure_writable(skb, skb_transport_offset(skb) +
+ sizeof(struct tcphdr));
if (unlikely(err))
return err;
@@ -574,7 +563,7 @@ static int set_sctp(struct sk_buff *skb, struct sw_flow_key *key,
int err;
unsigned int sctphoff = skb_transport_offset(skb);
- err = make_writable(skb, sctphoff + sizeof(struct sctphdr));
+ err = skb_ensure_writable(skb, sctphoff + sizeof(struct sctphdr));
if (unlikely(err))
return err;
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [patch net-next v4 8/9] net: move vlan pop/push functions into common code
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
` (6 preceding siblings ...)
2014-11-19 13:05 ` [patch net-next v4 7/9] net: move make_writable helper into common code Jiri Pirko
@ 2014-11-19 13:05 ` Jiri Pirko
2014-11-21 16:11 ` Pravin Shelar
2014-11-19 13:05 ` [patch net-next v4 9/9] sched: introduce vlan action Jiri Pirko
` (2 subsequent siblings)
10 siblings, 1 reply; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:05 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
So it can be used from out of openvswitch code.
Did couple of cosmetic changes on the way, namely variable naming and
adding support for 8021AD proto.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
v3->v4:
- don't try to be smart and copy skb->mac_len manipulation as it is. It can be
adjusted later on.
- fix error path in do_execute_actions as suggested by Pravin
v2->v3:
- used previously introduced __vlan_insert_tag helper
- used skb_push/pull to get skb->data into needed point
- fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin
v1->v2:
- adjusted to fix recent ovs changes
- included change to use make_writable suggested by Eric
include/linux/skbuff.h | 2 +
net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
net/openvswitch/actions.c | 86 +++++-------------------------------------
3 files changed, 106 insertions(+), 77 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index e045516..78c299f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
int skb_ensure_writable(struct sk_buff *skb, int write_len);
+int skb_vlan_pop(struct sk_buff *skb);
+int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
struct skb_checksum_ops {
__wsum (*update)(const void *mem, int len, __wsum wsum);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d11bbe0..c906c5f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len)
}
EXPORT_SYMBOL(skb_ensure_writable);
+/* remove VLAN header from packet and update csum accordingly. */
+static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
+{
+ struct vlan_hdr *vhdr;
+ unsigned int offset = skb->data - skb_mac_header(skb);
+ int err;
+
+ __skb_push(skb, offset);
+ err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
+ if (unlikely(err))
+ goto pull;
+
+ skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
+
+ vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
+ *vlan_tci = ntohs(vhdr->h_vlan_TCI);
+
+ memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
+ __skb_pull(skb, VLAN_HLEN);
+
+ vlan_set_encap_proto(skb, vhdr);
+ skb->mac_header += VLAN_HLEN;
+
+ if (skb_network_offset(skb) < ETH_HLEN)
+ skb_set_network_header(skb, ETH_HLEN);
+
+ skb_reset_mac_len(skb);
+pull:
+ __skb_pull(skb, offset);
+
+ return err;
+}
+
+int skb_vlan_pop(struct sk_buff *skb)
+{
+ u16 vlan_tci;
+ __be16 vlan_proto;
+ int err;
+
+ if (likely(vlan_tx_tag_present(skb))) {
+ skb->vlan_tci = 0;
+ } else {
+ if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
+ skb->protocol != htons(ETH_P_8021AD)) ||
+ skb->len < VLAN_ETH_HLEN))
+ return 0;
+
+ err = __skb_vlan_pop(skb, &vlan_tci);
+ if (err)
+ return err;
+ }
+ /* move next vlan tag to hw accel tag */
+ if (likely((skb->protocol != htons(ETH_P_8021Q) &&
+ skb->protocol != htons(ETH_P_8021AD)) ||
+ skb->len < VLAN_ETH_HLEN))
+ return 0;
+
+ vlan_proto = skb->protocol;
+ err = __skb_vlan_pop(skb, &vlan_tci);
+ if (unlikely(err))
+ return err;
+
+ __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
+ return 0;
+}
+EXPORT_SYMBOL(skb_vlan_pop);
+
+int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
+{
+ if (vlan_tx_tag_present(skb)) {
+ unsigned int offset = skb->data - skb_mac_header(skb);
+ int err;
+
+ /* __vlan_insert_tag expect skb->data pointing to mac header.
+ * So change skb->data before calling it and change back to
+ * original position later
+ */
+ __skb_push(skb, offset);
+ err = __vlan_insert_tag(skb, skb->vlan_proto,
+ vlan_tx_tag_get(skb));
+ if (err)
+ return err;
+ skb->protocol = skb->vlan_proto;
+ skb->mac_len += VLAN_HLEN;
+ __skb_pull(skb, offset);
+
+ if (skb->ip_summed == CHECKSUM_COMPLETE)
+ skb->csum = csum_add(skb->csum, csum_partial(skb->data
+ + (2 * ETH_ALEN), VLAN_HLEN, 0));
+ }
+ __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
+ return 0;
+}
+EXPORT_SYMBOL(skb_vlan_push);
+
/**
* alloc_skb_with_frags - allocate skb with page frags
*
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 7ffa377..4e05ea1 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
return 0;
}
-/* remove VLAN header from packet and update csum accordingly. */
-static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
-{
- struct vlan_hdr *vhdr;
- int err;
-
- err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
- if (unlikely(err))
- return err;
-
- skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
-
- vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
- *current_tci = vhdr->h_vlan_TCI;
-
- memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
- __skb_pull(skb, VLAN_HLEN);
-
- vlan_set_encap_proto(skb, vhdr);
- skb->mac_header += VLAN_HLEN;
-
- if (skb_network_offset(skb) < ETH_HLEN)
- skb_set_network_header(skb, ETH_HLEN);
-
- /* Update mac_len for subsequent MPLS actions */
- skb_reset_mac_len(skb);
- return 0;
-}
-
static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
{
- __be16 tci;
int err;
- if (likely(vlan_tx_tag_present(skb))) {
- skb->vlan_tci = 0;
- } else {
- if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
- skb->len < VLAN_ETH_HLEN))
- return 0;
-
- err = __pop_vlan_tci(skb, &tci);
- if (err)
- return err;
- }
- /* move next vlan tag to hw accel tag */
- if (likely(skb->protocol != htons(ETH_P_8021Q) ||
- skb->len < VLAN_ETH_HLEN)) {
+ err = skb_vlan_pop(skb);
+ if (vlan_tx_tag_present(skb))
+ invalidate_flow_key(key);
+ else
key->eth.tci = 0;
- return 0;
- }
-
- invalidate_flow_key(key);
- err = __pop_vlan_tci(skb, &tci);
- if (unlikely(err))
- return err;
-
- __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
- return 0;
+ return err;
}
static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
const struct ovs_action_push_vlan *vlan)
{
- if (unlikely(vlan_tx_tag_present(skb))) {
- u16 current_tag;
-
- /* push down current VLAN tag */
- current_tag = vlan_tx_tag_get(skb);
-
- skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
- current_tag);
- if (!skb)
- return -ENOMEM;
- /* Update mac_len for subsequent MPLS actions */
- skb->mac_len += VLAN_HLEN;
-
- if (skb->ip_summed == CHECKSUM_COMPLETE)
- skb->csum = csum_add(skb->csum, csum_partial(skb->data
- + (2 * ETH_ALEN), VLAN_HLEN, 0));
-
+ if (vlan_tx_tag_present(skb))
invalidate_flow_key(key);
- } else {
+ else
key->eth.tci = vlan->vlan_tci;
- }
- __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
- return 0;
+ return skb_vlan_push(skb, vlan->vlan_tpid,
+ ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
}
static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
@@ -858,8 +792,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
case OVS_ACTION_ATTR_PUSH_VLAN:
err = push_vlan(skb, key, nla_data(a));
- if (unlikely(err)) /* skb already freed. */
- return err;
break;
case OVS_ACTION_ATTR_POP_VLAN:
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code
2014-11-19 13:05 ` [patch net-next v4 8/9] net: move vlan pop/push functions " Jiri Pirko
@ 2014-11-21 16:11 ` Pravin Shelar
2014-11-21 18:05 ` Jiri Pirko
0 siblings, 1 reply; 24+ messages in thread
From: Pravin Shelar @ 2014-11-21 16:11 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
On Wed, Nov 19, 2014 at 5:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> So it can be used from out of openvswitch code.
> Did couple of cosmetic changes on the way, namely variable naming and
> adding support for 8021AD proto.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>
> v3->v4:
> - don't try to be smart and copy skb->mac_len manipulation as it is. It can be
> adjusted later on.
I am fine with moving this code to generic library, as long as we can
adjust the mac_len as offset for MPLS header, otherwise this breaks
MPLS support.
> - fix error path in do_execute_actions as suggested by Pravin
> v2->v3:
> - used previously introduced __vlan_insert_tag helper
> - used skb_push/pull to get skb->data into needed point
> - fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin
> v1->v2:
> - adjusted to fix recent ovs changes
> - included change to use make_writable suggested by Eric
>
> include/linux/skbuff.h | 2 +
> net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> net/openvswitch/actions.c | 86 +++++-------------------------------------
> 3 files changed, 106 insertions(+), 77 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index e045516..78c299f 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
> struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
> int skb_ensure_writable(struct sk_buff *skb, int write_len);
> +int skb_vlan_pop(struct sk_buff *skb);
> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>
> struct skb_checksum_ops {
> __wsum (*update)(const void *mem, int len, __wsum wsum);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index d11bbe0..c906c5f 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len)
> }
> EXPORT_SYMBOL(skb_ensure_writable);
>
> +/* remove VLAN header from packet and update csum accordingly. */
> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
> +{
> + struct vlan_hdr *vhdr;
> + unsigned int offset = skb->data - skb_mac_header(skb);
> + int err;
> +
> + __skb_push(skb, offset);
> + err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
> + if (unlikely(err))
> + goto pull;
> +
> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
> +
> + vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> + *vlan_tci = ntohs(vhdr->h_vlan_TCI);
> +
> + memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> + __skb_pull(skb, VLAN_HLEN);
> +
> + vlan_set_encap_proto(skb, vhdr);
> + skb->mac_header += VLAN_HLEN;
> +
> + if (skb_network_offset(skb) < ETH_HLEN)
> + skb_set_network_header(skb, ETH_HLEN);
> +
> + skb_reset_mac_len(skb);
> +pull:
> + __skb_pull(skb, offset);
> +
> + return err;
> +}
> +
> +int skb_vlan_pop(struct sk_buff *skb)
> +{
> + u16 vlan_tci;
> + __be16 vlan_proto;
> + int err;
> +
> + if (likely(vlan_tx_tag_present(skb))) {
> + skb->vlan_tci = 0;
> + } else {
> + if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
> + skb->protocol != htons(ETH_P_8021AD)) ||
> + skb->len < VLAN_ETH_HLEN))
> + return 0;
> +
> + err = __skb_vlan_pop(skb, &vlan_tci);
> + if (err)
> + return err;
> + }
> + /* move next vlan tag to hw accel tag */
> + if (likely((skb->protocol != htons(ETH_P_8021Q) &&
> + skb->protocol != htons(ETH_P_8021AD)) ||
> + skb->len < VLAN_ETH_HLEN))
> + return 0;
> +
> + vlan_proto = skb->protocol;
> + err = __skb_vlan_pop(skb, &vlan_tci);
> + if (unlikely(err))
> + return err;
> +
> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> + return 0;
> +}
> +EXPORT_SYMBOL(skb_vlan_pop);
> +
> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
> +{
> + if (vlan_tx_tag_present(skb)) {
> + unsigned int offset = skb->data - skb_mac_header(skb);
> + int err;
> +
> + /* __vlan_insert_tag expect skb->data pointing to mac header.
> + * So change skb->data before calling it and change back to
> + * original position later
> + */
> + __skb_push(skb, offset);
> + err = __vlan_insert_tag(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> + if (err)
> + return err;
> + skb->protocol = skb->vlan_proto;
> + skb->mac_len += VLAN_HLEN;
> + __skb_pull(skb, offset);
> +
> + if (skb->ip_summed == CHECKSUM_COMPLETE)
> + skb->csum = csum_add(skb->csum, csum_partial(skb->data
> + + (2 * ETH_ALEN), VLAN_HLEN, 0));
> + }
> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> + return 0;
> +}
> +EXPORT_SYMBOL(skb_vlan_push);
> +
> /**
> * alloc_skb_with_frags - allocate skb with page frags
> *
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 7ffa377..4e05ea1 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> return 0;
> }
>
> -/* remove VLAN header from packet and update csum accordingly. */
> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
> -{
> - struct vlan_hdr *vhdr;
> - int err;
> -
> - err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
> - if (unlikely(err))
> - return err;
> -
> - skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
> -
> - vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> - *current_tci = vhdr->h_vlan_TCI;
> -
> - memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> - __skb_pull(skb, VLAN_HLEN);
> -
> - vlan_set_encap_proto(skb, vhdr);
> - skb->mac_header += VLAN_HLEN;
> -
> - if (skb_network_offset(skb) < ETH_HLEN)
> - skb_set_network_header(skb, ETH_HLEN);
> -
> - /* Update mac_len for subsequent MPLS actions */
> - skb_reset_mac_len(skb);
> - return 0;
> -}
> -
> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
> {
> - __be16 tci;
> int err;
>
> - if (likely(vlan_tx_tag_present(skb))) {
> - skb->vlan_tci = 0;
> - } else {
> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN))
> - return 0;
> -
> - err = __pop_vlan_tci(skb, &tci);
> - if (err)
> - return err;
> - }
> - /* move next vlan tag to hw accel tag */
> - if (likely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN)) {
> + err = skb_vlan_pop(skb);
> + if (vlan_tx_tag_present(skb))
> + invalidate_flow_key(key);
> + else
> key->eth.tci = 0;
> - return 0;
> - }
> -
> - invalidate_flow_key(key);
> - err = __pop_vlan_tci(skb, &tci);
> - if (unlikely(err))
> - return err;
> -
> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
> - return 0;
> + return err;
> }
>
> static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
> const struct ovs_action_push_vlan *vlan)
> {
> - if (unlikely(vlan_tx_tag_present(skb))) {
> - u16 current_tag;
> -
> - /* push down current VLAN tag */
> - current_tag = vlan_tx_tag_get(skb);
> -
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - current_tag);
> - if (!skb)
> - return -ENOMEM;
> - /* Update mac_len for subsequent MPLS actions */
> - skb->mac_len += VLAN_HLEN;
> -
> - if (skb->ip_summed == CHECKSUM_COMPLETE)
> - skb->csum = csum_add(skb->csum, csum_partial(skb->data
> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
> -
> + if (vlan_tx_tag_present(skb))
> invalidate_flow_key(key);
> - } else {
> + else
> key->eth.tci = vlan->vlan_tci;
> - }
> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
> - return 0;
> + return skb_vlan_push(skb, vlan->vlan_tpid,
> + ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
> }
>
> static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
> @@ -858,8 +792,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>
> case OVS_ACTION_ATTR_PUSH_VLAN:
> err = push_vlan(skb, key, nla_data(a));
> - if (unlikely(err)) /* skb already freed. */
> - return err;
> break;
>
> case OVS_ACTION_ATTR_POP_VLAN:
> --
> 1.9.3
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code
2014-11-21 16:11 ` Pravin Shelar
@ 2014-11-21 18:05 ` Jiri Pirko
2014-11-21 18:41 ` Pravin Shelar
0 siblings, 1 reply; 24+ messages in thread
From: Jiri Pirko @ 2014-11-21 18:05 UTC (permalink / raw)
To: Pravin Shelar
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
Fri, Nov 21, 2014 at 05:11:39PM CET, pshelar@nicira.com wrote:
>On Wed, Nov 19, 2014 at 5:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> So it can be used from out of openvswitch code.
>> Did couple of cosmetic changes on the way, namely variable naming and
>> adding support for 8021AD proto.
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> ---
>>
>> v3->v4:
>> - don't try to be smart and copy skb->mac_len manipulation as it is. It can be
>> adjusted later on.
>
>I am fine with moving this code to generic library, as long as we can
>adjust the mac_len as offset for MPLS header, otherwise this breaks
>MPLS support.
Pravin, I'm just moving the code without changing it in this aspect.
Maybe I'm missing something but can you tell me what you see wrong?
Thanks.
>
>> - fix error path in do_execute_actions as suggested by Pravin
>> v2->v3:
>> - used previously introduced __vlan_insert_tag helper
>> - used skb_push/pull to get skb->data into needed point
>> - fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin
>> v1->v2:
>> - adjusted to fix recent ovs changes
>> - included change to use make_writable suggested by Eric
>>
>> include/linux/skbuff.h | 2 +
>> net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
>> net/openvswitch/actions.c | 86 +++++-------------------------------------
>> 3 files changed, 106 insertions(+), 77 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index e045516..78c299f 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>> struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>> int skb_ensure_writable(struct sk_buff *skb, int write_len);
>> +int skb_vlan_pop(struct sk_buff *skb);
>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>>
>> struct skb_checksum_ops {
>> __wsum (*update)(const void *mem, int len, __wsum wsum);
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index d11bbe0..c906c5f 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len)
>> }
>> EXPORT_SYMBOL(skb_ensure_writable);
>>
>> +/* remove VLAN header from packet and update csum accordingly. */
>> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
>> +{
>> + struct vlan_hdr *vhdr;
>> + unsigned int offset = skb->data - skb_mac_header(skb);
>> + int err;
>> +
>> + __skb_push(skb, offset);
>> + err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>> + if (unlikely(err))
>> + goto pull;
>> +
>> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>> +
>> + vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>> + *vlan_tci = ntohs(vhdr->h_vlan_TCI);
>> +
>> + memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>> + __skb_pull(skb, VLAN_HLEN);
>> +
>> + vlan_set_encap_proto(skb, vhdr);
>> + skb->mac_header += VLAN_HLEN;
>> +
>> + if (skb_network_offset(skb) < ETH_HLEN)
>> + skb_set_network_header(skb, ETH_HLEN);
>> +
>> + skb_reset_mac_len(skb);
>> +pull:
>> + __skb_pull(skb, offset);
>> +
>> + return err;
>> +}
>> +
>> +int skb_vlan_pop(struct sk_buff *skb)
>> +{
>> + u16 vlan_tci;
>> + __be16 vlan_proto;
>> + int err;
>> +
>> + if (likely(vlan_tx_tag_present(skb))) {
>> + skb->vlan_tci = 0;
>> + } else {
>> + if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
>> + skb->protocol != htons(ETH_P_8021AD)) ||
>> + skb->len < VLAN_ETH_HLEN))
>> + return 0;
>> +
>> + err = __skb_vlan_pop(skb, &vlan_tci);
>> + if (err)
>> + return err;
>> + }
>> + /* move next vlan tag to hw accel tag */
>> + if (likely((skb->protocol != htons(ETH_P_8021Q) &&
>> + skb->protocol != htons(ETH_P_8021AD)) ||
>> + skb->len < VLAN_ETH_HLEN))
>> + return 0;
>> +
>> + vlan_proto = skb->protocol;
>> + err = __skb_vlan_pop(skb, &vlan_tci);
>> + if (unlikely(err))
>> + return err;
>> +
>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(skb_vlan_pop);
>> +
>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
>> +{
>> + if (vlan_tx_tag_present(skb)) {
>> + unsigned int offset = skb->data - skb_mac_header(skb);
>> + int err;
>> +
>> + /* __vlan_insert_tag expect skb->data pointing to mac header.
>> + * So change skb->data before calling it and change back to
>> + * original position later
>> + */
>> + __skb_push(skb, offset);
>> + err = __vlan_insert_tag(skb, skb->vlan_proto,
>> + vlan_tx_tag_get(skb));
>> + if (err)
>> + return err;
>> + skb->protocol = skb->vlan_proto;
>> + skb->mac_len += VLAN_HLEN;
>> + __skb_pull(skb, offset);
>> +
>> + if (skb->ip_summed == CHECKSUM_COMPLETE)
>> + skb->csum = csum_add(skb->csum, csum_partial(skb->data
>> + + (2 * ETH_ALEN), VLAN_HLEN, 0));
>> + }
>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(skb_vlan_push);
>> +
>> /**
>> * alloc_skb_with_frags - allocate skb with page frags
>> *
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index 7ffa377..4e05ea1 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
>> return 0;
>> }
>>
>> -/* remove VLAN header from packet and update csum accordingly. */
>> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
>> -{
>> - struct vlan_hdr *vhdr;
>> - int err;
>> -
>> - err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>> - if (unlikely(err))
>> - return err;
>> -
>> - skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>> -
>> - vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>> - *current_tci = vhdr->h_vlan_TCI;
>> -
>> - memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>> - __skb_pull(skb, VLAN_HLEN);
>> -
>> - vlan_set_encap_proto(skb, vhdr);
>> - skb->mac_header += VLAN_HLEN;
>> -
>> - if (skb_network_offset(skb) < ETH_HLEN)
>> - skb_set_network_header(skb, ETH_HLEN);
>> -
>> - /* Update mac_len for subsequent MPLS actions */
>> - skb_reset_mac_len(skb);
>> - return 0;
>> -}
>> -
>> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>> {
>> - __be16 tci;
>> int err;
>>
>> - if (likely(vlan_tx_tag_present(skb))) {
>> - skb->vlan_tci = 0;
>> - } else {
>> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
>> - skb->len < VLAN_ETH_HLEN))
>> - return 0;
>> -
>> - err = __pop_vlan_tci(skb, &tci);
>> - if (err)
>> - return err;
>> - }
>> - /* move next vlan tag to hw accel tag */
>> - if (likely(skb->protocol != htons(ETH_P_8021Q) ||
>> - skb->len < VLAN_ETH_HLEN)) {
>> + err = skb_vlan_pop(skb);
>> + if (vlan_tx_tag_present(skb))
>> + invalidate_flow_key(key);
>> + else
>> key->eth.tci = 0;
>> - return 0;
>> - }
>> -
>> - invalidate_flow_key(key);
>> - err = __pop_vlan_tci(skb, &tci);
>> - if (unlikely(err))
>> - return err;
>> -
>> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
>> - return 0;
>> + return err;
>> }
>>
>> static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
>> const struct ovs_action_push_vlan *vlan)
>> {
>> - if (unlikely(vlan_tx_tag_present(skb))) {
>> - u16 current_tag;
>> -
>> - /* push down current VLAN tag */
>> - current_tag = vlan_tx_tag_get(skb);
>> -
>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>> - current_tag);
>> - if (!skb)
>> - return -ENOMEM;
>> - /* Update mac_len for subsequent MPLS actions */
>> - skb->mac_len += VLAN_HLEN;
>> -
>> - if (skb->ip_summed == CHECKSUM_COMPLETE)
>> - skb->csum = csum_add(skb->csum, csum_partial(skb->data
>> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
>> -
>> + if (vlan_tx_tag_present(skb))
>> invalidate_flow_key(key);
>> - } else {
>> + else
>> key->eth.tci = vlan->vlan_tci;
>> - }
>> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>> - return 0;
>> + return skb_vlan_push(skb, vlan->vlan_tpid,
>> + ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>> }
>>
>> static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
>> @@ -858,8 +792,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>
>> case OVS_ACTION_ATTR_PUSH_VLAN:
>> err = push_vlan(skb, key, nla_data(a));
>> - if (unlikely(err)) /* skb already freed. */
>> - return err;
>> break;
>>
>> case OVS_ACTION_ATTR_POP_VLAN:
>> --
>> 1.9.3
>>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code
2014-11-21 18:05 ` Jiri Pirko
@ 2014-11-21 18:41 ` Pravin Shelar
2014-11-21 19:08 ` Jiri Pirko
2014-11-21 19:29 ` Jiri Benc
0 siblings, 2 replies; 24+ messages in thread
From: Pravin Shelar @ 2014-11-21 18:41 UTC (permalink / raw)
To: Jiri Pirko, Jiri Benc
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
On Fri, Nov 21, 2014 at 10:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Nov 21, 2014 at 05:11:39PM CET, pshelar@nicira.com wrote:
>>On Wed, Nov 19, 2014 at 5:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>> So it can be used from out of openvswitch code.
>>> Did couple of cosmetic changes on the way, namely variable naming and
>>> adding support for 8021AD proto.
>>>
>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>> ---
>>>
>>> v3->v4:
>>> - don't try to be smart and copy skb->mac_len manipulation as it is. It can be
>>> adjusted later on.
>>
>>I am fine with moving this code to generic library, as long as we can
>>adjust the mac_len as offset for MPLS header, otherwise this breaks
>>MPLS support.
>
> Pravin, I'm just moving the code without changing it in this aspect.
> Maybe I'm missing something but can you tell me what you see wrong?
> Thanks.
>
There is bug in the openvswitch code where vlan-pop does mac-length
reset rather than subtracting VLAN_HLEN from mac-len. MPLS code
depends on this. Now this patch moves the bug to common code. I am
fine with this patch and I can send fix to change code vlan-pop code
to adjust mac_len later on.
But I am worried about the comment made by Jiri Benc on earlier
version of this patch where is mentioned that subtracting VLAN_HLEN
can break common case for vlan-pop. In that case we can not change
this common function. Jiri Benc, Can you point me to the code, so the
we can work on solution to fix this issue.
>>
>>> - fix error path in do_execute_actions as suggested by Pravin
>>> v2->v3:
>>> - used previously introduced __vlan_insert_tag helper
>>> - used skb_push/pull to get skb->data into needed point
>>> - fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin
>>> v1->v2:
>>> - adjusted to fix recent ovs changes
>>> - included change to use make_writable suggested by Eric
>>>
>>> include/linux/skbuff.h | 2 +
>>> net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
>>> net/openvswitch/actions.c | 86 +++++-------------------------------------
>>> 3 files changed, 106 insertions(+), 77 deletions(-)
>>>
>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>> index e045516..78c299f 100644
>>> --- a/include/linux/skbuff.h
>>> +++ b/include/linux/skbuff.h
>>> @@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>>> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>>> struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>>> int skb_ensure_writable(struct sk_buff *skb, int write_len);
>>> +int skb_vlan_pop(struct sk_buff *skb);
>>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>>>
>>> struct skb_checksum_ops {
>>> __wsum (*update)(const void *mem, int len, __wsum wsum);
>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>> index d11bbe0..c906c5f 100644
>>> --- a/net/core/skbuff.c
>>> +++ b/net/core/skbuff.c
>>> @@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len)
>>> }
>>> EXPORT_SYMBOL(skb_ensure_writable);
>>>
>>> +/* remove VLAN header from packet and update csum accordingly. */
>>> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
>>> +{
>>> + struct vlan_hdr *vhdr;
>>> + unsigned int offset = skb->data - skb_mac_header(skb);
>>> + int err;
>>> +
>>> + __skb_push(skb, offset);
>>> + err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>>> + if (unlikely(err))
>>> + goto pull;
>>> +
>>> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>>> +
>>> + vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>>> + *vlan_tci = ntohs(vhdr->h_vlan_TCI);
>>> +
>>> + memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>>> + __skb_pull(skb, VLAN_HLEN);
>>> +
>>> + vlan_set_encap_proto(skb, vhdr);
>>> + skb->mac_header += VLAN_HLEN;
>>> +
>>> + if (skb_network_offset(skb) < ETH_HLEN)
>>> + skb_set_network_header(skb, ETH_HLEN);
>>> +
>>> + skb_reset_mac_len(skb);
>>> +pull:
>>> + __skb_pull(skb, offset);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +int skb_vlan_pop(struct sk_buff *skb)
>>> +{
>>> + u16 vlan_tci;
>>> + __be16 vlan_proto;
>>> + int err;
>>> +
>>> + if (likely(vlan_tx_tag_present(skb))) {
>>> + skb->vlan_tci = 0;
>>> + } else {
>>> + if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
>>> + skb->protocol != htons(ETH_P_8021AD)) ||
>>> + skb->len < VLAN_ETH_HLEN))
>>> + return 0;
>>> +
>>> + err = __skb_vlan_pop(skb, &vlan_tci);
>>> + if (err)
>>> + return err;
>>> + }
>>> + /* move next vlan tag to hw accel tag */
>>> + if (likely((skb->protocol != htons(ETH_P_8021Q) &&
>>> + skb->protocol != htons(ETH_P_8021AD)) ||
>>> + skb->len < VLAN_ETH_HLEN))
>>> + return 0;
>>> +
>>> + vlan_proto = skb->protocol;
>>> + err = __skb_vlan_pop(skb, &vlan_tci);
>>> + if (unlikely(err))
>>> + return err;
>>> +
>>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(skb_vlan_pop);
>>> +
>>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
>>> +{
>>> + if (vlan_tx_tag_present(skb)) {
>>> + unsigned int offset = skb->data - skb_mac_header(skb);
>>> + int err;
>>> +
>>> + /* __vlan_insert_tag expect skb->data pointing to mac header.
>>> + * So change skb->data before calling it and change back to
>>> + * original position later
>>> + */
>>> + __skb_push(skb, offset);
>>> + err = __vlan_insert_tag(skb, skb->vlan_proto,
>>> + vlan_tx_tag_get(skb));
>>> + if (err)
>>> + return err;
>>> + skb->protocol = skb->vlan_proto;
>>> + skb->mac_len += VLAN_HLEN;
>>> + __skb_pull(skb, offset);
>>> +
>>> + if (skb->ip_summed == CHECKSUM_COMPLETE)
>>> + skb->csum = csum_add(skb->csum, csum_partial(skb->data
>>> + + (2 * ETH_ALEN), VLAN_HLEN, 0));
>>> + }
>>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(skb_vlan_push);
>>> +
>>> /**
>>> * alloc_skb_with_frags - allocate skb with page frags
>>> *
>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>> index 7ffa377..4e05ea1 100644
>>> --- a/net/openvswitch/actions.c
>>> +++ b/net/openvswitch/actions.c
>>> @@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
>>> return 0;
>>> }
>>>
>>> -/* remove VLAN header from packet and update csum accordingly. */
>>> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
>>> -{
>>> - struct vlan_hdr *vhdr;
>>> - int err;
>>> -
>>> - err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>>> - if (unlikely(err))
>>> - return err;
>>> -
>>> - skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>>> -
>>> - vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>>> - *current_tci = vhdr->h_vlan_TCI;
>>> -
>>> - memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>>> - __skb_pull(skb, VLAN_HLEN);
>>> -
>>> - vlan_set_encap_proto(skb, vhdr);
>>> - skb->mac_header += VLAN_HLEN;
>>> -
>>> - if (skb_network_offset(skb) < ETH_HLEN)
>>> - skb_set_network_header(skb, ETH_HLEN);
>>> -
>>> - /* Update mac_len for subsequent MPLS actions */
>>> - skb_reset_mac_len(skb);
>>> - return 0;
>>> -}
>>> -
>>> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>> {
>>> - __be16 tci;
>>> int err;
>>>
>>> - if (likely(vlan_tx_tag_present(skb))) {
>>> - skb->vlan_tci = 0;
>>> - } else {
>>> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
>>> - skb->len < VLAN_ETH_HLEN))
>>> - return 0;
>>> -
>>> - err = __pop_vlan_tci(skb, &tci);
>>> - if (err)
>>> - return err;
>>> - }
>>> - /* move next vlan tag to hw accel tag */
>>> - if (likely(skb->protocol != htons(ETH_P_8021Q) ||
>>> - skb->len < VLAN_ETH_HLEN)) {
>>> + err = skb_vlan_pop(skb);
>>> + if (vlan_tx_tag_present(skb))
>>> + invalidate_flow_key(key);
>>> + else
>>> key->eth.tci = 0;
>>> - return 0;
>>> - }
>>> -
>>> - invalidate_flow_key(key);
>>> - err = __pop_vlan_tci(skb, &tci);
>>> - if (unlikely(err))
>>> - return err;
>>> -
>>> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
>>> - return 0;
>>> + return err;
>>> }
>>>
>>> static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
>>> const struct ovs_action_push_vlan *vlan)
>>> {
>>> - if (unlikely(vlan_tx_tag_present(skb))) {
>>> - u16 current_tag;
>>> -
>>> - /* push down current VLAN tag */
>>> - current_tag = vlan_tx_tag_get(skb);
>>> -
>>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>>> - current_tag);
>>> - if (!skb)
>>> - return -ENOMEM;
>>> - /* Update mac_len for subsequent MPLS actions */
>>> - skb->mac_len += VLAN_HLEN;
>>> -
>>> - if (skb->ip_summed == CHECKSUM_COMPLETE)
>>> - skb->csum = csum_add(skb->csum, csum_partial(skb->data
>>> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
>>> -
>>> + if (vlan_tx_tag_present(skb))
>>> invalidate_flow_key(key);
>>> - } else {
>>> + else
>>> key->eth.tci = vlan->vlan_tci;
>>> - }
>>> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>>> - return 0;
>>> + return skb_vlan_push(skb, vlan->vlan_tpid,
>>> + ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>>> }
>>>
>>> static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
>>> @@ -858,8 +792,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>>
>>> case OVS_ACTION_ATTR_PUSH_VLAN:
>>> err = push_vlan(skb, key, nla_data(a));
>>> - if (unlikely(err)) /* skb already freed. */
>>> - return err;
>>> break;
>>>
>>> case OVS_ACTION_ATTR_POP_VLAN:
>>> --
>>> 1.9.3
>>>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code
2014-11-21 18:41 ` Pravin Shelar
@ 2014-11-21 19:08 ` Jiri Pirko
2014-11-21 19:29 ` Jiri Benc
1 sibling, 0 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-21 19:08 UTC (permalink / raw)
To: Pravin Shelar
Cc: Jiri Benc, netdev, David Miller, Jamal Hadi Salim, Tom Herbert,
Eric Dumazet, Willem de Bruijn, Daniel Borkmann, mst, fw,
Paul.Durrant, Thomas Graf, Cong Wang
Fri, Nov 21, 2014 at 07:41:27PM CET, pshelar@nicira.com wrote:
>On Fri, Nov 21, 2014 at 10:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Fri, Nov 21, 2014 at 05:11:39PM CET, pshelar@nicira.com wrote:
>>>On Wed, Nov 19, 2014 at 5:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>>>> So it can be used from out of openvswitch code.
>>>> Did couple of cosmetic changes on the way, namely variable naming and
>>>> adding support for 8021AD proto.
>>>>
>>>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>>> ---
>>>>
>>>> v3->v4:
>>>> - don't try to be smart and copy skb->mac_len manipulation as it is. It can be
>>>> adjusted later on.
>>>
>>>I am fine with moving this code to generic library, as long as we can
>>>adjust the mac_len as offset for MPLS header, otherwise this breaks
>>>MPLS support.
>>
>> Pravin, I'm just moving the code without changing it in this aspect.
>> Maybe I'm missing something but can you tell me what you see wrong?
>> Thanks.
>>
>
>There is bug in the openvswitch code where vlan-pop does mac-length
>reset rather than subtracting VLAN_HLEN from mac-len. MPLS code
>depends on this. Now this patch moves the bug to common code. I am
>fine with this patch and I can send fix to change code vlan-pop code
>to adjust mac_len later on.
>But I am worried about the comment made by Jiri Benc on earlier
>version of this patch where is mentioned that subtracting VLAN_HLEN
>can break common case for vlan-pop. In that case we can not change
>this common function. Jiri Benc, Can you point me to the code, so the
>we can work on solution to fix this issue.
Lets move to common code and fix this there with follow-up patches.
This common code should work for all users.
>
>>>
>>>> - fix error path in do_execute_actions as suggested by Pravin
>>>> v2->v3:
>>>> - used previously introduced __vlan_insert_tag helper
>>>> - used skb_push/pull to get skb->data into needed point
>>>> - fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin
>>>> v1->v2:
>>>> - adjusted to fix recent ovs changes
>>>> - included change to use make_writable suggested by Eric
>>>>
>>>> include/linux/skbuff.h | 2 +
>>>> net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
>>>> net/openvswitch/actions.c | 86 +++++-------------------------------------
>>>> 3 files changed, 106 insertions(+), 77 deletions(-)
>>>>
>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>> index e045516..78c299f 100644
>>>> --- a/include/linux/skbuff.h
>>>> +++ b/include/linux/skbuff.h
>>>> @@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
>>>> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
>>>> struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
>>>> int skb_ensure_writable(struct sk_buff *skb, int write_len);
>>>> +int skb_vlan_pop(struct sk_buff *skb);
>>>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>>>>
>>>> struct skb_checksum_ops {
>>>> __wsum (*update)(const void *mem, int len, __wsum wsum);
>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>> index d11bbe0..c906c5f 100644
>>>> --- a/net/core/skbuff.c
>>>> +++ b/net/core/skbuff.c
>>>> @@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len)
>>>> }
>>>> EXPORT_SYMBOL(skb_ensure_writable);
>>>>
>>>> +/* remove VLAN header from packet and update csum accordingly. */
>>>> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
>>>> +{
>>>> + struct vlan_hdr *vhdr;
>>>> + unsigned int offset = skb->data - skb_mac_header(skb);
>>>> + int err;
>>>> +
>>>> + __skb_push(skb, offset);
>>>> + err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>>>> + if (unlikely(err))
>>>> + goto pull;
>>>> +
>>>> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>>>> +
>>>> + vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>>>> + *vlan_tci = ntohs(vhdr->h_vlan_TCI);
>>>> +
>>>> + memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>>>> + __skb_pull(skb, VLAN_HLEN);
>>>> +
>>>> + vlan_set_encap_proto(skb, vhdr);
>>>> + skb->mac_header += VLAN_HLEN;
>>>> +
>>>> + if (skb_network_offset(skb) < ETH_HLEN)
>>>> + skb_set_network_header(skb, ETH_HLEN);
>>>> +
>>>> + skb_reset_mac_len(skb);
>>>> +pull:
>>>> + __skb_pull(skb, offset);
>>>> +
>>>> + return err;
>>>> +}
>>>> +
>>>> +int skb_vlan_pop(struct sk_buff *skb)
>>>> +{
>>>> + u16 vlan_tci;
>>>> + __be16 vlan_proto;
>>>> + int err;
>>>> +
>>>> + if (likely(vlan_tx_tag_present(skb))) {
>>>> + skb->vlan_tci = 0;
>>>> + } else {
>>>> + if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
>>>> + skb->protocol != htons(ETH_P_8021AD)) ||
>>>> + skb->len < VLAN_ETH_HLEN))
>>>> + return 0;
>>>> +
>>>> + err = __skb_vlan_pop(skb, &vlan_tci);
>>>> + if (err)
>>>> + return err;
>>>> + }
>>>> + /* move next vlan tag to hw accel tag */
>>>> + if (likely((skb->protocol != htons(ETH_P_8021Q) &&
>>>> + skb->protocol != htons(ETH_P_8021AD)) ||
>>>> + skb->len < VLAN_ETH_HLEN))
>>>> + return 0;
>>>> +
>>>> + vlan_proto = skb->protocol;
>>>> + err = __skb_vlan_pop(skb, &vlan_tci);
>>>> + if (unlikely(err))
>>>> + return err;
>>>> +
>>>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(skb_vlan_pop);
>>>> +
>>>> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
>>>> +{
>>>> + if (vlan_tx_tag_present(skb)) {
>>>> + unsigned int offset = skb->data - skb_mac_header(skb);
>>>> + int err;
>>>> +
>>>> + /* __vlan_insert_tag expect skb->data pointing to mac header.
>>>> + * So change skb->data before calling it and change back to
>>>> + * original position later
>>>> + */
>>>> + __skb_push(skb, offset);
>>>> + err = __vlan_insert_tag(skb, skb->vlan_proto,
>>>> + vlan_tx_tag_get(skb));
>>>> + if (err)
>>>> + return err;
>>>> + skb->protocol = skb->vlan_proto;
>>>> + skb->mac_len += VLAN_HLEN;
>>>> + __skb_pull(skb, offset);
>>>> +
>>>> + if (skb->ip_summed == CHECKSUM_COMPLETE)
>>>> + skb->csum = csum_add(skb->csum, csum_partial(skb->data
>>>> + + (2 * ETH_ALEN), VLAN_HLEN, 0));
>>>> + }
>>>> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
>>>> + return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(skb_vlan_push);
>>>> +
>>>> /**
>>>> * alloc_skb_with_frags - allocate skb with page frags
>>>> *
>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>>> index 7ffa377..4e05ea1 100644
>>>> --- a/net/openvswitch/actions.c
>>>> +++ b/net/openvswitch/actions.c
>>>> @@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
>>>> return 0;
>>>> }
>>>>
>>>> -/* remove VLAN header from packet and update csum accordingly. */
>>>> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
>>>> -{
>>>> - struct vlan_hdr *vhdr;
>>>> - int err;
>>>> -
>>>> - err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
>>>> - if (unlikely(err))
>>>> - return err;
>>>> -
>>>> - skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
>>>> -
>>>> - vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
>>>> - *current_tci = vhdr->h_vlan_TCI;
>>>> -
>>>> - memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
>>>> - __skb_pull(skb, VLAN_HLEN);
>>>> -
>>>> - vlan_set_encap_proto(skb, vhdr);
>>>> - skb->mac_header += VLAN_HLEN;
>>>> -
>>>> - if (skb_network_offset(skb) < ETH_HLEN)
>>>> - skb_set_network_header(skb, ETH_HLEN);
>>>> -
>>>> - /* Update mac_len for subsequent MPLS actions */
>>>> - skb_reset_mac_len(skb);
>>>> - return 0;
>>>> -}
>>>> -
>>>> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>>> {
>>>> - __be16 tci;
>>>> int err;
>>>>
>>>> - if (likely(vlan_tx_tag_present(skb))) {
>>>> - skb->vlan_tci = 0;
>>>> - } else {
>>>> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
>>>> - skb->len < VLAN_ETH_HLEN))
>>>> - return 0;
>>>> -
>>>> - err = __pop_vlan_tci(skb, &tci);
>>>> - if (err)
>>>> - return err;
>>>> - }
>>>> - /* move next vlan tag to hw accel tag */
>>>> - if (likely(skb->protocol != htons(ETH_P_8021Q) ||
>>>> - skb->len < VLAN_ETH_HLEN)) {
>>>> + err = skb_vlan_pop(skb);
>>>> + if (vlan_tx_tag_present(skb))
>>>> + invalidate_flow_key(key);
>>>> + else
>>>> key->eth.tci = 0;
>>>> - return 0;
>>>> - }
>>>> -
>>>> - invalidate_flow_key(key);
>>>> - err = __pop_vlan_tci(skb, &tci);
>>>> - if (unlikely(err))
>>>> - return err;
>>>> -
>>>> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
>>>> - return 0;
>>>> + return err;
>>>> }
>>>>
>>>> static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
>>>> const struct ovs_action_push_vlan *vlan)
>>>> {
>>>> - if (unlikely(vlan_tx_tag_present(skb))) {
>>>> - u16 current_tag;
>>>> -
>>>> - /* push down current VLAN tag */
>>>> - current_tag = vlan_tx_tag_get(skb);
>>>> -
>>>> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>>>> - current_tag);
>>>> - if (!skb)
>>>> - return -ENOMEM;
>>>> - /* Update mac_len for subsequent MPLS actions */
>>>> - skb->mac_len += VLAN_HLEN;
>>>> -
>>>> - if (skb->ip_summed == CHECKSUM_COMPLETE)
>>>> - skb->csum = csum_add(skb->csum, csum_partial(skb->data
>>>> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
>>>> -
>>>> + if (vlan_tx_tag_present(skb))
>>>> invalidate_flow_key(key);
>>>> - } else {
>>>> + else
>>>> key->eth.tci = vlan->vlan_tci;
>>>> - }
>>>> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>>>> - return 0;
>>>> + return skb_vlan_push(skb, vlan->vlan_tpid,
>>>> + ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
>>>> }
>>>>
>>>> static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
>>>> @@ -858,8 +792,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>>>>
>>>> case OVS_ACTION_ATTR_PUSH_VLAN:
>>>> err = push_vlan(skb, key, nla_data(a));
>>>> - if (unlikely(err)) /* skb already freed. */
>>>> - return err;
>>>> break;
>>>>
>>>> case OVS_ACTION_ATTR_POP_VLAN:
>>>> --
>>>> 1.9.3
>>>>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code
2014-11-21 18:41 ` Pravin Shelar
2014-11-21 19:08 ` Jiri Pirko
@ 2014-11-21 19:29 ` Jiri Benc
2014-12-04 0:33 ` Pravin Shelar
1 sibling, 1 reply; 24+ messages in thread
From: Jiri Benc @ 2014-11-21 19:29 UTC (permalink / raw)
To: Pravin Shelar
Cc: Jiri Pirko, netdev, David Miller, Jamal Hadi Salim, Tom Herbert,
Eric Dumazet, Willem de Bruijn, Daniel Borkmann, mst, fw,
Paul.Durrant, Thomas Graf, Cong Wang
On Fri, 21 Nov 2014 10:41:27 -0800, Pravin Shelar wrote:
> There is bug in the openvswitch code where vlan-pop does mac-length
> reset rather than subtracting VLAN_HLEN from mac-len. MPLS code
> depends on this. Now this patch moves the bug to common code. I am
> fine with this patch and I can send fix to change code vlan-pop code
> to adjust mac_len later on.
> But I am worried about the comment made by Jiri Benc on earlier
> version of this patch where is mentioned that subtracting VLAN_HLEN
> can break common case for vlan-pop. In that case we can not change
> this common function. Jiri Benc, Can you point me to the code, so the
> we can work on solution to fix this issue.
In such case, a skb that enters the __pop_vlan_tci function looks like
this:
+--------+--------+-------------+-----+---------+-------
| dstmac | srcmac | ETH_P_8021Q | tci | ETH_P_x | data
+--------+--------+-------------+-----+---------+-------
^ ^
mac_header network_header
skb->protocol = ETH_P_8021Q
skb->mac_len = 14
After the function finishes, it's supposed to look like this:
+--------+--------+---------+-------
| dstmac | srcmac | ETH_P_x | data
+--------+--------+---------+-------
^ ^
mac_header network_header
skb->protocol = ETH_P_x
skb->mac_len = 14
Blindly decrementing mac_len wouldn't work here, you'd end up with
mac_len = 10. You'd either need to ensure this case doesn't happen (it
does with openvswitch currently) or detect this case.
Jiri
--
Jiri Benc
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code
2014-11-21 19:29 ` Jiri Benc
@ 2014-12-04 0:33 ` Pravin Shelar
2014-12-04 16:57 ` Jiri Benc
0 siblings, 1 reply; 24+ messages in thread
From: Pravin Shelar @ 2014-12-04 0:33 UTC (permalink / raw)
To: Jiri Benc
Cc: Jiri Pirko, netdev, David Miller, Jamal Hadi Salim, Tom Herbert,
Eric Dumazet, Willem de Bruijn, Daniel Borkmann, mst, fw,
Paul.Durrant, Thomas Graf, Cong Wang
On Fri, Nov 21, 2014 at 11:29 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Fri, 21 Nov 2014 10:41:27 -0800, Pravin Shelar wrote:
>> There is bug in the openvswitch code where vlan-pop does mac-length
>> reset rather than subtracting VLAN_HLEN from mac-len. MPLS code
>> depends on this. Now this patch moves the bug to common code. I am
>> fine with this patch and I can send fix to change code vlan-pop code
>> to adjust mac_len later on.
>> But I am worried about the comment made by Jiri Benc on earlier
>> version of this patch where is mentioned that subtracting VLAN_HLEN
>> can break common case for vlan-pop. In that case we can not change
>> this common function. Jiri Benc, Can you point me to the code, so the
>> we can work on solution to fix this issue.
>
> In such case, a skb that enters the __pop_vlan_tci function looks like
> this:
>
> +--------+--------+-------------+-----+---------+-------
> | dstmac | srcmac | ETH_P_8021Q | tci | ETH_P_x | data
> +--------+--------+-------------+-----+---------+-------
> ^ ^
> mac_header network_header
>
> skb->protocol = ETH_P_8021Q
> skb->mac_len = 14
>
> After the function finishes, it's supposed to look like this:
>
> +--------+--------+---------+-------
> | dstmac | srcmac | ETH_P_x | data
> +--------+--------+---------+-------
> ^ ^
> mac_header network_header
>
> skb->protocol = ETH_P_x
> skb->mac_len = 14
>
> Blindly decrementing mac_len wouldn't work here, you'd end up with
> mac_len = 10. You'd either need to ensure this case doesn't happen (it
> does with openvswitch currently) or detect this case.
>
OVS correctly sets mac header length in case of vlan header. Can you
give me OVS test case to reproduce this issue?
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch net-next v4 8/9] net: move vlan pop/push functions into common code
2014-12-04 0:33 ` Pravin Shelar
@ 2014-12-04 16:57 ` Jiri Benc
0 siblings, 0 replies; 24+ messages in thread
From: Jiri Benc @ 2014-12-04 16:57 UTC (permalink / raw)
To: Pravin Shelar
Cc: Jiri Pirko, netdev, David Miller, Jamal Hadi Salim, Tom Herbert,
Eric Dumazet, Willem de Bruijn, Daniel Borkmann, mst, fw,
Paul.Durrant, Thomas Graf, Cong Wang
On Wed, 3 Dec 2014 16:33:57 -0800, Pravin Shelar wrote:
> OVS correctly sets mac header length in case of vlan header. Can you
> give me OVS test case to reproduce this issue?
Set up ovs bridge with two ports, one of them tagged. Receive a packet
with two vlan headers (the first vlan tag corresponding to the second
port) on the untagged port.
Tried it just now with the latest net-next with printks added to
__skb_vlan_pop, __skb_vlan_pop is called twice for each packet, the
second invocation has the pointers set in the way I described.
Jiri
--
Jiri Benc
^ permalink raw reply [flat|nested] 24+ messages in thread
* [patch net-next v4 9/9] sched: introduce vlan action
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
` (7 preceding siblings ...)
2014-11-19 13:05 ` [patch net-next v4 8/9] net: move vlan pop/push functions " Jiri Pirko
@ 2014-11-19 13:05 ` Jiri Pirko
2014-11-19 13:08 ` [patch iproute2 v4] tc: add support for vlan tc action Jiri Pirko
2014-11-21 19:21 ` [patch net-next v4 0/9] sched: introduce vlan action David Miller
10 siblings, 0 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:05 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
This tc action allows to work with vlan tagged skbs. Two supported
sub-actions are header pop and header push.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
v1->v2:
- fixed protocol dumping in tcf_vlan_dump
include/net/tc_act/tc_vlan.h | 27 +++++
include/uapi/linux/tc_act/tc_vlan.h | 35 ++++++
net/sched/Kconfig | 11 ++
net/sched/Makefile | 1 +
net/sched/act_vlan.c | 207 ++++++++++++++++++++++++++++++++++++
5 files changed, 281 insertions(+)
create mode 100644 include/net/tc_act/tc_vlan.h
create mode 100644 include/uapi/linux/tc_act/tc_vlan.h
create mode 100644 net/sched/act_vlan.c
diff --git a/include/net/tc_act/tc_vlan.h b/include/net/tc_act/tc_vlan.h
new file mode 100644
index 0000000..c809c1d
--- /dev/null
+++ b/include/net/tc_act/tc_vlan.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2014 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __NET_TC_VLAN_H
+#define __NET_TC_VLAN_H
+
+#include <net/act_api.h>
+
+#define VLAN_F_POP 0x1
+#define VLAN_F_PUSH 0x2
+
+struct tcf_vlan {
+ struct tcf_common common;
+ int tcfv_action;
+ __be16 tcfv_push_vid;
+ __be16 tcfv_push_proto;
+};
+#define to_vlan(a) \
+ container_of(a->priv, struct tcf_vlan, common)
+
+#endif /* __NET_TC_VLAN_H */
diff --git a/include/uapi/linux/tc_act/tc_vlan.h b/include/uapi/linux/tc_act/tc_vlan.h
new file mode 100644
index 0000000..f7b8d44
--- /dev/null
+++ b/include/uapi/linux/tc_act/tc_vlan.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2014 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TC_VLAN_H
+#define __LINUX_TC_VLAN_H
+
+#include <linux/pkt_cls.h>
+
+#define TCA_ACT_VLAN 12
+
+#define TCA_VLAN_ACT_POP 1
+#define TCA_VLAN_ACT_PUSH 2
+
+struct tc_vlan {
+ tc_gen;
+ int v_action;
+};
+
+enum {
+ TCA_VLAN_UNSPEC,
+ TCA_VLAN_TM,
+ TCA_VLAN_PARMS,
+ TCA_VLAN_PUSH_VLAN_ID,
+ TCA_VLAN_PUSH_VLAN_PROTOCOL,
+ __TCA_VLAN_MAX,
+};
+#define TCA_VLAN_MAX (__TCA_VLAN_MAX - 1)
+
+#endif
diff --git a/net/sched/Kconfig b/net/sched/Kconfig
index a1a8e29..88618f8 100644
--- a/net/sched/Kconfig
+++ b/net/sched/Kconfig
@@ -686,6 +686,17 @@ config NET_ACT_CSUM
To compile this code as a module, choose M here: the
module will be called act_csum.
+config NET_ACT_VLAN
+ tristate "Vlan manipulation"
+ depends on NET_CLS_ACT
+ ---help---
+ Say Y here to push or pop vlan headers.
+
+ If unsure, say N.
+
+ To compile this code as a module, choose M here: the
+ module will be called act_vlan.
+
config NET_CLS_IND
bool "Incoming device classification"
depends on NET_CLS_U32 || NET_CLS_FW
diff --git a/net/sched/Makefile b/net/sched/Makefile
index 0a869a1..679f24a 100644
--- a/net/sched/Makefile
+++ b/net/sched/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_NET_ACT_PEDIT) += act_pedit.o
obj-$(CONFIG_NET_ACT_SIMP) += act_simple.o
obj-$(CONFIG_NET_ACT_SKBEDIT) += act_skbedit.o
obj-$(CONFIG_NET_ACT_CSUM) += act_csum.o
+obj-$(CONFIG_NET_ACT_VLAN) += act_vlan.o
obj-$(CONFIG_NET_SCH_FIFO) += sch_fifo.o
obj-$(CONFIG_NET_SCH_CBQ) += sch_cbq.o
obj-$(CONFIG_NET_SCH_HTB) += sch_htb.o
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
new file mode 100644
index 0000000..d735ecf
--- /dev/null
+++ b/net/sched/act_vlan.c
@@ -0,0 +1,207 @@
+/*
+ * Copyright (c) 2014 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/skbuff.h>
+#include <linux/rtnetlink.h>
+#include <linux/if_vlan.h>
+#include <net/netlink.h>
+#include <net/pkt_sched.h>
+
+#include <linux/tc_act/tc_vlan.h>
+#include <net/tc_act/tc_vlan.h>
+
+#define VLAN_TAB_MASK 15
+
+static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
+ struct tcf_result *res)
+{
+ struct tcf_vlan *v = a->priv;
+ int action;
+ int err;
+
+ spin_lock(&v->tcf_lock);
+ v->tcf_tm.lastuse = jiffies;
+ bstats_update(&v->tcf_bstats, skb);
+ action = v->tcf_action;
+
+ switch (v->tcfv_action) {
+ case TCA_VLAN_ACT_POP:
+ err = skb_vlan_pop(skb);
+ if (err)
+ goto drop;
+ break;
+ case TCA_VLAN_ACT_PUSH:
+ err = skb_vlan_push(skb, v->tcfv_push_proto, v->tcfv_push_vid);
+ if (err)
+ goto drop;
+ break;
+ default:
+ BUG();
+ }
+
+ goto unlock;
+
+drop:
+ action = TC_ACT_SHOT;
+ v->tcf_qstats.drops++;
+unlock:
+ spin_unlock(&v->tcf_lock);
+ return action;
+}
+
+static const struct nla_policy vlan_policy[TCA_VLAN_MAX + 1] = {
+ [TCA_VLAN_PARMS] = { .len = sizeof(struct tc_vlan) },
+ [TCA_VLAN_PUSH_VLAN_ID] = { .type = NLA_U16 },
+ [TCA_VLAN_PUSH_VLAN_PROTOCOL] = { .type = NLA_U16 },
+};
+
+static int tcf_vlan_init(struct net *net, struct nlattr *nla,
+ struct nlattr *est, struct tc_action *a,
+ int ovr, int bind)
+{
+ struct nlattr *tb[TCA_VLAN_MAX + 1];
+ struct tc_vlan *parm;
+ struct tcf_vlan *v;
+ int action;
+ __be16 push_vid = 0;
+ __be16 push_proto = 0;
+ int ret = 0;
+ int err;
+
+ if (!nla)
+ return -EINVAL;
+
+ err = nla_parse_nested(tb, TCA_VLAN_MAX, nla, vlan_policy);
+ if (err < 0)
+ return err;
+
+ if (!tb[TCA_VLAN_PARMS])
+ return -EINVAL;
+ parm = nla_data(tb[TCA_VLAN_PARMS]);
+ switch (parm->v_action) {
+ case TCA_VLAN_ACT_POP:
+ break;
+ case TCA_VLAN_ACT_PUSH:
+ if (!tb[TCA_VLAN_PUSH_VLAN_ID])
+ return -EINVAL;
+ push_vid = nla_get_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
+ if (push_vid >= VLAN_VID_MASK)
+ return -ERANGE;
+
+ if (tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]) {
+ push_proto = nla_get_be16(tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]);
+ switch (push_proto) {
+ case htons(ETH_P_8021Q):
+ case htons(ETH_P_8021AD):
+ break;
+ default:
+ return -EPROTONOSUPPORT;
+ }
+ } else {
+ push_proto = htons(ETH_P_8021Q);
+ }
+ break;
+ default:
+ return -EINVAL;
+ }
+ action = parm->v_action;
+
+ if (!tcf_hash_check(parm->index, a, bind)) {
+ ret = tcf_hash_create(parm->index, est, a, sizeof(*v), bind);
+ if (ret)
+ return ret;
+
+ ret = ACT_P_CREATED;
+ } else {
+ if (bind)
+ return 0;
+ tcf_hash_release(a, bind);
+ if (!ovr)
+ return -EEXIST;
+ }
+
+ v = to_vlan(a);
+
+ spin_lock_bh(&v->tcf_lock);
+
+ v->tcfv_action = action;
+ v->tcfv_push_vid = push_vid;
+ v->tcfv_push_proto = push_proto;
+
+ v->tcf_action = parm->action;
+
+ spin_unlock_bh(&v->tcf_lock);
+
+ if (ret == ACT_P_CREATED)
+ tcf_hash_insert(a);
+ return ret;
+}
+
+static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
+ int bind, int ref)
+{
+ unsigned char *b = skb_tail_pointer(skb);
+ struct tcf_vlan *v = a->priv;
+ struct tc_vlan opt = {
+ .index = v->tcf_index,
+ .refcnt = v->tcf_refcnt - ref,
+ .bindcnt = v->tcf_bindcnt - bind,
+ .action = v->tcf_action,
+ .v_action = v->tcfv_action,
+ };
+ struct tcf_t t;
+
+ if (nla_put(skb, TCA_VLAN_PARMS, sizeof(opt), &opt))
+ goto nla_put_failure;
+
+ if (v->tcfv_action == TCA_VLAN_ACT_PUSH &&
+ (nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_ID, v->tcfv_push_vid) ||
+ nla_put_be16(skb, TCA_VLAN_PUSH_VLAN_PROTOCOL, v->tcfv_push_proto)))
+ goto nla_put_failure;
+
+ t.install = jiffies_to_clock_t(jiffies - v->tcf_tm.install);
+ t.lastuse = jiffies_to_clock_t(jiffies - v->tcf_tm.lastuse);
+ t.expires = jiffies_to_clock_t(v->tcf_tm.expires);
+ if (nla_put(skb, TCA_VLAN_TM, sizeof(t), &t))
+ goto nla_put_failure;
+ return skb->len;
+
+nla_put_failure:
+ nlmsg_trim(skb, b);
+ return -1;
+}
+
+static struct tc_action_ops act_vlan_ops = {
+ .kind = "vlan",
+ .type = TCA_ACT_VLAN,
+ .owner = THIS_MODULE,
+ .act = tcf_vlan,
+ .dump = tcf_vlan_dump,
+ .init = tcf_vlan_init,
+};
+
+static int __init vlan_init_module(void)
+{
+ return tcf_register_action(&act_vlan_ops, VLAN_TAB_MASK);
+}
+
+static void __exit vlan_cleanup_module(void)
+{
+ tcf_unregister_action(&act_vlan_ops);
+}
+
+module_init(vlan_init_module);
+module_exit(vlan_cleanup_module);
+
+MODULE_AUTHOR("Jiri Pirko <jiri@resnulli.us>");
+MODULE_DESCRIPTION("vlan manipulation actions");
+MODULE_LICENSE("GPL v2");
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [patch iproute2 v4] tc: add support for vlan tc action
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
` (8 preceding siblings ...)
2014-11-19 13:05 ` [patch net-next v4 9/9] sched: introduce vlan action Jiri Pirko
@ 2014-11-19 13:08 ` Jiri Pirko
2014-11-21 11:31 ` [patch iproute2 v4.1] " Jiri Pirko
2014-12-03 17:35 ` [patch iproute2 v4] " Stephen Hemminger
2014-11-21 19:21 ` [patch net-next v4 0/9] sched: introduce vlan action David Miller
10 siblings, 2 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-19 13:08 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Reviewed-by: Cong Wang <cwang@twopensource.com>
---
v1->v2:
- included changes suggested by Jamal
include/linux/tc_act/tc_vlan.h | 35 +++++++
tc/Makefile | 1 +
tc/m_vlan.c | 221 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 257 insertions(+)
create mode 100644 include/linux/tc_act/tc_vlan.h
create mode 100644 tc/m_vlan.c
diff --git a/include/linux/tc_act/tc_vlan.h b/include/linux/tc_act/tc_vlan.h
new file mode 100644
index 0000000..f7b8d44
--- /dev/null
+++ b/include/linux/tc_act/tc_vlan.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2014 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TC_VLAN_H
+#define __LINUX_TC_VLAN_H
+
+#include <linux/pkt_cls.h>
+
+#define TCA_ACT_VLAN 12
+
+#define TCA_VLAN_ACT_POP 1
+#define TCA_VLAN_ACT_PUSH 2
+
+struct tc_vlan {
+ tc_gen;
+ int v_action;
+};
+
+enum {
+ TCA_VLAN_UNSPEC,
+ TCA_VLAN_TM,
+ TCA_VLAN_PARMS,
+ TCA_VLAN_PUSH_VLAN_ID,
+ TCA_VLAN_PUSH_VLAN_PROTOCOL,
+ __TCA_VLAN_MAX,
+};
+#define TCA_VLAN_MAX (__TCA_VLAN_MAX - 1)
+
+#endif
diff --git a/tc/Makefile b/tc/Makefile
index 1ab36c6..830c97d 100644
--- a/tc/Makefile
+++ b/tc/Makefile
@@ -40,6 +40,7 @@ TCMODULES += m_pedit.o
TCMODULES += m_skbedit.o
TCMODULES += m_csum.o
TCMODULES += m_simple.o
+TCMODULES += m_vlan.o
TCMODULES += p_ip.o
TCMODULES += p_icmp.o
TCMODULES += p_tcp.o
diff --git a/tc/m_vlan.c b/tc/m_vlan.c
new file mode 100644
index 0000000..872bf72
--- /dev/null
+++ b/tc/m_vlan.c
@@ -0,0 +1,221 @@
+/*
+ * m_vlan.c vlan manipulation module
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * Authors: Jiri Pirko <jiri@resnulli.us>
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <linux/if_ether.h>
+#include "utils.h"
+#include "rt_names.h"
+#include "tc_util.h"
+#include <linux/tc_act/tc_vlan.h>
+
+static void explain(void)
+{
+ fprintf(stderr, "Usage: vlan pop\n");
+ fprintf(stderr, " vlan push [ protocol VLANPROTO ] id VLANID\n");
+ fprintf(stderr, " VLANPROTO is one of 802.1Q or 802.1AD\n");
+ fprintf(stderr, " with default: 802.1Q\n");
+}
+
+static void usage(void)
+{
+ explain();
+ exit(-1);
+}
+
+static int parse_vlan(struct action_util *a, int *argc_p, char ***argv_p,
+ int tca_id, struct nlmsghdr *n)
+{
+ int argc = *argc_p;
+ char **argv = *argv_p;
+ struct rtattr *tail;
+ int action = 0;
+ __u16 id;
+ int id_set = 0;
+ __u16 proto;
+ int proto_set = 0;
+ struct tc_vlan parm = { 0 };
+
+ if (matches(*argv, "vlan") != 0)
+ return -1;
+
+ NEXT_ARG();
+
+ while (argc > 0) {
+ if (matches(*argv, "pop") == 0) {
+ if (action) {
+ fprintf(stderr, "unexpexted \"%s\" - action already specified\n",
+ *argv);
+ explain();
+ return -1;
+ }
+ action = TCA_VLAN_ACT_POP;
+ } else if (matches(*argv, "push") == 0) {
+ if (action) {
+ fprintf(stderr, "unexpexted \"%s\" - action already specified\n",
+ *argv);
+ explain();
+ return -1;
+ }
+ action = TCA_VLAN_ACT_PUSH;
+ } else if (matches(*argv, "id") == 0) {
+ if (action != TCA_VLAN_ACT_PUSH) {
+ fprintf(stderr, "\"%s\" is only valid for push\n",
+ *argv);
+ explain();
+ return -1;
+ }
+ NEXT_ARG();
+ if (get_u16(&id, *argv, 0))
+ invarg("id is invalid", *argv);
+ id_set = 1;
+ } else if (matches(*argv, "protocol") == 0) {
+ if (action != TCA_VLAN_ACT_PUSH) {
+ fprintf(stderr, "\"%s\" is only valid for push\n",
+ *argv);
+ explain();
+ return -1;
+ }
+ NEXT_ARG();
+ if (ll_proto_a2n(&proto, *argv))
+ invarg("protocol is invalid", *argv);
+ proto_set = 1;
+ } else if (matches(*argv, "help") == 0) {
+ usage();
+ } else {
+ break;
+ }
+ argc--;
+ argv++;
+ }
+
+ parm.action = TC_ACT_PIPE;
+ if (argc) {
+ if (matches(*argv, "reclassify") == 0) {
+ parm.action = TC_ACT_RECLASSIFY;
+ NEXT_ARG();
+ } else if (matches(*argv, "pipe") == 0) {
+ parm.action = TC_ACT_PIPE;
+ NEXT_ARG();
+ } else if (matches(*argv, "drop") == 0 ||
+ matches(*argv, "shot") == 0) {
+ parm.action = TC_ACT_SHOT;
+ NEXT_ARG();
+ } else if (matches(*argv, "continue") == 0) {
+ parm.action = TC_ACT_UNSPEC;
+ NEXT_ARG();
+ } else if (matches(*argv, "pass") == 0) {
+ parm.action = TC_ACT_OK;
+ NEXT_ARG();
+ }
+ }
+
+ if (argc) {
+ if (matches(*argv, "index") == 0) {
+ NEXT_ARG();
+ if (get_u32(&parm.index, *argv, 10)) {
+ fprintf(stderr, "vlan: Illegal \"index\"\n");
+ return -1;
+ }
+ argc--;
+ argv++;
+ }
+ }
+
+ if (action == TCA_VLAN_ACT_PUSH && !id_set) {
+ fprintf(stderr, "id needs to be set for push\n");
+ explain();
+ return -1;
+ }
+
+ parm.v_action = action;
+ tail = NLMSG_TAIL(n);
+ addattr_l(n, MAX_MSG, tca_id, NULL, 0);
+ addattr_l(n, MAX_MSG, TCA_VLAN_PARMS, &parm, sizeof(parm));
+ if (id_set)
+ addattr_l(n, MAX_MSG, TCA_VLAN_PUSH_VLAN_ID, &id, 2);
+ if (proto_set) {
+ if (proto != htons(ETH_P_8021Q) &&
+ proto != htons(ETH_P_8021AD)) {
+ fprintf(stderr, "protocol not supported\n");
+ explain();
+ return -1;
+ }
+
+ addattr_l(n, MAX_MSG, TCA_VLAN_PUSH_VLAN_PROTOCOL, &proto, 2);
+ }
+ tail->rta_len = (char *)NLMSG_TAIL(n) - (char *)tail;
+
+ *argc_p = argc;
+ *argv_p = argv;
+ return 0;
+}
+
+static int print_vlan(struct action_util *au, FILE *f, struct rtattr *arg)
+{
+ SPRINT_BUF(b1);
+ struct rtattr *tb[TCA_VLAN_MAX + 1];
+ __u16 val;
+ struct tc_vlan *parm;
+
+ if (arg == NULL)
+ return -1;
+
+ parse_rtattr_nested(tb, TCA_VLAN_MAX, arg);
+
+ if (!tb[TCA_VLAN_PARMS]) {
+ fprintf(f, "[NULL vlan parameters]");
+ return -1;
+ }
+ parm = RTA_DATA(tb[TCA_VLAN_PARMS]);
+
+ fprintf(f, " vlan");
+
+ switch(parm->v_action) {
+ case TCA_VLAN_ACT_POP:
+ fprintf(f, " pop");
+ break;
+ case TCA_VLAN_ACT_PUSH:
+ fprintf(f, " push");
+ if (tb[TCA_VLAN_PUSH_VLAN_ID]) {
+ val = rta_getattr_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
+ fprintf(f, " id %u", val);
+ }
+ if (tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]) {
+ fprintf(f, " protocol %s",
+ ll_proto_n2a(rta_getattr_u16(tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]),
+ b1, sizeof(b1)));
+ }
+ break;
+ }
+
+ fprintf(f, "\n\t index %d ref %d bind %d", parm->index, parm->refcnt,
+ parm->bindcnt);
+
+ if (show_stats) {
+ if (tb[TCA_VLAN_TM]) {
+ struct tcf_t *tm = RTA_DATA(tb[TCA_VLAN_TM]);
+ print_tm(f, tm);
+ }
+ }
+
+ fprintf(f, "\n ");
+
+ return 0;
+}
+
+struct action_util vlan_action_util = {
+ .id = "vlan",
+ .parse_aopt = parse_vlan,
+ .print_aopt = print_vlan,
+};
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [patch iproute2 v4.1] tc: add support for vlan tc action
2014-11-19 13:08 ` [patch iproute2 v4] tc: add support for vlan tc action Jiri Pirko
@ 2014-11-21 11:31 ` Jiri Pirko
2014-12-03 17:35 ` [patch iproute2 v4] " Stephen Hemminger
1 sibling, 0 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-21 11:31 UTC (permalink / raw)
To: netdev
Cc: davem, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang, vadim4j
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Reviewed-by: Cong Wang <cwang@twopensource.com>
---
v4->v4.1:
- fixed typo "unexpected" pointed out by Vadim Kochan
v1->v2:
- included changes suggested by Jamal
include/linux/tc_act/tc_vlan.h | 35 +++++++
tc/Makefile | 1 +
tc/m_vlan.c | 221 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 257 insertions(+)
create mode 100644 include/linux/tc_act/tc_vlan.h
create mode 100644 tc/m_vlan.c
diff --git a/include/linux/tc_act/tc_vlan.h b/include/linux/tc_act/tc_vlan.h
new file mode 100644
index 0000000..f7b8d44
--- /dev/null
+++ b/include/linux/tc_act/tc_vlan.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2014 Jiri Pirko <jiri@resnulli.us>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_TC_VLAN_H
+#define __LINUX_TC_VLAN_H
+
+#include <linux/pkt_cls.h>
+
+#define TCA_ACT_VLAN 12
+
+#define TCA_VLAN_ACT_POP 1
+#define TCA_VLAN_ACT_PUSH 2
+
+struct tc_vlan {
+ tc_gen;
+ int v_action;
+};
+
+enum {
+ TCA_VLAN_UNSPEC,
+ TCA_VLAN_TM,
+ TCA_VLAN_PARMS,
+ TCA_VLAN_PUSH_VLAN_ID,
+ TCA_VLAN_PUSH_VLAN_PROTOCOL,
+ __TCA_VLAN_MAX,
+};
+#define TCA_VLAN_MAX (__TCA_VLAN_MAX - 1)
+
+#endif
diff --git a/tc/Makefile b/tc/Makefile
index 1ab36c6..830c97d 100644
--- a/tc/Makefile
+++ b/tc/Makefile
@@ -40,6 +40,7 @@ TCMODULES += m_pedit.o
TCMODULES += m_skbedit.o
TCMODULES += m_csum.o
TCMODULES += m_simple.o
+TCMODULES += m_vlan.o
TCMODULES += p_ip.o
TCMODULES += p_icmp.o
TCMODULES += p_tcp.o
diff --git a/tc/m_vlan.c b/tc/m_vlan.c
new file mode 100644
index 0000000..171d268
--- /dev/null
+++ b/tc/m_vlan.c
@@ -0,0 +1,221 @@
+/*
+ * m_vlan.c vlan manipulation module
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ *
+ * Authors: Jiri Pirko <jiri@resnulli.us>
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <linux/if_ether.h>
+#include "utils.h"
+#include "rt_names.h"
+#include "tc_util.h"
+#include <linux/tc_act/tc_vlan.h>
+
+static void explain(void)
+{
+ fprintf(stderr, "Usage: vlan pop\n");
+ fprintf(stderr, " vlan push [ protocol VLANPROTO ] id VLANID\n");
+ fprintf(stderr, " VLANPROTO is one of 802.1Q or 802.1AD\n");
+ fprintf(stderr, " with default: 802.1Q\n");
+}
+
+static void usage(void)
+{
+ explain();
+ exit(-1);
+}
+
+static int parse_vlan(struct action_util *a, int *argc_p, char ***argv_p,
+ int tca_id, struct nlmsghdr *n)
+{
+ int argc = *argc_p;
+ char **argv = *argv_p;
+ struct rtattr *tail;
+ int action = 0;
+ __u16 id;
+ int id_set = 0;
+ __u16 proto;
+ int proto_set = 0;
+ struct tc_vlan parm = { 0 };
+
+ if (matches(*argv, "vlan") != 0)
+ return -1;
+
+ NEXT_ARG();
+
+ while (argc > 0) {
+ if (matches(*argv, "pop") == 0) {
+ if (action) {
+ fprintf(stderr, "unexpected \"%s\" - action already specified\n",
+ *argv);
+ explain();
+ return -1;
+ }
+ action = TCA_VLAN_ACT_POP;
+ } else if (matches(*argv, "push") == 0) {
+ if (action) {
+ fprintf(stderr, "unexpected \"%s\" - action already specified\n",
+ *argv);
+ explain();
+ return -1;
+ }
+ action = TCA_VLAN_ACT_PUSH;
+ } else if (matches(*argv, "id") == 0) {
+ if (action != TCA_VLAN_ACT_PUSH) {
+ fprintf(stderr, "\"%s\" is only valid for push\n",
+ *argv);
+ explain();
+ return -1;
+ }
+ NEXT_ARG();
+ if (get_u16(&id, *argv, 0))
+ invarg("id is invalid", *argv);
+ id_set = 1;
+ } else if (matches(*argv, "protocol") == 0) {
+ if (action != TCA_VLAN_ACT_PUSH) {
+ fprintf(stderr, "\"%s\" is only valid for push\n",
+ *argv);
+ explain();
+ return -1;
+ }
+ NEXT_ARG();
+ if (ll_proto_a2n(&proto, *argv))
+ invarg("protocol is invalid", *argv);
+ proto_set = 1;
+ } else if (matches(*argv, "help") == 0) {
+ usage();
+ } else {
+ break;
+ }
+ argc--;
+ argv++;
+ }
+
+ parm.action = TC_ACT_PIPE;
+ if (argc) {
+ if (matches(*argv, "reclassify") == 0) {
+ parm.action = TC_ACT_RECLASSIFY;
+ NEXT_ARG();
+ } else if (matches(*argv, "pipe") == 0) {
+ parm.action = TC_ACT_PIPE;
+ NEXT_ARG();
+ } else if (matches(*argv, "drop") == 0 ||
+ matches(*argv, "shot") == 0) {
+ parm.action = TC_ACT_SHOT;
+ NEXT_ARG();
+ } else if (matches(*argv, "continue") == 0) {
+ parm.action = TC_ACT_UNSPEC;
+ NEXT_ARG();
+ } else if (matches(*argv, "pass") == 0) {
+ parm.action = TC_ACT_OK;
+ NEXT_ARG();
+ }
+ }
+
+ if (argc) {
+ if (matches(*argv, "index") == 0) {
+ NEXT_ARG();
+ if (get_u32(&parm.index, *argv, 10)) {
+ fprintf(stderr, "vlan: Illegal \"index\"\n");
+ return -1;
+ }
+ argc--;
+ argv++;
+ }
+ }
+
+ if (action == TCA_VLAN_ACT_PUSH && !id_set) {
+ fprintf(stderr, "id needs to be set for push\n");
+ explain();
+ return -1;
+ }
+
+ parm.v_action = action;
+ tail = NLMSG_TAIL(n);
+ addattr_l(n, MAX_MSG, tca_id, NULL, 0);
+ addattr_l(n, MAX_MSG, TCA_VLAN_PARMS, &parm, sizeof(parm));
+ if (id_set)
+ addattr_l(n, MAX_MSG, TCA_VLAN_PUSH_VLAN_ID, &id, 2);
+ if (proto_set) {
+ if (proto != htons(ETH_P_8021Q) &&
+ proto != htons(ETH_P_8021AD)) {
+ fprintf(stderr, "protocol not supported\n");
+ explain();
+ return -1;
+ }
+
+ addattr_l(n, MAX_MSG, TCA_VLAN_PUSH_VLAN_PROTOCOL, &proto, 2);
+ }
+ tail->rta_len = (char *)NLMSG_TAIL(n) - (char *)tail;
+
+ *argc_p = argc;
+ *argv_p = argv;
+ return 0;
+}
+
+static int print_vlan(struct action_util *au, FILE *f, struct rtattr *arg)
+{
+ SPRINT_BUF(b1);
+ struct rtattr *tb[TCA_VLAN_MAX + 1];
+ __u16 val;
+ struct tc_vlan *parm;
+
+ if (arg == NULL)
+ return -1;
+
+ parse_rtattr_nested(tb, TCA_VLAN_MAX, arg);
+
+ if (!tb[TCA_VLAN_PARMS]) {
+ fprintf(f, "[NULL vlan parameters]");
+ return -1;
+ }
+ parm = RTA_DATA(tb[TCA_VLAN_PARMS]);
+
+ fprintf(f, " vlan");
+
+ switch(parm->v_action) {
+ case TCA_VLAN_ACT_POP:
+ fprintf(f, " pop");
+ break;
+ case TCA_VLAN_ACT_PUSH:
+ fprintf(f, " push");
+ if (tb[TCA_VLAN_PUSH_VLAN_ID]) {
+ val = rta_getattr_u16(tb[TCA_VLAN_PUSH_VLAN_ID]);
+ fprintf(f, " id %u", val);
+ }
+ if (tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]) {
+ fprintf(f, " protocol %s",
+ ll_proto_n2a(rta_getattr_u16(tb[TCA_VLAN_PUSH_VLAN_PROTOCOL]),
+ b1, sizeof(b1)));
+ }
+ break;
+ }
+
+ fprintf(f, "\n\t index %d ref %d bind %d", parm->index, parm->refcnt,
+ parm->bindcnt);
+
+ if (show_stats) {
+ if (tb[TCA_VLAN_TM]) {
+ struct tcf_t *tm = RTA_DATA(tb[TCA_VLAN_TM]);
+ print_tm(f, tm);
+ }
+ }
+
+ fprintf(f, "\n ");
+
+ return 0;
+}
+
+struct action_util vlan_action_util = {
+ .id = "vlan",
+ .parse_aopt = parse_vlan,
+ .print_aopt = print_vlan,
+};
--
1.9.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [patch iproute2 v4] tc: add support for vlan tc action
2014-11-19 13:08 ` [patch iproute2 v4] tc: add support for vlan tc action Jiri Pirko
2014-11-21 11:31 ` [patch iproute2 v4.1] " Jiri Pirko
@ 2014-12-03 17:35 ` Stephen Hemminger
1 sibling, 0 replies; 24+ messages in thread
From: Stephen Hemminger @ 2014-12-03 17:35 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, jhs, pshelar, therbert, edumazet, willemb,
dborkman, mst, fw, Paul.Durrant, tgraf, cwang
On Wed, 19 Nov 2014 14:08:26 +0100
Jiri Pirko <jiri@resnulli.us> wrote:
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> Reviewed-by: Cong Wang <cwang@twopensource.com>
Applied to net-next branch.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [patch net-next v4 0/9] sched: introduce vlan action
2014-11-19 13:04 [patch net-next v4 0/9] sched: introduce vlan action Jiri Pirko
` (9 preceding siblings ...)
2014-11-19 13:08 ` [patch iproute2 v4] tc: add support for vlan tc action Jiri Pirko
@ 2014-11-21 19:21 ` David Miller
2014-11-21 23:52 ` Jiri Pirko
10 siblings, 1 reply; 24+ messages in thread
From: David Miller @ 2014-11-21 19:21 UTC (permalink / raw)
To: jiri
Cc: netdev, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
From: Jiri Pirko <jiri@resnulli.us>
Date: Wed, 19 Nov 2014 14:04:54 +0100
> Please see the individual patches for info
Series applied, thanks Jiri.
Please work with others to resolve the MPLS header length subtraction
issue Pravin mentioned.
Thanks.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [patch net-next v4 0/9] sched: introduce vlan action
2014-11-21 19:21 ` [patch net-next v4 0/9] sched: introduce vlan action David Miller
@ 2014-11-21 23:52 ` Jiri Pirko
0 siblings, 0 replies; 24+ messages in thread
From: Jiri Pirko @ 2014-11-21 23:52 UTC (permalink / raw)
To: David Miller
Cc: netdev, jhs, pshelar, therbert, edumazet, willemb, dborkman, mst,
fw, Paul.Durrant, tgraf, cwang
Fri, Nov 21, 2014 at 08:21:07PM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Wed, 19 Nov 2014 14:04:54 +0100
>
>> Please see the individual patches for info
>
>Series applied, thanks Jiri.
>
>Please work with others to resolve the MPLS header length subtraction
>issue Pravin mentioned.
Sure thing.
>
>Thanks.
^ permalink raw reply [flat|nested] 24+ messages in thread