On Sun, Jul 5, 2020 at 7:33 PM Eli Britstein <[email protected]> wrote:
>
>
> On 6/29/2020 12:50 PM, Sriharsha Basavapatna wrote:
> > In this patch we check if action processing (apart from OUTPUT action),
> > should be skipped for a given dp_netdev_flow. Specifically, we check if
> > the action is TNL_PUSH and if it has been offloaded to HW, then we do not
> > push the tunnel header in SW. The datapath only executes the OUTPUT action.
> > The packet will be encapsulated in HW during transmit.
> >
> > Signed-off-by: Sriharsha Basavapatna <[email protected]>
> > ---
> > lib/dpif-netdev.c | 46 ++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 36 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 7adea8c40..b96a75d1f 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -114,6 +114,7 @@ COVERAGE_DEFINE(datapath_drop_invalid_port);
> > COVERAGE_DEFINE(datapath_drop_invalid_bond);
> > COVERAGE_DEFINE(datapath_drop_invalid_tnl_port);
> > COVERAGE_DEFINE(datapath_drop_rx_invalid_packet);
> > +COVERAGE_DEFINE(datapath_skip_tunnel_push);
> >
> > /* Protects against changes to 'dp_netdevs'. */
> > static struct ovs_mutex dp_netdev_mutex = OVS_MUTEX_INITIALIZER;
> > @@ -545,6 +546,16 @@ struct dp_netdev_flow {
> > bool dead;
> > uint32_t mark; /* Unique flow mark assigned to a flow */
> >
> > + /* The next two members are used to support partial offloading of
> > + * actions. The boolean flag tells if this flow has its actions
> > partially
> > + * offloaded. The egress port# tells if the action should be offloaded
> > + * on the egress (output) port instead of the in-port for the flow.
> > Note
> > + * that we support flows with a single egress port action.
> > + * (see MAX_ACTION_ATTRS for related comments).
> > + */
> > + bool partial_actions_offloaded;
> > + odp_port_t egress_offload_port;
> > +
> > /* Statistics. */
> > struct dp_netdev_flow_stats stats;
> >
> > @@ -817,7 +828,8 @@ static void dp_netdev_execute_actions(struct
> > dp_netdev_pmd_thread *pmd,
> > bool should_steal,
> > const struct flow *flow,
> > const struct nlattr *actions,
> > - size_t actions_len);
> > + size_t actions_len,
> > + const struct dp_netdev_flow
> > *dp_flow);
> > static void dp_netdev_input(struct dp_netdev_pmd_thread *,
> > struct dp_packet_batch *, odp_port_t port_no);
> > static void dp_netdev_recirculate(struct dp_netdev_pmd_thread *,
> > @@ -3954,7 +3966,7 @@ dpif_netdev_execute(struct dpif *dpif, struct
> > dpif_execute *execute)
> > dp_packet_batch_init_packet(&pp, execute->packet);
> > pp.do_not_steal = true;
> > dp_netdev_execute_actions(pmd, &pp, false, execute->flow,
> > - execute->actions, execute->actions_len);
> > + execute->actions, execute->actions_len,
> > NULL);
> > dp_netdev_pmd_flush_output_packets(pmd, true);
> >
> > if (pmd->core_id == NON_PMD_CORE_ID) {
> > @@ -6672,7 +6684,7 @@ packet_batch_per_flow_execute(struct
> > packet_batch_per_flow *batch,
> > actions = dp_netdev_flow_get_actions(flow);
> >
> > dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow,
> > - actions->actions, actions->size);
> > + actions->actions, actions->size, flow);
> > }
> >
> > static inline void
> > @@ -6967,7 +6979,7 @@ handle_packet_upcall(struct dp_netdev_pmd_thread *pmd,
> > * we'll send the packet up twice. */
> > dp_packet_batch_init_packet(&b, packet);
> > dp_netdev_execute_actions(pmd, &b, true, &match.flow,
> > - actions->data, actions->size);
> > + actions->data, actions->size, NULL);
> >
> > add_actions = put_actions->size ? put_actions : actions;
> > if (OVS_LIKELY(error != ENOSPC)) {
> > @@ -7202,6 +7214,7 @@ dp_netdev_recirculate(struct dp_netdev_pmd_thread
> > *pmd,
> > struct dp_netdev_execute_aux {
> > struct dp_netdev_pmd_thread *pmd;
> > const struct flow *flow;
> > + const struct dp_netdev_flow *dp_flow; /* for partial action offload
> > */
> > };
> >
> > static void
> > @@ -7346,7 +7359,7 @@ dp_execute_userspace_action(struct
> > dp_netdev_pmd_thread *pmd,
> > if (!error || error == ENOSPC) {
> > dp_packet_batch_init_packet(&b, packet);
> > dp_netdev_execute_actions(pmd, &b, should_steal, flow,
> > - actions->data, actions->size);
> > + actions->data, actions->size, NULL);
> > } else if (should_steal) {
> > dp_packet_delete(packet);
> > COVERAGE_INC(datapath_drop_userspace_action_error);
> > @@ -7455,6 +7468,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> > *packets_,
> > int type = nl_attr_type(a);
> > struct tx_port *p;
> > uint32_t packet_count, packets_dropped;
> > + struct dp_netdev_flow *dp_flow = aux->dp_flow;
> >
> > switch ((enum ovs_action_attr)type) {
> > case OVS_ACTION_ATTR_OUTPUT:
> > @@ -7477,9 +7491,20 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> > *packets_,
> > }
> > dp_packet_batch_apply_cutlen(packets_);
> > packet_count = dp_packet_batch_size(packets_);
> > - if (push_tnl_action(pmd, a, packets_)) {
> > - COVERAGE_ADD(datapath_drop_tunnel_push_error,
> > - packet_count);
> > + /* Execute tnl_push action in SW, if it is not offloaded as a
> > partial
> > + * action in HW. Otherwise, HW pushes the tunnel header during
> > output
> > + * processing. There's a small window here in which the offload
> > thread
> > + * offloads the flow, but the partial_actions_offloaded flag is
> > still
> > + * not set. In this case, as the packet is already encapsulated, it
> > + * wouldn't match the offloaded flow and the action won't be
> > executed
> > + * in HW.
> > + */
> > + if (!dp_flow || !dp_flow->partial_actions_offloaded) {
> > + if (push_tnl_action(pmd, a, packets_)) {
> > + COVERAGE_ADD(datapath_drop_tunnel_push_error,
> > packet_count);
> > + }
> > + } else {
>
> It is possible (though rare) that encap headers will match the original
> flow. In such case, in the time before the SW knows the HW rule is
> installed, the packet will be encapsulated by SW, and then again in HW.
>
> The rule is for example:
>
> set vxlan ip-version ipv4|ipv6 vni <vni> udp-src <udp-src> udp-dst
> <udp-dst> ip-src <ip-src> ip-dst <ip-dst> eth-src e4:11:22:33:44:50
> eth-dst b6:7a:b2:f1:d5:e4
>
> flow create 0 egress priority 0 group 0 transfer pattern eth src is
> e4:11:22:33:44:50 dst is b6:7a:b2:f1:d5:e4 type is 0x0800 / ipv4 ... end
> / actions vxlan_encap / end
>
> the original packet is:
>
> Ether(src=e4:11:22:33:44:50,dst=b6:7a:b2:f1:d5:e4)/IP()...
>
> after SW encap:
>
> Ether(src=e4:11:22:33:44:50,dst=b6:7a:b2:f1:d5:e4)/IP()/UDP()/VXLAN()/Ether(src=e4:11:22:33:44:50,dst=b6:7a:b2:f1:d5:e4)/IP()...
>
> It will hit the egress flow, and be encapsulated again.
>
> Need to add a validation that the encap header doesn't match the flow's
> matches, and reject the flow if it does.
I already responded to this in my email dated Jun-29-2020. I'm adding
it here for reference:
"I tested this configuration with a Broadcom-NIC. There is no issue
and the reason being the same as mentioned in the above comments. That
is, even with the same inner/outer header (packet already encapsulated
in SW), it doesn't match the offloaded rule and the packet gets
transmitted properly (no double-encap).".
Thanks,
-Harsha
>
> > + COVERAGE_ADD(datapath_skip_tunnel_push, packet_count);
> > }
> > return;
> >
> > @@ -7771,9 +7796,10 @@ static void
> > dp_netdev_execute_actions(struct dp_netdev_pmd_thread *pmd,
> > struct dp_packet_batch *packets,
> > bool should_steal, const struct flow *flow,
> > - const struct nlattr *actions, size_t actions_len)
> > + const struct nlattr *actions, size_t actions_len,
> > + const struct dp_netdev_flow *dp_flow)
> > {
> > - struct dp_netdev_execute_aux aux = { pmd, flow };
> > + struct dp_netdev_execute_aux aux = { pmd, flow, dp_flow };
> >
> > odp_execute_actions(&aux, packets, should_steal, actions,
> > actions_len, dp_execute_cb);
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
