On Thu, Jun 02, 2016 at 03:02:18PM -0700, pravin shelar wrote:
> On Wed, Jun 1, 2016 at 11:24 PM, Simon Horman
> <simon.hor...@netronome.com> wrote:
[...]
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index 15f130e4c22b..5567529904fa 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -300,6 +300,51 @@ static int set_eth_addr(struct sk_buff *skb, struct
> > sw_flow_key *flow_key,
> > return 0;
> > }
> >
> > +static int pop_eth(struct sk_buff *skb, struct sw_flow_key *key)
> > +{
> > + /* Pop outermost VLAN tag to skb metadata unless a VLAN tag
> > + * is already present there.
> > + */
> > + if ((skb->protocol == htons(ETH_P_8021Q) ||
> > + skb->protocol == htons(ETH_P_8021AD)) &&
> > + !skb_vlan_tag_present(skb)) {
> > + int err = skb_vlan_accel(skb);
> > + if (unlikely(err))
> > + return err;
> > + }
> > +
>
> I do not think we can keep just the vlan tag and pop ethernet header.
> There are multiple issues with this.
> First networking stack can not handle suck packet. second issue even
> after this patch OVS can not parse this type of packet. third this
> patch does not allow pop-eth action on vlan tagged packet.
> There is already separate vlan related actions in OVS so lets keep it simple.
I wonder if the best solution is to simply omit handling VLAN tags
in pop_eth for now. As you mention pop_eth is not permitted on such packets.
[...]
> > diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> > index 0ea128eeeab2..2d9777abcfc9 100644
> > --- 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. */
> > + key->eth.tci = 0;
> > + if (key->phy.is_layer3) {
> > + if (skb_vlan_tag_present(skb))
> > + key->eth.tci = htons(skb->vlan_tci);
> > + } else {
> > + eth = eth_hdr(skb);
> eth can be moved to this block.
Thanks, done.
[...]
> > @@ -723,9 +730,19 @@ int ovs_flow_key_extract(const struct ip_tunnel_info
> > *tun_info,
> > key->phy.skb_mark = skb->mark;
> > ovs_ct_fill_key(skb, key);
> > key->ovs_flow_hash = 0;
> > + key->phy.is_layer3 = skb->mac_len == 0;
> > key->recirc_id = 0;
> >
> > - return key_extract(skb, key);
> > + err = key_extract(skb, key);
> > + if (err < 0)
> > + return err;
> > +
> > + if (key->phy.is_layer3)
> > + key->eth.type = skb->protocol;
> Now key->eth.type is set in three different function, can you
> centralize in key_extract()?
Sure, I think that the instance above can be trivially moved into
key_extract() and the one in ovs_flow_key_update() can be removed.
[...]
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index 0bb650f4f219..1e1392c3c0ed 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
[...]
> > @@ -355,6 +359,7 @@ static const struct ovs_len_tbl
> > ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
> > [OVS_KEY_ATTR_CT_ZONE] = { .len = sizeof(u16) },
> > [OVS_KEY_ATTR_CT_MARK] = { .len = sizeof(u32) },
> > [OVS_KEY_ATTR_CT_LABELS] = { .len = sizeof(struct
> > ovs_key_ct_labels) },
> > + [OVS_KEY_ATTR_PACKET_ETHERTYPE] = { .len = sizeof(__be16) },
> > };
> >
> I do not see need for OVS_KEY_ATTR_PACKET_ETHERTYPE, we can use
> existing OVS_KEY_ATTR_ETHERTYPE to serialize the flow key. If there is
> no OVS_KEY_ATTR_ETHERNET attribute then its l3 packet.
The idea of OVS_KEY_ATTR_PACKET_ETHERTYPE is to allow communication of
the L2 type of the packet which is not present in an L3 packet. In terms
of GRE (non-TEB) this correlates to the Protocol Type field in the GRE
header.
