Looking through the patchset again, this time more deeply. Sorry for
the delay.
On Wed, 4 May 2016 16:36:30 +0900, Simon Horman wrote:
> +struct ovs_action_push_eth {
> + struct ovs_key_ethernet addresses;
> + __be16 eth_type;
Extra spaces.
> +static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> + skb_pull_rcsum(skb, ETH_HLEN);
> + skb_reset_mac_header(skb);
> + skb->mac_len -= ETH_HLEN;
> +
> + invalidate_flow_key(key);
> + return 0;
> +}
There's a fundamental question here: how should pop_eth behave when
vlan tag is present?
There are two options: either vlan is considered part of the Ethernet
header and pop_eth means implicitly resetting vlan tag, or packet can
have vlan tag even if it's not Ethernet.
This patch seems to implement the first option; however, skb->vlan_tci
should be reset and pop_eth should check whether the vlan tag is
present in the frame (deaccelerated) and remove it if it is. Otherwise,
the behavior of pop_eth would be inconsistent.
However, I'm not sure whether the second option does not make more
sense. It may, in fact, be needed - ARPHRD_NONE tunnel port could not
be set as an access port otherwise (unless I'm missing something).
In that case, pop_eth will need to put the vlan tag to skb->vlan_tci if
it's in the frame itself. Also, push_vlan and pop_vlan would need to be
modified to work with is_layer3 packets.
> +static int push_eth(struct sk_buff *skb, struct sw_flow_key *key,
> + const struct ovs_action_push_eth *ethh)
> +{
> + int err;
> +
> + /* De-accelerate any hardware accelerated VLAN tag added to a previous
> + * Ethernet header */
> + err = skb_vlan_deaccel(skb);
Why? Just keep it in skb->vlan_tci.
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -468,28 +468,31 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
>
> skb_reset_mac_header(skb);
>
> - /* Link layer. We are guaranteed to have at least the 14 byte Ethernet
> - * header in the linear data area.
> - */
> - eth = eth_hdr(skb);
> - ether_addr_copy(key->eth.src, eth->h_source);
> - ether_addr_copy(key->eth.dst, eth->h_dest);
> + /* Link layer. */
> + if (key->phy.is_layer3) {
> + key->eth.tci = 0;
Could make sense to use skb->vlan_tci, see above.
Thanks,
Jiri
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
