Re: [PATCH] bonding: add the sysfs interface to see RLB hash table

[Jay Vosburgh <fubar@us.ibm.com>]

Eric Dumazet <eric.dumazet@gmail.com> wrote:

>Le mardi 30 novembre 2010 à 19:01 +0900, Taku Izumi a écrit :
>> This patch provides the sysfs interface to see RLB hash table
>> like the following:
>> 
>> # cat /sys/class/net/bond0/bonding/rlb_hash_table
>> 
>> SourceIP        DestinationIP   Destination MAC   DEV
>>  10.124.196.205  10.124.196. 81 00:19:99:XX:XX:XX eth3
>>  10.124.196.205  10.124.196.222 00:0a:79:XX:XX:XX eth0
>>  10.124.196.205  10.124.196. 75 00:15:17:XX:XX:XX eth4
>>  10.124.196.205  10.124.196.  1 00:21:d8:XX:XX:XX eth3
>>  10.124.196.205  10.124.196.205 ff:ff:ff:ff:ff:ff eth0

	I'm reasonably sure something like this isn't going to be
acceptable in sysfs (it's much too large).

	In the proc file that bonding already uses, this type of
information isn't unreasonable, but I don't think that is the best place
for this, for two reasons.

	First, the table may have up to 256 entries.  Therefore, a
sufficiently populated table will easily overrun the one page of space
available to a sysfs show function or a proc seq_printf (per iteration),
so it will have to handle that.  The current code in bonding to do its
proc file already iterates over the slaves; adding another iteration
loop to handle this table seems overly complicated.  A well populated
table would also make the current proc file's output rather verbose,
particularly if the TLB table is added later.

	Second, it would have to hold the hash table spin lock, which
may provide an easy way to mess with bonding (user space doing "while 1
cat rlb_hash_table > /dev/null").

	Therefore, I'd suggest this go into debugfs somewhere, perhaps a
/sys/kernel/debug/bonding/rlb_hash_table (perhaps with a tlb_hash_table
as the logical pairing for the TX side), readable only by root.

	Alternatively, if there are objections to using debufs, a new
file in /proc/net/bonding could be used, although that seems cumbersome
(because it would have to be named to avoid conflicts, e.g.,
/proc/net/bonding/bond0_rlb_hash_table).

>why spaces in IP addresses ?
>
>> 
>> This is helpful to check if the receive load balancing works as expected.
>> 
>> Signed-off-by: Taku Izumi <izumi.taku@jp.fujitsu.com>
>> 
>> ---
>>  drivers/net/bonding/bond_sysfs.c |   56 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>> 
>> Index: net-next/drivers/net/bonding/bond_sysfs.c
>> ===================================================================
>> --- net-next.orig/drivers/net/bonding/bond_sysfs.c
>> +++ net-next/drivers/net/bonding/bond_sysfs.c
>> @@ -43,6 +43,7 @@
>>  #include <linux/nsproxy.h>
>> 
>>  #include "bonding.h"
>> +#include "bond_alb.h"
>> 
>>  #define to_dev(obj)	container_of(obj, struct device, kobj)
>>  #define to_bond(cd)	((struct bonding *)(netdev_priv(to_net_dev(cd))))
>> @@ -1643,6 +1644,60 @@ out:
>>  static DEVICE_ATTR(resend_igmp, S_IRUGO | S_IWUSR,
>>  		   bonding_show_resend_igmp, bonding_store_resend_igmp);
>> 
>> +/*
>> + *  Show RLB hash table
>> + */
>> +#define RLB_NULL_INDEX  0xffffffff

	
>> +static ssize_t bonding_show_rlb_hashtable(struct device *d,
>> +					  struct device_attribute *attr,
>> +					  char *buf)
>> +{
>> +	int count = 0;
>> +	struct bonding *bond = to_bond(d);
>> +	struct alb_bond_info *bond_info = &(BOND_ALB_INFO(bond));
>> +	struct rlb_client_info *client_info;
>> +	u32 hash_index;
>> +
>> +	if (bond->params.mode != BOND_MODE_ALB)
>> +		return count;
>> +
>> +	count += sprintf(buf + count, "SourceIP        "
>> +		"DestinationIP   Destination MAC   DEV\n");
>> +
>> +	spin_lock_bh(&(BOND_ALB_INFO(bond).rx_hashtbl_lock));
>> +
>> +	hash_index = bond_info->rx_hashtbl_head;
>> +	for (; hash_index != RLB_NULL_INDEX; hash_index = client_info->next) {
>> +		client_info = &(bond_info->rx_hashtbl[hash_index]);
>> +
>> +		count += sprintf(buf + count,
>> +			"%3d.%3d.%3d.%3d %3d.%3d.%3d.%3d "
>> +			"%02x:%02x:%02x:%02x:%02x:%02x %s\n",
>
>
>Oh well, I guess you dont read Joe patches on netdev ;)
>
>Please take a look at %pI4 and %pM

	Agreed.

	-J

>sprintf(buf + count, "%pI4 %pI4 %pM %s\n", ...)
>
>
>> +			client_info->ip_src & 0xff,
>> +			(client_info->ip_src >> 8) & 0xff,
>> +			(client_info->ip_src >> 16) & 0xff,
>> +			(client_info->ip_src >> 24) & 0xff,
>> +			client_info->ip_dst & 0xff,
>> +			(client_info->ip_dst >> 8) & 0xff,
>> +			(client_info->ip_dst >> 16) & 0xff,
>> +			(client_info->ip_dst >> 24) & 0xff,
>> +			client_info->mac_dst[0],
>> +			client_info->mac_dst[1],
>> +			client_info->mac_dst[2],
>> +			client_info->mac_dst[3],
>> +			client_info->mac_dst[4],
>> +			client_info->mac_dst[5],
>> +			client_info->slave->dev->name);
>> +	}
>> +
>

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