Re: [PATCH net-next v15] openvswitch: enable NSH support

On Sun, Nov 5, 2017 at 8:19 PM, Yang, Yi [off-list ref] wrote:

On Sat, Nov 04, 2017 at 10:29:46PM +0800, Pravin Shelar wrote:

quoted

On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang [off-list ref] wrote:

quoted

+int nsh_push(struct sk_buff *skb, const struct nshhdr *pushed_nh)
+{
+       struct nshhdr *nh;
+       size_t length = nsh_hdr_len(pushed_nh);
+       u8 next_proto;
+
+       if (skb->mac_len) {
+               next_proto = TUN_P_ETHERNET;
+       } else {
+               next_proto = tun_p_from_eth_p(skb->protocol);
+               if (!next_proto)
+                       return -EAFNOSUPPORT;

check for supported protocols can be moved to flow install validation
in __ovs_nla_copy_actions().

quoted

+       }
+
+       /* Add the NSH header */
+       if (skb_cow_head(skb, length) < 0)
+               return -ENOMEM;
+
+       skb_push(skb, length);
+       nh = (struct nshhdr *)(skb->data);
+       memcpy(nh, pushed_nh, length);
+       nh->np = next_proto;
+
+       skb->protocol = htons(ETH_P_NSH);
+       skb_reset_mac_header(skb);
+       skb_reset_network_header(skb);
+       skb_reset_mac_len(skb);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(nsh_push);
+
+int nsh_pop(struct sk_buff *skb)
+{
+       struct nshhdr *nh;
+       size_t length;
+       __be16 inner_proto;
+
+       if (!pskb_may_pull(skb, NSH_BASE_HDR_LEN))
+               return -ENOMEM;
+       nh = (struct nshhdr *)(skb->data);
+       length = nsh_hdr_len(nh);
+       inner_proto = tun_p_to_eth_p(nh->np);

same as above, this check can be moved to flow install __ovs_nla_copy_actions().

Pravin, these two functions are not only for OVS, you can see it is
net/nsh/nsh.c, Jiri and Eric mentioned they also could be used by TC.

I think it can be easily done by other caller of these function, or
you can refactor the APIs itself.

I understand you expect some checks should be moved to slow path, but
for there two cases, we can't remove them into __ovs_nla_copy_actions.

It is not just about optimization, but having these check in flow
install allows OVS userspace to probe level of  NSH support in
datapath.