Git SHA: 58b95eb1ad31ab0f292d86bc5df2dda0dada25ef
Author: Eelco Chaudron <[email protected]>
Subject: dpif-offload-tc: Remove netdev-offload dependency.
This commit removes netdev-offload dependencies from netdev-offload-tc
and places it under full control of the dpif-netdev-tc layer. It refactors
the offload_info structure into tc_offload_info and updates function
signatures to use dpif parameters instead of netdev-offload APIs.
> diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c
> index d8fae7ed6..259e87029 100644
> --- a/lib/dpif-offload-tc.c
> +++ b/lib/dpif-offload-tc.c
> @@ -78,7 +78,14 @@ static int
> dpif_offload_tc_enable_offload(struct dpif_offload *dpif_offload,
> struct dpif_offload_port_mgr_port *port)
> {
> + int ret = netdev_offload_tc_init(port->netdev);
> + if (ret) {
> + VLOG_WARN("%s: Failed assigning flow API 'tc', error %d",
> + netdev_get_name(port->netdev), ret);
> + return ret;
> + }
> dpif_offload_set_netdev_offload(port->netdev, dpif_offload);
> + VLOG_INFO("%s: Assigned flow API 'tc'", netdev_get_name(port->netdev));
> return 0;
> }
[ ... ]
> @@ -645,7 +652,7 @@ dpif_offload_tc_parse_flow_put(struct dpif_offload_tc
> *offload_tc,
>
> info.recirc_id_shared_with_tc = offload_tc->recirc_id_shared;
>
> - err = netdev_offload_tc_flow_put(port->netdev, &match,
> + err = netdev_offload_tc_flow_put(dpif, port->netdev, &match,
Could this function call get a NULL dpif parameter? The code adds dpif as
the first parameter but there's no null check before passing it through.
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 8c8c72a65..e27564f04 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
[ ... ]
> @@ -2190,12 +2190,14 @@ netdev_tc_parse_nl_actions(struct dpif *dpif, struct
> netdev *netdev,
>
> if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
> odp_port_t port = nl_attr_get_odp_port(nla);
> - struct netdev *outdev = netdev_ports_get(
> - port, netdev_get_dpif_type(netdev));
> -
> + struct netdev *outdev = dpif_offload_get_netdev_by_port_id(dpif,
> + NULL,
> + port);
> if (!outdev) {
> - VLOG_DBG_RL(&rl, "Can't find netdev for output port %d",
> port);
> - return ENODEV;
> + VLOG_DBG_RL(&rl,
> + "Can't find offloaded netdev for output port %d",
> + port);
> + return EOPNOTSUPP;
> }
Is this error handling change intentional from ENODEV to EOPNOTSUPP? This
changes the errno semantics and might affect callers that depend on ENODEV.
[ ... ]
> if (!dpif_offload_netdev_same_offload(netdev, outdev)) {
> VLOG_DBG_RL(&rl,
> "Flow API provider mismatch between ingress (%s)
> "
> "and egress (%s) ports",
> netdev_get_name(netdev),
> netdev_get_name(outdev));
> - netdev_close(outdev);
> return EOPNOTSUPP;
> }
Does the outdev returned by dpif_offload_get_netdev_by_port_id need to be
freed or closed? The original code called netdev_close(outdev) in error
paths and after use.
[ ... ]
> action->out.ifindex_out = netdev_get_ifindex(outdev);
> if (action->out.ifindex_out < 0) {
> VLOG_DBG_RL(&rl,
> "Can't find ifindex for output port %s, error
> %d",
> netdev_get_name(outdev),
> action->out.ifindex_out);
> - netdev_close(outdev);
> return -action->out.ifindex_out;
> }
>
> action->out.ingress = is_internal_port(netdev_get_type(outdev));
> action->type = TC_ACT_OUTPUT;
> flower->action_count++;
> - netdev_close(outdev);
Same question here - the removal of netdev_close(outdev) might create a
resource leak if the new API requires explicit cleanup.
> diff --git a/lib/netdev-offload-tc.h b/lib/netdev-offload-tc.h
> index 1b5f33524..e6798b70e 100644
> --- a/lib/netdev-offload-tc.h
> +++ b/lib/netdev-offload-tc.h
> @@ -29,8 +29,19 @@ struct netdev_tc_flow_dump {
> bool terse;
> };
>
> +/* Flow offloading. */
> +struct tc_offload_info {
> + bool recirc_id_shared_with_tc; /* Indicates whether tc chains will be in
> + * sync with datapath recirc ids. */
> +
> + bool tc_modify_flow; /* Indicates tc modified the flow. */
> + bool tc_modify_flow_deleted; /* Indicate the tc modify flow put success
> + * to delete the original flow. */
> +};
This struct duplicates some fields from the original offload_info. Are
there any initialization or compatibility concerns when converting between
these structures?
> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
> index 0dfebae1b..0882214a9 100644
> --- a/lib/netdev-offload.h
> +++ b/lib/netdev-offload.h
[ ... ]
> /* Flow offloading. */
> struct offload_info {
> - bool recirc_id_shared_with_tc; /* Indicates whever tc chains will be in
> - * sync with datapath recirc ids. */
> -
> /*
> * The flow mark id assigned to the flow. If any pkts hit the flow,
> * it will be in the pkt meta data.
> */
> uint32_t flow_mark;
>
> - bool tc_modify_flow; /* Indicates tc modified the flow. */
> - 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. */
> };
With these fields moved to tc_offload_info, are there any existing users
of struct offload_info that might be affected by these missing fields?
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 13f3d707e..6a4218553 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -154,8 +154,6 @@ netdev_initialize(void)
> netdev_register_provider(&netdev_internal_class);
> netdev_register_provider(&netdev_tap_class);
> netdev_vport_tunnel_register();
> -
> - netdev_register_flow_api_provider(&netdev_offload_tc);
Removing this registration might affect systems that depend on the tc flow
API being automatically available. Are there any fallback mechanisms or
compatibility considerations?
> @@ -2448,3 +2446,13 @@ netdev_free_custom_stats_counters(struct
> netdev_custom_stats *custom_stats)
> }
> }
> }
> +
> +uint32_t
> +netdev_get_block_id(struct netdev *netdev)
> +{
> + const struct netdev_class *class = netdev->netdev_class;
> +
> + return (class->get_block_id
> + ? class->get_block_id(netdev)
> + : 0);
> +}
Why was this function moved from netdev-offload.c to netdev.c? Does this
indicate a change in the API layering or dependency structure?
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
