I need to analyze this patch for potential regressions. Let me examine the commit details and code changes.
Git SHA: 5df37b695c1ace89e51ddd2d4440a733e649799d.1762950453.git.echau...@redhat.com Author: Eelco Chaudron <[email protected]> Subject: [PATCH 21/41] dpif-offload: Add API to find offload implementation type. This commit introduces per-offload-provider implementation types and removes the synced_dp abstraction from the dpif level. The new API allows querying the offload implementation type and which provider offloads a given port. Looking at the unified diff, I'll analyze the code for potential issues: > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index fe9c8b7be..6575cb9e8 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -9958,7 +9958,6 @@ dpif_netdev_bond_stats_get(struct dpif *dpif, uint32_t > bond_id, > const struct dpif_class dpif_netdev_class = { > "netdev", > true, /* cleanup_required */ > - true, /* synced_dp_layers */ > dpif_netdev_init, > dpif_netdev_enumerate, > dpif_netdev_port_open_type, > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 8a9acc904..04d7c8ed6 100644 > --- a/lib/dpif-netlink.c > +++ b/lib/dpif-netlink.c > @@ -4363,7 +4363,6 @@ dpif_netlink_cache_set_size(struct dpif *dpif_, > uint32_t level, uint32_t size) > const struct dpif_class dpif_netlink_class = { > "system", > false, /* cleanup_required */ > - false, /* synced_dp_layers */ > NULL, /* init */ > dpif_netlink_enumerate, > NULL, [ ... ] > diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c > index 77229c861..62eef398b 100644 > --- a/lib/dpif-offload.c > +++ b/lib/dpif-offload.c > @@ -561,6 +561,26 @@ dpif_offload_port_set_config(struct dpif *dpif, > odp_port_t port_no, > } > } > > +struct dpif_offload * > +dpif_offload_port_offloaded_by(const struct dpif *dpif, odp_port_t port_no) > +{ > + struct dp_offload *dp_offload = dpif_offload_get_dp_offload(dpif); > + struct dpif_offload *offload, *offload_return = NULL; > + > + if (!dp_offload || !dpif_offload_is_offload_enabled()) { > + return NULL; > + } > + > + LIST_FOR_EACH (offload, dpif_list_node, &dp_offload->offload_providers) { > + if (offload->class->get_netdev(offload, port_no)) { > + offload_return = offload; > + break; > + } > + } > + > + return offload_return; > +} Can this code handle NULL return values from get_netdev() correctly? The function checks the boolean result but doesn't verify if get_netdev can crash on NULL class pointers. > +enum dpif_offload_impl_type > +dpif_offload_get_impl_type_by_class(const char *type) > +{ > + enum dpif_offload_impl_type impl_type = DPIF_OFFLOAD_IMPL_NONE; > + struct shash_node *node; > + > + ovs_mutex_lock(&dpif_offload_mutex); > + SHASH_FOR_EACH (node, &dpif_offload_classes) { > + const struct dpif_offload_class *class = node->data; > + > + if (!strcmp(type, class->type)) { > + impl_type = class->impl_type; > + break; > + } > + } > + ovs_mutex_unlock(&dpif_offload_mutex); > + > + return impl_type; > +} Can this function handle a NULL type parameter? The strcmp call could crash if type is NULL. > diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c > index d0c537bc9..b324793f0 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -826,12 +826,8 @@ udpif_get_n_flows(struct udpif *udpif) > atomic_store_relaxed(&udpif->n_flows_timestamp, now); > dpif_get_dp_stats(udpif->dpif, &stats); > flow_count = stats.n_flows; > - > - if (!dpif_synced_dp_layers(udpif->dpif)) { > - /* If the dpif layer does not sync the flows, we need to include > - * the hardware offloaded flows separately. */ > - flow_count += dpif_offload_flow_get_n_offloaded(udpif->dpif); > - } > + flow_count += dpif_offload_flow_get_n_offloaded_by_impl( > + udpif->dpif, DPIF_OFFLOAD_IMPL_HW_ONLY); Will this change in flow counting behavior preserve backward compatibility? The old code conditionally added all offloaded flows based on the synced_dp_layers check, but now it only counts DPIF_OFFLOAD_IMPL_HW_ONLY flows unconditionally. > +static bool > +did_dp_hw_only_offload_change(const char *old_type, const char *new_type) > +{ > + enum dpif_offload_impl_type old_impl = old_type ? > + dpif_offload_get_impl_type_by_class(old_type) : > DPIF_OFFLOAD_IMPL_NONE; > + enum dpif_offload_impl_type new_impl = new_type ? > + dpif_offload_get_impl_type_by_class(new_type) : > DPIF_OFFLOAD_IMPL_NONE; > + > + if (old_impl != new_impl && (old_impl == DPIF_OFFLOAD_IMPL_HW_ONLY || > + new_impl == DPIF_OFFLOAD_IMPL_HW_ONLY)) { > + return true; > + } > + return false; > +} The conditional logic structure here looks correct, but does the function need to handle error cases from dpif_offload_get_impl_type_by_class()? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
