On 9/16/25 3:56 PM, David Marchand wrote: > Protect concurrent access when calling netdev_get_features() > or netdev_get_speed() for BSD netdevs.
Would be nice ot have a note here that this patch is actually adding the cache for the features and that's the reason for adding the locks. Maybe the subject line should reflect that as well. It is a little confusing that the patch description reads like a bug fix, but it isn't, as before the change the locking was not necessary. > > Signed-off-by: David Marchand <[email protected]> > --- > Changes since v3: > - added the missing part for caching retrieved values, > > --- > lib/netdev-bsd.c | 117 ++++++++++++++++++++++++++++++----------------- > 1 file changed, 74 insertions(+), 43 deletions(-) > > diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c > index c7cd59376a..8287c264db 100644 > --- a/lib/netdev-bsd.c > +++ b/lib/netdev-bsd.c > @@ -92,6 +92,11 @@ struct netdev_bsd { > struct eth_addr etheraddr; > int mtu; > int carrier; > + int get_features_error; > + > + enum netdev_features current; > + enum netdev_features advertised; > + enum netdev_features supported; > > int tap_fd; /* TAP character device, if any, otherwise -1. */ > > @@ -106,7 +111,8 @@ enum { > VALID_ETHERADDR = 1 << 1, > VALID_IN = 1 << 2, > VALID_MTU = 1 << 3, > - VALID_CARRIER = 1 << 4 > + VALID_CARRIER = 1 << 4, > + VALID_FEATURES = 1 << 5, > }; > > #define PCAP_SNAPLEN 2048 > @@ -1099,28 +1105,26 @@ netdev_bsd_parse_media(int media) > return supported; > } > > -/* > - * Stores the features supported by 'netdev' into each of '*current', > - * '*advertised', '*supported', and '*peer' that are non-null. Each value > is a > - * bitmap of "enum ofp_port_features" bits, in host byte order. Returns 0 if > - * successful, otherwise a positive errno value. On failure, all of the > - * passed-in values are set to 0. > - */ > -static int > -netdev_bsd_get_features(const struct netdev *netdev, > - enum netdev_features *current, uint32_t *advertised, > - enum netdev_features *supported, uint32_t *peer) > +static void > +netdev_bsd_read_features(struct netdev_bsd *netdev) > { > struct ifmediareq ifmr; > int *media_list; > - int i; > int error; > + int i; > > + if (netdev->cache_valid & VALID_FEATURES) { > + return; > + } > > /* 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->up), > + sizeof ifmr.ifm_name); > + > + media_list = xcalloc(ifmr.ifm_count, sizeof(int)); > + ifmr.ifm_ulist = media_list; > > /* We make two SIOCGIFMEDIA ioctl calls. The first to determine the > * number of supported modes, and a second with a buffer to retrieve > @@ -1128,16 +1132,13 @@ 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)); > - return error; > + netdev_get_name(&netdev->up), ovs_strerror(error)); > + goto cleanup; > } > > - media_list = xcalloc(ifmr.ifm_count, sizeof(int)); > - ifmr.ifm_ulist = media_list; > - > 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->up)); > error = EINVAL; > goto cleanup; > } > @@ -1145,53 +1146,83 @@ 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->up), ovs_strerror(error)); > goto cleanup; > } > > /* Current settings. */ > - *current = netdev_bsd_parse_media(ifmr.ifm_active); > - if (!*current) { > - *current = NETDEV_F_OTHER; > + netdev->current = netdev_bsd_parse_media(ifmr.ifm_active); > + if (!netdev->current) { > + netdev->current = NETDEV_F_OTHER; > } > > /* Advertised features. */ > - *advertised = netdev_bsd_parse_media(ifmr.ifm_current); > + netdev->advertised = netdev_bsd_parse_media(ifmr.ifm_current); > > /* Supported features. */ > - *supported = 0; > + netdev->supported = 0; > for (i = 0; i < ifmr.ifm_count; i++) { > - *supported |= netdev_bsd_parse_media(ifmr.ifm_ulist[i]); > + netdev->supported |= netdev_bsd_parse_media(ifmr.ifm_ulist[i]); > } > > - /* Peer advertisements. */ > - *peer = 0; /* XXX */ > - > error = 0; > + > cleanup: > free(media_list); > + netdev->cache_valid |= VALID_FEATURES; > + netdev->get_features_error = error; > +} > + > +/* > + * Stores the features supported by 'netdev' into each of '*current', > + * '*advertised', '*supported', and '*peer' that are non-null. Each value > is a > + * bitmap of "enum ofp_port_features" bits, in host byte order. Returns 0 if > + * successful, otherwise a positive errno value. On failure, all of the > + * passed-in values are set to 0. > + */ > +static int > +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_); > + int error; > + > + ovs_mutex_lock(&netdev->mutex); > + netdev_bsd_read_features(netdev); > + if (!netdev->get_features_error) { > + *current = netdev->current; > + *advertised = netdev->advertised; > + *supported = netdev->supported; > + *peer = 0; > + } > + error = netdev->get_features_error; > + ovs_mutex_unlock(&netdev->mutex); > + > return error; > } > > static int > -netdev_bsd_get_speed(const struct netdev *netdev, uint32_t *current, > +netdev_bsd_get_speed(const struct netdev *netdev_, uint32_t *current, > uint32_t *max) > { > - 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; > - } > - > - *current = MIN(UINT32_MAX, > - netdev_features_to_bps(f_current, 0) / 1000000ULL); > - *max = MIN(UINT32_MAX, > - netdev_features_to_bps(f_supported, 0) / 1000000ULL); > + ovs_mutex_lock(&netdev->mutex); > + netdev_bsd_read_features(netdev); > + if (!netdev->get_features_error) { > + *current = MIN(UINT32_MAX, > + netdev_features_to_bps(netdev->current, 0) > + / 1000000ULL); > + *max = MIN(UINT32_MAX, > + netdev_features_to_bps(netdev->supported, 0) > + / 1000000ULL); nit: I find the following variant a bit easier on eyes: *current = MIN(UINT32_MAX, netdev_features_to_bps(netdev->current, 0) / 1000000ULL); *max = MIN(UINT32_MAX, netdev_features_to_bps(netdev->supported, 0) / 1000000ULL); Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
