git sha: 06698787215d0b91c702a700e57241cf1ff096a2
Author: Eelco Chaudron <[email protected]>
Subject: dpif-offload: Add flow operations to dpif-offload-tc.
This commit moves flow put/del/get APIs from netdev-offload layer to
dpif-offload-tc layer, removing the dependency on netdev-offload
infrastructure while still maintaining it for netdev initialization.
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index e161930be..165f76ace 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2216,7 +2216,7 @@ out:
> * Change flags to create the flow in kernel */
> put->flags &= ~DPIF_FP_MODIFY;
> put->flags |= DPIF_FP_CREATE;
> - } else if (del_err != ENOENT) {
> + } else if (del_err != ENOENT && del_err != EOPNOTSUPP) {
> VLOG_ERR_RL(&rl, "failed to delete offloaded flow: %s",
> ovs_strerror(del_err));
> /* stop proccesing the flow in kernel */
This error handling change affects when error messages are logged. Does
this code now silently ignore EOPNOTSUPP deletion failures that should
be reported?
> diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c
> index c343cd9c1..d8fae7ed6 100644
> --- a/lib/dpif-offload-tc.c
> +++ b/lib/dpif-offload-tc.c
[ ... ]
> +static int
> +dpif_offload_tc_parse_flow_put(struct dpif_offload_tc *offload_tc,
> + struct dpif *dpif, struct dpif_flow_put *put)
> +{
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> + struct dpif_offload_port_mgr_port *port;
> + const struct nlattr *nla;
> + struct offload_info info;
> + struct match match;
> + odp_port_t in_port;
> + size_t left;
> + int err;
> +
> + info.tc_modify_flow_deleted = false;
> + info.tc_modify_flow = false;
Is it safe to leave other fields in info uninitialized? Does this code
introduce potential use of uninitialized fields like flow_mark or
orig_in_port?
> +
> + if (put->flags & DPIF_FP_PROBE) {
> + return EOPNOTSUPP;
> + }
> +
> + err = parse_key_and_mask_to_match(put->key, put->key_len, put->mask,
> + put->mask_len, &match);
> + if (err) {
> + return err;
> + }
> +
> + in_port = match.flow.in_port.odp_port;
> + port = dpif_offload_port_mgr_find_by_odp_port(offload_tc->port_mgr,
> + in_port);
> + if (!port) {
> + return EOPNOTSUPP;
> + }
> +
> + /* Check the output port for a tunnel. */
> + NL_ATTR_FOR_EACH (nla, left, put->actions, put->actions_len) {
> + if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
> + struct dpif_offload_port_mgr_port *mgr_port;
> + odp_port_t out_port;
> +
> + out_port = nl_attr_get_odp_port(nla);
> + mgr_port = dpif_offload_port_mgr_find_by_odp_port(
> + offload_tc->port_mgr, out_port);
> +
> + if (!mgr_port) {
> + err = EOPNOTSUPP;
> + goto out;
> + }
> + }
> + }
This loop only handles the first OUTPUT action it encounters due to the
goto on error. If there are multiple OUTPUT actions in the same flow,
does this code incorrectly fail on the second one even if both ports
exist?
[ ... ]
> +out:
> + if (err && err != EEXIST && (put->flags & DPIF_FP_MODIFY)) {
> + /* Modified rule can't be offloaded, try and delete from HW. */
> + int del_err = 0;
> +
> + if (!info.tc_modify_flow_deleted) {
> + del_err = netdev_offload_tc_flow_del(put->ufid, put->stats);
> + }
> +
> + if (!del_err) {
> + /* Delete from hw success, so old flow was offloaded.
> + * Change flags to create the flow at the dpif level. */
> + put->flags &= ~DPIF_FP_MODIFY;
> + put->flags |= DPIF_FP_CREATE;
> + } else if (del_err != ENOENT) {
> + VLOG_ERR_RL(&rl, "failed to delete offloaded flow: %s",
> + ovs_strerror(del_err));
> + /* Stop processing the flow in kernel. */
> + err = 0;
> + }
> + }
The error handling logic here seems duplicated with dpif-netlink.c.
Does this create conflicting error handling paths when both layers try
to manage the same flow modification failures?
> +static int
> +dpif_offload_tc_parse_flow_get(struct dpif_offload_tc *offload_tc,
> + struct dpif_flow_get *get)
> +{
> + struct dpif_offload_port_mgr_port *port;
> + struct dpif_flow *dpif_flow = get->flow;
> + struct odputil_keybuf maskbuf;
> + struct odputil_keybuf keybuf;
> + struct odputil_keybuf actbuf;
> + struct ofpbuf key, mask, act;
> + struct dpif_flow_stats stats;
> + struct dpif_flow_attrs attrs;
> + uint64_t act_buf[1024 / 8];
> + struct nlattr *actions;
> + struct match match;
> + struct ofpbuf buf;
> + int err = ENOENT;
> +
> + ofpbuf_use_stack(&buf, &act_buf, sizeof act_buf);
> +
> + DPIF_OFFLOAD_PORT_MGR_PORT_FOR_EACH (port, offload_tc->port_mgr) {
> + if (!netdev_offload_tc_flow_get(port->netdev, &match, &actions,
> + get->ufid, &stats, &attrs, &buf)) {
> + err = 0;
> + break;
> + }
> + }
This loop searches all ports to find the flow. Does this scale poorly
with large numbers of ports, and could it return incorrect results if
the same UFID exists on multiple ports?
[ ... ]
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 0f9d97d6d..8c8c72a65 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
[ ... ]
> +int
> +netdev_offload_tc_flow_put(struct netdev *netdev, struct match *match,
> + struct nlattr *actions, size_t actions_len,
> + const ovs_u128 *ufid, struct offload_info *info,
> + struct dpif_flow_stats *stats)
> {
[ ... ]
> @@ -2672,6 +2672,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> if (get_ufid_tc_mapping(ufid, &id) == 0) {
> VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d",
> id.handle, id.prio);
> + info->tc_modify_flow = true;
> info->tc_modify_flow_deleted = !del_filter_and_ufid_mapping(
> &id, ufid, &adjust_stats);
> }
With this change, tc_modify_flow is only set true inside this if block.
Can the flag remain false for legitimate flow modifications when
get_ufid_tc_mapping fails?
> +int
> +netdev_offload_tc_flow_del(const ovs_u128 *ufid,
> + struct dpif_flow_stats *stats)
> {
Does this function signature change break compatibility since it no longer
takes a netdev parameter? Could this cause issues with flows that need
device-specific cleanup?
> @@ -3464,8 +3464,5 @@ dpif_offload_tc_meter_del(const struct dpif_offload
> *offload OVS_UNUSED,
>
> const struct netdev_flow_api netdev_offload_tc = {
> .type = "linux_tc",
> - .flow_put = netdev_tc_flow_put,
> - .flow_get = netdev_tc_flow_get,
> - .flow_del = netdev_tc_flow_del,
> .init_flow_api = netdev_tc_init_flow_api,
> };
Removing these function pointers from the netdev_flow_api structure
while keeping them as global functions seems to create an inconsistent
API. Does this break any existing callers that expect these operations
to be available through the netdev layer?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
