| Message ID | 20200120150830.16262-15-elibr@mellanox.com |
|---|---|
| State | Deferred |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | netdev datapath vxlan 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: ERROR: Author should not be also be co-author. Lines checked: 76, Warnings: 0, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
Hi Eli, Thanks for the patch, very interesting work. I'm trying to understand the patch. some questions below: On Mon, Jan 20, 2020 at 7:09 AM Eli Britstein <elibr@mellanox.com> wrote: > > A miss in virtual port offloads means the flow with tnl_pop was > offloaded, but not the following one. Recover the state and continue > with SW processing. Why do we have a miss in virtual port offloads? Isn't we only offloading to physical port or uplink? at patch 25/25, you mentioned "For virtual port (as "vxlan"), HW rules match tunnel properties (outer header) and inner packet fields, and with a decap action. The rules are placed on all uplinks as they are the potential for the origin of the traffic." > > Co-authored-by: Eli Britstein <elibr@mellanox.com> > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > Reviewed-by: Roni Bar Yanai <roniba@mellanox.com> > Signed-off-by: Eli Britstein <elibr@mellanox.com> > --- > lib/netdev-offload-dpdk.c | 34 +++++++++++++++++++++++++++++++++- > 1 file changed, 33 insertions(+), 1 deletion(-) > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index fc890b915..c4d77c115 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -385,7 +385,6 @@ put_flow_miss_ctx_id(uint32_t flow_ctx_id) > put_context_data_by_id(&flow_miss_ctx_md, flow_ctx_id); > } > > -OVS_UNUSED > static int > find_flow_miss_ctx(int flow_ctx_id, struct flow_miss_ctx *ctx) > { > @@ -1769,10 +1768,43 @@ out: > return ret; > } > > +static int > +netdev_offload_dpdk_hw_miss_packet_recover(struct netdev *netdev, > + uint32_t flow_miss_ctx_id, > + struct dp_packet *packet) > +{ > + struct flow_miss_ctx flow_miss_ctx; > + struct netdev *vport_netdev; > + > + if (find_flow_miss_ctx(flow_miss_ctx_id, &flow_miss_ctx)) { > + return -1; > + } > + > + if (flow_miss_ctx.vport != ODPP_NONE) { > + vport_netdev = netdev_ports_get(flow_miss_ctx.vport, > + netdev->dpif_type); > + if (vport_netdev) { > + pkt_metadata_init(&packet->md, flow_miss_ctx.vport); > + if (vport_netdev->netdev_class->pop_header) { > + vport_netdev->netdev_class->pop_header(packet); IIUC, we need to pop header here because now we translate tnl_pop to mark + jump. So the outer tunnel header does not get pop/unset in hardware, so at SW side before upcall to OVS, we need to pop here, right? William
On 7/11/2020 9:36 PM, William Tu wrote: > Hi Eli, > Thanks for the patch, very interesting work. > I'm trying to understand the patch. some questions below: > > On Mon, Jan 20, 2020 at 7:09 AM Eli Britstein <elibr@mellanox.com> wrote: >> A miss in virtual port offloads means the flow with tnl_pop was >> offloaded, but not the following one. Recover the state and continue >> with SW processing. > Why do we have a miss in virtual port offloads? Isn't we only > offloading to physical port or uplink? > at patch 25/25, you mentioned > "For virtual port (as "vxlan"), HW rules match tunnel properties > (outer header) and inner packet fields, and with a decap action. The > rules are placed on all uplinks as they are the potential for the origin > of the traffic." Yes, the offloads are only for physical ports. Regarding misses, please see below. > >> Co-authored-by: Eli Britstein <elibr@mellanox.com> >> Signed-off-by: Ophir Munk <ophirmu@mellanox.com> >> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com> >> Signed-off-by: Eli Britstein <elibr@mellanox.com> >> --- >> lib/netdev-offload-dpdk.c | 34 +++++++++++++++++++++++++++++++++- >> 1 file changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >> index fc890b915..c4d77c115 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -385,7 +385,6 @@ put_flow_miss_ctx_id(uint32_t flow_ctx_id) >> put_context_data_by_id(&flow_miss_ctx_md, flow_ctx_id); >> } >> >> -OVS_UNUSED >> static int >> find_flow_miss_ctx(int flow_ctx_id, struct flow_miss_ctx *ctx) >> { >> @@ -1769,10 +1768,43 @@ out: >> return ret; >> } >> >> +static int >> +netdev_offload_dpdk_hw_miss_packet_recover(struct netdev *netdev, >> + uint32_t flow_miss_ctx_id, >> + struct dp_packet *packet) >> +{ >> + struct flow_miss_ctx flow_miss_ctx; >> + struct netdev *vport_netdev; >> + >> + if (find_flow_miss_ctx(flow_miss_ctx_id, &flow_miss_ctx)) { >> + return -1; >> + } >> + >> + if (flow_miss_ctx.vport != ODPP_NONE) { >> + vport_netdev = netdev_ports_get(flow_miss_ctx.vport, >> + netdev->dpif_type); >> + if (vport_netdev) { >> + pkt_metadata_init(&packet->md, flow_miss_ctx.vport); >> + if (vport_netdev->netdev_class->pop_header) { >> + vport_netdev->netdev_class->pop_header(packet); > IIUC, we need to pop header here because now we translate > tnl_pop to mark + jump. > So the outer tunnel header does not get pop/unset in hardware, > so at SW side before upcall to OVS, we need to pop here, right? The HW offload flows follow the SW model. Once we have more than one flow to process a packet in HW, we are exposed to misses, in case the packet doesn't complete its full path in HW, but only part of it. In this case, the first flow (the classification flow) is hit, but the vport's one may not exist. We should recover the packet as if it was processed in SW from the beginning, to maintain correctness of OVS. For example counters - the HW already counted in the first flow. We should not count it again in SW. Please note that we would like to progress with an approach to enhance DPDK and put some of the logic inside the PMD. See: Link to RFC: http://mails.dpdk.org/archives/dev/2020-June/169656.html <https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmails.dpdk.org%2Farchives%2Fdev%2F2020-June%2F169656.html&data=02%7C01%7Celibr%40mellanox.com%7C164938c31cfc4d387ec508d82485207e%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637299501500880396&sdata=tGbu071CKUaKVI3gL9oSU%2FQAL998gF8ydnYmKL2ml6o%3D&reserved=0> Link to patchset: http://mails.dpdk.org/archives/dev/2020-June/171590.html <https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmails.dpdk.org%2Farchives%2Fdev%2F2020-June%2F171590.html&data=02%7C01%7Celibr%40mellanox.com%7C164938c31cfc4d387ec508d82485207e%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C637299501500880396&sdata=Y0NLf0u5QNU%2FPxOkSYdP66fKf9Z0HFE%2B7FPTRNQhG0g%3D&reserved=0> > > William
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index fc890b915..c4d77c115 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -385,7 +385,6 @@ put_flow_miss_ctx_id(uint32_t flow_ctx_id) put_context_data_by_id(&flow_miss_ctx_md, flow_ctx_id); } -OVS_UNUSED static int find_flow_miss_ctx(int flow_ctx_id, struct flow_miss_ctx *ctx) { @@ -1769,10 +1768,43 @@ out: return ret; } +static int +netdev_offload_dpdk_hw_miss_packet_recover(struct netdev *netdev, + uint32_t flow_miss_ctx_id, + struct dp_packet *packet) +{ + struct flow_miss_ctx flow_miss_ctx; + struct netdev *vport_netdev; + + if (find_flow_miss_ctx(flow_miss_ctx_id, &flow_miss_ctx)) { + return -1; + } + + if (flow_miss_ctx.vport != ODPP_NONE) { + vport_netdev = netdev_ports_get(flow_miss_ctx.vport, + netdev->dpif_type); + if (vport_netdev) { + pkt_metadata_init(&packet->md, flow_miss_ctx.vport); + if (vport_netdev->netdev_class->pop_header) { + vport_netdev->netdev_class->pop_header(packet); + dp_packet_reset_offload(packet); + packet->md.in_port.odp_port = flow_miss_ctx.vport; + } else { + VLOG_ERR("vport nedtdev=%s with no pop_header method", + netdev_get_name(vport_netdev)); + } + netdev_close(vport_netdev); + } + } + + return 0; +} + const struct netdev_flow_api netdev_offload_dpdk = { .type = "dpdk_flow_api", .flow_put = netdev_offload_dpdk_flow_put, .flow_del = netdev_offload_dpdk_flow_del, .init_flow_api = netdev_offload_dpdk_init_flow_api, .flow_get = netdev_offload_dpdk_flow_get, + .hw_miss_packet_recover = netdev_offload_dpdk_hw_miss_packet_recover, };