[PATCH net-next] rtnetlink: fix spelling errors

[PATCH net-next] rtnetlink: fix spelling errors

[Stephen Hemminger]

Minor spelling mistakes.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/include/net/rtnetlink.h	2012-02-27 08:43:02.400935781 -0800
+++ b/include/net/rtnetlink.h	2012-04-09 13:24:02.705034830 -0700
@@ -41,7 +41,7 @@ static inline int rtnl_msg_family(const
  *	@get_size: Function to calculate required room for dumping device
  *		   specific netlink attributes
  *	@fill_info: Function to dump device specific netlink attributes
- *	@get_xstats_size: Function to calculate required room for dumping devic
+ *	@get_xstats_size: Function to calculate required room for dumping device
  *			  specific statistics
  *	@fill_xstats: Function to dump device specific statistics
  */
@@ -94,7 +94,7 @@ extern void	rtnl_link_unregister(struct
  * 	@fill_link_af: Function to fill IFLA_AF_SPEC with address family
  * 		       specific netlink attributes.
  * 	@get_link_af_size: Function to calculate size of address family specific
- * 			   netlink attributes exlusive the container attribute.
+ * 			   netlink attributes exclusive the container attribute.
  *	@validate_link_af: Validate a IFLA_AF_SPEC attribute, must check attr
  *			   for invalid configuration settings.
  * 	@set_link_af: Function to parse a IFLA_AF_SPEC attribute and modify

Re: [PATCH net-next] rtnetlink: fix spelling errors

[Ben Hutchings]

On Mon, 2012-04-09 at 13:27 -0700, Stephen Hemminger wrote:
> Minor spelling mistakes.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> --- a/include/net/rtnetlink.h	2012-02-27 08:43:02.400935781 -0800
> +++ b/include/net/rtnetlink.h	2012-04-09 13:24:02.705034830 -0700
> @@ -41,7 +41,7 @@ static inline int rtnl_msg_family(const
>   *	@get_size: Function to calculate required room for dumping device
>   *		   specific netlink attributes
>   *	@fill_info: Function to dump device specific netlink attributes
> - *	@get_xstats_size: Function to calculate required room for dumping devic
> + *	@get_xstats_size: Function to calculate required room for dumping device
>   *			  specific statistics
>   *	@fill_xstats: Function to dump device specific statistics
>   */
> @@ -94,7 +94,7 @@ extern void	rtnl_link_unregister(struct
>   * 	@fill_link_af: Function to fill IFLA_AF_SPEC with address family
>   * 		       specific netlink attributes.
>   * 	@get_link_af_size: Function to calculate size of address family specific
> - * 			   netlink attributes exlusive the container attribute.
> + * 			   netlink attributes exclusive the container attribute.
[...]

That's now a real word but it's not grammatically correct; 'exclusive
the' should be changed to 'exclusive of the' or 'excluding the'.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH] rtnetlink: fix comments

[Stephen Hemminger]

Fix spelling and references in rtnetlink.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
v2 - fix bogus comment on get_link_af_size and add get_tx_queues

--- a/include/net/rtnetlink.h	2012-02-27 08:43:02.400935781 -0800
+++ b/include/net/rtnetlink.h	2012-04-10 21:19:01.369508395 -0700
@@ -41,9 +41,11 @@ static inline int rtnl_msg_family(const
  *	@get_size: Function to calculate required room for dumping device
  *		   specific netlink attributes
  *	@fill_info: Function to dump device specific netlink attributes
- *	@get_xstats_size: Function to calculate required room for dumping devic
+ *	@get_xstats_size: Function to calculate required room for dumping device
  *			  specific statistics
  *	@fill_xstats: Function to dump device specific statistics
+ *	@get_tx_queues: Function to determine number of transmit queues to create when
+ *		        creating a new device.
  */
 struct rtnl_link_ops {
 	struct list_head	list;
@@ -94,7 +96,7 @@ extern void	rtnl_link_unregister(struct
  * 	@fill_link_af: Function to fill IFLA_AF_SPEC with address family
  * 		       specific netlink attributes.
  * 	@get_link_af_size: Function to calculate size of address family specific
- * 			   netlink attributes exlusive the container attribute.
+ * 			   netlink attributes.
  *	@validate_link_af: Validate a IFLA_AF_SPEC attribute, must check attr
  *			   for invalid configuration settings.
  * 	@set_link_af: Function to parse a IFLA_AF_SPEC attribute and modify

[PATCH net-next] rtnetlink & bonding: change args got get_tx_queues

[Stephen Hemminger]

Change get_tx_queues, drop unsused arg/return value real_tx_queues,
and use return by value (with error) rather than call by reference.

Probably bonding should just change to LLTX and the whole get_tx_queues
API could disappear!

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
 drivers/net/bonding/bond_main.c |    7 ++-----
 include/net/rtnetlink.h         |    5 ++---
 net/core/rtnetlink.c            |    8 ++++----
 3 files changed, 8 insertions(+), 12 deletions(-)

--- a/drivers/net/bonding/bond_main.c	2012-04-09 11:18:09.109148332 -0700
+++ b/drivers/net/bonding/bond_main.c	2012-04-10 21:23:07.380267810 -0700
@@ -4779,12 +4779,9 @@ static int bond_validate(struct nlattr *
 	return 0;
 }
 
-static int bond_get_tx_queues(struct net *net, struct nlattr *tb[],
-			      unsigned int *num_queues,
-			      unsigned int *real_num_queues)
+static int bond_get_tx_queues(struct net *net, const struct nlattr *tb[])
 {
-	*num_queues = tx_queues;
-	return 0;
+	return tx_queues;
 }
 
 static struct rtnl_link_ops bond_link_ops __read_mostly = {
--- a/include/net/rtnetlink.h	2012-04-10 21:19:01.369508395 -0700
+++ b/include/net/rtnetlink.h	2012-04-10 21:24:57.897506149 -0700
@@ -77,9 +77,8 @@ struct rtnl_link_ops {
 	size_t			(*get_xstats_size)(const struct net_device *dev);
 	int			(*fill_xstats)(struct sk_buff *skb,
 					       const struct net_device *dev);
-	int			(*get_tx_queues)(struct net *net, struct nlattr *tb[],
-						 unsigned int *tx_queues,
-						 unsigned int *real_tx_queues);
+	int			(*get_tx_queues)(struct net *net,
+						 const struct nlattr *tb[]);
 };
 
 extern int	__rtnl_link_register(struct rtnl_link_ops *ops);
--- a/net/core/rtnetlink.c	2012-04-09 11:18:09.673154299 -0700
+++ b/net/core/rtnetlink.c	2012-04-10 21:20:42.002637653 -0700
@@ -1641,14 +1641,14 @@ struct net_device *rtnl_create_link(stru
 	int err;
 	struct net_device *dev;
 	unsigned int num_queues = 1;
-	unsigned int real_num_queues = 1;
 
 	if (ops->get_tx_queues) {
-		err = ops->get_tx_queues(src_net, tb, &num_queues,
-					 &real_num_queues);
-		if (err)
+		err = ops->get_tx_queues(src_net, tb);
+		if (err < 0)
 			goto err;
+		num_queues = err;
 	}
+
 	err = -ENOMEM;
 	dev = alloc_netdev_mq(ops->priv_size, ifname, ops->setup, num_queues);
 	if (!dev)

Re: [PATCH net-next] rtnetlink & bonding: change args got get_tx_queues

[Eric Dumazet]

On Tue, 2012-04-10 at 21:34 -0700, Stephen Hemminger wrote:
> Change get_tx_queues, drop unsused arg/return value real_tx_queues,
> and use return by value (with error) rather than call by reference.
> 
> Probably bonding should just change to LLTX and the whole get_tx_queues
> API could disappear!

Absolutely ;)

Re: [PATCH net-next] rtnetlink & bonding: change args got get_tx_queues

[Stephen Hemminger]

On Wed, 11 Apr 2012 07:55:47 +0200
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Tue, 2012-04-10 at 21:34 -0700, Stephen Hemminger wrote:
> > Change get_tx_queues, drop unsused arg/return value real_tx_queues,
> > and use return by value (with error) rather than call by reference.
> > 
> > Probably bonding should just change to LLTX and the whole get_tx_queues
> > API could disappear!
> 
> Absolutely ;)
> 
> 

It is more complex than that (actually the bonding driver is a mess).
The bonding device is already using Lockless Transmit and transmit queue length
of zero (good), but it then does some queue mapping of it's own which
is unnecessary.

Multiqueue only makes sense if there is a queue, otherwise the skb
can transparently pass through the layered device (vlan, bridge, bond)
and get queued on the real physical device.

Right now, trying to see if there is any impact by just leaving
bond device as single queue.

Re: [PATCH net-next] rtnetlink & bonding: change args got get_tx_queues

[Jay Vosburgh]

Stephen Hemminger <shemminger@vyatta.com> wrote:

>On Wed, 11 Apr 2012 07:55:47 +0200
>Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> On Tue, 2012-04-10 at 21:34 -0700, Stephen Hemminger wrote:
>> > Change get_tx_queues, drop unsused arg/return value real_tx_queues,
>> > and use return by value (with error) rather than call by reference.
>> > 
>> > Probably bonding should just change to LLTX and the whole get_tx_queues
>> > API could disappear!
>> 
>> Absolutely ;)
>> 
>> 
>
>It is more complex than that (actually the bonding driver is a mess).
>The bonding device is already using Lockless Transmit and transmit queue length
>of zero (good), but it then does some queue mapping of it's own which
>is unnecessary.
>
>Multiqueue only makes sense if there is a queue, otherwise the skb
>can transparently pass through the layered device (vlan, bridge, bond)
>and get queued on the real physical device.
>
>Right now, trying to see if there is any impact by just leaving
>bond device as single queue.

	The multiqueue support in bonding is intended to permit
individual slaves to be assigned a particular queue id, which then
permits tc filter actions to steer traffic to particular slaves.

	The relevant part of Documentation/networking/bonding.txt:

The queue_id for a slave can be set using the command:

# echo "eth1:2" > /sys/class/net/bond0/bonding/queue_id

Any interface that needs a queue_id set should set it with multiple calls
like the one above until proper priorities are set for all interfaces.  On
distributions that allow configuration via initscripts, multiple 'queue_id'
arguments can be added to BONDING_OPTS to set all needed slave queues.

These queue id's can be used in conjunction with the tc utility to configure
a multiqueue qdisc and filters to bias certain traffic to transmit on certain
slave devices.  For instance, say we wanted, in the above configuration to
force all traffic bound to 192.168.1.100 to use eth1 in the bond as its output
device. The following commands would accomplish this:

# tc qdisc add dev bond0 handle 1 root multiq

# tc filter add dev bond0 protocol ip parent 1: prio 1 u32 match ip dst \
        192.168.1.100 action skbedit queue_mapping 2

These commands tell the kernel to attach a multiqueue queue discipline to the
bond0 interface and filter traffic enqueued to it, such that packets with a dst
ip of 192.168.1.100 have their output queue mapping value overwritten to 2.
This value is then passed into the driver, causing the normal output path
selection policy to be overridden, selecting instead qid 2, which maps to eth1.

Note that qid values begin at 1.  Qid 0 is reserved to initiate to the driver
that normal output policy selection should take place.  One benefit to simply
leaving the qid for a slave to 0 is the multiqueue awareness in the bonding
driver that is now present.  This awareness allows tc filters to be placed on
slave devices as well as bond devices and the bonding driver will simply act as
a pass-through for selecting output queues on the slave device rather than 
output port selection.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next] rtnetlink & bonding: change args got get_tx_queues

[Stephen Hemminger]

On Wed, 11 Apr 2012 11:13:35 -0700
Jay Vosburgh <fubar@us.ibm.com> wrote:

> Stephen Hemminger <shemminger@vyatta.com> wrote:
> 
> >On Wed, 11 Apr 2012 07:55:47 +0200
> >Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >> On Tue, 2012-04-10 at 21:34 -0700, Stephen Hemminger wrote:
> >> > Change get_tx_queues, drop unsused arg/return value real_tx_queues,
> >> > and use return by value (with error) rather than call by reference.
> >> > 
> >> > Probably bonding should just change to LLTX and the whole get_tx_queues
> >> > API could disappear!
> >> 
> >> Absolutely ;)
> >> 
> >> 
> >
> >It is more complex than that (actually the bonding driver is a mess).
> >The bonding device is already using Lockless Transmit and transmit queue length
> >of zero (good), but it then does some queue mapping of it's own which
> >is unnecessary.
> >
> >Multiqueue only makes sense if there is a queue, otherwise the skb
> >can transparently pass through the layered device (vlan, bridge, bond)
> >and get queued on the real physical device.
> >
> >Right now, trying to see if there is any impact by just leaving
> >bond device as single queue.
> 
> 	The multiqueue support in bonding is intended to permit
> individual slaves to be assigned a particular queue id, which then
> permits tc filter actions to steer traffic to particular slaves.
> 
> 	The relevant part of Documentation/networking/bonding.txt:
> 
> The queue_id for a slave can be set using the command:
> 
> # echo "eth1:2" > /sys/class/net/bond0/bonding/queue_id
> 
> Any interface that needs a queue_id set should set it with multiple calls
> like the one above until proper priorities are set for all interfaces.  On
> distributions that allow configuration via initscripts, multiple 'queue_id'
> arguments can be added to BONDING_OPTS to set all needed slave queues.
> 
> These queue id's can be used in conjunction with the tc utility to configure
> a multiqueue qdisc and filters to bias certain traffic to transmit on certain
> slave devices.  For instance, say we wanted, in the above configuration to
> force all traffic bound to 192.168.1.100 to use eth1 in the bond as its output
> device. The following commands would accomplish this:
> 
> # tc qdisc add dev bond0 handle 1 root multiq
> 
> # tc filter add dev bond0 protocol ip parent 1: prio 1 u32 match ip dst \
>         192.168.1.100 action skbedit queue_mapping 2
> 
> These commands tell the kernel to attach a multiqueue queue discipline to the
> bond0 interface and filter traffic enqueued to it, such that packets with a dst
> ip of 192.168.1.100 have their output queue mapping value overwritten to 2.
> This value is then passed into the driver, causing the normal output path
> selection policy to be overridden, selecting instead qid 2, which maps to eth1.
> 
> Note that qid values begin at 1.  Qid 0 is reserved to initiate to the driver
> that normal output policy selection should take place.  One benefit to simply
> leaving the qid for a slave to 0 is the multiqueue awareness in the bonding
> driver that is now present.  This awareness allows tc filters to be placed on
> slave devices as well as bond devices and the bonding driver will simply act as
> a pass-through for selecting output queues on the slave device rather than 
> output port selection.
> 

But that choice makes performance worse for the simple case of bonding
2 10G NIC's on a 64 core system. I think you are overloading the concept of
queue id to make a classification value. Wasn't marking intended for that?

Re: [PATCH net-next] rtnetlink & bonding: change args got get_tx_queues

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 10 Apr 2012 21:34:43 -0700

> Change get_tx_queues, drop unsused arg/return value real_tx_queues,
> and use return by value (with error) rather than call by reference.
> 
> Probably bonding should just change to LLTX and the whole get_tx_queues
> API could disappear!
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Applied, thanks Stephen.

Re: [PATCH] rtnetlink: fix comments

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Tue, 10 Apr 2012 21:32:59 -0700

> Fix spelling and references in rtnetlink.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> ---
> v2 - fix bogus comment on get_link_af_size and add get_tx_queues

Applied to net-next, thanks.