Re: [PATCH iproute2 v2] ipaddress: strengthen check on 'label' input
On Fri, Jun 1, 2018 at 9:56 PM, Stephen Hemminger
wrote:
> On Tue, 29 May 2018 16:57:07 +0200
> Patrick Talbert wrote:
>
>> As mentioned in the ip-address man page, an address label must
>> be equal to the device name or prefixed by the device name
>> followed by a colon. Currently the only check on this input is
>> to see if the device name appears at the beginning of the label
>> string.
>>
>> This commit adds an additional check to ensure label == dev or
>> continues with a colon.
>>
>> Signed-off-by: Patrick Talbert
>> Suggested-by: Stephen Hemminger
>
> Yes, this looks better but still have some feedback.
>
>> ---
>> ip/ipaddress.c | 21 +++--
>> 1 file changed, 19 insertions(+), 2 deletions(-)
>>
>> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
>> index 00da14c..fce2008 100644
>> --- a/ip/ipaddress.c
>> +++ b/ip/ipaddress.c
>> @@ -2040,6 +2040,22 @@ static bool ipaddr_is_multicast(inet_prefix *a)
>> return false;
>> }
>>
>> +static bool is_valid_label(const char *dev, const char *label)
>> +{
>> + char alias[strlen(dev) + 1];
>> +
>> + if (strlen(label) < strlen(dev))
>> + return false;
>> +
>> + strcpy(alias, dev);
>> + strcat(alias, ":");
>> + if (strncmp(label, dev, strlen(dev)) == 0 ||
>> + strncmp(label, alias, strlen(alias)) == 0)
>> + return true;
>> + else
>> + return false;
>> +}
>
> This string copying and comparison still is much more overhead than it
> needs to be. The following tests out to be equivalent with a single strncmp
> and strlen.
>
> Why not just:
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index 00da14c6f97c..eac489e94fe4 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -2040,6 +2040,16 @@ static bool ipaddr_is_multicast(inet_prefix *a)
> return false;
> }
>
> +static bool is_valid_label(const char *label, const char *dev)
> +{
> + size_t len = strlen(dev);
> +
> + if (strncmp(label, dev, len) != 0)
> + return false;
> +
> + return label[len] == '\0' || label[len] == ':';
> +}
> +
Woah. This is way better. v3 coming up
Thank you for all of your help with this... and by help I mean writing
the patch.
>
>
> Doesn't matter much now, but code seems to get copied.
[PATCH iproute2 v3] ipaddress: strengthen check on 'label' input
As mentioned in the ip-address man page, an address label must
be equal to the device name or prefixed by the device name
followed by a colon. Currently the only check on this input is
to see if the device name appears at the beginning of the label
string.
This commit adds an additional check to ensure label == dev or
continues with a colon.
Signed-off-by: Patrick Talbert
Suggested-by: Stephen Hemminger
---
ip/ipaddress.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index bbd35e7..713962b 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -2065,6 +2065,16 @@ static bool ipaddr_is_multicast(inet_prefix *a)
return false;
}
+static bool is_valid_label(const char *dev, const char *label)
+{
+ size_t len = strlen(dev);
+
+ if (strncmp(label, dev, len) != 0)
+ return false;
+
+ return label[len] == '\0' || label[len] == ':';
+}
+
static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
{
struct {
@@ -2208,8 +2218,9 @@ static int ipaddr_modify(int cmd, int flags, int argc,
char **argv)
fprintf(stderr, "Not enough information: \"dev\" argument is
required.\n");
return -1;
}
- if (l && matches(d, l) != 0) {
- fprintf(stderr, "\"dev\" (%s) must match \"label\" (%s).\n", d,
l);
+ if (l && ! is_valid_label(d, l)) {
+ fprintf(stderr, "\"label\" (%s) must match \"dev\" (%s) or be
prefixed by"
+ " \"dev\" with a colon.\n", l, d);
return -1;
}
--
1.8.3.1
[PATCH iproute2 v2] ipaddress: strengthen check on 'label' input
As mentioned in the ip-address man page, an address label must
be equal to the device name or prefixed by the device name
followed by a colon. Currently the only check on this input is
to see if the device name appears at the beginning of the label
string.
This commit adds an additional check to ensure label == dev or
continues with a colon.
Signed-off-by: Patrick Talbert
Suggested-by: Stephen Hemminger
---
ip/ipaddress.c | 21 +++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 00da14c..fce2008 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -2040,6 +2040,22 @@ static bool ipaddr_is_multicast(inet_prefix *a)
return false;
}
+static bool is_valid_label(const char *dev, const char *label)
+{
+ char alias[strlen(dev) + 1];
+
+ if (strlen(label) < strlen(dev))
+ return false;
+
+ strcpy(alias, dev);
+ strcat(alias, ":");
+ if (strncmp(label, dev, strlen(dev)) == 0 ||
+ strncmp(label, alias, strlen(alias)) == 0)
+ return true;
+ else
+ return false;
+}
+
static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
{
struct {
@@ -2174,8 +2190,9 @@ static int ipaddr_modify(int cmd, int flags, int argc,
char **argv)
fprintf(stderr, "Not enough information: \"dev\" argument is
required.\n");
return -1;
}
- if (l && matches(d, l) != 0) {
- fprintf(stderr, "\"dev\" (%s) must match \"label\" (%s).\n", d,
l);
+ if (l && ! is_valid_label(d, l)) {
+ fprintf(stderr, "\"label\" (%s) must match \"dev\" (%s) or be
prefixed by"
+ " \"dev\" with a colon.\n", l, d);
return -1;
}
--
1.8.3.1
[PATCH iproute2] ipaddress: strengthen check on 'label' input
As mentioned in the ip-address man page, an address label must
be equal to the device name or prefixed by the device name
followed by a colon. Currently the only check on this input is
to see if the device name appears at the beginning of the label
string.
This commit adds an additional check to ensure label == dev or
continues with a colon.
Signed-off-by: Patrick Talbert <ptalb...@redhat.com>
---
ip/ipaddress.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index aecc9a1..edcf821 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -2168,9 +2168,14 @@ static int ipaddr_modify(int cmd, int flags, int argc,
char **argv)
fprintf(stderr, "Not enough information: \"dev\" argument is
required.\n");
return -1;
}
- if (l && matches(d, l) != 0) {
- fprintf(stderr, "\"dev\" (%s) must match \"label\" (%s).\n", d,
l);
- return -1;
+ if (l) {
+ size_t d_len = strlen(d);
+
+ if (!(matches(d, l) == 0 && (l[d_len] == '\0' || l[d_len] ==
':'))) {
+ fprintf(stderr, "\"label\" (%s) must match \"dev\" (%s)
or be prefixed by"
+ " \"dev\" with a colon.\n", l, d);
+ return -1;
+ }
}
if (peer_len == 0 && local_len) {
--
1.8.3.1
Re: [PATCH net-next] [net] softnet_data: Split time_squeeze counter to provide budget_squeeze
On Tue, Jan 23, 2018 at 6:07 PM, Eric Dumazet <eric.duma...@gmail.com> wrote: > On Tue, 2018-01-23 at 10:37 -0500, Patrick Talbert wrote: >> Add a 'budget_squeeze' counter to be able to differenciate between a >> NAPI poll ending with outstanding work because of a lack of budget >> (netdev_budget) versus ending because of a lack of time >> (netdev_budget_usecs). >> >> Signed-off-by: Patrick Talbert <ptalb...@redhat.com> > > >> --- a/net/core/net-procfs.c >> +++ b/net/core/net-procfs.c >> @@ -160,11 +160,11 @@ static int softnet_seq_show(struct seq_file *seq, void >> *v) >> #endif >> >> seq_printf(seq, >> -"%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n", >> +"%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x >> %08x\n", >> sd->processed, sd->dropped, sd->time_squeeze, 0, >> 0, 0, 0, 0, /* was fastroute */ >> 0, /* was cpu_collision */ >> -sd->received_rps, flow_limit_count); >> +sd->received_rps, flow_limit_count, sd->budget_squeeze); >> return 0; >> } > > Please no more updates on /proc files. We want to deprecate them > eventually. > > Fair enough. How is the commit without the net-procfs.c change? Also, I apologize for not knowing, but if it is preferred for such info to no longer be exposed via procfs, where should it be instead? Thank you, Patrick
[PATCH net-next] [net] softnet_data: Split time_squeeze counter to provide budget_squeeze
Add a 'budget_squeeze' counter to be able to differenciate between a
NAPI poll ending with outstanding work because of a lack of budget
(netdev_budget) versus ending because of a lack of time
(netdev_budget_usecs).
Signed-off-by: Patrick Talbert <ptalb...@redhat.com>
---
include/linux/netdevice.h | 1 +
net/core/dev.c| 7 +--
net/core/net-procfs.c | 4 ++--
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ed0799a..97e923d 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2789,6 +2789,7 @@ struct softnet_data {
/* stats */
unsigned intprocessed;
unsigned inttime_squeeze;
+ unsigned intbudget_squeeze;
unsigned intreceived_rps;
#ifdef CONFIG_RPS
struct softnet_data *rps_ipi_list;
diff --git a/net/core/dev.c b/net/core/dev.c
index 94435cd..99ce20a 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5749,11 +5749,14 @@ static __latent_entropy void net_rx_action(struct
softirq_action *h)
* Allow this to run for 2 jiffies since which will allow
* an average latency of 1.5/HZ.
*/
- if (unlikely(budget <= 0 ||
-time_after_eq(jiffies, time_limit))) {
+ if (unlikely(time_after_eq(jiffies, time_limit))) {
sd->time_squeeze++;
break;
}
+ if (unlikely(budget <= 0)) {
+ sd->budget_squeeze++;
+ break;
+ }
}
local_irq_disable();
diff --git a/net/core/net-procfs.c b/net/core/net-procfs.c
index e010bb8..912e47a 100644
--- a/net/core/net-procfs.c
+++ b/net/core/net-procfs.c
@@ -160,11 +160,11 @@ static int softnet_seq_show(struct seq_file *seq, void *v)
#endif
seq_printf(seq,
- "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x\n",
+ "%08x %08x %08x %08x %08x %08x %08x %08x %08x %08x %08x
%08x\n",
sd->processed, sd->dropped, sd->time_squeeze, 0,
0, 0, 0, 0, /* was fastroute */
0, /* was cpu_collision */
- sd->received_rps, flow_limit_count);
+ sd->received_rps, flow_limit_count, sd->budget_squeeze);
return 0;
}
--
1.8.3.1
Re: [PATCH net-next 1/1] [net] bonding: Add NUMA notice
On Sat, Oct 7, 2017 at 6:20 PM, David Miller <da...@davemloft.net> wrote: > From: Patrick Talbert <ptalb...@redhat.com> > Date: Thu, 5 Oct 2017 16:23:45 -0400 > >> Network performance can suffer when a load balancing bond uses slave >> interfaces which are in different NUMA domains. >> >> This compares the NUMA domain of a newly enslaved interface against any >> existing enslaved interfaces and prints a warning if they do not match. >> >> Signed-off-by: Patrick Talbert <ptalb...@redhat.com> > > This is a bit over the top, and doesn't even handle cases where > the device has no specific NUMA node (-1). Hello David, The motivation was simply to have a notification in place if the interface to-be-enslaved does not match the existing one(s). We have seen performance issues with bonding which were eventually tracked down to this mismatched NUMA domain issue. I thought it was worth having the *mild* warning because it can have a decent impact yet is probably not an obvious thing to check for most users. Though I now see your point. If the bonded interfaces are VLANs, and their underlying physical interfaces happen to be in different NUMA domains, then my check will not know as the VLAN interface numa_node member will be -1 no matter what. That's probably a pretty common setup but adding the logic to check for it probably isn't worth it. Patrick
Re: [PATCH net-next 1/1] [net] bonding: Add NUMA notice
On Thu, Oct 5, 2017 at 5:46 PM, Andrew Lunn <and...@lunn.ch> wrote: > On Thu, Oct 05, 2017 at 04:23:45PM -0400, Patrick Talbert wrote: >> Network performance can suffer when a load balancing bond uses slave >> interfaces which are in different NUMA domains. >> >> This compares the NUMA domain of a newly enslaved interface against any >> existing enslaved interfaces and prints a warning if they do not match. > > Hi Patrick > > Is there a bonding mode which might actually want to do this? Send on > the local domain, unless it is overloaded, in which case send it to > the other domain? > I suppose there could theoretically be a bonding mode that could do that, but currently no such mode exists. > There is also this talk for netdev: > > https://netdevconf.org/2.2/session.html?shochat-devicemgmt-talk >From reading the abstract there, it sounds like such a device driver would want to abstract away the numa location of the underlying devices from the "unified" net device it presents to the kernel. > > Andrew My goal with the patch is not to prevent some one from bonding whichever interfaces they want, only to notify them that what they are doing is *likely* to be less than ideal from a performance perspective. Even if some theoretical load balancing bonding mode was intelligent enough to consider NUMA when choosing a transmit interface, it never has control over the interface traffic is received on (excluding the strange balance-alb mode). Patrick
[PATCH net-next 1/1] [net] bonding: Add NUMA notice
Network performance can suffer when a load balancing bond uses slave
interfaces which are in different NUMA domains.
This compares the NUMA domain of a newly enslaved interface against any
existing enslaved interfaces and prints a warning if they do not match.
Signed-off-by: Patrick Talbert <ptalb...@redhat.com>
---
:100644 100644 b19dc03... 250a969... M drivers/net/bonding/bond_main.c
drivers/net/bonding/bond_main.c | 16
1 file changed, 16 insertions(+)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index b19dc03..250a969 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -55,6 +55,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1450,6 +1451,21 @@ int bond_enslave(struct net_device *bond_dev, struct
net_device *slave_dev)
}
}
+ if (bond_has_slaves(bond)) {
+ struct list_head *iter;
+ struct slave *slave;
+
+ bond_for_each_slave(bond, slave, iter) {
+ if (slave_dev->dev.numa_node !=
+ slave->dev->dev.numa_node) {
+ netdev_warn(bond_dev,
+ "%s does not match NUMA domain of
existing slaves. This could have a performance impact.",
+ slave_dev->name);
+ break;
+ }
+ }
+ }
+
call_netdevice_notifiers(NETDEV_JOIN, slave_dev);
/* If this is the first slave, then we need to set the master's hardware
--
1.8.3.1
