On 7/3/25 9:45 AM, David Marchand wrote:
> As OVS does not know of the 25G speed, matching on a list of known speed
> for deducing full duplex capability is broken.
>
> Add a new netdev op to retrieve the duplex status for the existing netdev
> implementations.
>
> Reported-at: https://issues.redhat.com/browse/FDP-883
> Signed-off-by: David Marchand <david.march...@redhat.com>
> ---
> include/openvswitch/netdev.h | 2 +-
> lib/netdev-bsd.c | 31 ++++++++++++++++++++++++++-----
> lib/netdev-dpdk.c | 20 ++++++++++++++++++++
> lib/netdev-linux-private.h | 1 +
> lib/netdev-linux.c | 35 +++++++++++++++++++++++++++++++++++
> lib/netdev-provider.h | 7 +++++++
> lib/netdev.c | 30 +++++++++++++++++++++++-------
> ofproto/ofproto-dpif-sflow.c | 7 +++----
> tests/system-interface.at | 12 ++++++++++++
> vswitchd/bridge.c | 8 +++-----
> 10 files changed, 131 insertions(+), 22 deletions(-)
>
> diff --git a/include/openvswitch/netdev.h b/include/openvswitch/netdev.h
> index 83e8633dda..f79bfd1cdd 100644
> --- a/include/openvswitch/netdev.h
> +++ b/include/openvswitch/netdev.h
> @@ -135,7 +135,7 @@ int netdev_get_features(const struct netdev *,
> int netdev_get_speed(const struct netdev *, uint32_t *current, uint32_t
> *max);
> uint64_t netdev_features_to_bps(enum netdev_features features,
> uint64_t default_bps);
> -bool netdev_features_is_full_duplex(enum netdev_features features);
> +int netdev_get_duplex(const struct netdev *, bool *);
May want to give a name to the boolean, it's not clear what it means exactly.
> int netdev_set_advertisements(struct netdev *, enum netdev_features
> advertise);
> void netdev_features_format(struct ds *, enum netdev_features);
>
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index c7cd59376a..26e1286ac0 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -92,6 +92,7 @@ struct netdev_bsd {
> struct eth_addr etheraddr;
> int mtu;
> int carrier;
> + bool full_duplex; /* Cached value, see netdev_bsd_get_features(). */
It's not really cached, we're calling the netdev_bsd_get_features()
every time.
>
> int tap_fd; /* TAP character device, if any, otherwise -1. */
>
> @@ -1107,10 +1108,11 @@ netdev_bsd_parse_media(int media)
> * passed-in values are set to 0.
> */
> static int
> -netdev_bsd_get_features(const struct netdev *netdev,
> +netdev_bsd_get_features(const struct netdev *netdev_,
> enum netdev_features *current, uint32_t *advertised,
> enum netdev_features *supported, uint32_t *peer)
> {
> + struct netdev_bsd *netdev = netdev_bsd_cast(netdev_);
> struct ifmediareq ifmr;
> int *media_list;
> int i;
> @@ -1120,7 +1122,7 @@ netdev_bsd_get_features(const struct netdev *netdev,
> /* XXX Look into SIOCGIFCAP instead of SIOCGIFMEDIA */
>
> memset(&ifmr, 0, sizeof(ifmr));
> - ovs_strlcpy(ifmr.ifm_name, netdev_get_name(netdev), sizeof
> ifmr.ifm_name);
> + ovs_strlcpy(ifmr.ifm_name, netdev_get_name(netdev_), sizeof
> ifmr.ifm_name);
>
> /* We make two SIOCGIFMEDIA ioctl calls. The first to determine the
> * number of supported modes, and a second with a buffer to retrieve
> @@ -1128,7 +1130,7 @@ netdev_bsd_get_features(const struct netdev *netdev,
> error = af_inet_ioctl(SIOCGIFMEDIA, &ifmr);
> if (error) {
> VLOG_DBG_RL(&rl, "%s: ioctl(SIOCGIFMEDIA) failed: %s",
> - netdev_get_name(netdev), ovs_strerror(error));
> + netdev_get_name(netdev_), ovs_strerror(error));
> return error;
> }
>
> @@ -1137,7 +1139,7 @@ netdev_bsd_get_features(const struct netdev *netdev,
>
> if (IFM_TYPE(ifmr.ifm_current) != IFM_ETHER) {
> VLOG_DBG_RL(&rl, "%s: doesn't appear to be ethernet",
> - netdev_get_name(netdev));
> + netdev_get_name(netdev_));
> error = EINVAL;
> goto cleanup;
> }
> @@ -1145,7 +1147,7 @@ netdev_bsd_get_features(const struct netdev *netdev,
> error = af_inet_ioctl(SIOCGIFMEDIA, &ifmr);
> if (error) {
> VLOG_DBG_RL(&rl, "%s: ioctl(SIOCGIFMEDIA) failed: %s",
> - netdev_get_name(netdev), ovs_strerror(error));
> + netdev_get_name(netdev_), ovs_strerror(error));
> goto cleanup;
> }
>
> @@ -1154,6 +1156,7 @@ netdev_bsd_get_features(const struct netdev *netdev,
> if (!*current) {
> *current = NETDEV_F_OTHER;
> }
> + netdev->full_duplex = ifmr.ifm_active & IFM_FDX;
Technically, we should be holding a lock for this and for the read.
It may be simpler to convert this into internal function with one more
return value and create a netdev_bsd_get_features() wrapper along the
existing netdev_bsd_get_speed() and the new netdev_bsd_get_duplex().
>
> /* Advertised features. */
> *advertised = netdev_bsd_parse_media(ifmr.ifm_current);
> @@ -1194,6 +1197,23 @@ netdev_bsd_get_speed(const struct netdev *netdev,
> uint32_t *current,
> return 0;
> }
>
> +static int
> +netdev_bsd_get_duplex(const struct netdev *netdev_, bool *full_duplex)
> +{
> + enum netdev_features f_current, f_supported, f_advertised, f_peer;
> + struct netdev_bsd *netdev = netdev_bsd_cast(netdev_);
> + int error;
> +
> + error = netdev_bsd_get_features(netdev_, &f_current, &f_advertised,
> + &f_supported, &f_peer);
> + if (error) {
> + return error;
> + }
> +
> + *full_duplex = netdev->full_duplex;
> + return 0;
> +}
> +
> /*
> * Assigns 'addr' as 'netdev''s IPv4 address and 'mask' as its netmask. If
> * 'addr' is INADDR_ANY, 'netdev''s IPv4 address is cleared. Returns a
> @@ -1522,6 +1542,7 @@ netdev_bsd_update_flags(struct netdev *netdev_, enum
> netdev_flags off,
> .get_stats = netdev_bsd_get_stats, \
> .get_features = netdev_bsd_get_features, \
> .get_speed = netdev_bsd_get_speed, \
> + .get_duplex = netdev_bsd_get_duplex, \
> .set_in4 = netdev_bsd_set_in4, \
> .get_addr_list = netdev_bsd_get_addr_list, \
> .get_next_hop = netdev_bsd_get_next_hop, \
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 17b4d66779..5692111b32 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -4177,6 +4177,25 @@ out:
> return 0;
> }
>
> +static int
> +netdev_dpdk_get_duplex(const struct netdev *netdev, bool *full_duplex)
> +{
> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> + int err;
> +
> + ovs_mutex_lock(&dev->mutex);
> + if (dev->link.link_speed != RTE_ETH_SPEED_NUM_UNKNOWN) {
> + *full_duplex = dev->link.link_duplex == RTE_ETH_LINK_FULL_DUPLEX;
> + err = 0;
nit: May be better to just set the err to zero at the declaration point.
> + } else {
> + err = EOPNOTSUPP;
> + }
> + ovs_mutex_unlock(&dev->mutex);
> +
> +
One too many empty lines.
> + return err;
> +}
> +
> static struct ingress_policer *
> netdev_dpdk_policer_construct(uint32_t rate, uint32_t burst)
> {
> @@ -6889,6 +6908,7 @@ parse_vhost_config(const struct smap *ovs_other_config)
> .get_custom_stats = netdev_dpdk_get_custom_stats, \
> .get_features = netdev_dpdk_get_features, \
> .get_speed = netdev_dpdk_get_speed, \
> + .get_duplex = netdev_dpdk_get_duplex, \
> .get_status = netdev_dpdk_get_status, \
> .reconfigure = netdev_dpdk_reconfigure, \
> .rxq_recv = netdev_dpdk_rxq_recv
> diff --git a/lib/netdev-linux-private.h b/lib/netdev-linux-private.h
> index 8e572e3b3b..4b5b606755 100644
> --- a/lib/netdev-linux-private.h
> +++ b/lib/netdev-linux-private.h
> @@ -94,6 +94,7 @@ struct netdev_linux {
> enum netdev_features advertised; /* Cached from ETHTOOL_GSET. */
> enum netdev_features supported; /* Cached from ETHTOOL_GSET. */
> uint32_t current_speed; /* Cached from ETHTOOL_GSET. */
> + uint8_t current_duplex; /* Cached from ETHTOOL_GSET. */
>
> struct ethtool_drvinfo drvinfo; /* Cached from ETHTOOL_GDRVINFO. */
> struct tc *tc;
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 5dad2e7ed7..fb3cbf0597 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2682,6 +2682,7 @@ netdev_linux_read_features(struct netdev_linux *netdev)
> } else {
> netdev->current = NETDEV_F_OTHER;
> }
> + netdev->current_duplex = ecmd.duplex;
>
> if (ecmd.port == PORT_TP) {
> netdev->current |= NETDEV_F_COPPER;
> @@ -2765,6 +2766,38 @@ netdev_linux_get_speed(const struct netdev *netdev_,
> uint32_t *current,
> return error;
> }
>
> +static int
> +netdev_linux_get_duplex_locked(struct netdev_linux *netdev, bool
> *full_duplex)
> +{
> + int err;
> +
> + if (netdev_linux_netnsid_is_remote(netdev)) {
> + return EOPNOTSUPP;
> + }
> +
> + netdev_linux_read_features(netdev);
> + err = netdev->get_features_error;
> + if (!err && netdev->current_duplex == DUPLEX_UNKNOWN) {
> + err = EOPNOTSUPP;
> + }
> + if (!err) {
> + *full_duplex = netdev->current_duplex == DUPLEX_FULL;
> + }
> + return err;
> +}
> +
> +static int
> +netdev_linux_get_duplex(const struct netdev *netdev_, bool *full_duplex)
> +{
> + struct netdev_linux *netdev = netdev_linux_cast(netdev_);
> + int error;
> +
> + ovs_mutex_lock(&netdev->mutex);
> + error = netdev_linux_get_duplex_locked(netdev, full_duplex);
> + ovs_mutex_unlock(&netdev->mutex);
> + return error;
> +}
> +
> /* Set the features advertised by 'netdev' to 'advertise'. */
> static int
> netdev_linux_set_advertisements(struct netdev *netdev_,
> @@ -3889,6 +3922,7 @@ const struct netdev_class netdev_linux_class = {
> .get_stats = netdev_linux_get_stats,
> .get_features = netdev_linux_get_features,
> .get_speed = netdev_linux_get_speed,
> + .get_duplex = netdev_linux_get_duplex,
> .get_status = netdev_linux_get_status,
> .get_block_id = netdev_linux_get_block_id,
> .send = netdev_linux_send,
> @@ -3906,6 +3940,7 @@ const struct netdev_class netdev_tap_class = {
> .get_stats = netdev_tap_get_stats,
> .get_features = netdev_linux_get_features,
> .get_speed = netdev_linux_get_speed,
> + .get_duplex = netdev_linux_get_duplex,
> .get_status = netdev_linux_get_status,
> .send = netdev_linux_send,
> .rxq_construct = netdev_linux_rxq_construct,
> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> index 5ae3794699..33a68c49c2 100644
> --- a/lib/netdev-provider.h
> +++ b/lib/netdev-provider.h
> @@ -514,6 +514,13 @@ struct netdev_class {
> int (*get_speed)(const struct netdev *netdev, uint32_t *current,
> uint32_t *max);
>
> + /* Stores the current duplex status of 'netdev' into '*full_duplex'.
> + * 'true' means full duplex, 'false' means half duplex.
> + *
> + * This function may be set to null if it would always return EOPNOTSUPP.
> + */
> + int (*get_duplex)(const struct netdev *netdev, bool *full_duplex);
> +
> /* Set the features advertised by 'netdev' to 'advertise', which is a
> * set of NETDEV_F_* bits.
> *
> diff --git a/lib/netdev.c b/lib/netdev.c
> index df5b35232d..f32003a95c 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -1288,14 +1288,30 @@ netdev_features_to_bps(enum netdev_features features,
> : default_bps);
> }
>
> -/* Returns true if any of the NETDEV_F_* bits that indicate a full-duplex
> link
> - * are set in 'features', otherwise false. */
I see that netdev_get_speed() is missing a description comment, but we shouldn't
follow a bad example. Should add a short description for the
netdev_get_duplex().
Also, do we need to make it always initialize the full_duplex argument? Other
functions do that. Might make sense to follow the same logic.
> -bool
> -netdev_features_is_full_duplex(enum netdev_features features)
> +int
> +netdev_get_duplex(const struct netdev *netdev, bool *full_duplex)
> {
> - return (features & (NETDEV_F_10MB_FD | NETDEV_F_100MB_FD |
> NETDEV_F_1GB_FD
> - | NETDEV_F_10GB_FD | NETDEV_F_40GB_FD
> - | NETDEV_F_100GB_FD | NETDEV_F_1TB_FD)) != 0;
> + int error;
> +
> + error = netdev->netdev_class->get_duplex
> + ? netdev->netdev_class->get_duplex(netdev, full_duplex)
> + : EOPNOTSUPP;
> +
> + if (error == EOPNOTSUPP) {
> + enum netdev_features current;
> +
> + error = netdev_get_features(netdev, ¤t, NULL, NULL, NULL);
> + if (!error && (current & NETDEV_F_OTHER)) {
> + error = EOPNOTSUPP;
> + }
> + if (!error) {
> + *full_duplex = (current & (NETDEV_F_10MB_FD | NETDEV_F_100MB_FD
> + | NETDEV_F_1GB_FD | NETDEV_F_10GB_FD
> + | NETDEV_F_40GB_FD |
> NETDEV_F_100GB_FD
> + | NETDEV_F_1TB_FD)) != 0;
> + }
> + }
> + return error;
> }
>
> /* Set the features advertised by 'netdev' to 'advertise'. Returns 0 if
> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> index c5403e27aa..e043d7cbc8 100644
> --- a/ofproto/ofproto-dpif-sflow.c
> +++ b/ofproto/ofproto-dpif-sflow.c
> @@ -298,7 +298,6 @@ sflow_agent_get_counters(void *ds_, SFLPoller *poller,
> struct dpif_sflow *ds = ds_;
> SFLCounters_sample_element elem, lacp_elem, of_elem, name_elem;
> SFLCounters_sample_element eth_elem;
> - enum netdev_features current;
> struct dpif_sflow_port *dsp;
> SFLIf_counters *counters;
> SFLEthernet_counters* eth_counters;
> @@ -307,6 +306,7 @@ sflow_agent_get_counters(void *ds_, SFLPoller *poller,
> struct lacp_member_stats lacp_stats;
> uint32_t curr_speed;
> const char *ifName;
> + bool full_duplex;
>
> dsp = dpif_sflow_find_port(ds, u32_to_odp(poller->bridgePort));
> if (!dsp) {
> @@ -317,11 +317,10 @@ sflow_agent_get_counters(void *ds_, SFLPoller *poller,
> counters = &elem.counterBlock.generic;
> counters->ifIndex = SFL_DS_INDEX(poller->dsi);
> counters->ifType = 6;
> - if (!netdev_get_features(dsp->ofport->netdev, ¤t, NULL, NULL,
> NULL)) {
> + if (!netdev_get_duplex(dsp->ofport->netdev, &full_duplex)) {
> /* The values of ifDirection come from MAU MIB (RFC 2668): 0 =
> unknown,
> 1 = full-duplex, 2 = half-duplex, 3 = in, 4=out */
> - counters->ifDirection = (netdev_features_is_full_duplex(current)
> - ? 1 : 2);
> + counters->ifDirection = full_duplex ? 1 : 2;
> } else {
> counters->ifDirection = 0;
> }
> diff --git a/tests/system-interface.at b/tests/system-interface.at
> index cb0835ad6b..15c4a0e2e1 100644
> --- a/tests/system-interface.at
> +++ b/tests/system-interface.at
> @@ -207,5 +207,17 @@ AT_CHECK([ovs-vsctl get interface tap0 link_speed], [0],
> [dnl
> 50000000000
> ])
>
> +AT_CHECK([ovs-vsctl get interface tap0 duplex], [0], [dnl
> +full
> +])
May be worth setting the duplex to full before checking. In case the
default value ever changes.
> +
> +AT_CHECK([ip link set dev tap0 down])
> +AT_CHECK([ethtool -s tap0 duplex half])
> +AT_CHECK([ip link set dev tap0 up])
> +
> +AT_CHECK([ovs-vsctl get interface tap0 duplex], [0], [dnl
> +half
> +])
> +
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 456b784c01..7b49d1b037 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -2532,9 +2532,9 @@ iface_refresh_netdev_status(struct iface *iface)
> {
> struct smap smap;
>
> - enum netdev_features current;
> enum netdev_flags flags;
> const char *link_state;
> + bool full_duplex;
nit: The one line below breaks the reverse x-mass tree, but it's better
to at least keep the other ones in a relative order.
> struct eth_addr mac;
> int64_t bps, mtu_64, ifindex64, link_resets;
> int mtu, error;
> @@ -2576,11 +2576,9 @@ iface_refresh_netdev_status(struct iface *iface)
> link_resets = netdev_get_carrier_resets(iface->netdev);
> ovsrec_interface_set_link_resets(iface->cfg, &link_resets, 1);
>
> - error = netdev_get_features(iface->netdev, ¤t, NULL, NULL, NULL);
> + error = netdev_get_duplex(iface->netdev, &full_duplex);
> if (!error) {
> - ovsrec_interface_set_duplex(iface->cfg,
> - netdev_features_is_full_duplex(current)
> - ? "full" : "half");
> + ovsrec_interface_set_duplex(iface->cfg, full_duplex ? "full" :
> "half");
> } else {
> ovsrec_interface_set_duplex(iface->cfg, NULL);
> }
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
