[libvirt] [PATCH 0/5] Make virNetDevGetFeatures faster
Reuse the socket across ioctl calls. On my machine with one hardware network interface and three "software" ones, this speeds the udev node_device driver startup 11x (from ~220 ms to ~20 ms) Ján Tomko (5): Move struct elem out of virNetDevGetFeatures Split out virNetDevGetEthtoolFeatures Split out virNetDevGetEthtoolGFeatures Return bool in virNetDevFeatureAvailable Reuse the socket in virNetDevGetFeatures src/util/virnetdev.c | 212 +++++++++++++++++++++++++++++---------------------- 1 file changed, 120 insertions(+), 92 deletions(-) -- 2.7.3
Rename struct elem to ethtool_to_virnetdev_feature and move it out of the function to allow reusing it. --- src/util/virnetdev.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7db4497..354e6f7 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3239,6 +3239,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) return ret; } +struct ethtool_to_virnetdev_feature { + const int cmd; + const virNetDevFeature feat; +}; + /** * virNetDevFeatureAvailable @@ -3303,12 +3308,8 @@ virNetDevGetFeatures(const char *ifname, # if HAVE_DECL_ETHTOOL_GFEATURES struct ethtool_gfeatures *g_cmd; # endif - struct elem{ - const int cmd; - const virNetDevFeature feat; - }; /* legacy ethtool getters */ - struct elem cmds[] = { + struct ethtool_to_virnetdev_feature cmds[] = { {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM}, {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM}, {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG}, @@ -3339,7 +3340,7 @@ virNetDevGetFeatures(const char *ifname, # if HAVE_DECL_ETHTOOL_GFLAGS size_t j = -1; /* ethtool masks */ - struct elem flags[] = { + struct ethtool_to_virnetdev_feature flags[] = { # if HAVE_DECL_ETH_FLAG_LRO {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO}, # endif -- 2.7.3
On Mon, Jun 06, 2016 at 09:39:24 +0200, Ján Tomko wrote:
Rename struct elem to ethtool_to_virnetdev_feature and move it out of the function to allow reusing it. --- src/util/virnetdev.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7db4497..354e6f7 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3239,6 +3239,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) return ret; }
+struct ethtool_to_virnetdev_feature {
This name is very unorthodox. ACK to this patch if you choose a more compliant name.
On Mon, Jun 06, 2016 at 04:25:02PM +0200, Peter Krempa wrote:
On Mon, Jun 06, 2016 at 09:39:24 +0200, Ján Tomko wrote:
Rename struct elem to ethtool_to_virnetdev_feature and move it out of the function to allow reusing it. --- src/util/virnetdev.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7db4497..354e6f7 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3239,6 +3239,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) return ret; }
+struct ethtool_to_virnetdev_feature {
This name is very unorthodox.
ACK to this patch if you choose a more compliant name.
How about virNetDevEthtoolFeatureCmd? Jan
On Mon, Jun 06, 2016 at 16:37:15 +0200, Ján Tomko wrote:
On Mon, Jun 06, 2016 at 04:25:02PM +0200, Peter Krempa wrote:
On Mon, Jun 06, 2016 at 09:39:24 +0200, Ján Tomko wrote:
Rename struct elem to ethtool_to_virnetdev_feature and move it out of the function to allow reusing it. --- src/util/virnetdev.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 7db4497..354e6f7 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3239,6 +3239,11 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) return ret; }
+struct ethtool_to_virnetdev_feature {
This name is very unorthodox.
ACK to this patch if you choose a more compliant name.
How about virNetDevEthtoolFeatureCmd?
That sounds good
Split out the features that we probe via various ethtool commands and ETHTOOL_GFLAGS. --- src/util/virnetdev.c | 105 ++++++++++++++++++++++++++++----------------------- 1 file changed, 57 insertions(+), 48 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 354e6f7..210fcda 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3266,6 +3266,62 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) } +static void +virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, + const char *ifname) +{ + size_t i; + struct ethtool_value cmd = { 0 }; + + /* legacy ethtool getters */ + struct ethtool_to_virnetdev_feature ethtool_cmds[] = { + {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM}, + {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM}, + {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG}, + {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO}, +# if HAVE_DECL_ETHTOOL_GGSO + {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO}, +# endif +# if HAVE_DECL_ETHTOOL_GGRO + {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO}, +# endif + }; + +# if HAVE_DECL_ETHTOOL_GFLAGS + /* ethtool masks */ + struct ethtool_to_virnetdev_feature flags[] = { +# if HAVE_DECL_ETH_FLAG_LRO + {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO}, +# endif +# if HAVE_DECL_ETH_FLAG_TXVLAN + {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN}, + {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN}, +# endif +# if HAVE_DECL_ETH_FLAG_NTUBLE + {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE}, +# endif +# if HAVE_DECL_ETH_FLAG_RXHASH + {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH}, +# endif + }; + + for (i = 0; i < ARRAY_CARDINALITY(ethtool_cmds); i++) { + cmd.cmd = ethtool_cmds[i].cmd; + if (virNetDevFeatureAvailable(ifname, &cmd) == 1) + ignore_value(virBitmapSetBit(bitmap, ethtool_cmds[i].feat)); + } + + cmd.cmd = ETHTOOL_GFLAGS; + if (virNetDevFeatureAvailable(ifname, &cmd) == 1) { + for (i = 0; i < ARRAY_CARDINALITY(flags); i++) { + if (cmd.data & flags[i].cmd) + ignore_value(virBitmapSetBit(bitmap, flags[i].feat)); + } + } +# endif +} + + # if HAVE_DECL_ETHTOOL_GFEATURES /** * virNetDevGFeatureAvailable @@ -3303,24 +3359,9 @@ int virNetDevGetFeatures(const char *ifname, virBitmapPtr *out) { - size_t i = -1; - struct ethtool_value cmd = { 0 }; # if HAVE_DECL_ETHTOOL_GFEATURES struct ethtool_gfeatures *g_cmd; # endif - /* legacy ethtool getters */ - struct ethtool_to_virnetdev_feature cmds[] = { - {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM}, - {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM}, - {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG}, - {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO}, -# if HAVE_DECL_ETHTOOL_GGSO - {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO}, -# endif -# if HAVE_DECL_ETHTOOL_GGRO - {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO}, -# endif - }; if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST))) return -1; @@ -3331,39 +3372,7 @@ virNetDevGetFeatures(const char *ifname, return 0; } - for (i = 0; i < ARRAY_CARDINALITY(cmds); i++) { - cmd.cmd = cmds[i].cmd; - if (virNetDevFeatureAvailable(ifname, &cmd) == 1) - ignore_value(virBitmapSetBit(*out, cmds[i].feat)); - } - -# if HAVE_DECL_ETHTOOL_GFLAGS - size_t j = -1; - /* ethtool masks */ - struct ethtool_to_virnetdev_feature flags[] = { -# if HAVE_DECL_ETH_FLAG_LRO - {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO}, -# endif -# if HAVE_DECL_ETH_FLAG_TXVLAN - {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN}, - {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN}, -# endif -# if HAVE_DECL_ETH_FLAG_NTUBLE - {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE}, -# endif -# if HAVE_DECL_ETH_FLAG_RXHASH - {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH}, -# endif - }; - - cmd.cmd = ETHTOOL_GFLAGS; - if (virNetDevFeatureAvailable(ifname, &cmd) == 1) { - for (j = 0; j < ARRAY_CARDINALITY(flags); j++) { - if (cmd.data & flags[j].cmd) - ignore_value(virBitmapSetBit(*out, flags[j].feat)); - } - } -# endif + virNetDevGetEthtoolFeatures(*out, ifname); # if HAVE_DECL_ETHTOOL_GFEATURES if (VIR_ALLOC_VAR(g_cmd, -- 2.7.3
On Mon, Jun 06, 2016 at 09:39:25 +0200, Ján Tomko wrote:
Split out the features that we probe via various ethtool commands and ETHTOOL_GFLAGS. --- src/util/virnetdev.c | 105 ++++++++++++++++++++++++++++----------------------- 1 file changed, 57 insertions(+), 48 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 354e6f7..210fcda 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3266,6 +3266,62 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) }
+static void +virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, + const char *ifname) +{ + size_t i; + struct ethtool_value cmd = { 0 }; + + /* legacy ethtool getters */ + struct ethtool_to_virnetdev_feature ethtool_cmds[] = { + {ETHTOOL_GRXCSUM, VIR_NET_DEV_FEAT_GRXCSUM}, + {ETHTOOL_GTXCSUM, VIR_NET_DEV_FEAT_GTXCSUM}, + {ETHTOOL_GSG, VIR_NET_DEV_FEAT_GSG}, + {ETHTOOL_GTSO, VIR_NET_DEV_FEAT_GTSO}, +# if HAVE_DECL_ETHTOOL_GGSO + {ETHTOOL_GGSO, VIR_NET_DEV_FEAT_GGSO}, +# endif +# if HAVE_DECL_ETHTOOL_GGRO + {ETHTOOL_GGRO, VIR_NET_DEV_FEAT_GGRO}, +# endif + }; + +# if HAVE_DECL_ETHTOOL_GFLAGS + /* ethtool masks */ + struct ethtool_to_virnetdev_feature flags[] = { +# if HAVE_DECL_ETH_FLAG_LRO + {ETH_FLAG_LRO, VIR_NET_DEV_FEAT_LRO}, +# endif +# if HAVE_DECL_ETH_FLAG_TXVLAN + {ETH_FLAG_RXVLAN, VIR_NET_DEV_FEAT_RXVLAN}, + {ETH_FLAG_TXVLAN, VIR_NET_DEV_FEAT_TXVLAN}, +# endif +# if HAVE_DECL_ETH_FLAG_NTUBLE + {ETH_FLAG_NTUPLE, VIR_NET_DEV_FEAT_NTUPLE}, +# endif +# if HAVE_DECL_ETH_FLAG_RXHASH + {ETH_FLAG_RXHASH, VIR_NET_DEV_FEAT_RXHASH}, +# endif + }; + + for (i = 0; i < ARRAY_CARDINALITY(ethtool_cmds); i++) {
This is guarded by HAVE_DECL_ETHTOOL_GFLAGS so it is not equivalent to the code below.
+ cmd.cmd = ethtool_cmds[i].cmd; + if (virNetDevFeatureAvailable(ifname, &cmd) == 1) + ignore_value(virBitmapSetBit(bitmap, ethtool_cmds[i].feat)); + } + + cmd.cmd = ETHTOOL_GFLAGS; + if (virNetDevFeatureAvailable(ifname, &cmd) == 1) { + for (i = 0; i < ARRAY_CARDINALITY(flags); i++) { + if (cmd.data & flags[i].cmd) + ignore_value(virBitmapSetBit(bitmap, flags[i].feat)); + } + } +# endif +}
ACK if you move the first loop out of HAVE_DECL_ETHTOOL_GFLAGS but you need to keep the second one in place.
Move out the code depending on HAVE_DECL_ETHTOOL_GFEATURES. --- src/util/virnetdev.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 210fcda..4705891 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3342,6 +3342,32 @@ virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd) ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); return ret; } + + +static int +virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap, + const char *ifname) +{ + struct ethtool_gfeatures *g_cmd; + + if (VIR_ALLOC_VAR(g_cmd, + struct ethtool_get_features_block, GFEATURES_SIZE) < 0) + return -1; + + g_cmd->cmd = ETHTOOL_GFEATURES; + g_cmd->size = GFEATURES_SIZE; + if (virNetDevGFeatureAvailable(ifname, g_cmd) == 1) + ignore_value(virBitmapSetBit(bitmap, VIR_NET_DEV_FEAT_TXUDPTNL)); + VIR_FREE(g_cmd); + return 0; +} +# else +static int +virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED) +{ + return 0; +} # endif @@ -3359,10 +3385,6 @@ int virNetDevGetFeatures(const char *ifname, virBitmapPtr *out) { -# if HAVE_DECL_ETHTOOL_GFEATURES - struct ethtool_gfeatures *g_cmd; -# endif - if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST))) return -1; @@ -3374,16 +3396,8 @@ virNetDevGetFeatures(const char *ifname, virNetDevGetEthtoolFeatures(*out, ifname); -# if HAVE_DECL_ETHTOOL_GFEATURES - if (VIR_ALLOC_VAR(g_cmd, - struct ethtool_get_features_block, GFEATURES_SIZE) < 0) + if (virNetDevGetEthtoolGFeatures(*out, ifname) < 0) return -1; - g_cmd->cmd = ETHTOOL_GFEATURES; - g_cmd->size = GFEATURES_SIZE; - if (virNetDevGFeatureAvailable(ifname, g_cmd) == 1) - ignore_value(virBitmapSetBit(*out, VIR_NET_DEV_FEAT_TXUDPTNL)); - VIR_FREE(g_cmd); -# endif if (virNetDevRDMAFeature(ifname, out) < 0) return -1; -- 2.7.3
On Mon, Jun 06, 2016 at 09:39:26 +0200, Ján Tomko wrote:
Move out the code depending on HAVE_DECL_ETHTOOL_GFEATURES. --- src/util/virnetdev.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-)
ACK
Simplify the logic --- src/util/virnetdev.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 4705891..52f02d3 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3254,15 +3254,14 @@ struct ethtool_to_virnetdev_feature { * * Returns 0 if not found, 1 on success, and -1 on failure. */ -static int +static bool virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) { - int ret = -1; - cmd = (void*)cmd; - if (!virNetDevSendEthtoolIoctl(ifname, cmd)) - ret = cmd->data > 0 ? 1 : 0; - return ret; + if (virNetDevSendEthtoolIoctl(ifname, cmd) == 0 && + cmd->data > 0) + return true; + return false; } @@ -3307,12 +3306,12 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, for (i = 0; i < ARRAY_CARDINALITY(ethtool_cmds); i++) { cmd.cmd = ethtool_cmds[i].cmd; - if (virNetDevFeatureAvailable(ifname, &cmd) == 1) + if (virNetDevFeatureAvailable(ifname, &cmd)) ignore_value(virBitmapSetBit(bitmap, ethtool_cmds[i].feat)); } cmd.cmd = ETHTOOL_GFLAGS; - if (virNetDevFeatureAvailable(ifname, &cmd) == 1) { + if (virNetDevFeatureAvailable(ifname, &cmd)) { for (i = 0; i < ARRAY_CARDINALITY(flags); i++) { if (cmd.data & flags[i].cmd) ignore_value(virBitmapSetBit(bitmap, flags[i].feat)); @@ -3332,15 +3331,13 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, * * Returns 0 if not found, 1 on success, and -1 on failure. */ -static int +static bool virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd) { - int ret = -1; - cmd = (void*)cmd; - if (!virNetDevSendEthtoolIoctl(ifname, cmd)) - ret = FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); - return ret; + if (virNetDevSendEthtoolIoctl(ifname, cmd) == 0) + return !!FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); + return false; } @@ -3356,7 +3353,7 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap, g_cmd->cmd = ETHTOOL_GFEATURES; g_cmd->size = GFEATURES_SIZE; - if (virNetDevGFeatureAvailable(ifname, g_cmd) == 1) + if (virNetDevGFeatureAvailable(ifname, g_cmd)) ignore_value(virBitmapSetBit(bitmap, VIR_NET_DEV_FEAT_TXUDPTNL)); VIR_FREE(g_cmd); return 0; -- 2.7.3
On Mon, Jun 06, 2016 at 09:39:27 +0200, Ján Tomko wrote:
Simplify the logic --- src/util/virnetdev.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 4705891..52f02d3 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3254,15 +3254,14 @@ struct ethtool_to_virnetdev_feature { * * Returns 0 if not found, 1 on success, and -1 on failure.
This is no longer valid after this patch.
*/ -static int +static bool virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) { - int ret = -1; - cmd = (void*)cmd; - if (!virNetDevSendEthtoolIoctl(ifname, cmd)) - ret = cmd->data > 0 ? 1 : 0; - return ret; + if (virNetDevSendEthtoolIoctl(ifname, cmd) == 0 && + cmd->data > 0) + return true; + return false; }
@@ -3307,12 +3306,12 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap,
for (i = 0; i < ARRAY_CARDINALITY(ethtool_cmds); i++) { cmd.cmd = ethtool_cmds[i].cmd; - if (virNetDevFeatureAvailable(ifname, &cmd) == 1) + if (virNetDevFeatureAvailable(ifname, &cmd)) ignore_value(virBitmapSetBit(bitmap, ethtool_cmds[i].feat)); }
cmd.cmd = ETHTOOL_GFLAGS; - if (virNetDevFeatureAvailable(ifname, &cmd) == 1) { + if (virNetDevFeatureAvailable(ifname, &cmd)) { for (i = 0; i < ARRAY_CARDINALITY(flags); i++) { if (cmd.data & flags[i].cmd) ignore_value(virBitmapSetBit(bitmap, flags[i].feat)); @@ -3332,15 +3331,13 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, * * Returns 0 if not found, 1 on success, and -1 on failure.
Neither this.
*/ -static int
ACK with docs fixed.
This speeds up node_device_udev driver startup 11x. --- src/util/virnetdev.c | 69 +++++++++++++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 31 deletions(-) diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 52f02d3..7863e8a 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3206,20 +3206,11 @@ virNetDevRDMAFeature(const char *ifname, * Returns 0 on success, -1 on failure. */ static int -virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) +virNetDevSendEthtoolIoctl(int fd, struct ifreq *ifr) { int ret = -1; - int fd = -1; - struct ifreq ifr; - - /* Ultimately uses AF_PACKET for socket which requires privileged - * daemon support. - */ - if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) - return ret; - ifr.ifr_data = cmd; - ret = ioctl(fd, SIOCETHTOOL, &ifr); + ret = ioctl(fd, SIOCETHTOOL, ifr); if (ret != 0) { switch (errno) { case EINVAL: /* kernel doesn't support SIOCETHTOOL */ @@ -3230,12 +3221,10 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) break; default: virReportSystemError(errno, "%s", _("ethtool ioctl error")); - goto cleanup; + break; } } - cleanup: - VIR_FORCE_CLOSE(fd); return ret; } @@ -3255,10 +3244,10 @@ struct ethtool_to_virnetdev_feature { * Returns 0 if not found, 1 on success, and -1 on failure. */ static bool -virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) +virNetDevFeatureAvailable(int fd, struct ifreq *ifr, struct ethtool_value *cmd) { - cmd = (void*)cmd; - if (virNetDevSendEthtoolIoctl(ifname, cmd) == 0 && + ifr->ifr_data = (void*)cmd; + if (virNetDevSendEthtoolIoctl(fd, ifr) == 0 && cmd->data > 0) return true; return false; @@ -3267,7 +3256,8 @@ virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) static void virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, - const char *ifname) + int fd, + struct ifreq *ifr) { size_t i; struct ethtool_value cmd = { 0 }; @@ -3306,12 +3296,12 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, for (i = 0; i < ARRAY_CARDINALITY(ethtool_cmds); i++) { cmd.cmd = ethtool_cmds[i].cmd; - if (virNetDevFeatureAvailable(ifname, &cmd)) + if (virNetDevFeatureAvailable(fd, ifr, &cmd)) ignore_value(virBitmapSetBit(bitmap, ethtool_cmds[i].feat)); } cmd.cmd = ETHTOOL_GFLAGS; - if (virNetDevFeatureAvailable(ifname, &cmd)) { + if (virNetDevFeatureAvailable(fd, ifr, &cmd)) { for (i = 0; i < ARRAY_CARDINALITY(flags); i++) { if (cmd.data & flags[i].cmd) ignore_value(virBitmapSetBit(bitmap, flags[i].feat)); @@ -3332,10 +3322,12 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, * Returns 0 if not found, 1 on success, and -1 on failure. */ static bool -virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd) +virNetDevGFeatureAvailable(int fd, + struct ifreq *ifr, + struct ethtool_gfeatures *cmd) { - cmd = (void*)cmd; - if (virNetDevSendEthtoolIoctl(ifname, cmd) == 0) + ifr->ifr_data = (void*)cmd; + if (virNetDevSendEthtoolIoctl(fd, ifr) == 0) return !!FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); return false; } @@ -3343,7 +3335,8 @@ virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd) static int virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap, - const char *ifname) + int fd, + struct ifreq *ifr) { struct ethtool_gfeatures *g_cmd; @@ -3353,7 +3346,7 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap, g_cmd->cmd = ETHTOOL_GFEATURES; g_cmd->size = GFEATURES_SIZE; - if (virNetDevGFeatureAvailable(ifname, g_cmd)) + if (virNetDevGFeatureAvailable(fd, ifr, g_cmd)) ignore_value(virBitmapSetBit(bitmap, VIR_NET_DEV_FEAT_TXUDPTNL)); VIR_FREE(g_cmd); return 0; @@ -3361,7 +3354,8 @@ virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap, # else static int virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap ATTRIBUTE_UNUSED, - const char *ifname ATTRIBUTE_UNUSED) + int fd ATTRIBUTE_UNUSED, + struct ifreq *ifr ATTRIBUGE_UNUSED) { return 0; } @@ -3382,6 +3376,10 @@ int virNetDevGetFeatures(const char *ifname, virBitmapPtr *out) { + struct ifreq ifr; + int ret = -1; + int fd = -1; + if (!(*out = virBitmapNew(VIR_NET_DEV_FEAT_LAST))) return -1; @@ -3391,14 +3389,23 @@ virNetDevGetFeatures(const char *ifname, return 0; } - virNetDevGetEthtoolFeatures(*out, ifname); + /* Ultimately uses AF_PACKET for socket which requires privileged + * daemon support. + */ + if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) + goto cleanup; - if (virNetDevGetEthtoolGFeatures(*out, ifname) < 0) - return -1; + virNetDevGetEthtoolFeatures(*out, fd, &ifr); + + if (virNetDevGetEthtoolGFeatures(*out, fd, &ifr) < 0) + goto cleanup; if (virNetDevRDMAFeature(ifname, out) < 0) - return -1; - return 0; + goto cleanup; + + ret = 0; + cleanup: + return ret; } #else int -- 2.7.3
On Mon, Jun 06, 2016 at 09:39:28 +0200, Ján Tomko wrote:
This speeds up node_device_udev driver startup 11x. --- src/util/virnetdev.c | 69 +++++++++++++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 31 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 52f02d3..7863e8a 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -3206,20 +3206,11 @@ virNetDevRDMAFeature(const char *ifname, * Returns 0 on success, -1 on failure. */ static int -virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) +virNetDevSendEthtoolIoctl(int fd, struct ifreq *ifr)
You changed arguments but didn't fix the comment that documents them.
{ int ret = -1; - int fd = -1; - struct ifreq ifr; - - /* Ultimately uses AF_PACKET for socket which requires privileged - * daemon support. - */ - if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) - return ret;
- ifr.ifr_data = cmd; - ret = ioctl(fd, SIOCETHTOOL, &ifr); + ret = ioctl(fd, SIOCETHTOOL, ifr); if (ret != 0) { switch (errno) { case EINVAL: /* kernel doesn't support SIOCETHTOOL */ @@ -3230,12 +3221,10 @@ virNetDevSendEthtoolIoctl(const char *ifname, void *cmd) break; default: virReportSystemError(errno, "%s", _("ethtool ioctl error")); - goto cleanup; + break;
These error reports don't make much sense now, but I guess we can keep them for debugging purposes.
} }
- cleanup: - VIR_FORCE_CLOSE(fd); return ret; }
@@ -3255,10 +3244,10 @@ struct ethtool_to_virnetdev_feature { * Returns 0 if not found, 1 on success, and -1 on failure.
Again. Argument change is not documented.
*/ static bool -virNetDevFeatureAvailable(const char *ifname, struct ethtool_value *cmd) +virNetDevFeatureAvailable(int fd, struct ifreq *ifr, struct ethtool_value *cmd) { - cmd = (void*)cmd; - if (virNetDevSendEthtoolIoctl(ifname, cmd) == 0 && + ifr->ifr_data = (void*)cmd; + if (virNetDevSendEthtoolIoctl(fd, ifr) == 0 && cmd->data > 0) return true; return false;
@@ -3332,10 +3322,12 @@ virNetDevGetEthtoolFeatures(virBitmapPtr bitmap, * Returns 0 if not found, 1 on success, and -1 on failure.
And here too ...
*/ static bool -virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd) +virNetDevGFeatureAvailable(int fd, + struct ifreq *ifr, + struct ethtool_gfeatures *cmd) { - cmd = (void*)cmd; - if (virNetDevSendEthtoolIoctl(ifname, cmd) == 0) + ifr->ifr_data = (void*)cmd; + if (virNetDevSendEthtoolIoctl(fd, ifr) == 0) return !!FEATURE_BIT_IS_SET(cmd->features, TX_UDP_TNL, active); return false; } @@ -3343,7 +3335,8 @@ virNetDevGFeatureAvailable(const char *ifname, struct ethtool_gfeatures *cmd)
static int virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap, - const char *ifname) + int fd, + struct ifreq *ifr)
Broken formatting.
{ struct ethtool_gfeatures *g_cmd;
[...]
@@ -3391,14 +3389,23 @@ virNetDevGetFeatures(const char *ifname, return 0; }
- virNetDevGetEthtoolFeatures(*out, ifname); + /* Ultimately uses AF_PACKET for socket which requires privileged + * daemon support. + */ + if ((fd = virNetDevSetupControl(ifname, &ifr)) < 0) + goto cleanup;
- if (virNetDevGetEthtoolGFeatures(*out, ifname) < 0) - return -1; + virNetDevGetEthtoolFeatures(*out, fd, &ifr); + + if (virNetDevGetEthtoolGFeatures(*out, fd, &ifr) < 0) + goto cleanup;
if (virNetDevRDMAFeature(ifname, out) < 0) - return -1; - return 0; + goto cleanup; + + ret = 0; + cleanup:
You forgot to close the socket.
+ return ret; } #else int
ACK with the issues fixed.
On Mon, Jun 06, Ján Tomko wrote:
virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap ATTRIBUTE_UNUSED, - const char *ifname ATTRIBUTE_UNUSED) + int fd ATTRIBUTE_UNUSED, + struct ifreq *ifr ATTRIBUGE_UNUSED)
Typo.
On Wed, Jun 08, 2016 at 10:01:34PM +0200, Olaf Hering wrote:
On Mon, Jun 06, Ján Tomko wrote:
virNetDevGetEthtoolGFeatures(virBitmapPtr bitmap ATTRIBUTE_UNUSED, - const char *ifname ATTRIBUTE_UNUSED) + int fd ATTRIBUTE_UNUSED, + struct ifreq *ifr ATTRIBUGE_UNUSED)
Typo.
Thanks; fixed. Jan
participants (3)
-
Ján Tomko -
Olaf Hering -
Peter Krempa