On 12/10/2019 12:09 PM, Sathya Perla wrote: > On Tue, Dec 10, 2019 at 1:21 PM Eli Britstein <el...@mellanox.com> wrote: >> >> On 12/10/2019 8:52 AM, Sathya Perla wrote: >>> On Sun, Dec 8, 2019 at 6:53 PM Eli Britstein <el...@mellanox.com> wrote: >>> ... >>>> +static int >>>> +parse_clone_actions(struct netdev *netdev, >>>> + struct flow_actions *actions, >>>> + const struct nlattr *clone_actions, >>>> + const size_t clone_actions_len, >>>> + struct offload_info *info) >>>> +{ >>>> + const struct nlattr *ca; >>>> + unsigned int cleft; >>>> + >>>> + NL_ATTR_FOR_EACH_UNSAFE (ca, cleft, clone_actions, clone_actions_len) >>>> { >>>> + int clone_type = nl_attr_type(ca); >>>> + >>>> + if (clone_type == OVS_ACTION_ATTR_TUNNEL_PUSH) { >>>> + const struct ovs_action_push_tnl *tnl_push = nl_attr_get(ca); >>>> + struct rte_flow_action_raw_encap *raw_encap = >>>> + xzalloc(sizeof *raw_encap); >>>> + >>>> + raw_encap->data = (uint8_t *)tnl_push->header; >>>> + raw_encap->preserve = NULL; >>>> + raw_encap->size = tnl_push->header_len; >>>> + >>>> + add_flow_action(actions, RTE_FLOW_ACTION_TYPE_RAW_ENCAP, >>>> + raw_encap); >>> Hi, converting OVS_TUNNLE_PUSH into RTE_RAW_ENCAP loses the 'tnl_type' >>> information provided by OVS. RAW_ENCAP provides the tunnel header just >>> as a blob of bits which may not be ideal for NIC HW to offload. >>> >>> How about using tnl_push->tnl_type field to parse the header and >>> compose specific tunnel encap actions like RTE_VXLAN_ENCAP, >>> RTE_NVGRE_ENCAP etc. >>> This would be also be more inline with how it's done with TC-offload. >> I see your point. However, struct ovs_action_push_tnl has the "header" >> field just as a raw binary buffer, and not detailed like in TC. >> "tnl_type" has a comment /* For logging. */. It is indeed used for >> logging, as in lib/odp-util.c, in function format_odp_tnl_push_header. > This is not entirely true. Checkout propagate_tunnel_data_to_flow() > where tnl_type is being used > to figure out nw_proto. > >> using VXLAN_ENCAP instead of RAW_ENCAP will make the push less generic, >> as I will have to parse the header to build the vxlan_encap fields, just >> so they can re-build as a raw header in the PMD level, so I don't see >> the offload benefit of it. > Why are you assuming that all PMDs will rebuild the tunnel header > fields as a 'raw header'? As far as I saw in DPDK code, if I'm not wrong, all PMDs that support it do it like this. Anyway, it is indeed not relevant. > What if the NIC HW needs tunnel specific headers to be programmed > separately for each tunnel type? > >> Another point is that this way it will support any header, not just >> VXLAN (as an example). > Given that VXLAN and NVGRE ENCAP and DECAP actions are already defined > in rte_flow, how about > we use them for vxlan and nvgre and use RAW for the rest of the tunnel types? > We can support all tunnel headers even this way.....
Again, I see your point. However, in OVS level, we have the encap as a raw buffer and DPDK supports it natively using RAW_ENCAP. For that reason I think we should use the straight forward method. I think that if there is a PMD that requires the fields separately, it is under its responsibility to parse it, and not forcing the application to do it. As it's a valid attribute in DPDK, and it's the natural way for OVS to use, I think we should use it. If it is from some reason not valid, in the future, DPDK will deprecate it. > > thanks! _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev