On Mon, Mar 15, 2021 at 3:57 PM Eli Britstein <el...@nvidia.com> wrote:
>
>
> On 3/15/2021 11:04 AM, Sriharsha Basavapatna wrote:
> > On Tue, Mar 2, 2021 at 4:56 PM Eli Britstein <el...@nvidia.com> wrote:
> >> From: Sriharsha Basavapatna <sriharsha.basavapa...@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.basavapa...@broadcom.com>
> >> Signed-off-by: Eli Britstein <el...@nvidia.com>
> >> Reviewed-by: Gaetan Rivet <gaet...@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
> >
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
