Message ID | 20210302112536.32680-13-elibr@nvidia.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Netdev vxlan-decap offload | expand |
Bleep bloop. Greetings Eli Britstein, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Eli Britstein <elibr@nvidia.com> Lines checked: 174, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Tue, Mar 2, 2021 at 4:56 PM Eli Britstein <elibr@nvidia.com> wrote: > > From: Sriharsha Basavapatna <sriharsha.basavapatna@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@broadcom.com> > Signed-off-by: Eli Britstein <elibr@nvidia.com> > Reviewed-by: Gaetan Rivet <gaetanr@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. 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
On 3/15/2021 11:04 AM, Sriharsha Basavapatna wrote: > On Tue, Mar 2, 2021 at 4:56 PM Eli Britstein <elibr@nvidia.com> wrote: >> From: Sriharsha Basavapatna <sriharsha.basavapatna@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@broadcom.com> >> Signed-off-by: Eli Britstein <elibr@nvidia.com> >> Reviewed-by: Gaetan Rivet <gaetanr@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 >
On Mon, Mar 15, 2021 at 3:57 PM Eli Britstein <elibr@nvidia.com> wrote: > > > On 3/15/2021 11:04 AM, Sriharsha Basavapatna wrote: > > On Tue, Mar 2, 2021 at 4:56 PM Eli Britstein <elibr@nvidia.com> wrote: > >> From: Sriharsha Basavapatna <sriharsha.basavapatna@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@broadcom.com> > >> Signed-off-by: Eli Britstein <elibr@nvidia.com> > >> Reviewed-by: Gaetan Rivet <gaetanr@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? Yes, that should be ok. > > > > 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 > >
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; }