* [RFC net-next 1/3] bonding: add infrastructure for an option API
From: Nikolay Aleksandrov @ 2014-01-10 13:11 UTC (permalink / raw)
To: netdev; +Cc: Nikolay Aleksandrov
In-Reply-To: <1389359495-9700-1-git-send-email-nikolay@redhat.com>
This patch adds the necessary basic infrastructure to support
centralized and unified option manipulation API for the bonding. The new
structure bond_option will be used to describe each option with its
dependencies on modes which will be checked automatically thus removing a
lot of duplicated code. Also automatic range checking is added for
non-string options. Currently the option setting function requires RTNL to
be acquired prior to calling it, since many options already rely on RTNL
it seemed like the best choice to protect all against common race
conditions.
In order to add an option the following steps need to be done:
1. Add an entry BOND_OPT_<option> to bond_options.h so it gets a unique id
and a bit corresponding to the id
2. Add a bond_option entry to the bond_opts[] array in bond_options.c which
describes the option, its dependencies and its manipulation function
3. Add code to export the option through sysfs and/or as a module parameter
(the sysfs export will be made automatically in the future)
The current available option types are:
BOND_OPTVAL_INTEGER - range option, the flags should be used to define
it properly
BOND_OPTVAL_STRING - string option or an option which wants to do its
own validation and conversion since the buffer is
passed unmodified to the option manipulation function
The options can have different flags set, currently the following are
supported:
BOND_OPTFLAG_NOSLAVES - require that the bond device has no slaves prior
to setting the option
BOND_OPTFLAG_IFDOWN - require that the bond device is down prior to
setting the option
There's a new value structure to describe different types of values
which can have the following flags:
BOND_VALFLAG_DEFAULT - marks the default option (permanent string alias
to this option is "default")
BOND_VALFLAG_MIN - the minimum value that this option can have
BOND_VALFLAG_MAX - the maximum value that this option can have
An example would be nice here, so if we have an option which can have
the values "off"(2), "special"(4, default) and supports a range, say
16 - 32, it should be defined as follows:
"off", 2,
"special", 4, BOND_VALFLAG_DEFAULT,
"rangemin", 16, BOND_VALFLAG_MIN,
"rangemax", 32, BOND_VALFLAG_MAX
So we have the valid intervals: [2, 2], [4, 4], [16, 32]
BOND_VALFLAG_(MIN|MAX) can be used to specify a valid range for an
option, if MIN is omitted then 0 is considered as a minimum. If an
exact match is found in the values[] table it will be returned,
otherwise the range is tried (if available).
The values[] table which describes the allowed values for an option is
not mandatory for BOND_OPTVAL_STRING, but if it's present it will be
used.
For BOND_OPTVAL_INTEGER the values[] table is mandatory.
Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
drivers/net/bonding/bond_options.c | 317 +++++++++++++++++++++++++++++++++++++
drivers/net/bonding/bond_options.h | 82 ++++++++++
drivers/net/bonding/bonding.h | 1 +
3 files changed, 400 insertions(+)
create mode 100644 drivers/net/bonding/bond_options.h
diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
index 945a666..7765efb 100644
--- a/drivers/net/bonding/bond_options.c
+++ b/drivers/net/bonding/bond_options.c
@@ -17,8 +17,325 @@
#include <linux/rwlock.h>
#include <linux/rcupdate.h>
#include <linux/reciprocal_div.h>
+#include <linux/ctype.h>
#include "bonding.h"
+static struct bond_option bond_opts[] = {
+ { }
+};
+
+/* Searches for a value in opt's values[] table */
+struct bond_value_tbl *bond_opt_get_val(unsigned int option, int val)
+{
+ struct bond_option *opt;
+ int i;
+
+ opt = bond_opt_get(option);
+ if (WARN_ON(!opt))
+ return NULL;
+ for (i = 0; opt->values && opt->values[i].name; i++)
+ if (opt->values[i].value == val)
+ return &opt->values[i];
+
+ return NULL;
+}
+
+/* Searches for a value in opt's values[] table which matches the flagmask */
+static struct bond_value_tbl *bond_opt_get_flags(const struct bond_option *opt,
+ u32 flagmask)
+{
+ int i;
+
+ for (i = 0; opt->values && opt->values[i].name; i++)
+ if (opt->values[i].flags & flagmask)
+ return &opt->values[i];
+
+ return NULL;
+}
+
+static bool bond_opt_check_range(const struct bond_option *opt, int val)
+{
+ struct bond_value_tbl *minval, *maxval;
+
+ minval = bond_opt_get_flags(opt, BOND_VALFLAG_MIN);
+ maxval = bond_opt_get_flags(opt, BOND_VALFLAG_MAX);
+ if (val < 0 || (minval && val < minval->value) ||
+ (maxval && val > maxval->value))
+ return false;
+
+ return true;
+}
+
+/**
+ * bond_opt_parse - parse option value
+ * @tbl: an option's values[] table to parse against
+ * @bufarg: value to parse
+ * @retval: pointer where to store the parsed value
+ * @string: how to treat bufarg (as string or integer)
+ *
+ * This functions tries to extract the value from bufarg and check if it's
+ * a possible match for the option. It shouldn't be possible to have a non-NULL
+ * return with @retval being < 0.
+ */
+struct bond_value_tbl *bond_opt_parse(const struct bond_option *opt,
+ void *bufarg, int *retval, bool string)
+{
+ char *buf, *p, valstr[BOND_MAX_MODENAME_LEN + 1] = { 0, };
+ struct bond_value_tbl *tbl, *maxval = NULL, *ret = NULL;
+ int valint = -1, i, rv;
+
+ tbl = opt->values;
+ if (!tbl)
+ goto out;
+
+ if (string) {
+ buf = bufarg;
+ for (p = (char *)buf; *p; p++)
+ if (!(isdigit(*p) || isspace(*p)))
+ break;
+
+ if (*p)
+ rv = sscanf(buf, "%20s", valstr);
+ else
+ rv = sscanf(buf, "%d", &valint);
+
+ if (!rv)
+ goto out;
+ } else {
+ valint = *(int *)bufarg;
+ }
+
+ for (i = 0; tbl[i].name; i++) {
+ /* Obtain maxval in case we don't match anything */
+ if (tbl[i].flags & BOND_VALFLAG_MAX)
+ maxval = &tbl[i];
+
+ /* Check for exact match */
+ if (!strcmp(valstr, "default") &&
+ (tbl[i].flags & BOND_VALFLAG_DEFAULT))
+ ret = &tbl[i];
+ else if (valint == tbl[i].value)
+ ret = &tbl[i];
+ else if (!strcmp(valstr, tbl[i].name))
+ ret = &tbl[i];
+
+ /* Exact match */
+ if (ret) {
+ valint = ret->value;
+ goto out;
+ }
+ }
+ /* Possible range match */
+ if (bond_opt_check_range(opt, valint))
+ ret = maxval;
+out:
+ if (ret)
+ *retval = valint;
+ else
+ *retval = -ENOENT;
+
+ return ret;
+}
+
+/* Check opt's dependencies against bond mode and currently set options */
+static int bond_opt_check_deps(struct bonding *bond,
+ const struct bond_option *opt)
+{
+ struct bond_params *params = &bond->params;
+
+ if (test_bit(params->mode, &opt->unsuppmodes))
+ return -EACCES;
+ if ((opt->flags & BOND_OPTFLAG_NOSLAVES) && bond_has_slaves(bond))
+ return -ENOTEMPTY;
+ if ((opt->flags & BOND_OPTFLAG_IFDOWN) && (bond->dev->flags & IFF_UP))
+ return -EBUSY;
+
+ return 0;
+}
+
+static void bond_opt_dep_print(struct bonding *bond,
+ const struct bond_option *opt)
+{
+ struct bond_params *params;
+
+ params = &bond->params;
+ if (test_bit(params->mode, &opt->unsuppmodes))
+ pr_err("%s: option %s: mode dependency failed\n",
+ bond->dev->name, opt->name);
+}
+
+static void bond_opt_error_interpret(struct bonding *bond,
+ const struct bond_option *opt,
+ int error, char *buf)
+{
+ struct bond_value_tbl *minval, *maxval;
+ char *p;
+
+ p = strchr(buf, '\n');
+ if (p)
+ *p = '\0';
+ switch (error) {
+ case -EINVAL:
+ pr_err("%s: option %s: invalid value (%s).\n",
+ bond->dev->name, opt->name, buf);
+ if (opt->valtype == BOND_OPTVAL_STRING)
+ break;
+ minval = bond_opt_get_flags(opt, BOND_VALFLAG_MIN);
+ maxval = bond_opt_get_flags(opt, BOND_VALFLAG_MAX);
+ if (!maxval)
+ break;
+ pr_err("%s: option %s: allowed values %d - %d.\n",
+ bond->dev->name, opt->name, minval ? minval->value : 0,
+ maxval->value);
+ break;
+ case -EACCES:
+ bond_opt_dep_print(bond, opt);
+ break;
+ case -ENOTEMPTY:
+ pr_err("%s: option %s: unable to set because the bond has slaves.\n",
+ bond->dev->name, opt->name);
+ break;
+ case -EBUSY:
+ pr_err("%s: option %s: unable to set because the bond device is up.\n",
+ bond->dev->name, opt->name);
+ break;
+ default:
+ break;
+ }
+}
+
+static int bond_opt_call_intset(struct bonding *bond,
+ const struct bond_option *opt,
+ int newval)
+{
+ struct bond_value_tbl *valptr;
+ int value = newval;
+
+ if (opt->valtype != BOND_OPTVAL_INTEGER)
+ return -EINVAL;
+ /* Check value and store the result in it */
+ valptr = bond_opt_parse(opt, &value, &value, false);
+ if (!valptr)
+ return -EINVAL;
+
+ return opt->set(bond, &value);
+}
+
+/**
+ * bond_opt_call_set - convert buf and check the value based on opt->valtype
+ * @bond: bond device to set on
+ * @opt: option to set
+ * @buf: value to set the option to
+ *
+ * This function converts buf to the appropriate value based on the opt's
+ * valtype.
+ */
+static int bond_opt_call_set(struct bonding *bond,
+ const struct bond_option *opt,
+ char *buf)
+{
+ struct bond_value_tbl *valptr;
+ int value = -1;
+
+ if (opt->valtype == BOND_OPTVAL_INTEGER) {
+ valptr = bond_opt_parse(opt, buf, &value, true);
+ if (!valptr)
+ return -EINVAL;
+ return opt->set(bond, &value);
+ } else {
+ return opt->set(bond, buf);
+ }
+}
+
+/**
+ * __bond_opt_set - set a bonding option
+ * @bond: target bond device
+ * @option: option to set
+ * @buf: value to set it to
+ *
+ * This function is used to change the bond's option value to newval, it can be
+ * used for both enabling/changing an option and for disabling it. RTNL lock
+ * must be obtained before calling this function.
+ */
+int __bond_opt_set(struct bonding *bond, unsigned int option, char *buf)
+{
+ const struct bond_option *opt;
+ int ret = -ENOENT;
+
+ ASSERT_RTNL();
+
+ opt = bond_opt_get(option);
+ if (WARN_ON(!buf) || WARN_ON(!opt))
+ goto out;
+ ret = bond_opt_check_deps(bond, opt);
+ if (ret)
+ goto out;
+ ret = bond_opt_call_set(bond, opt, buf);
+out:
+ if (ret)
+ bond_opt_error_interpret(bond, opt, ret, buf);
+
+ return ret;
+}
+
+/* See comment for __bond_opt_set. This function is used to set a
+ * BOND_OPTVAL_INTEGER option to a value directly without string parsing. The
+ * value is still checked against the option's values[] table.
+ */
+int __bond_opt_intset(struct bonding *bond, unsigned int option, int value)
+{
+ const struct bond_option *opt;
+ int ret = -ENOENT;
+
+ ASSERT_RTNL();
+
+ opt = bond_opt_get(option);
+ if (WARN_ON(!opt))
+ return ret;
+ ret = bond_opt_check_deps(bond, opt);
+ if (ret)
+ return ret;
+ ret = bond_opt_call_intset(bond, opt, value);
+
+ return ret;
+}
+
+/**
+ * bond_opt_tryset_rtnl - try to acquire rtnl and call __bond_opt_set
+ * @bond: target bond device
+ * @option: option to set
+ * @buf: value to set it to
+ *
+ * This function tries to acquire RTNL without blocking and if successful
+ * calls __bond_opt_set, otherwise returns -EAGAIN to notify the caller.
+ */
+int bond_opt_tryset_rtnl(struct bonding *bond, unsigned int option, char *buf)
+{
+ int ret;
+
+ if (!rtnl_trylock())
+ return -EAGAIN;
+ ret = __bond_opt_set(bond, option, buf);
+ rtnl_unlock();
+
+ return ret;
+}
+
+/**
+ * bond_opt_get - get a pointer to an option
+ * @option: option for which to return a pointer
+ *
+ * This function checks if option is valid and if so returns a pointer
+ * to its entry in the bond_opts[] option array.
+ */
+struct bond_option *bond_opt_get(unsigned int option)
+{
+ if (!BOND_OPT_VALID(option))
+ return NULL;
+
+ return &bond_opts[option];
+}
+
int bond_option_mode_set(struct bonding *bond, int mode)
{
if (bond_parm_tbl_lookup(mode, bond_mode_tbl) < 0) {
diff --git a/drivers/net/bonding/bond_options.h b/drivers/net/bonding/bond_options.h
new file mode 100644
index 0000000..c72ecec
--- /dev/null
+++ b/drivers/net/bonding/bond_options.h
@@ -0,0 +1,82 @@
+/*
+ * drivers/net/bond/bond_options.h - bonding options
+ * Copyright (c) 2013 Nikolay Aleksandrov <nikolay@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef _BOND_OPTIONS_H
+#define _BOND_OPTIONS_H
+
+#define BOND_OPT_VALID(opt) ((opt) < BOND_OPT_LAST)
+#define BOND_MODE_ALL_EX(x) (~(x))
+
+/* Option value types */
+enum {
+ BOND_OPTVAL_INTEGER,
+ BOND_OPTVAL_STRING
+};
+
+/* Option flags:
+ * BOND_OPTFLAG_NOSLAVES - check if the bond device is empty before setting
+ * BOND_OPTFLAG_IFDOWN - check if the bond device is down before setting
+ */
+enum {
+ BOND_OPTFLAG_NOSLAVES = BIT(0),
+ BOND_OPTFLAG_IFDOWN = BIT(1)
+};
+
+/* Value type flags:
+ * BOND_VALFLAG_DEFAULT - mark the value as default
+ * BOND_VALFLAG_(MIN|MAX) - mark the value as min/max
+ */
+enum {
+ BOND_VALFLAG_DEFAULT = BIT(0),
+ BOND_VALFLAG_MIN = BIT(1),
+ BOND_VALFLAG_MAX = BIT(2)
+};
+
+/* Option IDs, their bit positions correspond to their IDs */
+enum {
+ BOND_OPT_LAST
+};
+
+struct bond_value_tbl {
+ char *name;
+ int value;
+ u32 flags;
+};
+
+struct bonding;
+
+struct bond_option {
+ int id;
+ char *name;
+ char *desc;
+ u32 flags;
+ u8 valtype;
+
+ /* unsuppmodes is used to denote modes in which the option isn't
+ * supported.
+ */
+ unsigned long unsuppmodes;
+ /* supported values which this option can have, can be a subset of
+ * BOND_OPTVAL_RANGE's value range
+ */
+ struct bond_value_tbl *values;
+
+ int (*set)(struct bonding *bond, void *newval);
+};
+
+void bond_opt_init(void);
+int __bond_opt_set(struct bonding *bond, unsigned int option, char *buf);
+int __bond_opt_intset(struct bonding *bond, unsigned int option, int value);
+int bond_opt_tryset_rtnl(struct bonding *bond, unsigned int option, char *buf);
+struct bond_value_tbl *bond_opt_parse(const struct bond_option *opt,
+ void *bufarg, int *retval, bool string);
+struct bond_option *bond_opt_get(unsigned int option);
+struct bond_value_tbl *bond_opt_get_val(unsigned int option, int val);
+#endif /* _BOND_OPTIONS_H */
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 955dc48..71e751a 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -25,6 +25,7 @@
#include <linux/etherdevice.h>
#include "bond_3ad.h"
#include "bond_alb.h"
+#include "bond_options.h"
#define DRV_VERSION "3.7.1"
#define DRV_RELDATE "April 27, 2011"
--
1.8.1.4
^ permalink raw reply related
* Re: [PATCH net-next v3 2/9] xen-netback: Change TX path from grant copy to mapping
From: Wei Liu @ 2014-01-10 13:16 UTC (permalink / raw)
To: Zoltan Kiss
Cc: Wei Liu, ian.campbell, xen-devel, netdev, linux-kernel,
jonathan.davies
In-Reply-To: <20140110114534.GE29180@zion.uk.xensource.com>
On Fri, Jan 10, 2014 at 11:45:34AM +0000, Wei Liu wrote:
> On Fri, Jan 10, 2014 at 11:35:08AM +0000, Zoltan Kiss wrote:
> [...]
> >
> > >>@@ -920,6 +852,18 @@ static int xenvif_tx_check_gop(struct xenvif *vif,
> > >> err = gop->status;
> > >> if (unlikely(err))
> > >> xenvif_idx_release(vif, pending_idx, XEN_NETIF_RSP_ERROR);
> > >>+ else {
> > >>+ if (vif->grant_tx_handle[pending_idx] !=
> > >>+ NETBACK_INVALID_HANDLE) {
> > >>+ netdev_err(vif->dev,
> > >>+ "Stale mapped handle! pending_idx %x handle %x\n",
> > >>+ pending_idx, vif->grant_tx_handle[pending_idx]);
> > >>+ BUG();
> > >>+ }
> > >>+ set_phys_to_machine(idx_to_pfn(vif, pending_idx),
> > >>+ FOREIGN_FRAME(gop->dev_bus_addr >> PAGE_SHIFT));
> > >
> > >What happens when you don't have this?
> > Your frags will be filled with garbage. I don't understand exactly
> > what this function does, someone might want to enlighten us? I've
> > took it's usage from classic kernel.
> > Also, it might be worthwhile to check the return value and BUG if
> > it's false, but I don't know what exactly that return value means.
> >
>
> This is actually part of gnttab_map_refs. As you're using hypercall
> directly this becomes very fragile.
>
To make it clear, set_phys_to_machine is done within m2p_add_override.
Wei.
^ permalink raw reply
* [PATCH net-next] net: vxlan: when lower dev unregisters remove vxlan dev as well
From: Daniel Borkmann @ 2014-01-10 13:01 UTC (permalink / raw)
To: davem; +Cc: netdev
We can create a vxlan device with an explicit underlying carrier.
In that case, when the carrier link is being deleted from the
system (e.g. due to module unload) we should also clean up all
created vxlan devices on top of it since otherwise we're in an
inconsistent state in vxlan device. In that case, the user needs
to remove all such devices, while in case of other virtual devs
that sit on top of physical ones, it is usually the case that
these devices do unregister automatically as well and do not
leave the burden on the user.
This work is not necessary when vxlan device was not created with
a real underlying device, as connections can resume in that case
when driver is plugged again. But at least for the other cases,
we should go ahead and do the cleanup on removal.
`ip -d link show vxlan13` after carrier removal before this patch:
5: vxlan13: <BROADCAST,MULTICAST> mtu 1450 qdisc noop state DOWN mode DEFAULT group default
link/ether 1e:47:da:6d:4d:99 brd ff:ff:ff:ff:ff:ff promiscuity 0
vxlan id 13 group 239.0.0.10 dev 2 port 32768 61000 ageing 300
^^^^^
While at it, we should also use vxlan_dellink() handler in
vxlan_exit_net() as otherwise we seem to leak memory on module
unload since only half of the cleanup is being done.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
drivers/net/vxlan.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 63 insertions(+), 7 deletions(-)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 481f85d..39035ba 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -2656,6 +2656,54 @@ static struct rtnl_link_ops vxlan_link_ops __read_mostly = {
.fill_info = vxlan_fill_info,
};
+static void vxlan_handle_lowerdev_unregister(struct vxlan_net *vn,
+ struct net_device *dev)
+{
+ struct vxlan_dev *vxlan, *next;
+ LIST_HEAD(list_kill);
+
+ BUG_ON(!rtnl_is_locked());
+
+ list_for_each_entry_safe(vxlan, next, &vn->vxlan_list, next) {
+ struct vxlan_rdst *dst = &vxlan->default_dst;
+
+ /* In case we created vxlan device with carrier
+ * and we loose the carrier due to module unload
+ * we also need to remove vxlan device. In other
+ * cases, it's not necessary and remote_ifindex
+ * is 0 here, so no matches.
+ */
+ if (dst->remote_ifindex == dev->ifindex)
+ vxlan_dellink(vxlan->dev, &list_kill);
+ }
+
+ unregister_netdevice_many(&list_kill);
+ list_del(&list_kill);
+}
+
+static int vxlan_lowerdev_event(struct notifier_block *unused,
+ unsigned long event, void *ptr)
+{
+ struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+ struct vxlan_net *vn = net_generic(dev_net(dev), vxlan_net_id);
+
+ switch (event) {
+ case NETDEV_UNREGISTER:
+ /* Twiddle thumbs on netns device moves. */
+ if (dev->reg_state != NETREG_UNREGISTERING)
+ break;
+
+ vxlan_handle_lowerdev_unregister(vn, dev);
+ break;
+ }
+
+ return NOTIFY_DONE;
+}
+
+static struct notifier_block vxlan_notifier_block __read_mostly = {
+ .notifier_call = vxlan_lowerdev_event,
+};
+
static __net_init int vxlan_init_net(struct net *net)
{
struct vxlan_net *vn = net_generic(net, vxlan_net_id);
@@ -2673,14 +2721,16 @@ static __net_init int vxlan_init_net(struct net *net)
static __net_exit void vxlan_exit_net(struct net *net)
{
struct vxlan_net *vn = net_generic(net, vxlan_net_id);
- struct vxlan_dev *vxlan;
- LIST_HEAD(list);
+ struct vxlan_dev *vxlan, *next;
+ LIST_HEAD(list_kill);
rtnl_lock();
- list_for_each_entry(vxlan, &vn->vxlan_list, next)
- unregister_netdevice_queue(vxlan->dev, &list);
- unregister_netdevice_many(&list);
+ list_for_each_entry_safe(vxlan, next, &vn->vxlan_list, next)
+ vxlan_dellink(vxlan->dev, &list_kill);
+ unregister_netdevice_many(&list_kill);
rtnl_unlock();
+
+ list_del(&list_kill);
}
static struct pernet_operations vxlan_net_ops = {
@@ -2704,12 +2754,17 @@ static int __init vxlan_init_module(void)
if (rc)
goto out1;
- rc = rtnl_link_register(&vxlan_link_ops);
+ rc = register_netdevice_notifier(&vxlan_notifier_block);
if (rc)
goto out2;
- return 0;
+ rc = rtnl_link_register(&vxlan_link_ops);
+ if (rc)
+ goto out3;
+ return 0;
+out3:
+ unregister_netdevice_notifier(&vxlan_notifier_block);
out2:
unregister_pernet_device(&vxlan_net_ops);
out1:
@@ -2721,6 +2776,7 @@ late_initcall(vxlan_init_module);
static void __exit vxlan_cleanup_module(void)
{
rtnl_link_unregister(&vxlan_link_ops);
+ unregister_netdevice_notifier(&vxlan_notifier_block);
destroy_workqueue(vxlan_wq);
unregister_pernet_device(&vxlan_net_ops);
rcu_barrier();
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next] net: make dev_set_mtu() honor notification return code
From: Veaceslav Falico @ 2014-01-10 12:48 UTC (permalink / raw)
To: netdev
Cc: Veaceslav Falico, Jiri Pirko, David S. Miller, Eric Dumazet,
Alexander Duyck, Nicolas Dichtel
Currently, after changing the MTU for a device, dev_set_mtu() calls
NETDEV_CHANGEMTU notification, however doesn't verify it's return code -
which can be NOTIFY_BAD - i.e. some of the net notifier blocks refused this
change, and continues nevertheless.
To fix this, verify the return code, and if it's an error - then revert the
MTU to the original one, and pass the error code.
CC: Jiri Pirko <jiri@resnulli.us>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Alexander Duyck <alexander.h.duyck@intel.com>
CC: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
net/core/dev.c | 29 ++++++++++++++++++++---------
1 file changed, 20 insertions(+), 9 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index ce01847..1c570ff 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5287,6 +5287,17 @@ int dev_change_flags(struct net_device *dev, unsigned int flags)
}
EXPORT_SYMBOL(dev_change_flags);
+static int __dev_set_mtu(struct net_device *dev, int new_mtu)
+{
+ const struct net_device_ops *ops = dev->netdev_ops;
+
+ if (ops->ndo_change_mtu)
+ return ops->ndo_change_mtu(dev, new_mtu);
+
+ dev->mtu = new_mtu;
+ return 0;
+}
+
/**
* dev_set_mtu - Change maximum transfer unit
* @dev: device
@@ -5296,8 +5307,7 @@ EXPORT_SYMBOL(dev_change_flags);
*/
int dev_set_mtu(struct net_device *dev, int new_mtu)
{
- const struct net_device_ops *ops = dev->netdev_ops;
- int err;
+ int err, orig_mtu;
if (new_mtu == dev->mtu)
return 0;
@@ -5309,14 +5319,15 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
if (!netif_device_present(dev))
return -ENODEV;
- err = 0;
- if (ops->ndo_change_mtu)
- err = ops->ndo_change_mtu(dev, new_mtu);
- else
- dev->mtu = new_mtu;
+ orig_mtu = dev->mtu;
+ err = __dev_set_mtu(dev, new_mtu);
- if (!err)
- call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
+ if (!err) {
+ err = call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
+ err = notifier_to_errno(err);
+ if (err)
+ __dev_set_mtu(dev, orig_mtu);
+ }
return err;
}
EXPORT_SYMBOL(dev_set_mtu);
--
1.8.4
^ permalink raw reply related
* DiePresse.com Mein Freund, der Fahrradhändler
From: franklinbuthelezi1 @ 2014-01-10 12:33 UTC (permalink / raw)
To: netdev
Folgender Artikel auf http://diepresse.com wird Ihnen von Franklin Buthelezi empfohlen:
Mein Freund, der Fahrradhändler
Manche Dinge verändern sich über die langen Wintermonate hinweg.
http://diepresse.com/home/leben/mode/kolumnezumtag/548717/index.do
Notiz:
Dear friend,
I am the international operations manager of ABSA bank South Africa. My name is Franklin Buthelezi. I have a sensitive and private offer from the top executive to seek your partnership in re-profiling some offshore investment funds worth 11.5M U.S.D (Eleven Million five hundred thousand United States Dollars) I am constrained however to withhold most of the details for now. This is a legitimate transaction.
If we agree on the terms you will be given 15% of the total funds, if you are interested please write back via email providing me your personal details and phone number for further directives.
If you are interested and willing to render your assistance please respond via my private email address stated below.
I look forward to your response.
Best regards
Franklin Buthelezi
Email: franklinbuthelezi1@yahoo.com
------------------------------------
DiePresse.com übernimmt keine Haftung für den Inhalt der persönlichen Nachricht.
© DiePresse.com - http://diepresse.com
^ permalink raw reply
* Re: [PATCH net] bonding: reset the slave's mtu when its be changed
From: Veaceslav Falico @ 2014-01-10 12:19 UTC (permalink / raw)
To: Ding Tianhong; +Cc: Jay Vosburgh, Netdev, David S. Miller
In-Reply-To: <52CFDA63.8070601@huawei.com>
On Fri, Jan 10, 2014 at 07:32:51PM +0800, Ding Tianhong wrote:
>All slave should have the same mtu with mastet's, and the bond do it when
>enslave the slave, but the user could change the slave's mtu, it will cause
>the master and slave have different mtu, althrough in AB mode, it does not
>matter if the slave is not the current slave, but in other mode, it is incorrect,
>so reset the slave's mtu like the master set.
Why "net"? It's not a bugfix, it's a feature, and really discussable.
Also, wrt the actual change - why do you think it's incorrect for slaves in
bonding mode other than AB to have different MTU values? I don't see any
reason for it, from the top of the head.
>
>Cc: Jay Vosburgh <fubar@us.ibm.com>
>Cc: Veaceslav Falico <vfalico@redhat.com>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bond_main.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 398e299..e7b5bcf 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,
> */
> break;
> case NETDEV_CHANGEMTU:
>- /*
>- * TODO: Should slaves be allowed to
>- * independently alter their MTU? For
>- * an active-backup bond, slaves need
>- * not be the same type of device, so
>- * MTUs may vary. For other modes,
>- * slaves arguably should have the
>- * same MTUs. To do this, we'd need to
>- * take over the slave's change_mtu
>- * function for the duration of their
>- * servitude.
>+ /* All slave should have the same mtu
>+ * as master.
> */
>+ if (slave->dev->mtu != bond->dev->mtu) {
If we've got the event then it means it was changed to something different.
No need to verify.
>+ int res;
>+ slave->original_mtu = slave->dev->mtu;
If we're refusing to apply the *new* mtu, then why should we save it as the
original? The original_mtu is the mtu that the slave had before it was
enslaved.
>+ res = dev_set_mtu(slave->dev, bond->dev->mtu);
>+ if (res)
>+ pr_debug("Error %d calling dev_set_mtu for slave %s\n",
>+ res, slave->dev->name);
>+ }
Also, bonding should be vocal about changing forcibly the mtu - otherwise
we'd end up with silently dropping the changes:
ifconfig eth0 mtu 9000
echo $?
-> 0
ifconfig eth0
MTU: 1500
or something like that, it will pass it up, refusing changes:
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index e06c445..0b36045 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2846,19 +2846,8 @@ static int bond_slave_netdev_event(unsigned long event,
*/
break;
case NETDEV_CHANGEMTU:
- /*
- * TODO: Should slaves be allowed to
- * independently alter their MTU? For
- * an active-backup bond, slaves need
- * not be the same type of device, so
- * MTUs may vary. For other modes,
- * slaves arguably should have the
- * same MTUs. To do this, we'd need to
- * take over the slave's change_mtu
- * function for the duration of their
- * servitude.
- */
- break;
+ /* don't permit slaves to change their MTU */
+ return NOTIFY_BAD;
case NETDEV_CHANGENAME:
/*
* TODO: handle changing the primary's name
> break;
> case NETDEV_CHANGENAME:
> /*
>--
>1.8.0
>
>
^ permalink raw reply related
* Re: [PATCH net-next 1/4] bonding: update the primary when slave name changed
From: Ding Tianhong @ 2014-01-10 11:55 UTC (permalink / raw)
To: Veaceslav Falico; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <20140110111143.GB4132@redhat.com>
On 2014/1/10 19:11, Veaceslav Falico wrote:
> On Fri, Jan 10, 2014 at 07:05:34PM +0800, Ding Tianhong wrote:
>> On 2014/1/10 15:44, Veaceslav Falico wrote:
>>> If it's not the primray
>>> slave, and we don't have one - select it as a new primary and, again, see
>>> if we need to select a new active slave.
>>
>> I don't think so , I think if it is not the primary slave and we don't have one,
>> no need to do anything, just a normal slave change its name.
>
> If primary == "my_eth0", you have 2 slaves - "eth0" and "eth1", thus null
> primary_slave, and rename eth0 to my_eth0 - then you need to set
> primary_slave to my_eth0.
>
> If primary == "my_eth0", you have 2 slaves - "my_eth0" and "eth1", thus
> primary_slave == dev with name "my_eth0", and rename "my_eth0" to "eth0" -
> then you must set primary_slave to NULL.
>
> And after either of these you must see if you need to re-select the active
> slave, as it might have been forced by the primary_slave, which has been
> modified.
>
> You might also want to add some pr_info() about adding/removing
> primary_slave, as the user to be aware.
>
Ok thanks.
Regards
Ding
>
^ permalink raw reply
* Re: [PATCH net-next v3 2/9] xen-netback: Change TX path from grant copy to mapping
From: Wei Liu @ 2014-01-10 11:45 UTC (permalink / raw)
To: Zoltan Kiss
Cc: Wei Liu, ian.campbell, xen-devel, netdev, linux-kernel,
jonathan.davies
In-Reply-To: <52CFDAEC.5080708@citrix.com>
On Fri, Jan 10, 2014 at 11:35:08AM +0000, Zoltan Kiss wrote:
[...]
>
> >>@@ -920,6 +852,18 @@ static int xenvif_tx_check_gop(struct xenvif *vif,
> >> err = gop->status;
> >> if (unlikely(err))
> >> xenvif_idx_release(vif, pending_idx, XEN_NETIF_RSP_ERROR);
> >>+ else {
> >>+ if (vif->grant_tx_handle[pending_idx] !=
> >>+ NETBACK_INVALID_HANDLE) {
> >>+ netdev_err(vif->dev,
> >>+ "Stale mapped handle! pending_idx %x handle %x\n",
> >>+ pending_idx, vif->grant_tx_handle[pending_idx]);
> >>+ BUG();
> >>+ }
> >>+ set_phys_to_machine(idx_to_pfn(vif, pending_idx),
> >>+ FOREIGN_FRAME(gop->dev_bus_addr >> PAGE_SHIFT));
> >
> >What happens when you don't have this?
> Your frags will be filled with garbage. I don't understand exactly
> what this function does, someone might want to enlighten us? I've
> took it's usage from classic kernel.
> Also, it might be worthwhile to check the return value and BUG if
> it's false, but I don't know what exactly that return value means.
>
This is actually part of gnttab_map_refs. As you're using hypercall
directly this becomes very fragile.
So the right thing to do is to fix gnttab_map_refs.
> >
> >> if (skb_is_nonlinear(skb) && skb_headlen(skb) < PKT_PROT_LEN) {
> >> int target = min_t(int, skb->len, PKT_PROT_LEN);
> >>@@ -1581,6 +1541,8 @@ static int xenvif_tx_submit(struct xenvif *vif)
> >> if (checksum_setup(vif, skb)) {
> >> netdev_dbg(vif->dev,
> >> "Can't setup checksum in net_tx_action\n");
> >>+ if (skb_shinfo(skb)->destructor_arg)
> >>+ skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
> >
> >Do you still care setting the flag even if this skb is not going to be
> >delivered? If so can you state clearly the reason just like the
> >following hunk?
> Of course, otherwise the pages wouldn't be sent back to the guest.
> I've added a comment.
>
OK, Thanks! That means whenever SKB leaves netback we need to add this
flag.
> >>@@ -1715,7 +1685,7 @@ static inline void xenvif_tx_dealloc_action(struct xenvif *vif)
> >> int xenvif_tx_action(struct xenvif *vif, int budget)
> >> {
> >> unsigned nr_gops;
> >>- int work_done;
> >>+ int work_done, ret;
> >>
> >> if (unlikely(!tx_work_todo(vif)))
> >> return 0;
> >>@@ -1725,7 +1695,10 @@ int xenvif_tx_action(struct xenvif *vif, int budget)
> >> if (nr_gops == 0)
> >> return 0;
> >>
> >>- gnttab_batch_copy(vif->tx_copy_ops, nr_gops);
> >>+ ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref,
> >>+ vif->tx_map_ops,
> >>+ nr_gops);
> >
> >Why do you need to replace gnttab_batch_copy with hypercall? In the
> >ideal situation gnttab_batch_copy should behave the same as directly
> >hypercall but it also handles GNTST_eagain for you.
>
> I don't need gnttab_batch_copy at all, I'm using the grant mapping
> hypercall here.
>
Oops, my bad! Ignore that one.
Wei.
> Regards,
>
> Zoli
^ permalink raw reply
* [PATCH net] bonding: reset the slave's mtu when its be changed
From: Ding Tianhong @ 2014-01-10 11:32 UTC (permalink / raw)
To: Jay Vosburgh, Veaceslav Falico, Netdev, David S. Miller
All slave should have the same mtu with mastet's, and the bond do it when
enslave the slave, but the user could change the slave's mtu, it will cause
the master and slave have different mtu, althrough in AB mode, it does not
matter if the slave is not the current slave, but in other mode, it is incorrect,
so reset the slave's mtu like the master set.
Cc: Jay Vosburgh <fubar@us.ibm.com>
Cc: Veaceslav Falico <vfalico@redhat.com>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
drivers/net/bonding/bond_main.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 398e299..e7b5bcf 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -2882,18 +2882,17 @@ static int bond_slave_netdev_event(unsigned long event,
*/
break;
case NETDEV_CHANGEMTU:
- /*
- * TODO: Should slaves be allowed to
- * independently alter their MTU? For
- * an active-backup bond, slaves need
- * not be the same type of device, so
- * MTUs may vary. For other modes,
- * slaves arguably should have the
- * same MTUs. To do this, we'd need to
- * take over the slave's change_mtu
- * function for the duration of their
- * servitude.
+ /* All slave should have the same mtu
+ * as master.
*/
+ if (slave->dev->mtu != bond->dev->mtu) {
+ int res;
+ slave->original_mtu = slave->dev->mtu;
+ res = dev_set_mtu(slave->dev, bond->dev->mtu);
+ if (res)
+ pr_debug("Error %d calling dev_set_mtu for slave %s\n",
+ res, slave->dev->name);
+ }
break;
case NETDEV_CHANGENAME:
/*
--
1.8.0
^ permalink raw reply related
* Re: [PATCH net-next v3 2/9] xen-netback: Change TX path from grant copy to mapping
From: Zoltan Kiss @ 2014-01-10 11:35 UTC (permalink / raw)
To: Wei Liu; +Cc: ian.campbell, xen-devel, netdev, linux-kernel, jonathan.davies
In-Reply-To: <20140109153015.GF12164@zion.uk.xensource.com>
On 09/01/14 15:30, Wei Liu wrote:
> On Wed, Jan 08, 2014 at 12:10:11AM +0000, Zoltan Kiss wrote:
>> v3:
>> - delete a surplus checking from tx_action
>> - remove stray line
>> - squash xenvif_idx_unmap changes into the first patch
>> - init spinlocks
>> - call map hypercall directly instead of gnttab_map_refs()
>
> I suppose this is to avoid touching m2p override as well, just as
> previous patch uses unmap hypercall directly.
Yes.
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -122,7 +122,9 @@ static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)
>> BUG_ON(skb->dev != dev);
>>
>> /* Drop the packet if vif is not ready */
>> - if (vif->task == NULL || !xenvif_schedulable(vif))
>> + if (vif->task == NULL ||
>> + vif->dealloc_task == NULL ||
>> + !xenvif_schedulable(vif))
>
> Indentation.
Fixed, and the later ones as well
>> @@ -920,6 +852,18 @@ static int xenvif_tx_check_gop(struct xenvif *vif,
>> err = gop->status;
>> if (unlikely(err))
>> xenvif_idx_release(vif, pending_idx, XEN_NETIF_RSP_ERROR);
>> + else {
>> + if (vif->grant_tx_handle[pending_idx] !=
>> + NETBACK_INVALID_HANDLE) {
>> + netdev_err(vif->dev,
>> + "Stale mapped handle! pending_idx %x handle %x\n",
>> + pending_idx, vif->grant_tx_handle[pending_idx]);
>> + BUG();
>> + }
>> + set_phys_to_machine(idx_to_pfn(vif, pending_idx),
>> + FOREIGN_FRAME(gop->dev_bus_addr >> PAGE_SHIFT));
>
> What happens when you don't have this?
Your frags will be filled with garbage. I don't understand exactly what
this function does, someone might want to enlighten us? I've took it's
usage from classic kernel.
Also, it might be worthwhile to check the return value and BUG if it's
false, but I don't know what exactly that return value means.
>
>> if (skb_is_nonlinear(skb) && skb_headlen(skb) < PKT_PROT_LEN) {
>> int target = min_t(int, skb->len, PKT_PROT_LEN);
>> @@ -1581,6 +1541,8 @@ static int xenvif_tx_submit(struct xenvif *vif)
>> if (checksum_setup(vif, skb)) {
>> netdev_dbg(vif->dev,
>> "Can't setup checksum in net_tx_action\n");
>> + if (skb_shinfo(skb)->destructor_arg)
>> + skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
>
> Do you still care setting the flag even if this skb is not going to be
> delivered? If so can you state clearly the reason just like the
> following hunk?
Of course, otherwise the pages wouldn't be sent back to the guest. I've
added a comment.
>> @@ -1715,7 +1685,7 @@ static inline void xenvif_tx_dealloc_action(struct xenvif *vif)
>> int xenvif_tx_action(struct xenvif *vif, int budget)
>> {
>> unsigned nr_gops;
>> - int work_done;
>> + int work_done, ret;
>>
>> if (unlikely(!tx_work_todo(vif)))
>> return 0;
>> @@ -1725,7 +1695,10 @@ int xenvif_tx_action(struct xenvif *vif, int budget)
>> if (nr_gops == 0)
>> return 0;
>>
>> - gnttab_batch_copy(vif->tx_copy_ops, nr_gops);
>> + ret = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref,
>> + vif->tx_map_ops,
>> + nr_gops);
>
> Why do you need to replace gnttab_batch_copy with hypercall? In the
> ideal situation gnttab_batch_copy should behave the same as directly
> hypercall but it also handles GNTST_eagain for you.
I don't need gnttab_batch_copy at all, I'm using the grant mapping
hypercall here.
Regards,
Zoli
^ permalink raw reply
* Re: [net-next 14/16] i40e: enable PTP
From: Richard Cochran @ 2014-01-10 11:26 UTC (permalink / raw)
To: Jeff Kirsher
Cc: davem, Jacob Keller, netdev, gospo, sassmann, Jesse Brandeburg
In-Reply-To: <1389344336-1558-15-git-send-email-jeffrey.t.kirsher@intel.com>
On Fri, Jan 10, 2014 at 12:58:54AM -0800, Jeff Kirsher wrote:
> +/**
> + * i40e_ptp_enable - Enable/disable ancillary features of the PHC subsystem
> + * @ptp: The PTP clock structure
> + * @rq: The requested feature to change
> + * @on: Enable/disable flag
> + *
> + * The XL710 does not support any of the ancillary features of the PHY
> + * subsystem, so this function may just return.
> + **/
Typo, PHC subsystem? Otherwise, the driver looks good to me.
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next 1/4] bonding: update the primary when slave name changed
From: Veaceslav Falico @ 2014-01-10 11:11 UTC (permalink / raw)
To: Ding Tianhong; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <52CFD3FE.9000805@huawei.com>
On Fri, Jan 10, 2014 at 07:05:34PM +0800, Ding Tianhong wrote:
>On 2014/1/10 15:44, Veaceslav Falico wrote:
>> If it's not the primray
>> slave, and we don't have one - select it as a new primary and, again, see
>> if we need to select a new active slave.
>
>I don't think so , I think if it is not the primary slave and we don't have one,
>no need to do anything, just a normal slave change its name.
If primary == "my_eth0", you have 2 slaves - "eth0" and "eth1", thus null
primary_slave, and rename eth0 to my_eth0 - then you need to set
primary_slave to my_eth0.
If primary == "my_eth0", you have 2 slaves - "my_eth0" and "eth1", thus
primary_slave == dev with name "my_eth0", and rename "my_eth0" to "eth0" -
then you must set primary_slave to NULL.
And after either of these you must see if you need to re-select the active
slave, as it might have been forced by the primary_slave, which has been
modified.
You might also want to add some pr_info() about adding/removing
primary_slave, as the user to be aware.
^ permalink raw reply
* Re: [PATCH net-next 1/4] bonding: update the primary when slave name changed
From: Ding Tianhong @ 2014-01-10 11:05 UTC (permalink / raw)
To: Veaceslav Falico; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <20140110074433.GA26273@redhat.com>
On 2014/1/10 15:44, Veaceslav Falico wrote:
> On Fri, Jan 10, 2014 at 12:20:45PM +0800, Ding Tianhong wrote:
>> On 2014/1/9 20:30, Veaceslav Falico wrote:
>>> On Thu, Jan 09, 2014 at 08:23:58PM +0800, Ding Tianhong wrote:
>>>> On 2014/1/9 19:46, Veaceslav Falico wrote:
>>>>> On Thu, Jan 09, 2014 at 07:20:36PM +0800, Ding Tianhong wrote:
>>>>>> If the primary_slave's name changed, but the bond->prams.primay was
>>>>>> still using the old name, it is conflict with the meaning of the
>>>>>> primary, so update the primary when the slave change its name.
>>>>>
>>>>> Nope, the bonding parameter, which is set by the user, shouldn't change
>>>>> because of an interface name change.
>>>>>
>>>> Yes, I know what you mean, but it is not bug fix, just make it more better,
>>>> do not you feel it strange that the primary was different with primary_slave's name?
>>>
>>> Yep, that's an issue - that's why there is the TODO. We shouldn't, though,
>>> change the primary param, but rather check if the slave (that changed name)
>>> is (already not) eligible for primary_slave.
>>>
>>
>> Ok，So，summarize your and my opinion, I think there are two ways to fix this:
>>
>> 1. just like my patch said.
>
> No, the primary string is user-set, and it should *not* be changed by
> kernel.
>
>> 2. check if the primary is not the primary_slave, make the primary_slave = NULL, this means
>> the primary_slave is no valid.
>
> Check the slave that changed name - if it's the primary slave, remove it,
> and see if we need to select the new active slave.
Ok, agree.
> If it's not the primray
> slave, and we don't have one - select it as a new primary and, again, see
> if we need to select a new active slave.
I don't think so , I think if it is not the primary slave and we don't have one,
no need to do anything, just a normal slave change its name.
Regards
Ding
>
>> 3. ?? did you have any better ideas?
>>
>> Regards
>> Ding
>>
>>>>
>>>
>>> .
>>>
>>
>>
>
> .
>
^ permalink raw reply
* [PATCH v4 net-next 3/3] bonding: fix __get_active_agg() RCU logic
From: Veaceslav Falico @ 2014-01-10 10:59 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, dingtianhong, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389351585-19615-1-git-send-email-vfalico@redhat.com>
Currently, the implementation is meaningless - once again, we take the
slave structure and use it after we've exited RCU critical section.
Fix this by removing the rcu_read_lock() from __get_active_agg(), and
ensuring that all its callers are holding RCU.
Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
CC: dingtianhong@huawei.com
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
Notes:
v2 -> v3:
Use the RCU primitives.
v1 -> v2:
Don't use RCU primitives as we can hold RTNL.
drivers/net/bonding/bond_3ad.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index b49f421..cce1f1b 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -678,6 +678,8 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
/**
* __get_active_agg - get the current active aggregator
* @aggregator: the aggregator we're looking at
+ *
+ * Caller must hold RCU lock.
*/
static struct aggregator *__get_active_agg(struct aggregator *aggregator)
{
@@ -685,13 +687,9 @@ static struct aggregator *__get_active_agg(struct aggregator *aggregator)
struct list_head *iter;
struct slave *slave;
- rcu_read_lock();
bond_for_each_slave_rcu(bond, slave, iter)
- if (SLAVE_AD_INFO(slave).aggregator.is_active) {
- rcu_read_unlock();
+ if (SLAVE_AD_INFO(slave).aggregator.is_active)
return &(SLAVE_AD_INFO(slave).aggregator);
- }
- rcu_read_unlock();
return NULL;
}
@@ -1499,11 +1497,11 @@ static void ad_agg_selection_logic(struct aggregator *agg)
struct slave *slave;
struct port *port;
+ rcu_read_lock();
origin = agg;
active = __get_active_agg(agg);
best = (active && agg_device_up(active)) ? active : NULL;
- rcu_read_lock();
bond_for_each_slave_rcu(bond, slave, iter) {
agg = &(SLAVE_AD_INFO(slave).aggregator);
--
1.8.4
^ permalink raw reply related
* [PATCH v4 net-next 2/3] bonding: fix __get_first_agg RCU usage
From: Veaceslav Falico @ 2014-01-10 10:59 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, dingtianhong, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389351585-19615-1-git-send-email-vfalico@redhat.com>
Currently, the RCU read lock usage is just wrong - it gets the slave struct
under RCU and continues to use it when RCU lock is released.
However, it's still safe to do this cause we didn't need the
rcu_read_lock() initially - all of the __get_first_agg() callers are either
holding RCU read lock or the RTNL lock, so that we can't sync while in it.
Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
CC: dingtianhong@huawei.com
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
Notes:
v3 -> v4: add rcu_read_lock() to silence lockdep.
v2 -> v3:
Use the rcu primitives.
v1 -> v2:
Don't use RCU primitives as we can hold RTNL.
drivers/net/bonding/bond_3ad.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index da0d7c5..b49f421 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -143,11 +143,13 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
*
* Return the aggregator of the first slave in @bond, or %NULL if it can't be
* found.
+ * The caller must hold RCU or RTNL lock.
*/
static inline struct aggregator *__get_first_agg(struct port *port)
{
struct bonding *bond = __get_bond_by_port(port);
struct slave *first_slave;
+ struct aggregator *agg;
/* If there's no bond for this port, or bond has no slaves */
if (bond == NULL)
@@ -155,9 +157,10 @@ static inline struct aggregator *__get_first_agg(struct port *port)
rcu_read_lock();
first_slave = bond_first_slave_rcu(bond);
+ agg = first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
rcu_read_unlock();
- return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
+ return agg;
}
/**
--
1.8.4
^ permalink raw reply related
* [PATCH v4 net-next 1/3] bonding: fix bond_3ad_set_carrier() RCU usage
From: Veaceslav Falico @ 2014-01-10 10:59 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, dingtianhong, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389351585-19615-1-git-send-email-vfalico@redhat.com>
Currently, its usage is just plainly wrong. It first gets a slave under
RCU, and, after releasing the RCU lock, continues to use it - whilst it can
be freed.
Fix this by ensuring that bond_3ad_set_carrier() holds RCU till it uses its
slave (or its agg).
Fixes: be79bd048ab ("bonding: add RCU for bond_3ad_state_machine_handler()")
CC: dingtianhong@huawei.com
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
Notes:
v3 -> v4:
Remove the useless goto out.
v2 -> v3:
Just wrap RCU for the whole usage of our slave.
v1 -> v2:
Don't use _rcu primitives as we can be called under RTNL too.
v2 -> v3:
Just wrap RCU for the whole usage of our slave.
v1 -> v2:
Don't use _rcu primitives as we can be called under RTNL too.
v1 -> v2:
Don't use _rcu primitives as we can be called under RTNL too.
drivers/net/bonding/bond_3ad.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 29db1ca..da0d7c5 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2327,32 +2327,32 @@ int bond_3ad_set_carrier(struct bonding *bond)
{
struct aggregator *active;
struct slave *first_slave;
+ int ret = 1;
rcu_read_lock();
first_slave = bond_first_slave_rcu(bond);
- rcu_read_unlock();
- if (!first_slave)
- return 0;
+ if (!first_slave) {
+ ret = 0;
+ goto out;
+ }
active = __get_active_agg(&(SLAVE_AD_INFO(first_slave).aggregator));
if (active) {
/* are enough slaves available to consider link up? */
if (active->num_of_ports < bond->params.min_links) {
if (netif_carrier_ok(bond->dev)) {
netif_carrier_off(bond->dev);
- return 1;
+ goto out;
}
} else if (!netif_carrier_ok(bond->dev)) {
netif_carrier_on(bond->dev);
- return 1;
+ goto out;
}
- return 0;
- }
-
- if (netif_carrier_ok(bond->dev)) {
+ } else if (netif_carrier_ok(bond->dev)) {
netif_carrier_off(bond->dev);
- return 1;
}
- return 0;
+out:
+ rcu_read_unlock();
+ return ret;
}
/**
--
1.8.4
^ permalink raw reply related
* [PATCH v4 net-next 0/3] bonding: fix bond_3ad RCU usage
From: Veaceslav Falico @ 2014-01-10 10:59 UTC (permalink / raw)
To: netdev; +Cc: dingtianhong, Jay Vosburgh, Andy Gospodarek, Veaceslav Falico
While digging through bond_3ad.c I've found that the RCU usage there is
just wrong - it's used as a kind of mutex/spinlock instead of RCU.
v3->v4: remove useless goto and wrap __get_first_agg() in proper RCU.
v2->v3: make bond_3ad_set_carrier() use RCU read lock for the whole
function, so that all other functions will be protected by RCU as well.
This way we can use _rcu variants everywhere.
v1->v2: use generic primitives instead of _rcu ones cause we can hold RTNL
lock without RCU one, which is still safe.
This patchset is on top of bond_3ad.c cleanup:
http://www.spinics.net/lists/netdev/msg265447.html
CC: dingtianhong@huawei.com
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: netdev@vger.kernel.org
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
^ permalink raw reply
* Re: [PATCH v3 net-next 2/3] bonding: fix __get_first_agg RCU usage
From: Veaceslav Falico @ 2014-01-10 10:53 UTC (permalink / raw)
To: Ding Tianhong; +Cc: netdev, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <52CFCEDD.6030202@huawei.com>
On Fri, Jan 10, 2014 at 06:43:41PM +0800, Ding Tianhong wrote:
>On 2014/1/10 17:18, Veaceslav Falico wrote:
>> - rcu_read_lock();
>> first_slave = bond_first_slave_rcu(bond);
>> - rcu_read_unlock();
>>
>I am afraid the lockdep check will calling some warming:
>bond_3ad_unbind_slave -> __get_first_agg -> bond_first_slave_rcu -> netdev_lower_get_first_private_rcu -> list_first_or_null_rcu
>
>but the bond_3ad_unbind_slave is not in RCU.
Yep, right, I'm always colliding with my next patchset which removes it
completely, so it doesn't whine.
Will resend.
>
>Regards
>Ding
>> return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
>> }
>>
>
>
^ permalink raw reply
* Re: [PATCH v3 net-next 3/3] bonding: fix __get_active_agg() RCU logic
From: Ding Tianhong @ 2014-01-10 10:48 UTC (permalink / raw)
To: Veaceslav Falico, netdev; +Cc: Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389345523-5497-4-git-send-email-vfalico@redhat.com>
On 2014/1/10 17:18, Veaceslav Falico wrote:
> Currently, the implementation is meaningless - once again, we take the
> slave structure and use it after we've exited RCU critical section.
>
> Fix this by removing the rcu_read_lock() from __get_active_agg(), and
> ensuring that all its callers are holding RCU.
>
> Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
> CC: dingtianhong@huawei.com
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>
> Notes:
> v2 -> v3:
> Use the RCU primitives.
>
> v1 -> v2:
> Don't use RCU primitives as we can hold RTNL.
>
> drivers/net/bonding/bond_3ad.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 27dac0e..112afa8 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -674,6 +674,8 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
> /**
> * __get_active_agg - get the current active aggregator
> * @aggregator: the aggregator we're looking at
> + *
> + * Caller must hold RCU lock.
> */
> static struct aggregator *__get_active_agg(struct aggregator *aggregator)
> {
> @@ -681,13 +683,9 @@ static struct aggregator *__get_active_agg(struct aggregator *aggregator)
> struct list_head *iter;
> struct slave *slave;
>
> - rcu_read_lock();
> bond_for_each_slave_rcu(bond, slave, iter)
> - if (SLAVE_AD_INFO(slave).aggregator.is_active) {
> - rcu_read_unlock();
> + if (SLAVE_AD_INFO(slave).aggregator.is_active)
> return &(SLAVE_AD_INFO(slave).aggregator);
> - }
> - rcu_read_unlock();
>
> return NULL;
> }
> @@ -1495,11 +1493,11 @@ static void ad_agg_selection_logic(struct aggregator *agg)
> struct slave *slave;
> struct port *port;
>
> + rcu_read_lock();
> origin = agg;
> active = __get_active_agg(agg);
> best = (active && agg_device_up(active)) ? active : NULL;
>
> - rcu_read_lock();
> bond_for_each_slave_rcu(bond, slave, iter) {
> agg = &(SLAVE_AD_INFO(slave).aggregator);
>
>
Great.
Acked-by: Ding Tianhong <dingtianhong@huawei.com>
^ permalink raw reply
* Re: [PATCH v3 net-next 2/3] bonding: fix __get_first_agg RCU usage
From: Ding Tianhong @ 2014-01-10 10:43 UTC (permalink / raw)
To: Veaceslav Falico, netdev; +Cc: Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389345523-5497-3-git-send-email-vfalico@redhat.com>
On 2014/1/10 17:18, Veaceslav Falico wrote:
> Currently, the RCU read lock usage is just wrong - it gets the slave struct
> under RCU and continues to use it when RCU lock is released.
>
> However, it's still safe to do this cause we didn't need the
> rcu_read_lock() initially - all of the __get_first_agg() callers are either
> holding RCU read lock or the RTNL lock, so that we can't sync while in it.
>
> So, remove the useless rcu locking and add a comment.
>
> Fixes: be79bd048 ("bonding: add RCU for bond_3ad_state_machine_handler()")
> CC: dingtianhong@huawei.com
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>
> Notes:
> v2 -> v3:
> Use the rcu primitives.
>
> v1 -> v2:
> Don't use RCU primitives as we can hold RTNL.
>
> drivers/net/bonding/bond_3ad.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 9ff55eb..27dac0e 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -143,6 +143,7 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
> *
> * Return the aggregator of the first slave in @bond, or %NULL if it can't be
> * found.
> + * The caller must hold RCU lock.
> */
> static inline struct aggregator *__get_first_agg(struct port *port)
> {
> @@ -153,9 +154,7 @@ static inline struct aggregator *__get_first_agg(struct port *port)
> if (bond == NULL)
> return NULL;
>
> - rcu_read_lock();
> first_slave = bond_first_slave_rcu(bond);
> - rcu_read_unlock();
>
I am afraid the lockdep check will calling some warming:
bond_3ad_unbind_slave -> __get_first_agg -> bond_first_slave_rcu -> netdev_lower_get_first_private_rcu -> list_first_or_null_rcu
but the bond_3ad_unbind_slave is not in RCU.
Regards
Ding
> return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
> }
>
^ permalink raw reply
* RE: [PATCH v2 3/4] net: make tcp_cleanup_rbuf private
From: David Laight @ 2014-01-10 10:38 UTC (permalink / raw)
To: 'David Miller', dan.j.williams@intel.com
Cc: ncardwell@google.com, dmaengine@vger.kernel.org,
yoshfuji@linux-ipv6.org, netdev@vger.kernel.org,
jmorris@namei.org, kuznet@ms2.inr.ac.ru, kaber@trash.net,
linux-kernel@vger.kernel.org
In-Reply-To: <20140109.154232.65818281679170677.davem@davemloft.net>
From: David Miller
...
> > On Thu, Jan 9, 2014 at 12:26 PM, Neal Cardwell <ncardwell@google.com> wrote:
> >> On Thu, Jan 9, 2014 at 3:16 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> >>> net_dma was the only external user so this can become local to tcp.c
> >>> again.
> >> ...
> >>> -void tcp_cleanup_rbuf(struct sock *sk, int copied)
> >>> +static void cleanup_rbuf(struct sock *sk, int copied)
> >>
> >> I would vote to keep the tcp_ prefix. In the TCP code base that is the
> >> more common idiom, even for internal/static TCP functions, and
> >> personally I find it easier to read and work with in stack traces,
> >> etc. My 2 cents.
> >>
> >
> > Ok. It was cleanup_rbuf() in a former life, but one vote for leaving
> > the name as is is enough for me.
>
> You can make that two votes :)
I suspect DM adds 2000 votes :-)
Keeping the prefix makes it easier to grep for, and makes it more
obvious that it isn't a generic function (when being called).
David
^ permalink raw reply
* Re: [net-next 05/16] i40e: fix long lines
From: Joe Perches @ 2014-01-10 10:35 UTC (permalink / raw)
To: David Laight
Cc: Jeff Kirsher, davem@davemloft.net, Mitch Williams,
netdev@vger.kernel.org, gospo@redhat.com, sassmann@redhat.com,
Jesse Brandeburg
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D457950@AcuExch.aculab.com>
On Fri, 2014-01-10 at 10:02 +0000, David Laight wrote:
> > Another way this could be changed is to put the
> > return value on a separate line like:
> >
> > i40e_status
> > i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 seid, bool set,
> > struct i40e_asq_cmd_details *cmd_details)
>
> Personally I prefer that so I can find the definition by grepping
> for '^function_name' - but it doesn't seem to be done in any linux
> source files.
That form is used in a lot of files.
$ git grep -E "^[a-z_]+\("
^ permalink raw reply
* Re: [PATCH v3 net-next 1/3] bonding: fix bond_3ad_set_carrier() RCU usage
From: Ding Tianhong @ 2014-01-10 10:34 UTC (permalink / raw)
To: Veaceslav Falico, netdev; +Cc: Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389345523-5497-2-git-send-email-vfalico@redhat.com>
On 2014/1/10 17:18, Veaceslav Falico wrote:
> Currently, its usage is just plainly wrong. It first gets a slave under
> RCU, and, after releasing the RCU lock, continues to use it - whilst it can
> be freed.
>
> Fix this by ensuring that bond_3ad_set_carrier() holds RCU till it uses its
> slave (or its agg).
>
> Fixes: be79bd048ab ("bonding: add RCU for bond_3ad_state_machine_handler()")
> CC: dingtianhong@huawei.com
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>
> Notes:
> v2 -> v3:
> Just wrap RCU for the whole usage of our slave.
>
> v1 -> v2:
> Don't use _rcu primitives as we can be called under RTNL too.
>
> v1 -> v2:
> Don't use _rcu primitives as we can be called under RTNL too.
>
> drivers/net/bonding/bond_3ad.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 29db1ca..9ff55eb 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -2327,32 +2327,33 @@ int bond_3ad_set_carrier(struct bonding *bond)
> {
> struct aggregator *active;
> struct slave *first_slave;
> + int ret = 1;
>
> rcu_read_lock();
> first_slave = bond_first_slave_rcu(bond);
> - rcu_read_unlock();
> - if (!first_slave)
> - return 0;
> + if (!first_slave) {
> + ret = 0;
> + goto out;
> + }
> active = __get_active_agg(&(SLAVE_AD_INFO(first_slave).aggregator));
> if (active) {
> /* are enough slaves available to consider link up? */
> if (active->num_of_ports < bond->params.min_links) {
> if (netif_carrier_ok(bond->dev)) {
> netif_carrier_off(bond->dev);
> - return 1;
> + goto out;
> }
> } else if (!netif_carrier_ok(bond->dev)) {
> netif_carrier_on(bond->dev);
> - return 1;
> + goto out;
> }
> - return 0;
> - }
> -
> - if (netif_carrier_ok(bond->dev)) {
> + } else if (netif_carrier_ok(bond->dev)) {
> netif_carrier_off(bond->dev);
> - return 1;
> + goto out;
no need for this line, but it is not a big issue.
Regards
Ding
> }
> - return 0;
> +out:
> + rcu_read_unlock();
> + return ret;
> }
>
> /**
>
^ permalink raw reply
* RE: [PATCH -next] qlcnic: fix compiler warning
From: David Laight @ 2014-01-10 10:15 UTC (permalink / raw)
To: 'Shahed Shaikh', Martin Kaiser, Himanshu Madhani,
Rajesh Borundia
Cc: linux-kernel, trivial@kernel.org, netdev
In-Reply-To: <262CB373A6D1F14F9B81E82F74F77D5A46F52B8E@avmb2.qlogic.org>
From: Shahed Shaikh
> Adding netdev.
>
> > -----Original Message-----
> > From: Martin Kaiser,,, [mailto:martin@reykholt.kaiser.cx] On Behalf Of
> > Martin Kaiser
> > Sent: Thursday, January 09, 2014 9:29 PM
> > To: Himanshu Madhani; Rajesh Borundia
> > Cc: linux-kernel; trivial@kernel.org
> > Subject: [PATCH -next] qlcnic: fix compiler warning
> >
> > Add an explicit cast to fix the following warning (seen on Debian Wheezy, gcc
> > 4.7.2)
> >
> > CC [M] drivers/net/wireless/rtlwifi/rtl8192ce/trx.o
> > drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c: In function
> > qlcnic_send_filter:
> > drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c:349:3: warning:
> > passing argument 2 of ether_addr_equal from incompatible pointer type
> > [enabled by default]
> > In file included from include/linux/if_vlan.h:16:0,
> > from drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c:9:
> > include/linux/etherdevice.h:244:20: note: expected const u8 * but
> > argument is of type u64 *
> >
>
> If I am not wrong, this patch should go to David's net-next tree.
>
> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
>
> Acked-by: Shahed Shaikh <shahed.shaikh@qlogic.com>
>
> Thanks,
> Shahed
> > ---
> > drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> > b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> > index 6373f60..3557154 100644
> > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c
> > @@ -346,7 +346,7 @@ static void qlcnic_send_filter(struct qlcnic_adapter
> > *adapter,
> > head = &(adapter->fhash.fhead[hindex]);
> >
> > hlist_for_each_entry_safe(tmp_fil, n, head, fnode) {
> > - if (ether_addr_equal(tmp_fil->faddr, &src_addr) &&
> > + if (ether_addr_equal(tmp_fil->faddr, (const u8 *)&src_addr)
> > &&
> > tmp_fil->vlan_id == vlan_id) {
> > if (jiffies > (QLCNIC_READD_AGE * HZ + tmp_fil-
> > >ftime))
> > qlcnic_change_filter(adapter, &src_addr,
> > --
> > 1.7.10.4
A quick look at the code seems to imply that this code is somewhat buggy.
'src_addr' is defined a u64 even though it is a 6 byte mac address.
On big-endian systems the wrong bytes get accessed all over the place.
Also when ether_addr_equal() in inlined the code ends up accessing src_addr
as 32bit and 16bit data - which don't have to be synchronised against
and writes to the 64bit item.
If might be that ether_addr_equal() (etc) should have a gcc asm
statement with a "memory" constraint against the mac address buffers.
David
^ permalink raw reply
* RE: [net-next 05/16] i40e: fix long lines
From: David Laight @ 2014-01-10 10:02 UTC (permalink / raw)
To: 'Joe Perches', Jeff Kirsher
Cc: davem@davemloft.net, Mitch Williams, netdev@vger.kernel.org,
gospo@redhat.com, sassmann@redhat.com, Jesse Brandeburg
In-Reply-To: <1389344869.24222.31.camel@joe-AO722>
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c
> b/drivers/net/ethernet/intel/i40e/i40e_common.c
> []
> > @@ -681,7 +681,8 @@ aq_add_vsi_exit:
> > * @cmd_details: pointer to command details structure or NULL
> > **/
> > i40e_status i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw,
> > - u16 seid, bool set, struct i40e_asq_cmd_details *cmd_details)
> > + u16 seid, bool set,
> > + struct i40e_asq_cmd_details *cmd_details)
>
> Another way this could be changed is to put the
> return value on a separate line like:
>
> i40e_status
> i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 seid, bool set,
> struct i40e_asq_cmd_details *cmd_details)
Personally I prefer that so I can find the definition by grepping
for '^function_name' - but it doesn't seem to be done in any linux
source files.
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
> []
> > @@ -93,9 +93,9 @@ i40e_status i40e_aq_set_vsi_broadcast(struct i40e_hw *hw,
> > u16 vsi_id, bool set_filter,
> > struct i40e_asq_cmd_details *cmd_details);
> > i40e_status i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw,
> > - u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);
> > + u16 vsi_id, bool set, struct i40e_asq_cmd_details *cmd_details);
>
> i40e_status
> i40e_aq_set_vsi_unicast_promiscuous(struct i40e_hw *hw, u16 vsi_id, bool set,
> struct i40e_asq_cmd_details *cmd_details);
>
> etc...
>
> but once you use extremely long 35+ characters
> function names, 80 columns gets a bit silly too.
Again this only a real problem when the coding stand requires that
continuation lines have the parameters lined up under the '('.
If they are indented by 4 spaces (as many of the BSDs do) then you
do stand a chance of laying out a call in a reasonable number of
vertical lines.
My Actual preferred layout is to exclude tabs, indent by 4 spaces
and double-indent continuation lines.
I'll also start continuation lines (of expressions) with an operator
to make it even clearer they are continuation lines.
When I'm scan-reading code I tend to concentrate on the LHS of each line.
- so that is where the information need to be.
Not that I expect Linux to change!
(Other code bases have much worse layout rules.)
David
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox