* [PATCH net-next v2] ipv6: move IPV6_TCLASS_SHIFT into ipv6.h
From: roy.qing.li @ 2014-01-10 10:03 UTC (permalink / raw)
To: netdev
From: Li RongQing <roy.qing.li@gmail.com>
Two places defined IPV6_TCLASS_SHIFT, so we should move it into ipv6.h,
and use this macro as possible.
Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
Sorry, version 1 has bug in fib6_rule_match it should be frist
mask flowlabel with IPV6_TCLASS_MASK, then call ntohl
include/net/ipv6.h | 1 +
net/ipv6/fib6_rules.c | 4 +++-
net/ipv6/ip6_gre.c | 2 --
net/ipv6/ip6_tunnel.c | 2 --
net/ipv6/ipv6_sockglue.c | 4 +++-
5 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 12079c6..d819f7a 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -239,6 +239,7 @@ struct ip6_flowlabel {
#define IPV6_FLOWINFO_MASK cpu_to_be32(0x0FFFFFFF)
#define IPV6_FLOWLABEL_MASK cpu_to_be32(0x000FFFFF)
#define IPV6_TCLASS_MASK (IPV6_FLOWINFO_MASK & ~IPV6_FLOWLABEL_MASK)
+#define IPV6_TCLASS_SHIFT 20
struct ipv6_fl_socklist {
struct ipv6_fl_socklist __rcu *next;
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index 3fd0a57..d50c5ca 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -150,6 +150,8 @@ static int fib6_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
{
struct fib6_rule *r = (struct fib6_rule *) rule;
struct flowi6 *fl6 = &fl->u.ip6;
+ u8 tclass = ntohl(fl6->flowlabel & IPV6_TCLASS_MASK) >>
+ IPV6_TCLASS_SHIFT;
if (r->dst.plen &&
!ipv6_prefix_equal(&fl6->daddr, &r->dst.addr, r->dst.plen))
@@ -169,7 +171,7 @@ static int fib6_rule_match(struct fib_rule *rule, struct flowi *fl, int flags)
return 0;
}
- if (r->tclass && r->tclass != ((ntohl(fl6->flowlabel) >> 20) & 0xff))
+ if (r->tclass && r->tclass != tclass)
return 0;
return 1;
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index e7a440d..f3ffb43 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -61,8 +61,6 @@ static bool log_ecn_error = true;
module_param(log_ecn_error, bool, 0644);
MODULE_PARM_DESC(log_ecn_error, "Log packets received with corrupted ECN");
-#define IPV6_TCLASS_SHIFT 20
-
#define HASH_SIZE_SHIFT 5
#define HASH_SIZE (1 << HASH_SIZE_SHIFT)
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c
index 1e5e240..5db8d31 100644
--- a/net/ipv6/ip6_tunnel.c
+++ b/net/ipv6/ip6_tunnel.c
@@ -69,8 +69,6 @@ MODULE_ALIAS_NETDEV("ip6tnl0");
#define IP6_TNL_TRACE(x...) do {;} while(0)
#endif
-#define IPV6_TCLASS_SHIFT 20
-
#define HASH_SIZE_SHIFT 5
#define HASH_SIZE (1 << HASH_SIZE_SHIFT)
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index af0ecb9..1c9aa35 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -1019,7 +1019,9 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
put_cmsg(&msg, SOL_IPV6, IPV6_HOPLIMIT, sizeof(hlim), &hlim);
}
if (np->rxopt.bits.rxtclass) {
- int tclass = ntohl(np->rcv_flowinfo & IPV6_TCLASS_MASK) >> 20;
+ int tclass = ntohl(np->rcv_flowinfo &
+ IPV6_TCLASS_MASK) >>
+ IPV6_TCLASS_SHIFT;
put_cmsg(&msg, SOL_IPV6, IPV6_TCLASS, sizeof(tclass), &tclass);
}
if (np->rxopt.bits.rxoinfo) {
--
1.7.10.4
^ permalink raw reply related
* 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
* 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: [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: [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 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: [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 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: 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
* [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
* [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 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 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
* 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
* 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: [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 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
* [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: 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
* 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] 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
* 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
* [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
* [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
* 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
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