[ovs-dev] [PATCH V3 12/14] dpif-netdev: Provide orig_in_port in metadata for tunneled packets

Eli Britstein elibr at nvidia.com
Mon Mar 15 10:27:03 UTC 2021


On 3/15/2021 11:04 AM, Sriharsha Basavapatna wrote:
> On Tue, Mar 2, 2021 at 4:56 PM Eli Britstein <elibr at nvidia.com> wrote:
>> From: Sriharsha Basavapatna <sriharsha.basavapatna at broadcom.com>
>>
>> When an encapsulated packet is recirculated through a TUNNEL_POP
>> action, the metadata gets reinitialized and the originating physical
>> port information is lost. When this flow gets processed by the vport
>> and it needs to be offloaded, we can't figure out the physical port
>> through which the tunneled packet was received.
>>
>> Add a new member to the metadata: 'orig_in_port'. This is passed to
>> the next stage during recirculation and the offload layer can use it
>> to offload the flow to this physical port.
>>
>> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna at broadcom.com>
>> Signed-off-by: Eli Britstein <elibr at nvidia.com>
>> Reviewed-by: Gaetan Rivet <gaetanr at nvidia.com>
>> ---
>>   lib/dpif-netdev.c    | 20 ++++++++++++++------
>>   lib/netdev-offload.h |  1 +
>>   lib/packets.h        |  8 +++++++-
>>   3 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 58cad7ded..291c6eaa4 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -430,6 +430,7 @@ struct dp_flow_offload_item {
>>       struct match match;
>>       struct nlattr *actions;
>>       size_t actions_len;
>> +    odp_port_t orig_in_port; /* Originating in_port for tnl flows. */
>>
>>       struct ovs_list node;
>>   };
>> @@ -2695,11 +2696,13 @@ dp_netdev_flow_offload_put(struct dp_flow_offload_item *offload)
>>           }
>>       }
>>       info.flow_mark = mark;
>> +    info.orig_in_port = offload->orig_in_port;
>>
>>       port = netdev_ports_get(in_port, dpif_type_str);
>>       if (!port) {
>>           goto err_free;
>>       }
>> +
>>       /* Taking a global 'port_mutex' to fulfill thread safety restrictions for
>>        * the netdev-offload-dpdk module. */
>>       ovs_mutex_lock(&pmd->dp->port_mutex);
>> @@ -2797,7 +2800,8 @@ queue_netdev_flow_del(struct dp_netdev_pmd_thread *pmd,
>>   static void
>>   queue_netdev_flow_put(struct dp_netdev_pmd_thread *pmd,
>>                         struct dp_netdev_flow *flow, struct match *match,
>> -                      const struct nlattr *actions, size_t actions_len)
>> +                      const struct nlattr *actions, size_t actions_len,
>> +                      odp_port_t orig_in_port)
>>   {
>>       struct dp_flow_offload_item *offload;
>>       int op;
>> @@ -2823,6 +2827,7 @@ queue_netdev_flow_put(struct dp_netdev_pmd_thread *pmd,
>>       offload->actions = xmalloc(actions_len);
>>       memcpy(offload->actions, actions, actions_len);
>>       offload->actions_len = actions_len;
>> +    offload->orig_in_port = orig_in_port;
>>
>>       dp_netdev_append_flow_offload(offload);
>>   }
>> @@ -3624,7 +3629,8 @@ dp_netdev_get_mega_ufid(const struct match *match, ovs_u128 *mega_ufid)
>>   static struct dp_netdev_flow *
>>   dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
>>                      struct match *match, const ovs_u128 *ufid,
>> -                   const struct nlattr *actions, size_t actions_len)
>> +                   const struct nlattr *actions, size_t actions_len,
>> +                   odp_port_t orig_in_port)
>>       OVS_REQUIRES(pmd->flow_mutex)
>>   {
>>       struct ds extra_info = DS_EMPTY_INITIALIZER;
>> @@ -3690,7 +3696,8 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
>>       cmap_insert(&pmd->flow_table, CONST_CAST(struct cmap_node *, &flow->node),
>>                   dp_netdev_flow_hash(&flow->ufid));
>>
>> -    queue_netdev_flow_put(pmd, flow, match, actions, actions_len);
>> +    queue_netdev_flow_put(pmd, flow, match, actions, actions_len,
>> +                          orig_in_port);
>>
>>       if (OVS_UNLIKELY(!VLOG_DROP_DBG((&upcall_rl)))) {
>>           struct ds ds = DS_EMPTY_INITIALIZER;
>> @@ -3761,7 +3768,7 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
>>       if (!netdev_flow) {
>>           if (put->flags & DPIF_FP_CREATE) {
>>               dp_netdev_flow_add(pmd, match, ufid, put->actions,
>> -                               put->actions_len);
>> +                               put->actions_len, ODPP_NONE);
>>           } else {
>>               error = ENOENT;
>>           }
>> @@ -3777,7 +3784,7 @@ flow_put_on_pmd(struct dp_netdev_pmd_thread *pmd,
>>               ovsrcu_set(&netdev_flow->actions, new_actions);
>>
>>               queue_netdev_flow_put(pmd, netdev_flow, match,
>> -                                  put->actions, put->actions_len);
>> +                                  put->actions, put->actions_len, ODPP_NONE);
>>
>>               if (stats) {
>>                   get_dpif_flow_status(pmd->dp, netdev_flow, stats, NULL);
>> @@ -7232,6 +7239,7 @@ handle_packet_upcall(struct dp_netdev_pmd_thread *pmd,
>>       ovs_u128 ufid;
>>       int error;
>>       uint64_t cycles = cycles_counter_update(&pmd->perf_stats);
>> +    odp_port_t orig_in_port = packet->md.orig_in_port;
>>
>>       match.tun_md.valid = false;
>>       miniflow_expand(&key->mf, &match.flow);
>> @@ -7281,7 +7289,7 @@ handle_packet_upcall(struct dp_netdev_pmd_thread *pmd,
>>           if (OVS_LIKELY(!netdev_flow)) {
>>               netdev_flow = dp_netdev_flow_add(pmd, &match, &ufid,
>>                                                add_actions->data,
>> -                                             add_actions->size);
>> +                                             add_actions->size, orig_in_port);
>>           }
>>           ovs_mutex_unlock(&pmd->flow_mutex);
>>           uint32_t hash = dp_netdev_flow_hash(&netdev_flow->ufid);
>> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
>> index 5bf89f891..e7fcedae9 100644
>> --- a/lib/netdev-offload.h
>> +++ b/lib/netdev-offload.h
>> @@ -76,6 +76,7 @@ struct offload_info {
>>
>>       bool tc_modify_flow_deleted; /* Indicate the tc modify flow put success
>>                                     * to delete the original flow. */
>> +    odp_port_t orig_in_port; /* Originating in_port for tnl flows. */
>>   };
>>
>>   int netdev_flow_flush(struct netdev *);
>> diff --git a/lib/packets.h b/lib/packets.h
>> index 481bc22fa..515bb59b1 100644
>> --- a/lib/packets.h
>> +++ b/lib/packets.h
>> @@ -115,6 +115,7 @@ PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
>>       uint32_t ct_mark;           /* Connection mark. */
>>       ovs_u128 ct_label;          /* Connection label. */
>>       union flow_in_port in_port; /* Input port. */
>> +    odp_port_t orig_in_port;    /* Originating in_port for tunneled packets */
>>       struct conn *conn;          /* Cached conntrack connection. */
>>       bool reply;                 /* True if reply direction. */
>>       bool icmp_related;          /* True if ICMP related. */
>> @@ -143,10 +144,14 @@ BUILD_ASSERT_DECL(offsetof(struct pkt_metadata, cacheline2) ==
>>   static inline void
>>   pkt_metadata_init_tnl(struct pkt_metadata *md)
>>   {
>> +    odp_port_t orig_in_port;
>> +
>>       /* Zero up through the tunnel metadata options. The length and table
>>        * are before this and as long as they are empty, the options won't
>> -     * be looked at. */
>> +     * be looked at. Keep the orig_in_port field. */
>> +    orig_in_port = md->in_port.odp_port;
>>       memset(md, 0, offsetof(struct pkt_metadata, tunnel.metadata.opts));
>> +    md->orig_in_port = orig_in_port;
>>   }
>>
>>   static inline void
>> @@ -173,6 +178,7 @@ pkt_metadata_init(struct pkt_metadata *md, odp_port_t port)
>>       md->tunnel.ip_dst = 0;
>>       md->tunnel.ipv6_dst = in6addr_any;
>>       md->in_port.odp_port = port;
>> +    md->orig_in_port = port;
>>       md->conn = NULL;
>>   }
>>
>> --
>> 2.28.0.2311.g225365fb51
>>
> I don't see this code (from my initial patch). It is needed to handle
> flow-modify.
OK. Now I understand what this is for. However, I think it would be 
better to extract the orig_in_port from the kept "physdev". This way way 
we won't need to keep the odp_port in the offload layer. What do you think?
>
> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
> index 6cfdea8fc..bbdc561fd 100644
> --- a/lib/netdev-offload-dpdk.c
> +++ b/lib/netdev-offload-dpdk.c
> @@ -63,6 +63,7 @@  struct rte_flow_data {
>       bool actions_offloaded;
>       struct dpif_flow_stats stats;
>       struct ovs_list indirect;
> +    odp_port_t orig_in_port;
>   };
>
>   static uint32_t
> @@ -1935,6 +1936,10 @@  netdev_offload_dpdk_flow_put(struct netdev
> *netdev, struct match *match,
>       if (rte_flow_data && rte_flow_data->rte_flow) {
>           old_stats = rte_flow_data->stats;
>           modification = true;
> +        if (netdev_vport_is_vport_class(netdev->netdev_class)) {
> +            /* Retrieve orig_in_port saved earlier */
> +            info->orig_in_port = rte_flow_data->orig_in_port;
> +        }
>           ret = netdev_offload_dpdk_flow_destroy(rte_flow_data);
>           if (ret < 0) {
>               return ret;
> @@ -1949,6 +1954,10 @@  netdev_offload_dpdk_flow_put(struct netdev
> *netdev, struct match *match,
>       if (modification) {
>           rte_flow_data->stats = old_stats;
>       }
> +    if (netdev_vport_is_vport_class(netdev->netdev_class)) {
> +        /*  Save orig_in_port; need this if the flow gets modified later */
> +        rte_flow_data->orig_in_port = info->orig_in_port;
> +    }
>       if (stats) {
>           *stats = rte_flow_data->stats;
>       }
>
> Thanks,
> -Harsha
>


More information about the dev mailing list