Looking at this patch, I found several issues that should be addressed.
git sha: 345a032ee4471d896b162abfb85f51a12668f253
Author: Eelco Chaudron <[email protected]>
One line subject: dpif-offload: Add port registration and management APIs.
This patch introduces a common port management layer for offload
providers and integrates it into the dpif-offload subsystem. Existing
dummy offload provider is updated to use the new APIs.
> diff --git a/lib/dpif-offload-dpdk.c b/lib/dpif-offload-dpdk.c
> index 6ac4af8d8..76e0ba15f 100644
> --- a/lib/dpif-offload-dpdk.c
> +++ b/lib/dpif-offload-dpdk.c
> @@ -18,6 +18,8 @@
>
> #include "dpif-offload.h"
> #include "dpif-offload-provider.h"
> +#include "netdev-provider.h"
> +#include "netdev-vport.h"
> #include "util.h"
>
> #include "openvswitch/vlog.h"
> @@ -100,6 +102,20 @@ dpif_offload_dpdk_set_config(struct dpif_offload
> *offload_,
> }
> }
>
> +static bool
> +dpif_offload_dpdk_can_offload(struct dpif_offload *offload OVS_UNUSED,
> + struct netdev *netdev)
> +{
> + if (netdev_vport_is_vport_class(netdev->netdev_class)
> + && strcmp(netdev_get_dpif_type(netdev), "netdev")) {
Is there a potential NULL dereference here? Can netdev_get_dpif_type
return NULL in dpif_offload_dpdk_can_offload?
> diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c
> index c09530daa..2c9081438 100644
> --- a/lib/dpif-offload-tc.c
> +++ b/lib/dpif-offload-tc.c
> @@ -80,6 +86,19 @@ dpif_offload_tc_set_config(struct dpif_offload *offload,
> }
> }
>
> +static bool
> +dpif_offload_tc_can_offload(struct dpif_offload *dpif_offload OVS_UNUSED,
> + struct netdev *netdev)
> +{
> + if (netdev_vport_is_vport_class(netdev->netdev_class) &&
> + strcmp(netdev_get_dpif_type(netdev), "system")) {
Similar potential issue here - can netdev_get_dpif_type return NULL in
dpif_offload_tc_can_offload?
> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
> index 874e4db3f..f48af4e2d 100644
> --- a/lib/dpif-offload.c
> +++ b/lib/dpif-offload.c
> @@ -167,7 +167,7 @@ dpif_offload_attach_provider_to_dp_offload__(struct
> dp_offload *dp_offload,
> LIST_FOR_EACH (offload_entry, dpif_list_node, providers_list) {
> if (offload_entry == offload || !strcmp(offload->name,
> offload_entry->name)) {
> - return EEXIST;
> + return -EEXIST;
> }
> }
Why is this error code being negated in
dpif_offload_attach_provider_to_dp_offload__? The surrounding code seems
to expect positive errno values.
> @@ -192,7 +194,7 @@ dpif_offload_attach_dp_offload(struct dpif *dpif,
> {
> ovsrcu_set(&dpif->dp_offload, dp_offload);
> ovs_refcount_ref(&dp_offload->ref_cnt);
> - return 0;
> + return EEXIST;
> }
This change looks suspicious - why is dpif_offload_attach_dp_offload now
returning EEXIST unconditionally instead of success?
> +bool
> +dpif_offload_port_mgr_add(struct dpif_offload_port_mgr *mgr,
> + struct dpif_offload_port_mgr_port *port,
> + struct netdev *netdev, odp_port_t port_no,
> + bool need_ifindex)
> +{
> + ovs_assert(netdev);
> +
> + memset(port, 0, sizeof *port);
> + port->netdev = netdev_ref(netdev);
> + port->port_no = port_no;
> + port->ifindex = need_ifindex ? netdev_get_ifindex(netdev) : -1;
> +
> + ovs_mutex_lock(&mgr->cmap_mod_lock);
> +
> + if (dpif_offload_port_mgr_find_by_odp_port(mgr, port_no)
> + || dpif_offload_port_mgr_find_by_ifindex(mgr, port->ifindex)
> + || dpif_offload_port_mgr_find_by_netdev(mgr, port->netdev)) {
> +
> + ovs_mutex_unlock(&mgr->cmap_mod_lock);
> + return false;
> + }
Does this code leak the netdev reference in dpif_offload_port_mgr_add
when the insertion fails? The netdev_ref is called before the duplicate
check, but there's no matching netdev_close on the error path.
> diff --git a/lib/dpif.c b/lib/dpif.c
> index a173d643a..0afc7b721 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -365,7 +379,9 @@ do_open(const char *name, const char *type, bool create,
> struct dpif **dpifp)
> - dpif_offload_attach_providers(dpif);
> + new_offload_provider = dpif_offload_attach_providers(dpif) == EEXIST
> + ? false : true;
The logic here seems backwards given the change to
dpif_offload_attach_dp_offload. If that function now always returns
EEXIST on success, wouldn't this make new_offload_provider always false?
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index ff5a75a36..fbd60d377 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -6516,7 +6516,7 @@ out:
> }
>
> bool
> -netdev_dpdk_flow_api_supported(struct netdev *netdev)
> +netdev_dpdk_flow_api_supported(struct netdev *netdev, bool check_only)
> {
> struct netdev_dpdk *dev;
> bool ret = false;
> @@ -6535,7 +6535,7 @@ netdev_dpdk_flow_api_supported(struct netdev *netdev)
> dev = netdev_dpdk_cast(netdev);
> ovs_mutex_lock(&dev->mutex);
> if (dev->type == DPDK_DEV_ETH) {
> - if (dev->requested_rx_steer_flags) {
> + if (dev->requested_rx_steer_flags && !check_only) {
[ ... ]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
