* [net v2 6/6] i40e: clean up coccicheck reported errors
From: Jeff Kirsher @ 2013-09-27 12:35 UTC (permalink / raw)
To: davem; +Cc: Jesse Brandeburg, netdev, gospo, sassmann, Jeff Kirsher
In-Reply-To: <1380285358-15685-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
coccicheck shows:
drivers/net/ethernet/intel/i40e/i40e_adminq.c:704:2-8: Replace memcpy
with struct assignment
drivers/net/ethernet/intel/i40e/i40e_adminq.c:763:1-7: Replace memcpy
with struct assignment
drivers/net/ethernet/intel/i40e/i40e_adminq.c:810:2-8: Replace memcpy
with struct assignment
drivers/net/ethernet/intel/i40e/i40e_common.c:510:2-8: Replace memcpy
with struct assignment
Fix each of them with a *a = *b;
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_adminq.c | 7 +++----
drivers/net/ethernet/intel/i40e/i40e_common.c | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_adminq.c b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
index 0c524fa..cfef7fc 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_adminq.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_adminq.c
@@ -701,8 +701,7 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw,
details = I40E_ADMINQ_DETAILS(hw->aq.asq, hw->aq.asq.next_to_use);
if (cmd_details) {
- memcpy(details, cmd_details,
- sizeof(struct i40e_asq_cmd_details));
+ *details = *cmd_details;
/* If the cmd_details are defined copy the cookie. The
* cpu_to_le32 is not needed here because the data is ignored
@@ -760,7 +759,7 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw,
desc_on_ring = I40E_ADMINQ_DESC(hw->aq.asq, hw->aq.asq.next_to_use);
/* if the desc is available copy the temp desc to the right place */
- memcpy(desc_on_ring, desc, sizeof(struct i40e_aq_desc));
+ *desc_on_ring = *desc;
/* if buff is not NULL assume indirect command */
if (buff != NULL) {
@@ -807,7 +806,7 @@ i40e_status i40e_asq_send_command(struct i40e_hw *hw,
/* if ready, copy the desc back to temp */
if (i40e_asq_done(hw)) {
- memcpy(desc, desc_on_ring, sizeof(struct i40e_aq_desc));
+ *desc = *desc_on_ring;
if (buff != NULL)
memcpy(buff, dma_buff->va, buff_size);
retval = le16_to_cpu(desc->retval);
diff --git a/drivers/net/ethernet/intel/i40e/i40e_common.c b/drivers/net/ethernet/intel/i40e/i40e_common.c
index c21df7b..1e4ea13 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_common.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_common.c
@@ -507,7 +507,7 @@ i40e_status i40e_aq_get_link_info(struct i40e_hw *hw,
/* save link status information */
if (link)
- memcpy(link, hw_link_info, sizeof(struct i40e_link_status));
+ *link = *hw_link_info;
/* flag cleared so helper functions don't call AQ again */
hw->phy.get_link_info = false;
--
1.8.3.1
^ permalink raw reply related
* [net v2 5/6] i40e: better return values
From: Jeff Kirsher @ 2013-09-27 12:35 UTC (permalink / raw)
To: davem; +Cc: Jesse Brandeburg, netdev, gospo, sassmann, Joe Perches,
Jeff Kirsher
In-Reply-To: <1380285358-15685-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Jesse Brandeburg <jesse.brandeburg@intel.com>
As mentioned by Joe Perches, clean up return values in some functions
making sure to have consistent return types, not mixing types.
A couple of Joe's comments suggested returning void, but since
the functions in question are ndo defined, the return values are fixed.
So make a comment in the header that notes this is a function called by
net_device_ops.
v2: fix post increment bug in return
CC: Joe Perches <joe@perches.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Kavindya Deegala <kavindya.s.deegala@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 37 ++++++++++++++++-------------
1 file changed, 21 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 60c7152..221aa47 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -1787,6 +1787,8 @@ int i40e_vsi_add_vlan(struct i40e_vsi *vsi, s16 vid)
* i40e_vsi_kill_vlan - Remove vsi membership for given vlan
* @vsi: the vsi being configured
* @vid: vlan id to be removed (0 = untagged only , -1 = any)
+ *
+ * Return: 0 on success or negative otherwise
**/
int i40e_vsi_kill_vlan(struct i40e_vsi *vsi, s16 vid)
{
@@ -1860,37 +1862,39 @@ int i40e_vsi_kill_vlan(struct i40e_vsi *vsi, s16 vid)
* i40e_vlan_rx_add_vid - Add a vlan id filter to HW offload
* @netdev: network interface to be adjusted
* @vid: vlan id to be added
+ *
+ * net_device_ops implementation for adding vlan ids
**/
static int i40e_vlan_rx_add_vid(struct net_device *netdev,
__always_unused __be16 proto, u16 vid)
{
struct i40e_netdev_priv *np = netdev_priv(netdev);
struct i40e_vsi *vsi = np->vsi;
- int ret;
+ int ret = 0;
if (vid > 4095)
- return 0;
+ return -EINVAL;
+
+ netdev_info(netdev, "adding %pM vid=%d\n", netdev->dev_addr, vid);
- netdev_info(vsi->netdev, "adding %pM vid=%d\n",
- netdev->dev_addr, vid);
/* If the network stack called us with vid = 0, we should
* indicate to i40e_vsi_add_vlan() that we want to receive
* any traffic (i.e. with any vlan tag, or untagged)
*/
ret = i40e_vsi_add_vlan(vsi, vid ? vid : I40E_VLAN_ANY);
- if (!ret) {
- if (vid < VLAN_N_VID)
- set_bit(vid, vsi->active_vlans);
- }
+ if (!ret && (vid < VLAN_N_VID))
+ set_bit(vid, vsi->active_vlans);
- return 0;
+ return ret;
}
/**
* i40e_vlan_rx_kill_vid - Remove a vlan id filter from HW offload
* @netdev: network interface to be adjusted
* @vid: vlan id to be removed
+ *
+ * net_device_ops implementation for adding vlan ids
**/
static int i40e_vlan_rx_kill_vid(struct net_device *netdev,
__always_unused __be16 proto, u16 vid)
@@ -1898,15 +1902,16 @@ static int i40e_vlan_rx_kill_vid(struct net_device *netdev,
struct i40e_netdev_priv *np = netdev_priv(netdev);
struct i40e_vsi *vsi = np->vsi;
- netdev_info(vsi->netdev, "removing %pM vid=%d\n",
- netdev->dev_addr, vid);
+ netdev_info(netdev, "removing %pM vid=%d\n", netdev->dev_addr, vid);
+
/* return code is ignored as there is nothing a user
* can do about failure to remove and a log message was
- * already printed from another function
+ * already printed from the other function
*/
i40e_vsi_kill_vlan(vsi, vid);
clear_bit(vid, vsi->active_vlans);
+
return 0;
}
@@ -3324,7 +3329,8 @@ static void i40e_pf_unquiesce_all_vsi(struct i40e_pf *pf)
**/
static u8 i40e_dcb_get_num_tc(struct i40e_dcbx_config *dcbcfg)
{
- int num_tc = 0, i;
+ u8 num_tc = 0;
+ int i;
/* Scan the ETS Config Priority Table to find
* traffic class enabled for a given priority
@@ -3339,9 +3345,7 @@ static u8 i40e_dcb_get_num_tc(struct i40e_dcbx_config *dcbcfg)
/* Traffic class index starts from zero so
* increment to return the actual count
*/
- num_tc++;
-
- return num_tc;
+ return num_tc + 1;
}
/**
@@ -3491,6 +3495,7 @@ static int i40e_vsi_get_bw_info(struct i40e_vsi *vsi)
/* 3 bits out of 4 for each TC */
vsi->bw_ets_max_quanta[i] = (u8)((tc_bw_max >> (i*4)) & 0x7);
}
+
return 0;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 1/2] bonding: correctly verify for the first slave in bond_enslave
From: Veaceslav Falico @ 2013-09-27 13:10 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1380287458-3488-1-git-send-email-vfalico@redhat.com>
After commit 1f718f0f4f97145f4072d2d72dcf85069ca7226d ("bonding: populate
neighbour's private on enslave"), we've moved the actual 'linking' in the
end of the function - so that, once linked, the slave is ready to be used,
and is not still in the process of enslaving.
However, 802.3ad verified if it's the first slave by looking at the
if (bond_first_slave(bond) == new_slave)
which, because we've moved the linking to the end, became broken - on the
first slave bond_first_slave(bond) returns NULL.
Fix this by verifying if the prev_slave, that equals bond_last_slave(), is
actually populated - if it is - then it's not the first slave, and vice
versa.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d5c3153..0367f80 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1557,7 +1557,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
*/
bond_set_slave_inactive_flags(new_slave);
/* if this is the first slave */
- if (bond_first_slave(bond) == new_slave) {
+ if (!prev_slave) {
SLAVE_AD_INFO(new_slave).id = 1;
/* Initialize AD with the number of times that the AD timer is called in 1 second
* can be called only after the mac address of the bond is set
--
1.8.4
^ permalink raw reply related
* [PATCH net-next 0/2] bonding: fix 3ad slave (de)init
From: Veaceslav Falico @ 2013-09-27 13:10 UTC (permalink / raw)
To: netdev; +Cc: Jay Vosburgh, Andy Gospodarek, Veaceslav Falico
After 1f718f0f4f97145f4072d2d72dcf85069ca7226d ("bonding: populate
neighbour's private on enslave") the (de)linking of slaves in
bond_enslave/bond_release_one happens in the correct places - after we've
completely initialized the slave (for bond_enslave) and before we've even
began to de-init the slave (for bond_release_one, respectively).
This was done to prevent any RCU readers to see the half-initialized slave
(because the RCU readers aren't blocked by bond->lock or rtnl_lock
usually).
However, 802.3ad logic, in several places, relied on the fact that the
slave is still linked to the bond.
Fix it by correctly handling these cases - we shouldn't rely that the slave
is linked before fully initialized and, respectively, that the slave is
still linked while it's being removed.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 4 +++-
drivers/net/bonding/bond_main.c | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
^ permalink raw reply
* [PATCH net-next 2/2] bonding: verify if we still have slaves in bond_3ad_unbind_slave()
From: Veaceslav Falico @ 2013-09-27 13:10 UTC (permalink / raw)
To: netdev; +Cc: Veaceslav Falico, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1380287458-3488-1-git-send-email-vfalico@redhat.com>
After commit 1f718f0f4f97145f4072d2d72dcf85069ca7226d ("bonding: populate
neighbour's private on enslave"), we've moved the unlinking of the slave
to the earliest position possible - so that nobody will see an
half-uninited slave.
However, bond_3ad_unbind_slave() relied that, even while removing the last
slave, it is still accessible - via __get_first_agg() (and, eventually,
bond_first_slave()).
Fix that by verifying if the aggregator return is an actual aggregator, but
not NULL.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 1337eaf..2715ea8 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2056,7 +2056,9 @@ void bond_3ad_unbind_slave(struct slave *slave)
pr_info("%s: Removing an active aggregator\n",
slave->bond->dev->name);
// select new active aggregator
- ad_agg_selection_logic(__get_first_agg(port));
+ temp_aggregator = __get_first_agg(port);
+ if (temp_aggregator)
+ ad_agg_selection_logic(temp_aggregator);
}
}
}
--
1.8.4
^ permalink raw reply related
* [PATCH v3] powerpc/83xx: gianfar_ptp: select 1588 clock source through dts file
From: Aida Mynzhasova @ 2013-09-27 13:40 UTC (permalink / raw)
To: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
richardcochran-Re5JQEeQqe8AvxtiuMwx3w,
galak-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r
Currently IEEE 1588 timer reference clock source is determined through
hard-coded value in gianfar_ptp driver. This patch allows to select ptp
clock source by means of device tree file node.
For instance:
fsl,cksel = <0>;
for using external (TSEC_TMR_CLK input) high precision timer
reference clock.
Other acceptable values:
<1> : eTSEC system clock
<2> : eTSEC1 transmit clock
<3> : RTC clock input
When this attribute isn't used, eTSEC system clock will serve as
IEEE 1588 timer reference clock.
Signed-off-by: Aida Mynzhasova <aida.mynzhasova-Fmhy8gsqeTEvJsYlp49lxw@public.gmane.org>
---
Documentation/devicetree/bindings/net/fsl-tsec-phy.txt | 18 +++++++++++++++++-
drivers/net/ethernet/freescale/gianfar_ptp.c | 4 +++-
2 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
index 2c6be03..d2ea460 100644
--- a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
+++ b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
@@ -86,6 +86,7 @@ General Properties:
Clock Properties:
+ - fsl,cksel Timer reference clock source.
- fsl,tclk-period Timer reference clock period in nanoseconds.
- fsl,tmr-prsc Prescaler, divides the output clock.
- fsl,tmr-add Frequency compensation value.
@@ -97,7 +98,7 @@ Clock Properties:
clock. You must choose these carefully for the clock to work right.
Here is how to figure good values:
- TimerOsc = system clock MHz
+ TimerOsc = selected reference clock MHz
tclk_period = desired clock period nanoseconds
NominalFreq = 1000 / tclk_period MHz
FreqDivRatio = TimerOsc / NominalFreq (must be greater that 1.0)
@@ -114,6 +115,20 @@ Clock Properties:
Pulse Per Second (PPS) signal, since this will be offered to the PPS
subsystem to synchronize the Linux clock.
+ Reference clock source is determined by the value, which is holded
+ in CKSEL bits in TMR_CTRL register. "fsl,cksel" property keeps the
+ value, which will be directly written in those bits, that is why,
+ according to reference manual, the next clock sources can be used:
+
+ <0> - external high precision timer reference clock (TSEC_TMR_CLK
+ input is used for this purpose);
+ <1> - eTSEC system clock;
+ <2> - eTSEC1 transmit clock;
+ <3> - RTC clock input.
+
+ When this attribute is not used, eTSEC system clock will serve as
+ IEEE 1588 timer reference clock.
+
Example:
ptp_clock@24E00 {
@@ -121,6 +136,7 @@ Example:
reg = <0x24E00 0xB0>;
interrupts = <12 0x8 13 0x8>;
interrupt-parent = < &ipic >;
+ fsl,cksel = <1>;
fsl,tclk-period = <10>;
fsl,tmr-prsc = <100>;
fsl,tmr-add = <0x999999A4>;
diff --git a/drivers/net/ethernet/freescale/gianfar_ptp.c b/drivers/net/ethernet/freescale/gianfar_ptp.c
index 098f133..e006a09 100644
--- a/drivers/net/ethernet/freescale/gianfar_ptp.c
+++ b/drivers/net/ethernet/freescale/gianfar_ptp.c
@@ -452,7 +452,9 @@ static int gianfar_ptp_probe(struct platform_device *dev)
err = -ENODEV;
etsects->caps = ptp_gianfar_caps;
- etsects->cksel = DEFAULT_CKSEL;
+
+ if (get_of_u32(node, "fsl,cksel", &etsects->cksel))
+ etsects->cksel = DEFAULT_CKSEL;
if (get_of_u32(node, "fsl,tclk-period", &etsects->tclk_period) ||
get_of_u32(node, "fsl,tmr-prsc", &etsects->tmr_prsc) ||
--
1.8.1.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH net-next 0/9] bonding: remove bond_next_slave()
From: Veaceslav Falico @ 2013-09-27 14:11 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Jay Vosburgh, Andy Gospodarek,
Veaceslav Falico
(on top of "[PATCH net-next 0/2] bonding: fix 3ad slave (de)init" - the
patchset is essential)
Hi,
As Ben Hutchings and Nikolay Aleksandrov correctly noted -
bond_next_slave() already is not O(1), but rather O(n), so it's only useful
for one-off operations and shouldn't be used widely - for example, for list
traversal, which will take O(n^2) time, which will be disastrous for any
hot path with a large number of slaves.
Currently, bond_next_slave() is used several times in 802.3ad and for
seq_read - bond_info_seq_next().
The 802.3ad part uses it only in constructs like:
for (X = __get_first_X(); X; X = __get_next_X()) {
where __get_next_X() uses bond_next_slave().
This for can (and should) actually be replaced by the standard
bond_for_each_slave(bond, slave, iter) {
X = __get_X_by_slave(slave);
it's faster, easier to read, debug and maintain. Also, removes a lot of
code lines.
After replacing it, the only user of bond_next_slave() is
bond_info_seq_next() - which can, actually, implement it by itself, and not
call another function.
So, that way, we can completely remove the bond_next_slave(), cleanup and
optimize a bit.
p.s. the 802.3ad code is horrible, both style-wise and from the logical
point of view - so I've decided to *not* change anything except that what
this patch is intended to provide. The cleanup and some refactoring should
be done in another patchset, which I've began to work on already.
Thank you!
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 137 ++++++++++++++------------------------
drivers/net/bonding/bond_main.c | 2 +-
drivers/net/bonding/bond_procfs.c | 16 +++--
drivers/net/bonding/bonding.h | 31 ---------
4 files changed, 62 insertions(+), 124 deletions(-)
^ permalink raw reply
* [PATCH net-next 1/9] bonding: remove __get_next_port()
From: Veaceslav Falico @ 2013-09-27 14:11 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Veaceslav Falico, Jay Vosburgh,
Andy Gospodarek
In-Reply-To: <1380291125-5671-1-git-send-email-vfalico@redhat.com>
Currently this function is only used in constructs like
for (port = __get_first_port(bond); port; port = __get_next_port(port))
which is basicly the same as
bond_for_each_slave(bond, slave, iter) {
port = &(SLAVE_AD_INFO(slave).port);
but a more time consuming.
Remove the function and convert the users to bond_for_each_slave().
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 34 +++++++++-------------------------
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 2715ea8..c1535f8 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -149,28 +149,6 @@ static inline struct port *__get_first_port(struct bonding *bond)
}
/**
- * __get_next_port - get the next port in the bond
- * @port: the port we're looking at
- *
- * Return the port of the slave that is next in line of @port's slave in the
- * bond, or %NULL if it can't be found.
- */
-static inline struct port *__get_next_port(struct port *port)
-{
- struct bonding *bond = __get_bond_by_port(port);
- struct slave *slave = port->slave, *slave_next;
-
- // If there's no bond for this port, or this is the last slave
- if (bond == NULL)
- return NULL;
- slave_next = bond_next_slave(bond, slave);
- if (!slave_next || bond_is_first_slave(bond, slave_next))
- return NULL;
-
- return &(SLAVE_AD_INFO(slave_next).port);
-}
-
-/**
* __get_first_agg - get the first aggregator in the bond
* @bond: the bond we're looking at
*
@@ -2113,8 +2091,10 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
{
struct bonding *bond = container_of(work, struct bonding,
ad_work.work);
- struct port *port;
struct aggregator *aggregator;
+ struct list_head *iter;
+ struct slave *slave;
+ struct port *port;
read_lock(&bond->lock);
@@ -2139,7 +2119,8 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
}
// for each port run the state machines
- for (port = __get_first_port(bond); port; port = __get_next_port(port)) {
+ bond_for_each_slave(bond, slave, iter) {
+ port = &(SLAVE_AD_INFO(slave).port);
if (!port->slave) {
pr_warning("%s: Warning: Found an uninitialized port\n",
bond->dev->name);
@@ -2384,9 +2365,12 @@ int __bond_3ad_get_active_agg_info(struct bonding *bond,
struct ad_info *ad_info)
{
struct aggregator *aggregator = NULL;
+ struct list_head *iter;
+ struct slave *slave;
struct port *port;
- for (port = __get_first_port(bond); port; port = __get_next_port(port)) {
+ bond_for_each_slave(bond, slave, iter) {
+ port = &(SLAVE_AD_INFO(slave).port);
if (port->aggregator && port->aggregator->is_active) {
aggregator = port->aggregator;
break;
--
1.8.4
^ permalink raw reply related
* [PATCH net-next 2/9] bonding: remove __get_first_port()
From: Veaceslav Falico @ 2013-09-27 14:11 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Veaceslav Falico, Jay Vosburgh,
Andy Gospodarek
In-Reply-To: <1380291125-5671-1-git-send-email-vfalico@redhat.com>
Currently we have only one user of it, so it's kind of useless and just
obfusicates things.
Remove it and move the logic to the only user -
bond_3ad_state_machine_handler().
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index c1535f8..0f86d2b 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -136,19 +136,6 @@ static inline struct bonding *__get_bond_by_port(struct port *port)
}
/**
- * __get_first_port - get the first port in the bond
- * @bond: the bond we're looking at
- *
- * Return the port of the first slave in @bond, or %NULL if it can't be found.
- */
-static inline struct port *__get_first_port(struct bonding *bond)
-{
- struct slave *first_slave = bond_first_slave(bond);
-
- return first_slave ? &(SLAVE_AD_INFO(first_slave).port) : NULL;
-}
-
-/**
* __get_first_agg - get the first aggregator in the bond
* @bond: the bond we're looking at
*
@@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
// check if agg_select_timer timer after initialize is timed out
if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
+ slave = bond_first_slave(bond);
+ port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
+
// select the active aggregator for the bond
- if ((port = __get_first_port(bond))) {
+ if (port) {
if (!port->slave) {
pr_warning("%s: Warning: bond's first port is uninitialized\n",
bond->dev->name);
--
1.8.4
^ permalink raw reply related
* [PATCH net-next 3/9] bonding: make ad_port_selection_logic() use bond_for_each_slave()
From: Veaceslav Falico @ 2013-09-27 14:11 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Veaceslav Falico, Jay Vosburgh,
Andy Gospodarek
In-Reply-To: <1380291125-5671-1-git-send-email-vfalico@redhat.com>
Currently, ad_port_selection_logic() uses
for (aggregator = __get_first_agg(port); aggregator;
aggregator = __get_next_agg(aggregator)) {
construct, however it's suboptimal, difficult to read and understand.
Change it to a standard bond_for_each_slave(), so that we won't need
__get_first/next_agg() and have it more readable.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 0f86d2b..8b64ed4 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1239,12 +1239,17 @@ static void ad_port_selection_logic(struct port *port)
{
struct aggregator *aggregator, *free_aggregator = NULL, *temp_aggregator;
struct port *last_port = NULL, *curr_port;
+ struct list_head *iter;
+ struct bonding *bond;
+ struct slave *slave;
int found = 0;
// if the port is already Selected, do nothing
if (port->sm_vars & AD_PORT_SELECTED)
return;
+ bond = __get_bond_by_port(port);
+
// if the port is connected to other aggregator, detach it
if (port->aggregator) {
// detach the port from its former aggregator
@@ -1285,8 +1290,8 @@ static void ad_port_selection_logic(struct port *port)
}
}
// search on all aggregators for a suitable aggregator for this port
- for (aggregator = __get_first_agg(port); aggregator;
- aggregator = __get_next_agg(aggregator)) {
+ bond_for_each_slave(bond, slave, iter) {
+ aggregator = &(SLAVE_AD_INFO(slave).aggregator);
// keep a free aggregator for later use(if needed)
if (!aggregator->lag_ports) {
--
1.8.4
^ permalink raw reply related
* [PATCH net-next 4/9] bonding: make __get_active_agg() use bond_for_each_slave()
From: Veaceslav Falico @ 2013-09-27 14:12 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Veaceslav Falico, Jay Vosburgh,
Andy Gospodarek
In-Reply-To: <1380291125-5671-1-git-send-email-vfalico@redhat.com>
Currently we're relying on suboptimal construct
for (; aggregator; aggregator = __get_next_agg(aggregator)) {
where aggregator is an argument of __get_active_agg() which is _always_ the
first slave's aggregator - judging by all the callers, comments in the
ad_agg_selection_logic() and by logic.
Convert it to use the standard bond_for_each_slave().
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 8b64ed4..1109d82 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -720,16 +720,15 @@ static u32 __get_agg_bandwidth(struct aggregator *aggregator)
*/
static struct aggregator *__get_active_agg(struct aggregator *aggregator)
{
- struct aggregator *retval = NULL;
+ struct bonding *bond = aggregator->slave->bond;
+ struct list_head *iter;
+ struct slave *slave;
- for (; aggregator; aggregator = __get_next_agg(aggregator)) {
- if (aggregator->is_active) {
- retval = aggregator;
- break;
- }
- }
+ bond_for_each_slave(bond, slave, iter)
+ if (SLAVE_AD_INFO(slave).aggregator.is_active)
+ return &(SLAVE_AD_INFO(slave).aggregator);
- return retval;
+ return NULL;
}
/**
--
1.8.4
^ permalink raw reply related
* [PATCH net-next 5/9] bonding: make ad_agg_selection_logic() use bond_for_each_slave()
From: Veaceslav Falico @ 2013-09-27 14:12 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Veaceslav Falico, Jay Vosburgh,
Andy Gospodarek
In-Reply-To: <1380291125-5671-1-git-send-email-vfalico@redhat.com>
Convert all instances of
for (agg = __get_first_agg(); agg; agg = __get_next_port)
to the standard bond_for_each_slave(). Also, remove the useless checks
before calling bond_3ad_set_carrier() - if we have something NULL - it
would fire long ago, in __get_first/next_port(), per example.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 1109d82..6cd1444 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1484,19 +1484,23 @@ static int agg_device_up(const struct aggregator *agg)
static void ad_agg_selection_logic(struct aggregator *agg)
{
struct aggregator *best, *active, *origin;
+ struct bonding *bond = agg->slave->bond;
+ struct list_head *iter;
+ struct slave *slave;
struct port *port;
origin = agg;
active = __get_active_agg(agg);
best = (active && agg_device_up(active)) ? active : NULL;
- do {
+ bond_for_each_slave(bond, slave, iter) {
+ agg = &(SLAVE_AD_INFO(slave).aggregator);
+
agg->is_active = 0;
if (agg->num_of_ports && agg_device_up(agg))
best = ad_agg_selection_test(best, agg);
-
- } while ((agg = __get_next_agg(agg)));
+ }
if (best &&
__get_agg_selection_mode(best->lag_ports) == BOND_AD_STABLE) {
@@ -1534,8 +1538,8 @@ static void ad_agg_selection_logic(struct aggregator *agg)
best->lag_ports, best->slave,
best->slave ? best->slave->dev->name : "NULL");
- for (agg = __get_first_agg(best->lag_ports); agg;
- agg = __get_next_agg(agg)) {
+ bond_for_each_slave(bond, slave, iter) {
+ agg = &(SLAVE_AD_INFO(slave).aggregator);
pr_debug("Agg=%d; P=%d; a k=%d; p k=%d; Ind=%d; Act=%d\n",
agg->aggregator_identifier, agg->num_of_ports,
@@ -1583,13 +1587,7 @@ static void ad_agg_selection_logic(struct aggregator *agg)
}
}
- if (origin->slave) {
- struct bonding *bond;
-
- bond = bond_get_bond_by_slave(origin->slave);
- if (bond)
- bond_3ad_set_carrier(bond);
- }
+ bond_3ad_set_carrier(bond);
}
/**
--
1.8.4
^ permalink raw reply related
* [PATCH net-next 7/9] bonding: remove unused __get_next_agg()
From: Veaceslav Falico @ 2013-09-27 14:12 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Veaceslav Falico, Jay Vosburgh,
Andy Gospodarek
In-Reply-To: <1380291125-5671-1-git-send-email-vfalico@redhat.com>
It has no users, so it's safe to remove it completely.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 22 ----------------------
1 file changed, 22 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 6dbb84d..c62606a 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -155,28 +155,6 @@ static inline struct aggregator *__get_first_agg(struct port *port)
return first_slave ? &(SLAVE_AD_INFO(first_slave).aggregator) : NULL;
}
-/**
- * __get_next_agg - get the next aggregator in the bond
- * @aggregator: the aggregator we're looking at
- *
- * Return the aggregator of the slave that is next in line of @aggregator's
- * slave in the bond, or %NULL if it can't be found.
- */
-static inline struct aggregator *__get_next_agg(struct aggregator *aggregator)
-{
- struct slave *slave = aggregator->slave, *slave_next;
- struct bonding *bond = bond_get_bond_by_slave(slave);
-
- // If there's no bond for this aggregator, or this is the last slave
- if (bond == NULL)
- return NULL;
- slave_next = bond_next_slave(bond, slave);
- if (!slave_next || bond_is_first_slave(bond, slave_next))
- return NULL;
-
- return &(SLAVE_AD_INFO(slave_next).aggregator);
-}
-
/*
* __agg_has_partner
*
--
1.8.4
^ permalink raw reply related
* [PATCH net-next 6/9] bonding: make bond_3ad_unbind_slave() use bond_for_each_slave()
From: Veaceslav Falico @ 2013-09-27 14:12 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Veaceslav Falico, Jay Vosburgh,
Andy Gospodarek
In-Reply-To: <1380291125-5671-1-git-send-email-vfalico@redhat.com>
Convert all instances of
for (agg = __get_first_agg(); agg; agg = __get_next_port)
to the standard bond_for_each_slave().
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_3ad.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 6cd1444..6dbb84d 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -1936,6 +1936,9 @@ void bond_3ad_unbind_slave(struct slave *slave)
struct port *port, *prev_port, *temp_port;
struct aggregator *aggregator, *new_aggregator, *temp_aggregator;
int select_new_active_agg = 0;
+ struct bonding *bond = slave->bond;
+ struct slave *slave_iter;
+ struct list_head *iter;
// find the aggregator related to this slave
aggregator = &(SLAVE_AD_INFO(slave).aggregator);
@@ -1965,14 +1968,16 @@ void bond_3ad_unbind_slave(struct slave *slave)
// reason to search for new aggregator, and that we will find one
if ((aggregator->lag_ports != port) || (aggregator->lag_ports->next_port_in_aggregator)) {
// find new aggregator for the related port(s)
- new_aggregator = __get_first_agg(port);
- for (; new_aggregator; new_aggregator = __get_next_agg(new_aggregator)) {
+ bond_for_each_slave(bond, slave_iter, iter) {
+ new_aggregator = &(SLAVE_AD_INFO(slave_iter).aggregator);
// if the new aggregator is empty, or it is connected to our port only
if (!new_aggregator->lag_ports
|| ((new_aggregator->lag_ports == port)
&& !new_aggregator->lag_ports->next_port_in_aggregator))
break;
}
+ if (!slave_iter)
+ new_aggregator = NULL;
// if new aggregator found, copy the aggregator's parameters
// and connect the related lag_ports to the new aggregator
if ((new_aggregator) && ((!new_aggregator->lag_ports) || ((new_aggregator->lag_ports == port) && !new_aggregator->lag_ports->next_port_in_aggregator))) {
@@ -2032,8 +2037,8 @@ void bond_3ad_unbind_slave(struct slave *slave)
pr_debug("Unbinding port %d\n", port->actor_port_number);
// find the aggregator that this port is connected to
- temp_aggregator = __get_first_agg(port);
- for (; temp_aggregator; temp_aggregator = __get_next_agg(temp_aggregator)) {
+ bond_for_each_slave(bond, slave_iter, iter) {
+ temp_aggregator = &(SLAVE_AD_INFO(slave_iter).aggregator);
prev_port = NULL;
// search the port in the aggregator's related ports
for (temp_port = temp_aggregator->lag_ports; temp_port;
--
1.8.4
^ permalink raw reply related
* [PATCH net-next 8/9] bonding: don't use bond_next_slave() in bond_info_seq_next()
From: Veaceslav Falico @ 2013-09-27 14:12 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Veaceslav Falico, Jay Vosburgh,
Andy Gospodarek
In-Reply-To: <1380291125-5671-1-git-send-email-vfalico@redhat.com>
We don't need the circular loop there and it's the only current user of
bond_next_slave() - so just use the standard bond_for_each_slave().
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bond_procfs.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c
index 7af5646..fb868d6 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -31,17 +31,25 @@ static void *bond_info_seq_start(struct seq_file *seq, loff_t *pos)
static void *bond_info_seq_next(struct seq_file *seq, void *v, loff_t *pos)
{
struct bonding *bond = seq->private;
- struct slave *slave = v;
+ struct list_head *iter;
+ struct slave *slave;
+ bool found = false;
++*pos;
if (v == SEQ_START_TOKEN)
return bond_first_slave(bond);
- if (bond_is_last_slave(bond, slave))
+ if (bond_is_last_slave(bond, v))
return NULL;
- slave = bond_next_slave(bond, slave);
- return slave;
+ bond_for_each_slave(bond, slave, iter) {
+ if (found)
+ return slave;
+ if (slave == v)
+ found = true;
+ }
+
+ return NULL;
}
static void bond_info_seq_stop(struct seq_file *seq, void *v)
--
1.8.4
^ permalink raw reply related
* [PATCH net-next 9/9] bonding: remove bond_next_slave()
From: Veaceslav Falico @ 2013-09-27 14:12 UTC (permalink / raw)
To: netdev; +Cc: nikolay, bhutchings, Veaceslav Falico, Jay Vosburgh,
Andy Gospodarek
In-Reply-To: <1380291125-5671-1-git-send-email-vfalico@redhat.com>
There are no users left, so it's safe to remove.
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
drivers/net/bonding/bonding.h | 31 -------------------------------
1 file changed, 31 deletions(-)
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 5b71601..713b6af 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -89,9 +89,6 @@
#define bond_is_first_slave(bond, pos) (pos == bond_first_slave(bond))
#define bond_is_last_slave(bond, pos) (pos == bond_last_slave(bond))
-/* Since bond_first/last_slave can return NULL, these can return NULL too */
-#define bond_next_slave(bond, pos) __bond_next_slave(bond, pos)
-
/**
* bond_for_each_slave - iterate over all slaves
* @bond: the bond holding this list
@@ -244,34 +241,6 @@ struct bonding {
((struct slave *) rtnl_dereference(dev->rx_handler_data))
/**
- * __bond_next_slave - get the next slave after the one provided
- * @bond - bonding struct
- * @slave - the slave provided
- *
- * Returns the next slave after the slave provided, first slave if the
- * slave provided is the last slave and NULL if slave is not found
- */
-static inline struct slave *__bond_next_slave(struct bonding *bond,
- struct slave *slave)
-{
- struct slave *slave_iter;
- struct list_head *iter;
- bool found = false;
-
- netdev_for_each_lower_private(bond->dev, slave_iter, iter) {
- if (found)
- return slave_iter;
- if (slave_iter == slave)
- found = true;
- }
-
- if (found)
- return bond_first_slave(bond);
-
- return NULL;
-}
-
-/**
* Returns NULL if the net_device does not belong to any of the bond's slaves
*
* Caller must hold bond lock for read
--
1.8.4
^ permalink raw reply related
* Re: [PATCH net-next] virtio-net: switch to use XPS to choose txq
From: Michael S. Tsirkin @ 2013-09-27 14:35 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1380261444-40918-1-git-send-email-jasowang@redhat.com>
On Fri, Sep 27, 2013 at 01:57:24PM +0800, Jason Wang wrote:
> We used to use a percpu structure vq_index to record the cpu to queue
> mapping, this is suboptimal since it duplicates the work of XPS and
> loses all other XPS functionality such as allowing use to configure
> their own transmission steering strategy.
>
> So this patch switches to use XPS and suggest a default mapping when
> the number of cpus is equal to the number of queues. With XPS support,
> there's no need for keeping per-cpu vq_index and .ndo_select_queue(),
> so they were removed also.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
More lines deleted that added is good :)
But how does the result perform?
About the same?
> ---
> drivers/net/virtio_net.c | 55 +++++++--------------------------------------
> 1 files changed, 9 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index defec2b..4102c1b 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -127,9 +127,6 @@ struct virtnet_info {
> /* Does the affinity hint is set for virtqueues? */
> bool affinity_hint_set;
>
> - /* Per-cpu variable to show the mapping from CPU to virtqueue */
> - int __percpu *vq_index;
> -
> /* CPU hot plug notifier */
> struct notifier_block nb;
> };
> @@ -1063,7 +1060,6 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev,
> static void virtnet_clean_affinity(struct virtnet_info *vi, long hcpu)
> {
> int i;
> - int cpu;
>
> if (vi->affinity_hint_set) {
> for (i = 0; i < vi->max_queue_pairs; i++) {
> @@ -1073,20 +1069,11 @@ static void virtnet_clean_affinity(struct virtnet_info *vi, long hcpu)
>
> vi->affinity_hint_set = false;
> }
> -
> - i = 0;
> - for_each_online_cpu(cpu) {
> - if (cpu == hcpu) {
> - *per_cpu_ptr(vi->vq_index, cpu) = -1;
> - } else {
> - *per_cpu_ptr(vi->vq_index, cpu) =
> - ++i % vi->curr_queue_pairs;
> - }
> - }
> }
>
> static void virtnet_set_affinity(struct virtnet_info *vi)
> {
> + cpumask_var_t cpumask;
> int i;
> int cpu;
>
> @@ -1100,15 +1087,21 @@ static void virtnet_set_affinity(struct virtnet_info *vi)
> return;
> }
>
> + if (!alloc_cpumask_var(&cpumask, GFP_KERNEL))
> + return;
> +
> i = 0;
> for_each_online_cpu(cpu) {
> virtqueue_set_affinity(vi->rq[i].vq, cpu);
> virtqueue_set_affinity(vi->sq[i].vq, cpu);
> - *per_cpu_ptr(vi->vq_index, cpu) = i;
> + cpumask_clear(cpumask);
> + cpumask_set_cpu(cpu, cpumask);
> + netif_set_xps_queue(vi->dev, cpumask, i);
> i++;
> }
>
> vi->affinity_hint_set = true;
> + free_cpumask_var(cpumask);
> }
>
> static int virtnet_cpu_callback(struct notifier_block *nfb,
> @@ -1217,28 +1210,6 @@ static int virtnet_change_mtu(struct net_device *dev, int new_mtu)
> return 0;
> }
>
> -/* To avoid contending a lock hold by a vcpu who would exit to host, select the
> - * txq based on the processor id.
> - */
> -static u16 virtnet_select_queue(struct net_device *dev, struct sk_buff *skb)
> -{
> - int txq;
> - struct virtnet_info *vi = netdev_priv(dev);
> -
> - if (skb_rx_queue_recorded(skb)) {
> - txq = skb_get_rx_queue(skb);
> - } else {
> - txq = *__this_cpu_ptr(vi->vq_index);
> - if (txq == -1)
> - txq = 0;
> - }
> -
> - while (unlikely(txq >= dev->real_num_tx_queues))
> - txq -= dev->real_num_tx_queues;
> -
> - return txq;
> -}
> -
> static const struct net_device_ops virtnet_netdev = {
> .ndo_open = virtnet_open,
> .ndo_stop = virtnet_close,
> @@ -1250,7 +1221,6 @@ static const struct net_device_ops virtnet_netdev = {
> .ndo_get_stats64 = virtnet_stats,
> .ndo_vlan_rx_add_vid = virtnet_vlan_rx_add_vid,
> .ndo_vlan_rx_kill_vid = virtnet_vlan_rx_kill_vid,
> - .ndo_select_queue = virtnet_select_queue,
> #ifdef CONFIG_NET_POLL_CONTROLLER
> .ndo_poll_controller = virtnet_netpoll,
> #endif
> @@ -1559,10 +1529,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> if (vi->stats == NULL)
> goto free;
>
> - vi->vq_index = alloc_percpu(int);
> - if (vi->vq_index == NULL)
> - goto free_stats;
> -
> mutex_init(&vi->config_lock);
> vi->config_enable = true;
> INIT_WORK(&vi->config_work, virtnet_config_changed_work);
> @@ -1589,7 +1555,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
> err = init_vqs(vi);
> if (err)
> - goto free_index;
> + goto free_stats;
>
> netif_set_real_num_tx_queues(dev, 1);
> netif_set_real_num_rx_queues(dev, 1);
> @@ -1640,8 +1606,6 @@ free_recv_bufs:
> free_vqs:
> cancel_delayed_work_sync(&vi->refill);
> virtnet_del_vqs(vi);
> -free_index:
> - free_percpu(vi->vq_index);
> free_stats:
> free_percpu(vi->stats);
> free:
> @@ -1678,7 +1642,6 @@ static void virtnet_remove(struct virtio_device *vdev)
>
> flush_work(&vi->config_work);
>
> - free_percpu(vi->vq_index);
> free_percpu(vi->stats);
> free_netdev(vi->dev);
> }
> --
> 1.7.1
^ permalink raw reply
* RE: [PATCH net-next 2/9] bonding: remove __get_first_port()
From: David Laight @ 2013-09-27 14:50 UTC (permalink / raw)
To: Veaceslav Falico, netdev
Cc: nikolay, bhutchings, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1380291125-5671-3-git-send-email-vfalico@redhat.com>
> @@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>
> // check if agg_select_timer timer after initialize is timed out
> if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
> + slave = bond_first_slave(bond);
> + port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
> +
> // select the active aggregator for the bond
> - if ((port = __get_first_port(bond))) {
> + if (port) {
> if (!port->slave) {
> pr_warning("%s: Warning: bond's first port is uninitialized\n",
> bond->dev->name);
> --
Looks like that could be:
slave = bond_first_slave(bond);
if (slave) {
port = SLAVE_AD_INFO(slave).port;
and I assume 'slave == port->slave' so there is no need for the latter check?
David
^ permalink raw reply
* Re: [PATCH net-next 2/9] bonding: remove __get_first_port()
From: Veaceslav Falico @ 2013-09-27 14:58 UTC (permalink / raw)
To: David Laight; +Cc: netdev, nikolay, bhutchings, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7365@saturn3.aculab.com>
On Fri, Sep 27, 2013 at 03:50:12PM +0100, David Laight wrote:
>> @@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>>
>> // check if agg_select_timer timer after initialize is timed out
>> if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
>> + slave = bond_first_slave(bond);
>> + port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
>> +
>> // select the active aggregator for the bond
>> - if ((port = __get_first_port(bond))) {
>> + if (port) {
>> if (!port->slave) {
>> pr_warning("%s: Warning: bond's first port is uninitialized\n",
>> bond->dev->name);
>> --
>
>Looks like that could be:
> slave = bond_first_slave(bond);
> if (slave) {
> port = SLAVE_AD_INFO(slave).port;
>and I assume 'slave == port->slave' so there is no need for the latter check?
I've also fallen to this trap at first - slave->port can (virtually) be
NULL, and this way we'll panic on "if (!port->slave)".
>
> David
>
>
>
^ permalink raw reply
* Re: [PATCH net-next 2/9] bonding: remove __get_first_port()
From: Veaceslav Falico @ 2013-09-27 15:05 UTC (permalink / raw)
To: David Laight; +Cc: netdev, nikolay, bhutchings, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <20130927145825.GA14139@redhat.com>
On Fri, Sep 27, 2013 at 04:58:25PM +0200, Veaceslav Falico wrote:
>On Fri, Sep 27, 2013 at 03:50:12PM +0100, David Laight wrote:
>>>@@ -2104,8 +2091,11 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
>>>
>>> // check if agg_select_timer timer after initialize is timed out
>>> if (BOND_AD_INFO(bond).agg_select_timer && !(--BOND_AD_INFO(bond).agg_select_timer)) {
>>>+ slave = bond_first_slave(bond);
>>>+ port = slave ? &(SLAVE_AD_INFO(slave).port) : NULL;
>>>+
>>> // select the active aggregator for the bond
>>>- if ((port = __get_first_port(bond))) {
>>>+ if (port) {
>>> if (!port->slave) {
>>> pr_warning("%s: Warning: bond's first port is uninitialized\n",
>>> bond->dev->name);
>>>--
>>
>>Looks like that could be:
>> slave = bond_first_slave(bond);
>> if (slave) {
>> port = SLAVE_AD_INFO(slave).port;
>>and I assume 'slave == port->slave' so there is no need for the latter check?
>
>I've also fallen to this trap at first - slave->port can (virtually) be
>NULL, and this way we'll panic on "if (!port->slave)".
Err, forgot to address the 'slave == port->slave' - yes, virtually it
should be - if it's initialized (and, it should be) - however, as I've
stated in the cover letter - there are *tons* of cleanups/optimizations of
these kind that might be done here - and not to mix cleanups/optimizations
with the thing that these patches should do (remove bond_next_slave) - I've
decided to leave it as close to the original as possible.
>
>>
>> David
>>
>>
>>
^ permalink raw reply
* Re: [PATCH] tcp: TSQ can use a dynamic limit
From: Neal Cardwell @ 2013-09-27 15:08 UTC (permalink / raw)
To: Eric Dumazet
Cc: Cong Wang, David Miller, Wei Liu, Linux Kernel Network Developers,
Yuchung Cheng
In-Reply-To: <1380277734.30872.25.camel@edumazet-glaptop.roam.corp.google.com>
On Fri, Sep 27, 2013 at 6:28 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> When TCP Small Queues was added, we used a sysctl to limit amount of
> packets queues on Qdisc/device queues for a given TCP flow.
>
> Problem is this limit is either too big for low rates, or too small
> for high rates.
>
> Now TCP stack has rate estimation in sk->sk_pacing_rate, and TSO
> auto sizing, it can better control number of packets in Qdisc/device
> queues.
>
> New limit is two packets or at least 1 to 2 ms worth of packets.
>
> Low rates flows benefit from this patch by having even smaller
> number of packets in queues, allowing for faster recovery,
> better RTT estimations.
>
> High rates flows benefit from this patch by allowing more than 2 packets
> in flight as we had reports this was a limiting factor to reach line
> rate. [ In particular if TX completion is delayed because of coalescing
> parameters ]
>
> Example for a single flow on 10Gbp link controlled by FQ/pacing
>
> 14 packets in flight instead of 2
>
> $ tc -s -d qd
> qdisc fq 8001: dev eth0 root refcnt 32 limit 10000p flow_limit 100p
> buckets 1024 quantum 3028 initial_quantum 15140
> Sent 1168459366606 bytes 771822841 pkt (dropped 0, overlimits 0
> requeues 6822476)
> rate 9346Mbit 771713pps backlog 953820b 14p requeues 6822476
> 2047 flow, 2046 inactive, 1 throttled, delay 15673 ns
> 2372 gc, 0 highprio, 0 retrans, 9739249 throttled, 0 flows_plimit
>
> Note that sk_pacing_rate is currently set to twice the actual rate, but
> this might be refined in the future when a flow is in congestion
> avoidance.
>
> Additional change : skb->destructor should be set to tcp_wfree().
>
> A future patch (for linux 3.13+) might remove tcp_limit_output_bytes
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Cong Wang <xiyou.wangcong@gmail.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Neal Cardwell <ncardwell@google.com>
> ---
Acked-by: Neal Cardwell <ncardwell@google.com>
neal
^ permalink raw reply
* Re: question about map_read_chunks()
From: Tom Tucker @ 2013-09-27 15:23 UTC (permalink / raw)
To: Dan Carpenter
Cc: Tom Tucker, J. Bruce Fields, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-nfs-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20130927122108.GF6247@mwanda>
Hi Dan,
On 9/27/13 7:21 AM, Dan Carpenter wrote:
> I have looked at this again, and I still worry that it looks like a bug.
> (remote security related blah blah blah).
>
> regards,
> dan carpenter
>
> On Mon, Feb 20, 2012 at 12:50:19PM +0300, Dan Carpenter wrote:
>> I had a couple questions about some map_read_chunks().
>>
>> net/sunrpc/xprtrdma/svc_rdma_recvfrom.c
>>
>> 150 ch_bytes = ntohl(ch->rc_target.rs_length);
>> ^^^^^^^^
>> It look like this is 32 bits from the network?
>>
>> 151 head->arg.head[0] = rqstp->rq_arg.head[0];
>> 152 head->arg.tail[0] = rqstp->rq_arg.tail[0];
>> 153 head->arg.pages = &head->pages[head->count];
>> 154 head->hdr_count = head->count; /* save count of hdr pages */
>> 155 head->arg.page_base = 0;
>> 156 head->arg.page_len = ch_bytes;
>> 157 head->arg.len = rqstp->rq_arg.len + ch_bytes;
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> Can overflow.
>> 158 head->arg.buflen = rqstp->rq_arg.buflen + ch_bytes;
agreed.
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> Same. I didn't follow it through to see if an overflow matters. Does
>> it?
>>
>> 159 head->count++;
>> 160 chl_map->ch[0].start = 0;
>> 161 while (byte_count) {
>> 162 rpl_map->sge[sge_no].iov_base =
>> 163 page_address(rqstp->rq_arg.pages[page_no]) + page_off;
>> 164 sge_bytes = min_t(int, PAGE_SIZE-page_off, ch_bytes);
>> ^^^
>> This is the wrong cast to use. A large ch_bytes would be counted as a
>> negative value and get around the cap here.
True, but if we validate the wire data like we should, that's probably
not an issue.
>> 165 rpl_map->sge[sge_no].iov_len = sge_bytes;
>>
>> regards,
>> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3] powerpc/83xx: gianfar_ptp: select 1588 clock source through dts file
From: Kumar Gala @ 2013-09-27 15:28 UTC (permalink / raw)
To: Aida Mynzhasova
Cc: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ,
devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
richardcochran-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <1380289227-12029-1-git-send-email-aida.mynzhasova-Fmhy8gsqeTEvJsYlp49lxw@public.gmane.org>
On Sep 27, 2013, at 8:40 AM, Aida Mynzhasova wrote:
> Currently IEEE 1588 timer reference clock source is determined through
> hard-coded value in gianfar_ptp driver. This patch allows to select ptp
> clock source by means of device tree file node.
>
> For instance:
>
> fsl,cksel = <0>;
>
> for using external (TSEC_TMR_CLK input) high precision timer
> reference clock.
>
> Other acceptable values:
>
> <1> : eTSEC system clock
> <2> : eTSEC1 transmit clock
> <3> : RTC clock input
>
> When this attribute isn't used, eTSEC system clock will serve as
> IEEE 1588 timer reference clock.
>
> Signed-off-by: Aida Mynzhasova <aida.mynzhasova-Fmhy8gsqeTEvJsYlp49lxw@public.gmane.org>
> ---
> Documentation/devicetree/bindings/net/fsl-tsec-phy.txt | 18 +++++++++++++++++-
> drivers/net/ethernet/freescale/gianfar_ptp.c | 4 +++-
> 2 files changed, 20 insertions(+), 2 deletions(-)
Acked-by: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
- k--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] bonding: Fix broken promiscuity reference counting issue
From: Neil Horman @ 2013-09-27 16:22 UTC (permalink / raw)
To: netdev; +Cc: Neil Horman, Jay Vosburgh, Andy Gospodarek, Mark Wu,
David S. Miller
Recently grabbed this report:
https://bugzilla.redhat.com/show_bug.cgi?id=1005567
Of an issue in which the bonding driver, with an attached vlan encountered the
following errors when bond0 was taken down and back up:
dummy1: promiscuity touches roof, set promiscuity failed. promiscuity feature of
device might be broken.
The error occurs because, during __bond_release_one, if we release our last
slave, we take on a random mac address and issue a NETDEV_CHANGEADDR
notification. With an attached vlan, the vlan may see that the vlan and bond
mac address were in sync, but no longer are. This triggers a call to dev_uc_add
and dev_set_rx_mode, which enables IFF_PROMISC on the bond device. Then, when
we complete __bond_release_one, we use the current state of the bond flags to
determine if we should decrement the promiscuity of the releasing slave. But
since the bond changed promiscuity state during the release operation, we
incorrectly decrement the slave promisc count when it wasn't in promiscuous mode
to begin with, causing the above error
Fix is pretty simple, just cache the bonding flags at the start of the function
and use those when determining the need to set promiscuity.
This is also needed for the ALLMULTI flag
CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: Mark Wu <wudxw@linux.vnet.ibm.com>
CC: "David S. Miller" <davem@davemloft.net>
Reported-by: Mark Wu <wudxw@linux.vnet.ibm.com>
---
drivers/net/bonding/bond_main.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d5c3153..5373a8d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1710,6 +1710,7 @@ static int __bond_release_one(struct net_device *bond_dev,
struct bonding *bond = netdev_priv(bond_dev);
struct slave *slave, *oldcurrent;
struct sockaddr addr;
+ int old_flags = bond_dev->flags;
netdev_features_t old_features = bond_dev->features;
/* slave is not a slave or master is not master of this slave */
@@ -1841,12 +1842,18 @@ static int __bond_release_one(struct net_device *bond_dev,
* bond_change_active_slave(..., NULL)
*/
if (!USES_PRIMARY(bond->params.mode)) {
- /* unset promiscuity level from slave */
- if (bond_dev->flags & IFF_PROMISC)
+ /* unset promiscuity level from slave
+ * NOTE: The NETDEV_CHANGEADDR call above may change the value
+ * of the IFF_PROMISC flag in the bond_dev, but we need the
+ * value of that flag before that change, as that was the value
+ * when this slave was attached, so we cache at the start of the
+ * function and use it here. Same goes for ALLMULTI below
+ */
+ if (old_flags & IFF_PROMISC)
dev_set_promiscuity(slave_dev, -1);
/* unset allmulti level from slave */
- if (bond_dev->flags & IFF_ALLMULTI)
+ if (old_flags & IFF_ALLMULTI)
dev_set_allmulti(slave_dev, -1);
bond_hw_addr_flush(bond_dev, slave_dev);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] IPv6: Allow the MTU of ipip6 tunnel to be set below 1280
From: Oussama Ghorbel @ 2013-09-27 16:36 UTC (permalink / raw)
To: Oussama Ghorbel, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel
In-Reply-To: <20130927105856.GF28287@order.stressinduktion.org>
Please see my comments below
Regards,
Oussama
On Fri, Sep 27, 2013 at 11:58 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Fri, Sep 27, 2013 at 11:45:48AM +0100, Oussama Ghorbel wrote:
>> The ip6_tunnel.c module would be then dependent on ip_tunnel.c and may
>> be it would not be good thing?
>
> It could just be a static inline in some shared header. So there would
> be no compile-time dependency.
>
The higher limit of mtu in ip_tunnel_change_mtu() is a calculated value.
This high limit is calculated using the netdev priv struct ip_tunnel
(field hlen).
This netdev priv struct is different in ipv6, it's a ip6_tnl struct.
Therefore implementing a beautiful function like
ip_tunnel_valid_mtu(struct net_device *dev, int mtu) won't be
feasible, unless we implement it in macro or something like like
ip_tunnel_valid_mtu(struct net_device *dev, int hlen, int mtu) which
seems not very nice ...
What do yo think?
>> As I have check in v3.10 there is no call from ip6_tunnel to ip_tunnel...
>>
>> For information, there is no check for the maximum MTU for ipv4 in the
>> patch as this is not done for ipv6.
>
> I understand, but it would be better to limit the MTU here. There is a
> (non-jumo) IPV6_MAXPLEN constant.
>
> Looking through the source it seems grev6 does actually check this,
> so it would not hurt adding them here, too.
what if jumbograms is used, in that case, we can't use IPV6_MAXPLEN.
the limit would be the the full unsigned int.
However checking the higher limit for ipv4 would be useful.
Please also note in case the tunnel mode is any (ipv4 or ipv6 means
ip6_tnl.parms.proto = 0), then we will be required to take the most
restrict limit from both ipv4 and ipv6 which means the lower limit
will be 1280 and the higher limit will be about 65535
Do you agree on this?
>
> Otherwise, I think your patch is fine.
>
> Greetings,
>
> Hannes
>
^ 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