* [PATCH net-next 4/4] bonding: update bonding.txt for primary description
From: Ding Tianhong @ 2014-01-09 11:20 UTC (permalink / raw)
To: Jay Vosburgh, Veaceslav Falico, David S. Miller, Netdev
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
Documentation/networking/bonding.txt | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index a4d925e..5cdb229 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -657,7 +657,8 @@ primary
one slave is preferred over another, e.g., when one slave has
higher throughput than another.
- The primary option is only valid for active-backup mode.
+ The primary option is only valid for active-backup(1),
+ balance-tlb (5) and balance-alb (6) mode.
primary_reselect
--
1.8.0
^ permalink raw reply related
* [PATCH net-next 0/4] bonding: update the primary for bonding and fix some slight problem
From: Ding Tianhong @ 2014-01-09 11:20 UTC (permalink / raw)
To: Jay Vosburgh, Veaceslav Falico, David S. Miller, Netdev
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.
Do not save the primary when the device is not exist for bonding.
Modify some description for primary in bonding.txt.
Ding Tianhong (4):
bonding: update the primary when slave name changed
bonding: do not save non-existent device to bond primary in check params
bonding: do not save non-existent device to bond primary in sysfs
bonding: update bonding.txt for primary description.
Documentation/networking/bonding.txt | 3 ++-
drivers/net/bonding/bond_main.c | 34 +++++++++++++++++++++++++---------
drivers/net/bonding/bond_options.c | 6 ++++++
3 files changed, 33 insertions(+), 10 deletions(-)
--
1.8.0
^ permalink raw reply
* [PATCH v2 net-next 0/3] bonding: fix bond_3ad RCU usage
From: Veaceslav Falico @ 2014-01-09 11:20 UTC (permalink / raw)
To: netdev; +Cc: 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.
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: 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 v2 net-next 1/3] bonding: fix bond_3ad_set_carrier() RCU usage
From: Veaceslav Falico @ 2014-01-09 11:20 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, dingtianhong, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389266425-28365-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() is either under RCU read
lock or under the RTNL lock (bond_set_carrier() always holds it).
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:
v1 -> v2:
Don't use _rcu primitives as we can be called under RTNL too.
drivers/net/bonding/bond_3ad.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 29db1ca..cf5fab8 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1597,9 +1597,9 @@ static void ad_agg_selection_logic(struct aggregator *agg)
}
}
- rcu_read_unlock();
-
bond_3ad_set_carrier(bond);
+
+ rcu_read_unlock();
}
/**
@@ -2322,15 +2322,14 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
*
* Called by bond_set_carrier(). Return zero if carrier state does not
* change, nonzero if it does.
+ * The caller must hold RCU read lock or RTNL.
*/
int bond_3ad_set_carrier(struct bonding *bond)
{
struct aggregator *active;
struct slave *first_slave;
- rcu_read_lock();
- first_slave = bond_first_slave_rcu(bond);
- rcu_read_unlock();
+ first_slave = bond_first_slave(bond);
if (!first_slave)
return 0;
active = __get_active_agg(&(SLAVE_AD_INFO(first_slave).aggregator));
--
1.8.4
^ permalink raw reply related
* [PATCH v2 net-next 2/3] bonding: fix __get_first_agg RCU usage
From: Veaceslav Falico @ 2014-01-09 11:20 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, dingtianhong, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389266425-28365-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.
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:
v1 -> v2:
Don't use RCU primitives as we can hold RTNL.
drivers/net/bonding/bond_3ad.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index cf5fab8..d2782c8 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 either hold RCU or RTNL 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();
+ first_slave = bond_first_slave(bond);
return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
}
--
1.8.4
^ permalink raw reply related
* [PATCH v2 net-next 3/3] bonding: fix __get_active_agg() RCU logic
From: Veaceslav Falico @ 2014-01-09 11:20 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, dingtianhong, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389266425-28365-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 either RCU or RTNL lock.
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:
v1 -> v2:
Don't use RCU primitives as we can hold RTNL.
drivers/net/bonding/bond_3ad.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index d2782c8..845545b 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 either RCU or RTNL 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();
+ bond_for_each_slave(bond, slave, iter)
+ 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);
--
1.8.4
^ permalink raw reply related
* Re: [PATCH v2 net-next 1/3] bonding: fix bond_3ad_set_carrier() RCU usage
From: Ding Tianhong @ 2014-01-09 11:39 UTC (permalink / raw)
To: Veaceslav Falico, netdev; +Cc: Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389266425-28365-2-git-send-email-vfalico@redhat.com>
On 2014/1/9 19:20, 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() is either under RCU read
> lock or under the RTNL lock (bond_set_carrier() always holds it).
>
> 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:
> v1 -> v2:
> Don't use _rcu primitives as we can be called under RTNL too.
>
> drivers/net/bonding/bond_3ad.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index 29db1ca..cf5fab8 100644
> --- a/drivers/net/bonding/bond_3ad.c
> +++ b/drivers/net/bonding/bond_3ad.c
> @@ -1597,9 +1597,9 @@ static void ad_agg_selection_logic(struct aggregator *agg)
> }
> }
>
> - rcu_read_unlock();
> -
> bond_3ad_set_carrier(bond);
> +
> + rcu_read_unlock();
> }
>
> /**
> @@ -2322,15 +2322,14 @@ void bond_3ad_handle_link_change(struct slave *slave, char link)
> *
> * Called by bond_set_carrier(). Return zero if carrier state does not
> * change, nonzero if it does.
> + * The caller must hold RCU read lock or RTNL.
> */
> int bond_3ad_set_carrier(struct bonding *bond)
> {
> struct aggregator *active;
> struct slave *first_slave;
>
> - rcu_read_lock();
> - first_slave = bond_first_slave_rcu(bond);
> - rcu_read_unlock();
> + first_slave = bond_first_slave(bond);
> if (!first_slave)
> return 0;
> active = __get_active_agg(&(SLAVE_AD_INFO(first_slave).aggregator));
>
Agree, thanks.
Acked-by: Ding Tianhong <dingtianhong@huawei.com>
^ permalink raw reply
* Re: [PATCH net-next 1/4] bonding: update the primary when slave name changed
From: Veaceslav Falico @ 2014-01-09 11:46 UTC (permalink / raw)
To: Ding Tianhong; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <52CE8604.3010804@huawei.com>
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.
>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bond_main.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index e06c445..de646e2 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -2860,9 +2860,19 @@ static int bond_slave_netdev_event(unsigned long event,
> */
> break;
> case NETDEV_CHANGENAME:
>- /*
>- * TODO: handle changing the primary's name
>+ /* if the primary's name changed,
>+ * save the new name for primary.
> */
>+ if (USES_PRIMARY(bond->params.mode) &&
>+ bond->params.primary[0]) {
>+ if (bond->primary_slave &&
>+ strcmp(bond->params.primary,
>+ bond->primary_slave->dev->name)) {
>+ strncpy(bond->params.primary,
>+ bond->primary_slave->dev->name,
>+ IFNAMSIZ);
>+ }
>+ }
> break;
> case NETDEV_FEAT_CHANGE:
> bond_compute_features(bond);
>--
>1.8.0
>
>
^ permalink raw reply
* Re: [PATCH net-next 3/4] bonding: do not save non-existent device to bond primary in sysfs
From: Veaceslav Falico @ 2014-01-09 11:47 UTC (permalink / raw)
To: Ding Tianhong; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <52CE8616.2010902@huawei.com>
On Thu, Jan 09, 2014 at 07:20:54PM +0800, Ding Tianhong wrote:
>Do not save non-existent device to bond primary in sysfs.
Again, no - this interface might be enslaved after we've set the
parameters, as it usually does.
>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bond_options.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
>diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
>index 945a666..9b4518f 100644
>--- a/drivers/net/bonding/bond_options.c
>+++ b/drivers/net/bonding/bond_options.c
>@@ -501,6 +501,12 @@ int bond_option_primary_set(struct bonding *bond, const char *primary)
> goto out;
> }
>
>+ if (!__dev_get_by_name(dev_net(bond->dev), primary)) {
>+ pr_err("%s: unable to set non-existent device %s to primary.\n",
>+ bond->dev->name, primary);
>+ goto out;
>+ }
>+
> bond_for_each_slave(bond, slave, iter) {
> if (strncmp(slave->dev->name, primary, IFNAMSIZ) == 0) {
> pr_info("%s: Setting %s as primary slave.\n",
>--
>1.8.0
>
>
^ permalink raw reply
* Re: [PATCH net-next 2/4] bonding: do not save non-existent device to bond primary in check params
From: Veaceslav Falico @ 2014-01-09 11:48 UTC (permalink / raw)
To: Ding Tianhong; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <52CE8607.1050103@huawei.com>
On Thu, Jan 09, 2014 at 07:20:39PM +0800, Ding Tianhong wrote:
>When install the bonding, the primay will be use to distinguish
>the primary slave for ab, alb and tlb mode, but it is meanless
>to save a no existed device, so add check for it.
Same remark as for the sysfs patch - we can have arbitrary name of the
interface in the paramter.
>
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>---
> drivers/net/bonding/bond_main.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index de646e2..651c5fd 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -4246,13 +4246,19 @@ static int bond_check_params(struct bond_params *params)
> pr_debug("Warning: either miimon or arp_interval and arp_ip_target module parameters must be specified, otherwise bonding will not detect link failures! see bonding.txt for details.\n");
> }
>
>- if (primary && !USES_PRIMARY(bond_mode)) {
>- /* currently, using a primary only makes sense
>- * in active backup, TLB or ALB modes
>- */
>- pr_warning("Warning: %s primary device specified but has no effect in %s mode\n",
>- primary, bond_mode_name(bond_mode));
>- primary = NULL;
>+ if (primary) {
>+ if (!__dev_get_by_name(&init_net, primary)) {
>+ pr_warn("Warning: %s primary device is not exist\n",
>+ primary);
>+ primary = NULL;
>+ } else if (!USES_PRIMARY(bond_mode)) {
>+ /* currently, using a primary only makes sense
>+ * in active backup, TLB or ALB modes
>+ */
>+ pr_warn("Warning: %s primary device specified but has no effect in %s mode\n",
>+ primary, bond_mode_name(bond_mode));
>+ primary = NULL;
>+ }
> }
>
> if (primary && primary_reselect) {
>--
>1.8.0
>
>
^ permalink raw reply
* Re: [PATCH net-next 4/4] bonding: update bonding.txt for primary description
From: Veaceslav Falico @ 2014-01-09 11:49 UTC (permalink / raw)
To: Ding Tianhong; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <52CE8619.7080602@huawei.com>
On Thu, Jan 09, 2014 at 07:20:57PM +0800, Ding Tianhong wrote:
>Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
This one's good.
Acked-by: Veaceslav Falico <vfalico@redhat.com>
>---
> Documentation/networking/bonding.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
>index a4d925e..5cdb229 100644
>--- a/Documentation/networking/bonding.txt
>+++ b/Documentation/networking/bonding.txt
>@@ -657,7 +657,8 @@ primary
> one slave is preferred over another, e.g., when one slave has
> higher throughput than another.
>
>- The primary option is only valid for active-backup mode.
>+ The primary option is only valid for active-backup(1),
>+ balance-tlb (5) and balance-alb (6) mode.
>
> primary_reselect
>
>--
>1.8.0
>
>
^ permalink raw reply
* Re: [PATCH net 2/2] net: core: explicitly select a txq before doing l2 forwarding
From: Neil Horman @ 2014-01-09 11:53 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, e1000-devel, netdev, linux-kernel, John Fastabend, davem
In-Reply-To: <52CE5DC1.8070807@redhat.com>
On Thu, Jan 09, 2014 at 04:28:49PM +0800, Jason Wang wrote:
> On 01/08/2014 10:40 PM, Neil Horman wrote:
> > On Wed, Jan 08, 2014 at 11:21:21AM +0800, Jason Wang wrote:
> >> On 01/07/2014 09:17 PM, Neil Horman wrote:
> >>> On Tue, Jan 07, 2014 at 11:42:24AM +0800, Jason Wang wrote:
> >>>> On 01/06/2014 08:42 PM, Neil Horman wrote:
> >>>>> On Mon, Jan 06, 2014 at 11:21:07AM +0800, Jason Wang wrote:
> >>>>>> Currently, the tx queue were selected implicitly in ndo_dfwd_start_xmit(). The
> >>>>>> will cause several issues:
> >>>>>>
> >>>>>> - NETIF_F_LLTX was forced for macvlan device in this case which lead extra lock
> >>>>>> contention.
> >>>>>> - dev_hard_start_xmit() was called with NULL txq which bypasses the net device
> >>>>>> watchdog
> >>>>>> - dev_hard_start_xmit() does not check txq everywhere which will lead a crash
> >>>>>> when tso is disabled for lower device.
> >>>>>>
> >>>>>> Fix this by explicitly introducing a select queue method just for l2 forwarding
> >>>>>> offload (ndo_dfwd_select_queue), and introducing dfwd_direct_xmit() to do the
> >>>>>> queue selecting and transmitting for l2 forwarding.
> >>>>>>
> >>>>>> With this fixes, NETIF_F_LLTX could be preserved for macvlan and there's no need
> >>>>>> to check txq against NULL in dev_hard_start_xmit().
> >>>>>>
> >>>>>> In the future, it was also required for macvtap l2 forwarding support since it
> >>>>>> provides a necessary synchronization method.
> >>>>>>
> >>>>>> Cc: John Fastabend <john.r.fastabend@intel.com>
> >>>>>> Cc: Neil Horman <nhorman@tuxdriver.com>
> >>>>>> Cc: e1000-devel@lists.sourceforge.net
> >>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>> Instead of creating another operation here to do special queue selection, why
> >>>>> not just have ndo_dfwd_start_xmit include a pointer to a pointer in its argument
> >>>>> list, so it can pass the txq it used back to the caller (dev_hard_start_xmit)?
> >>>>> ndo_dfwd_start_xmit already knows which queue set to pick from (since their
> >>>>> reserved for the device doing the transmitting). It seems more clear to me than
> >>>>> creating a new netdevice operation.
> >>>> See commit 8ffab51b3dfc54876f145f15b351c41f3f703195 ("macvlan: lockless
> >>>> tx path"). The point is keep the tx path lockless to be efficient and
> >>>> simplicity for management. And macvtap multiqueue was also implemented
> >>>> with this assumption. The real contention should be done in the txq of
> >>>> lower device instead of macvlan itself. This is also needed for
> >>>> multiqueue macvtap.
> >>> Ok, I see how you're preserving LLTX here, and thats great, but it doesn't
> >>> really buy us anything that I can see. If a macvlan is using hardware
> >>> acceleration, it needs to arbitrate access to that hardware. Weather thats done
> >>> by locking the lowerdev's tx queue lock or by enforcing locking on the macvlan
> >>> itself is equivalent. The decision to use dfwd hardware acceleration is made on
> >>> open, so its not like theres any traffic that can avoid the lock, as it all goes
> >>> through the hardware. All I see that this has bought us is an extra net_device
> >>> method (which isn't a big deal, but not necessecary as I see it).
> >> As I replied to patch 1/2, looking at the code itself again. The locking
> >> on the lowerdev's tx queue is really need since we need synchronize with
> >> other control path. Two examples are dev watchdog and ixgbe_down() both
> >> of which will try to hold tx lock to synchronize the with transmission.
> >> Without holding the lowerdev tx lock, we may have more serious issues.
> >> Also, it's a little strange for a net device has two modes. Future
> >> developers need to care about two different tx lock paths which is sub
> >> optimal.
> >>
> > Ok, having looked at this for a few hours, I agree, locking in the lowerdev has
> > some definiate advantages in plugging the holes you've pointed out.
> >
> >> For the issue of an extra net_device method, if you don't like we can
> >> reuse the ndo_select_queue by also passing the accel_priv to that method.
> > I do, that actually simplifies things, since it lets us use the entire
> > dev_hard_start_xmit path unmodified, which gives us the locking your looking for
> > without having to create a new slimmed down variant of dev_hard_start_xmit.
> >
> > Regards
> > Neil
>
> Right, will post V2.
>
Thanks
Neil
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* [PATCH iproute2] vxlan: add missing dst port setup option
From: Daniel Borkmann @ 2014-01-09 11:56 UTC (permalink / raw)
To: stephen; +Cc: netdev
Kernel commit 823aa873bc ("vxlan: allow choosing destination port
per vxlan") and 553675fb5e ("vxlan: listen on multiple ports")
make it "now possible to define the same virtual network id but
with different UDP port values which can be useful for migration."
However, IFLA_VXLAN_PORT netlink attribute was available in the
kernel but hasn't been pushed to iproute2 in order to make use
of it, hence, add this option so that people can use it.
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
ip/iplink_vxlan.c | 17 ++++++++++++++++-
man/man8/ip-link.8.in | 6 ++++++
2 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index aa551d8..a9aedcf 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -25,7 +25,7 @@ static void explain(void)
{
fprintf(stderr, "Usage: ... vxlan id VNI [ { group | remote } ADDR ] [ local ADDR ]\n");
fprintf(stderr, " [ ttl TTL ] [ tos TOS ] [ dev PHYS_DEV ]\n");
- fprintf(stderr, " [ port MIN MAX ] [ [no]learning ]\n");
+ fprintf(stderr, " [ port MIN MAX ] [ dst_port PORT ] [ [no]learning ]\n");
fprintf(stderr, " [ [no]proxy ] [ [no]rsc ]\n");
fprintf(stderr, " [ [no]l2miss ] [ [no]l3miss ]\n");
fprintf(stderr, "\n");
@@ -43,6 +43,7 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
__u32 saddr = 0;
__u32 gaddr = 0;
__u32 daddr = 0;
+ __be16 dport = 0;
struct in6_addr saddr6 = IN6ADDR_ANY_INIT;
struct in6_addr gaddr6 = IN6ADDR_ANY_INIT;
struct in6_addr daddr6 = IN6ADDR_ANY_INIT;
@@ -144,6 +145,12 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
invarg("max port", *argv);
range.low = htons(minport);
range.high = htons(maxport);
+ } else if (!matches(*argv, "dst_port")) {
+ __u16 port;
+ NEXT_ARG();
+ if (get_u16(&port, *argv, 0))
+ invarg("dst_port", *argv);
+ dport = htons(port);
} else if (!matches(*argv, "nolearning")) {
learning = 0;
} else if (!matches(*argv, "learning")) {
@@ -218,6 +225,8 @@ static int vxlan_parse_opt(struct link_util *lu, int argc, char **argv,
if (range.low || range.high)
addattr_l(n, 1024, IFLA_VXLAN_PORT_RANGE,
&range, sizeof(range));
+ if (dport)
+ addattr16(n, 1024, IFLA_VXLAN_PORT, dport);
return 0;
}
@@ -293,6 +302,12 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
fprintf(f, "port %u %u ", ntohs(r->low), ntohs(r->high));
}
+ if (tb[IFLA_VXLAN_PORT]) {
+ __be16 dport = rta_getattr_u16(tb[IFLA_VXLAN_PORT]);
+ if (dport)
+ fprintf(f, "dst_port %u ", ntohs(dport));
+ }
+
if (tb[IFLA_VXLAN_LEARNING] &&
!rta_getattr_u8(tb[IFLA_VXLAN_LEARNING]))
fputs("nolearning ", f);
diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
index 3986a5a..b74461c 100644
--- a/man/man8/ip-link.8.in
+++ b/man/man8/ip-link.8.in
@@ -238,6 +238,8 @@ the following additional arguments are supported:
.R " ] [ "
.BI port " MIN MAX "
.R " ] [ "
+.BI dst_port " PORT "
+.R " ] [ "
.I "[no]learning "
.R " ] [ "
.I "[no]proxy "
@@ -291,6 +293,10 @@ parameter.
source ports to communicate to the remote VXLAN tunnel endpoint.
.sp
+.BI dst_port " PORT"
+- specifies the port to use as UDP destination port.
+
+.sp
.I [no]learning
- specifies if unknown source link layer addresses and IP addresses
are entered into the VXLAN device forwarding database.
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v2 net-next 2/3] bonding: fix __get_first_agg RCU usage
From: Ding Tianhong @ 2014-01-09 11:58 UTC (permalink / raw)
To: Veaceslav Falico, netdev; +Cc: Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389266425-28365-3-git-send-email-vfalico@redhat.com>
On 2014/1/9 19:20, 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:
> v1 -> v2:
> Don't use RCU primitives as we can hold RTNL.
>
> drivers/net/bonding/bond_3ad.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index cf5fab8..d2782c8 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 either hold RCU or RTNL 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();
> + first_slave = bond_first_slave(bond);
>
> return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
> }
>
Hi, Veaceslav:
Do you mean the bond_first_slave is safe in rcu_read_xxlock()?
#define bond_first_slave(bond) \
(bond_has_slaves(bond) ? \
netdev_adjacent_get_private(bond_slave_list(bond)->next) : \
NULL)
Regards
Ding
^ permalink raw reply
* Re: [PATCH v2 net-next 2/3] bonding: fix __get_first_agg RCU usage
From: Veaceslav Falico @ 2014-01-09 11:58 UTC (permalink / raw)
To: Ding Tianhong; +Cc: netdev, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <52CE8ED9.60609@huawei.com>
On Thu, Jan 09, 2014 at 07:58:17PM +0800, Ding Tianhong wrote:
>On 2014/1/9 19:20, 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:
>> v1 -> v2:
>> Don't use RCU primitives as we can hold RTNL.
>>
>> drivers/net/bonding/bond_3ad.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index cf5fab8..d2782c8 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 either hold RCU or RTNL 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();
>> + first_slave = bond_first_slave(bond);
>>
>> return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
>> }
>>
>
>Hi, Veaceslav:
>
>Do you mean the bond_first_slave is safe in rcu_read_xxlock()?
>
>#define bond_first_slave(bond) \
> (bond_has_slaves(bond) ? \
> netdev_adjacent_get_private(bond_slave_list(bond)->next) : \
> NULL)
Heh, good catch. We can't use neither _rcu primitives, because we can be
called under RTNL, and nor the RTNL ones, cause we can be called under RCU.
Not that easy to fix your RCU updates...
I'll take a look and send v3.
>
>
>Regards
>Ding
>
>
^ permalink raw reply
* Re: [PATCH v2 net-next 3/3] bonding: fix __get_active_agg() RCU logic
From: Ding Tianhong @ 2014-01-09 12:04 UTC (permalink / raw)
To: Veaceslav Falico, netdev; +Cc: Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1389266425-28365-4-git-send-email-vfalico@redhat.com>
On 2014/1/9 19:20, 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 either RCU or RTNL lock.
>
> 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:
> v1 -> v2:
> Don't use RCU primitives as we can hold RTNL.
>
> drivers/net/bonding/bond_3ad.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
> index d2782c8..845545b 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 either RCU or RTNL 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();
> + bond_for_each_slave(bond, slave, iter)
> + if (SLAVE_AD_INFO(slave).aggregator.is_active)
> return &(SLAVE_AD_INFO(slave).aggregator);
> - }
Is the bond_for_each_slave safe in rcu_read_lock()?
> - 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);
>
>
^ permalink raw reply
* Re: [PATCH v2 net-next 3/3] bonding: fix __get_active_agg() RCU logic
From: Veaceslav Falico @ 2014-01-09 12:13 UTC (permalink / raw)
To: Ding Tianhong; +Cc: netdev, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <52CE9049.6020206@huawei.com>
On Thu, Jan 09, 2014 at 08:04:25PM +0800, Ding Tianhong wrote:
>On 2014/1/9 19:20, 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 either RCU or RTNL lock.
>>
>> 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:
>> v1 -> v2:
>> Don't use RCU primitives as we can hold RTNL.
>>
>> drivers/net/bonding/bond_3ad.c | 12 +++++-------
>> 1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> index d2782c8..845545b 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 either RCU or RTNL 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();
>> + bond_for_each_slave(bond, slave, iter)
>> + if (SLAVE_AD_INFO(slave).aggregator.is_active)
>> return &(SLAVE_AD_INFO(slave).aggregator);
>> - }
>
>Is the bond_for_each_slave safe in rcu_read_lock()?
Yeah, good catch, responded in the second patch.
Thanks for the review!
>
>
>> - 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);
>>
>>
>
>
^ permalink raw reply
* Re: [PATCH net V2 1/2] macvlan: forbid L2 fowarding offload for macvtap
From: Neil Horman @ 2014-01-09 12:20 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, linux-kernel, mst, John Fastabend
In-Reply-To: <1389260252-48591-1-git-send-email-jasowang@redhat.com>
On Thu, Jan 09, 2014 at 05:37:31PM +0800, Jason Wang wrote:
> L2 fowarding offload will bypass the rx handler of real device. This will make
> the packet could not be forwarded to macvtap device. Another problem is the
> dev_hard_start_xmit() called for macvtap does not have any synchronization.
>
> Fix this by forbidding L2 forwarding for macvtap.
>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/net/macvlan.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 60406b0..5360f73 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -338,6 +338,8 @@ static const struct header_ops macvlan_hard_header_ops = {
> .cache_update = eth_header_cache_update,
> };
>
> +static struct rtnl_link_ops macvlan_link_ops;
> +
> static int macvlan_open(struct net_device *dev)
> {
> struct macvlan_dev *vlan = netdev_priv(dev);
> @@ -353,7 +355,8 @@ static int macvlan_open(struct net_device *dev)
> goto hash_add;
> }
>
> - if (lowerdev->features & NETIF_F_HW_L2FW_DOFFLOAD) {
> + if (lowerdev->features & NETIF_F_HW_L2FW_DOFFLOAD &&
> + dev->rtnl_link_ops == &macvlan_link_ops) {
> vlan->fwd_priv =
> lowerdev->netdev_ops->ndo_dfwd_add_station(lowerdev, dev);
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] xen-netback: use new skb_checksum_setup function
From: Wei Liu @ 2014-01-09 12:26 UTC (permalink / raw)
To: Paul Durrant; +Cc: netdev, xen-devel, Ian Campbell, Wei Liu
In-Reply-To: <1389261768-30606-3-git-send-email-paul.durrant@citrix.com>
On Thu, Jan 09, 2014 at 10:02:47AM +0000, Paul Durrant wrote:
> Use skb_checksum_setup to set up partial checksum offsets rather
> then a private implementation.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
If your change to core driver goes in then:
Acked-by: Wei Liu <wei.liu2@citrix.com>
Thanks
Wei.
^ permalink raw reply
* Re: [PATCH net-next 1/4] bonding: update the primary when slave name changed
From: Ding Tianhong @ 2014-01-09 12:23 UTC (permalink / raw)
To: Veaceslav Falico; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <20140109114636.GF5786@redhat.com>
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?
Regards
Ding
>>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>> drivers/net/bonding/bond_main.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index e06c445..de646e2 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -2860,9 +2860,19 @@ static int bond_slave_netdev_event(unsigned long event,
>> */
>> break;
>> case NETDEV_CHANGENAME:
>> - /*
>> - * TODO: handle changing the primary's name
>> + /* if the primary's name changed,
>> + * save the new name for primary.
>> */
>> + if (USES_PRIMARY(bond->params.mode) &&
>> + bond->params.primary[0]) {
>> + if (bond->primary_slave &&
>> + strcmp(bond->params.primary,
>> + bond->primary_slave->dev->name)) {
>> + strncpy(bond->params.primary,
>> + bond->primary_slave->dev->name,
>> + IFNAMSIZ);
>> + }
>> + }
>> break;
>> case NETDEV_FEAT_CHANGE:
>> bond_compute_features(bond);
>> --
>> 1.8.0
>>
>>
>
>
^ permalink raw reply
* Re: [PATCH net V2 2/2] net: core: explicitly select a txq before doing l2 forwarding
From: Neil Horman @ 2014-01-09 12:31 UTC (permalink / raw)
To: Jason Wang; +Cc: davem, netdev, linux-kernel, mst, John Fastabend, e1000-devel
In-Reply-To: <1389260252-48591-2-git-send-email-jasowang@redhat.com>
On Thu, Jan 09, 2014 at 05:37:32PM +0800, Jason Wang wrote:
> Currently, the tx queue were selected implicitly in ndo_dfwd_start_xmit(). The
> will cause several issues:
>
> - NETIF_F_LLTX were removed for macvlan, so txq lock were done for macvlan
> instead of lower device which misses the necessary txq synchronization for
> lower device such as txq stopping or frozen required by dev watchdog or
> control path.
> - dev_hard_start_xmit() was called with NULL txq which bypasses the net device
> watchdog.
> - dev_hard_start_xmit() does not check txq everywhere which will lead a crash
> when tso is disabled for lower device.
>
> Fix this by explicitly introducing a new param for .ndo_select_queue() for just
> selecting queues in the case of l2 forwarding offload. And also introducing
> dfwd_direct_xmit() to do the queue selecting, txq holding and transmitting for
> l2 forwarding.
>
> With this fixes, NETIF_F_LLTX could be preserved for macvlan and there's no need
> to check txq against NULL in dev_hard_start_xmit(). Also there's no need to keep
> a dedicated ndo_dfwd_start_xmit().
>
> In the future, it was also required for macvtap l2 forwarding support since it
> provides a necessary synchronization method.
>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: e1000-devel@lists.sourceforge.net
> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> ---
> Changes from V1:
> - Adding a new parameter to ndo_select_queue instead of a new method to select
> queue for l2 forwarding.
> - Remove the unnecessary ndo_dfwd_start_xmit() since txq was selected
> explicitly.
> - Keep NETIF_F_LLTX when netdev feature is changed.
> - Shape the commit log
A few minor nits inline.
><snip>
> }
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index 5360f73..7eb4c82 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -299,7 +299,7 @@ netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
>
> if (vlan->fwd_priv) {
> skb->dev = vlan->lowerdev;
> - ret = dev_hard_start_xmit(skb, skb->dev, NULL, vlan->fwd_priv);
> + ret = dfwd_direct_xmit(skb, skb->dev, vlan->fwd_priv);
> } else {
> ret = macvlan_queue_xmit(skb, dev);
> }
> @@ -366,7 +366,6 @@ static int macvlan_open(struct net_device *dev)
> if (IS_ERR_OR_NULL(vlan->fwd_priv)) {
> vlan->fwd_priv = NULL;
> } else {
> - dev->features &= ~NETIF_F_LLTX;
> return 0;
> }
After removing the features flag operation here, you don't need the braces
around the else statement either.
><snip>
> +int dfwd_direct_xmit(struct sk_buff *skb, struct net_device *dev,
> + void *accel_priv)
> +{
> + struct netdev_queue *txq;
> + int ret = NETDEV_TX_BUSY;
> + int index;
> +
> + BUG_ON(!dev->netdev_ops->ndo_select_queue);
> + index = dev->netdev_ops->ndo_select_queue(dev, skb, accel_priv);
> +
> + local_bh_disable();
> +
> + skb_set_queue_mapping(skb, index);
> + txq = netdev_get_tx_queue(dev, index);
> +
> + HARD_TX_LOCK(dev, txq, smp_processor_id());
> + if (!netif_xmit_frozen_or_stopped(txq))
> + ret = dev_hard_start_xmit(skb, dev, txq);
> + HARD_TX_UNLOCK(dev, txq);
> +
> + local_bh_enable();
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dfwd_direct_xmit);
> +
Now that we're using the common path to select a queue, can we just use
dev_queue_xmit here instead of creating our own transmit function? The txq we
select from the ixgbe card will just have a pfifo_fast queue on it (if not a
noop queue), so dev_queue_xmit should just fall into the dev_hard_start_xmit
path, and save us this extra coding.
Neil
^ permalink raw reply
* Re: [PATCH net-next 1/4] bonding: update the primary when slave name changed
From: Veaceslav Falico @ 2014-01-09 12:30 UTC (permalink / raw)
To: Ding Tianhong; +Cc: Jay Vosburgh, David S. Miller, Netdev
In-Reply-To: <52CE94DE.5030305@huawei.com>
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.
>
>Regards
>Ding
>
>
>>>
>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>> ---
>>> drivers/net/bonding/bond_main.c | 14 ++++++++++++--
>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index e06c445..de646e2 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -2860,9 +2860,19 @@ static int bond_slave_netdev_event(unsigned long event,
>>> */
>>> break;
>>> case NETDEV_CHANGENAME:
>>> - /*
>>> - * TODO: handle changing the primary's name
>>> + /* if the primary's name changed,
>>> + * save the new name for primary.
>>> */
>>> + if (USES_PRIMARY(bond->params.mode) &&
>>> + bond->params.primary[0]) {
>>> + if (bond->primary_slave &&
>>> + strcmp(bond->params.primary,
>>> + bond->primary_slave->dev->name)) {
>>> + strncpy(bond->params.primary,
>>> + bond->primary_slave->dev->name,
>>> + IFNAMSIZ);
>>> + }
>>> + }
>>> break;
>>> case NETDEV_FEAT_CHANGE:
>>> bond_compute_features(bond);
>>> --
>>> 1.8.0
>>>
>>>
>>
>>
>
>
^ permalink raw reply
* Re: [PATCHv4 net-next] xfrm: Namespacify xfrm_policy_sk_bundles
From: Steffen Klassert @ 2014-01-09 12:38 UTC (permalink / raw)
To: Fan Du; +Cc: Timo Teras, Eric Dumazet, davem, netdev
In-Reply-To: <52CB69DA.7000101@windriver.com>
On Tue, Jan 07, 2014 at 10:43:38AM +0800, Fan Du wrote:
>
> Yes, I tested sk policy with udp, when transmit, dst will be cached into sk
> by sk_dst_set. Let's leave current implementation as it is.
>
> Please kindly review if there is any concern about v4.
Why do you want to keep the current implementation? We don't
use the cached bundles. I'd remove this caching with the patch
below during the next development cycle if nobody has a good
reason why we should keep it.
Subject: [PATCH RFC] xfrm: Remove caching of xfrm_policy_sk_bundles
We currently cache socket policy bundles at xfrm_policy_sk_bundles.
These cached bundles are never used. Instead we create and cache
a new one whenever xfrm_lookup() is called on a socket policy.
Most protocols cache the used routes to the socket, so let's
remove the unused caching of socket policy bundles in xfrm.
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
include/net/netns/xfrm.h | 1 -
net/xfrm/xfrm_policy.c | 28 ----------------------------
2 files changed, 29 deletions(-)
diff --git a/include/net/netns/xfrm.h b/include/net/netns/xfrm.h
index 1006a26..cf65269 100644
--- a/include/net/netns/xfrm.h
+++ b/include/net/netns/xfrm.h
@@ -58,7 +58,6 @@ struct netns_xfrm {
struct dst_ops xfrm6_dst_ops;
#endif
spinlock_t xfrm_state_lock;
- spinlock_t xfrm_policy_sk_bundle_lock;
rwlock_t xfrm_policy_lock;
struct mutex xfrm_cfg_mutex;
};
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index a7487f3..1c79ca8 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -39,8 +39,6 @@
#define XFRM_QUEUE_TMO_MAX ((unsigned)(60*HZ))
#define XFRM_MAX_QUEUE_LEN 100
-static struct dst_entry *xfrm_policy_sk_bundles;
-
static DEFINE_SPINLOCK(xfrm_policy_afinfo_lock);
static struct xfrm_policy_afinfo __rcu *xfrm_policy_afinfo[NPROTO]
__read_mostly;
@@ -2107,13 +2105,6 @@ struct dst_entry *xfrm_lookup(struct net *net, struct dst_entry *dst_orig,
goto no_transform;
}
- dst_hold(&xdst->u.dst);
-
- spin_lock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
- xdst->u.dst.next = xfrm_policy_sk_bundles;
- xfrm_policy_sk_bundles = &xdst->u.dst;
- spin_unlock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
-
route = xdst->route;
}
}
@@ -2547,33 +2538,15 @@ static struct dst_entry *xfrm_negative_advice(struct dst_entry *dst)
return dst;
}
-static void __xfrm_garbage_collect(struct net *net)
-{
- struct dst_entry *head, *next;
-
- spin_lock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
- head = xfrm_policy_sk_bundles;
- xfrm_policy_sk_bundles = NULL;
- spin_unlock_bh(&net->xfrm.xfrm_policy_sk_bundle_lock);
-
- while (head) {
- next = head->next;
- dst_free(head);
- head = next;
- }
-}
-
void xfrm_garbage_collect(struct net *net)
{
flow_cache_flush();
- __xfrm_garbage_collect(net);
}
EXPORT_SYMBOL(xfrm_garbage_collect);
static void xfrm_garbage_collect_deferred(struct net *net)
{
flow_cache_flush_deferred();
- __xfrm_garbage_collect(net);
}
static void xfrm_init_pmtu(struct dst_entry *dst)
@@ -2942,7 +2915,6 @@ static int __net_init xfrm_net_init(struct net *net)
/* Initialize the per-net locks here */
spin_lock_init(&net->xfrm.xfrm_state_lock);
rwlock_init(&net->xfrm.xfrm_policy_lock);
- spin_lock_init(&net->xfrm.xfrm_policy_sk_bundle_lock);
mutex_init(&net->xfrm.xfrm_cfg_mutex);
return 0;
--
1.7.9.5
^ permalink raw reply related
* Re: mlx4 w/ IOMMU broken, kernel 3.12.6 -- resolved
From: David Lamparter @ 2014-01-09 12:46 UTC (permalink / raw)
To: Amir Vadai; +Cc: David Lamparter, netdev
In-Reply-To: <20140109075411.GA27750@mtl-eit-vdi-22.mtl.labs.mlnx>
On Thu, Jan 09, 2014 at 09:54:15AM +0200, Amir Vadai wrote:
> What is the firmware version?
> You can get it by issuing 'ethtool -i <interface>'
>
> Please make sure you're using the latest FW from mellanox.com.
Ah, indeed updating from 2.11.500 to 2.30.8000 has fixed the issue.
(The driver could print a warning for this?)
(Note: updating the firmware has been rather annoying due to the complex
and complicated way the OEM branding [this is an IBM card] is designed.
If you have some way of passing up user experience feedback, please put
a note down on this.)
> We still don't have MLNX_OFED package that supports this kernel.
I noticed, the kernel driver from the external package is referencing
things that have disappeared from the kernel around version 3.8. Though
I don't see why there is both an in-kernel driver and a separate
package...
Thanks for the firmware pointer,
David
^ permalink raw reply
* [net-next 00/15][pull request] Intel Wired LAN Driver Updates
From: Jeff Kirsher @ 2014-01-09 12:52 UTC (permalink / raw)
To: davem; +Cc: Jeff Kirsher, netdev, gospo, sassmann
This series contains updates to i40e only.
Anjali provides a fix where interrupts were not being re-enabled on ICR0
even though they were auto masked by hardware. Then provides a fix to
cleanup RSS initialization because it was doing some extra work, so
remove the extra work and any bugs it created when managing number of
queues. Since hardware requires a full packet template to be pointed to
when adding hardware flow filters, add the template and use it for
programming filters.
Jesse provides a fix to replace the use of driver specific defines with
kernel ETH_ALEN defines. Then disables packet split because with the
use of GRO, we do not need the extra bus overhead. Fixes spelling
error in code comment.
Kamil provides a fix for the driver where the hardware expects the MAC
address in a very specific format and the driver was filing the data
incorrectly.
Mitch provides a fix to resolve a panic on reset by adding checks to
VSI->rx_rings. Then shortens alloc_rx_buff_failed and
alloc_rx_page_failed variables since both part of an RX specific
structure so just remove the _rx part of the name. Then fixes
badly formatted lines, long lines and mis-formatted lines.
Shannon provides a fix to call AQ to release any reservation held by this
PF on the NVM resource lock on startup, in order to clear anything that
might have been left over from a previous run. Then removes interrupt on
AQ error since nearly everything we do is synchronous, using the
interrupt-on-error bit is unnecessary and causing unneeded interrupts.
Adds code to handle the ability to send messages among the physical
function interfaces by the admin queue.
Catherine sets the MFP flag earlier in software init and uses that flag
to decide if other hardware work-arounds are required which turns
off flow director in MFP mode.
The following are changes since commit 54b553e2c16001d13e0186cad2531764065f9a1b:
Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/net-next master
Anjali Singhai Jain (2):
i40e: Re-enable interrupt on ICR0
i40e: Cleanup reconfig rss path
Catherine Sullivan (1):
i40e: Turn flow director off in MFP mode
Greg Rose (2):
i40e: Fix GPL header
i40e: add a comment on barrier and fix panic on reset
Jesse Brandeburg (4):
i40e: use kernel specific defines
i40e: disable packet split
i40e: fix spelling errors
i40e: Add a dummy packet template
Kamil Krawczyk (1):
i40e: Fix MAC format in Write MAC address AQ cmd
Mitch Williams (2):
i40e: shorten wordy fields
i40e: trivial: formatting and checkpatch fixes
Shannon Nelson (3):
i40e: release NVM resource reservation on startup
i40e: remove interrupt on AQ error
i40e: accept pf to pf adminq messages
drivers/net/ethernet/intel/i40e/Makefile | 7 +--
drivers/net/ethernet/intel/i40e/i40e.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_adminq.c | 12 ++--
drivers/net/ethernet/intel/i40e/i40e_adminq.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_adminq_cmd.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_alloc.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_common.c | 14 +++--
drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 13 ++---
drivers/net/ethernet/intel/i40e/i40e_diag.c | 7 +--
drivers/net/ethernet/intel/i40e/i40e_diag.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 33 ++++++++---
drivers/net/ethernet/intel/i40e/i40e_hmc.c | 7 +--
drivers/net/ethernet/intel/i40e/i40e_hmc.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_lan_hmc.c | 7 +--
drivers/net/ethernet/intel/i40e/i40e_lan_hmc.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_main.c | 68 +++++++++++-----------
drivers/net/ethernet/intel/i40e/i40e_nvm.c | 7 +--
drivers/net/ethernet/intel/i40e/i40e_osdep.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_prototype.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_register.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_status.h | 7 +--
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 44 +++++++-------
drivers/net/ethernet/intel/i40e/i40e_txrx.h | 11 ++--
drivers/net/ethernet/intel/i40e/i40e_type.h | 15 ++---
drivers/net/ethernet/intel/i40e/i40e_virtchnl.h | 11 ++--
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c | 10 ++--
drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.h | 7 +--
27 files changed, 176 insertions(+), 174 deletions(-)
--
1.8.3.1
^ 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