On 11/01/2017 00:34, Joe Stringer wrote:
On 10 January 2017 at 07:50, Paul Blakey <[email protected]> wrote:
+ nl_msg_put_u32(&buf, OVS_ACTION_ATTR_OUTPUT, ifindex_out);
+ if (tnl_cfg && tnl_cfg->dst_port != 0) {
+ nl_msg_put_u32(&buf, OVS_TUNNEL_KEY_ATTR_TP_DST,
tnl_cfg->dst_port);
+ }
+ nl_msg_end_nested(&buf, out_off);
+
+ if (outdev) {
+ netdev_close(outdev);
+ }
+ } else {
+ nl_msg_put_unspec(&buf, nl_attr_type(nla), nl_attr_get(nla),
+ nl_attr_get_size(nla));
+ }
+ }
+ nl_msg_end_nested(&buf, offset);
Hmm. I'm a bit uneasy about this whole copying/rewriting of the
actions. Firstly, the tunnel stuff just looks wrong. A quick "git
grep" shows that OVS_TUNNEL_KEY_ATTR_TP_DST is using the wrong type,
and looking closer at the other places it is used, it looks like this
function is shortcutting a whole bunch of the tunnel config.
Besides the wrong type, I'm not sure what you mean, dpif_netlink_port_add__
seems to access the tunnel dst port the same way.
Yes, data type, bitwise differences compared to
dpif_netlink_port_add__(). Checking the ifindex.
Does tnl_cfg->exts need to be checked too? Are any of the other
'struct netdev_tunnel_config' fields relevant for output (or will
ignoring them potentially lead to a correctness issue)?
I recognise that the ODP port number needs to be translated into an
ifindex. Do we need to do anything else?
tunnel dst port for set action was missing in ACTION_SET , TUNNEL pair.
We needed that for offloading.
Broadly I was thinking that any and all fields that may be relevant to
tunnel outputting should be handled and checked - fail out if it can't
be interpreted or implemented on lower layers. Looking at 'enum
ovs_tunnel_key_attr', there's a whole bunch of fields which are
similar to OVS_TUNNEL_KEY_ATTR_TP_DST and it wasn't immediately
obvious to me why this function only serialises this one member of the
enum and doesn't need to handle any of the rest.
If I follow correctly, the answer is that for traditional OVS vports,
we configure the vport in kernel to include TP_DST (and a few other
things), then omit these from the flow. When the datapath performs
output action, it will fetch this information from the vport in
kernel. However, for the TC hardware offloads case there is no such
port configuration in kernel, so now we need to ensure that it is
encoded in the flow. Is that right?
Right (though not sure whats the reason).
+
+ if (outputs > 1) {
+ return false;
+ }
+
+ act = ofpbuf_at_assert(&buf, offset, sizeof(struct nlattr));
+ dev = netdev_hmap_port_get(in_port, dpif->dpif.dpif_class);
+ err = netdev_flow_put(dev, &match, CONST_CAST(struct nlattr *,
+ nl_attr_get(act)),
+ nl_attr_get_size(act), put->stats,
+ CONST_CAST(ovs_u128 *, put->ufid));
+ netdev_close(dev);
+
+ if (!err) {
+ if (put->flags & DPIF_FP_MODIFY) {
+ struct dpif_op *opp;
+ struct dpif_op op;
+
+ op.type = DPIF_OP_FLOW_DEL;
+ op.u.flow_del.key = put->key;
+ op.u.flow_del.key_len = put->key_len;
+ op.u.flow_del.ufid = put->ufid;
+ op.u.flow_del.pmd_id = put->pmd_id;
+ op.u.flow_del.stats = NULL;
+ op.u.flow_del.terse = false;
+
+ opp = &op;
+ dpif_netlink_operate__(dpif, &opp, 1);
Won't this just delete the flow that was just added?
The above FLOW_DEL calls the original operate (without offload) to delete it
if it from netlink kernel datapath. This will happen if we got a modify
request and we now
can offload the flow and previously couldn't (so its in kernel datapath).
Ah, I missed that it was the "dpif_netlink_operate__" variation. OK.
Is there a modify at the tc layer?
Yes there is, and implemented by given a already used handle.
So if we find the ufid in the map (ufid to handle/prio) already, we modify
it by using
the same handle.
OK, this seems fine at a glance.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
