On 29 Sep 2026, at 10:34, David Marchand wrote:
> Split the monolithic struct netdev_dpdk into three structures:
> - netdev_dpdk_common: fields shared between eth and vhost
> - netdev_dpdk_eth: ethernet specific fields
> - netdev_dpdk_vhost: vhost specific fields
>
> Each type specific struct embeds netdev_dpdk_common. The mutex and
> list_node move to the per-type structs since they are only relevant
> to one type.
>
> Signed-off-by: David Marchand <[email protected]>
Hi David,
See some comments below, mainly on padded cache line usage (waste).
But in general the patch looks good to me.
//Eelco
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
[...]
> +struct netdev_dpdk_eth {
> + PADDED_MEMBERS(CACHE_LINE_SIZE,
> dpdk_port_t port_id;
Wrapping a single dpdk_port_t in PADDED_MEMBERS creates
62 bytes of padding. Could port_id move down into the
non-padded section of the struct?
> + );
> +
> + struct netdev_dpdk_common common;
> +
> + PADDED_MEMBERS(CACHE_LINE_SIZE,
> + struct ovs_mutex mutex OVS_ACQ_AFTER(dpdk_eth_mutex);
> + /* In dpdk_eth_list. */
> + struct ovs_list list_node OVS_GUARDED_BY(dpdk_eth_mutex);
The mutex and list_node are not on the data path. Is
the PADDED_MEMBERS still needed here?
> );
[...]
> +struct netdev_dpdk_vhost {
> PADDED_MEMBERS(CACHE_LINE_SIZE,
Same question here. Only vid seems to be on the fast
path.
> + /* virtio identifier for vhost devices */
> + ovsrcu_index vid;
> +
> + /* True if vHost device is 'up' and has been reconfigured at least
> + * once. */
> + bool vhost_reconfigured;
> +
> + atomic_uint8_t vhost_tx_retries_max;
> );
> +
> + struct netdev_dpdk_common common;
> +
> + PADDED_MEMBERS(CACHE_LINE_SIZE,
> + struct ovs_mutex mutex OVS_ACQ_AFTER(dpdk_vhost_mutex);
> + /* In dpdk_vhost_list. */
> + struct ovs_list list_node OVS_GUARDED_BY(dpdk_vhost_mutex);
See comment in the ethernet variant.
> );
[...]
> -static void netdev_dpdk_configure_xstats(struct netdev_dpdk *dev);
> -static void netdev_dpdk_clear_xstats(struct netdev_dpdk *dev);
> +static void netdev_dpdk_configure_xstats(struct netdev_dpdk_eth *dev);
> +static void netdev_dpdk_clear_xstats(struct netdev_dpdk_eth *dev);
Per the coding style, the parameter name can be omitted
here where the type makes the purpose clear.
[...]
> + struct netdev_dpdk_vhost *dev = netdev_dpdk_vhost_cast(netdev);
> + struct netdev_dpdk_common *common = &dev->common;
This changes the reverse christmas tree layout for the
local declarations.
> int socket_id = rte_lcore_to_socket_id(rte_get_main_lcore());
[...]
> netdev_dpdk_eth_set_etheraddr(struct netdev *netdev, const struct eth_addr
> mac)
> {
nit: the vhost variants of set_etheraddr and set_policing
obtain common via netdev_dpdk_common_cast(netdev) while
the eth variants use &dev->common. Could these be made
consistent?
> - struct netdev_dpdk_common *common = netdev_dpdk_common_cast(netdev);
> - struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> + struct netdev_dpdk_eth *dev = netdev_dpdk_eth_cast(netdev);
> + struct netdev_dpdk_common *common = &dev->common;
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
