[ovs-dev] [PATCH V3 18/19] netdev-offload-dpdk: Support offload of clone tnl_push/output actions
Eli Britstein
elibr at mellanox.com
Tue Dec 10 11:55:25 UTC 2019
On 12/10/2019 12:09 PM, Sathya Perla wrote:
> On Tue, Dec 10, 2019 at 1:21 PM Eli Britstein <elibr at mellanox.com> wrote:
>>
>> On 12/10/2019 8:52 AM, Sathya Perla wrote:
>>> On Sun, Dec 8, 2019 at 6:53 PM Eli Britstein <elibr at 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!
More information about the dev
mailing list