Netdev List
 help / color / mirror / Atom feed
* Re: SFQ on HFSC leaf does not seem to work
From: Eric Dumazet @ 2011-12-23 16:16 UTC (permalink / raw)
  To: John A. Sullivan III; +Cc: netdev
In-Reply-To: <1324654013.10184.597.camel@denise.theartistscloset.com>

Le vendredi 23 décembre 2011 à 10:26 -0500, John A. Sullivan III a
écrit :

> Yes.  This is a problem I have with netem on ingress traffic.  I use the
> filter on ffff: to redirect to ifb0 for the ingress traffic shaping.  I
> cannot figure out a way to redirect a second time to ifb1 for the netem
> qdisc.  I tried putting two action mirred statements in the filter but
> that did not work.  Unlike eth1, I cannot attach a filter further down
> the ifb0 hfsc hierarchy because one can't redirect one ifb into another
> ifb.  Thus, the only way I could figure out how to do inbound netem was
> to replace the terminal qdisc with netem rather than SFQ.  I'd love to
> be able to do that differently.  I tried attaching netem to the SFQ but
> that failed (I assume because SFQ is classless) and I tried the other
> way around, attaching SFQ to netem since you mentioned netem could take
> a class but that did not work either.

Unfortunately, netem wants to control skbs itself, in a fifo queue.

To implement what you want, we would need to setup a second qdisc,
and when packets are dequeued from internal netem fifo, queue them in
second qdisc.

^ permalink raw reply

* [PATCH 3/3] NFS: don't mark freshly created callback sockets with XPT_CHNGBUF
From: Stanislav Kinsbursky @ 2011-12-23 15:53 UTC (permalink / raw)
  To: Trond.Myklebust
  Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, jbottomley,
	bfields, davem, devel
In-Reply-To: <20111223145145.847.46221.stgit@localhost6.localdomain6>

This is a cleanup patch.
Check for XPT_CHNGBUF is done only for UPD sockets. NFS callbacks doesn't use
them.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 fs/nfs/callback.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index d81040a..1bf3e9f 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -278,8 +278,6 @@ int nfs_callback_up(u32 minorversion, struct rpc_xprt *xprt)
 		goto out_err;
 	}
 
-	svc_sock_update_bufs(serv);
-
 	sprintf(svc_name, "nfsv4.%u-svc", minorversion);
 	cb_info->serv = serv;
 	cb_info->rqst = rqstp;

^ permalink raw reply related

* [PATCH 2/3] Lockd: don't mark freshly created sockets with XPT_CHNGBUF
From: Stanislav Kinsbursky @ 2011-12-23 15:53 UTC (permalink / raw)
  To: Trond.Myklebust
  Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, jbottomley,
	bfields, davem, devel
In-Reply-To: <20111223145145.847.46221.stgit@localhost6.localdomain6>

This is a cleanup patch. XPT_CHNGBUF bit is set on UDP socket creation. Calling
svc_sock_update_bufs from lockd after is redundant.
Below is Lockd UDP sockets creating call trace:

lockd_up {

make_socks
create_lockd_family
create_lockd_listener
svc_create_xprt
__svc_xpo_create
svc_udp_create
svc_create_socket
svc_setup_socket
svc_udp_init (raise XPT_CHNGBUF)

....

svc_sock_update_bufs (raise XPT_CHNGBUF again)

....

}

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 fs/lockd/svc.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index ff379ff..55fea92 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -292,7 +292,6 @@ int lockd_up(void)
 		goto destroy_and_out;
 	}
 
-	svc_sock_update_bufs(serv);
 	serv->sv_maxconn = nlm_max_connections;
 
 	nlmsvc_task = kthread_run(lockd, nlmsvc_rqst, serv->sv_name);

^ permalink raw reply related

* [PATCH 1/3] SUNPRC: remove marking service temporary sockets with XPT_CHNGBUF
From: Stanislav Kinsbursky @ 2011-12-23 15:53 UTC (permalink / raw)
  To: Trond.Myklebust
  Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, jbottomley,
	bfields, davem, devel
In-Reply-To: <20111223145145.847.46221.stgit@localhost6.localdomain6>

This is a cleanup patch.
Temporary sockets can be TCP only. But XPT_CHNGBUF service socket flag is
checked only for UDP sockets on receive.

Signed-off-by: Stanislav Kinsbursky <skinsbursky@parallels.com>

---
 net/sunrpc/svcsock.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 110735f..72b05f2 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1381,8 +1381,6 @@ void svc_sock_update_bufs(struct svc_serv *serv)
 	spin_lock_bh(&serv->sv_lock);
 	list_for_each_entry(svsk, &serv->sv_permsocks, sk_xprt.xpt_list)
 		set_bit(XPT_CHNGBUF, &svsk->sk_xprt.xpt_flags);
-	list_for_each_entry(svsk, &serv->sv_tempsocks, sk_xprt.xpt_list)
-		set_bit(XPT_CHNGBUF, &svsk->sk_xprt.xpt_flags);
 	spin_unlock_bh(&serv->sv_lock);
 }
 EXPORT_SYMBOL_GPL(svc_sock_update_bufs);

^ permalink raw reply related

* [PATCH 0/3] SUNPRC: redundant XPT_CHNGBUF set removed
From: Stanislav Kinsbursky @ 2011-12-23 15:53 UTC (permalink / raw)
  To: Trond.Myklebust
  Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, jbottomley,
	bfields, davem, devel

This is a cleanup path set

The following series consists of:

---

Stanislav Kinsbursky (3):
      SUNPRC: remove marking service temporary sockets with XPT_CHNGBUF
      Lockd: don't mark freshly created sockets with XPT_CHNGBUF
      NFS: don't mark freshly created callback sockets with XPT_CHNGBUF


 fs/lockd/svc.c       |    1 -
 fs/nfs/callback.c    |    2 --
 net/sunrpc/svcsock.c |    2 --
 3 files changed, 0 insertions(+), 5 deletions(-)

-- 
Signature

^ permalink raw reply

* Re: SFQ on HFSC leaf does not seem to work
From: John A. Sullivan III @ 2011-12-23 15:26 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1324652361.2223.31.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>

Thanks for asking those questions as I had questions about them but felt
I was doing enough spamming of the list.  I am also just in the midst of
gather some of the other information you requested.  I'll respond in
line - John

On Fri, 2011-12-23 at 15:59 +0100, Eric Dumazet wrote:
> Le vendredi 23 décembre 2011 à 09:38 -0500, John A. Sullivan III a
> écrit :
> 
> > Thanks very much, Eric.  gso and gso only was enabled but disabling it
> > does not seem to have solved the problem when I activate netem:
> > 
> 
> And your kernel version is ?
root@testswitch01:~# uname -a
Linux testswitch01 2.6.32-5-686 #1 SMP Mon Oct 3 04:15:24 UTC 2011 i686
GNU/Linux

This is Debian Squeeze i386

As described, I'm running two netcats - one going to the default queue
along with the pings and the other going to a different queue.
> 
> > root@testswitch01:~# ./tcplay
> > root@testswitch01:~# man ethtool
> > root@testswitch01:~# ethtool -k eth1
> > Offload parameters for eth1:
> > rx-checksumming: on
> > tx-checksumming: on
> > scatter-gather: on
> > tcp-segmentation-offload: off
> > udp-fragmentation-offload: off
> > generic-segmentation-offload: on
> > generic-receive-offload: off
> > large-receive-offload: off
> > ntuple-filters: off
> > receive-hashing: off
> > root@testswitch01:~# ethtool -K eth1 gso off
> > root@testswitch01:~# ethtool -k eth1
> > Offload parameters for eth1:
> > rx-checksumming: on
> > tx-checksumming: on
> > scatter-gather: on
> > tcp-segmentation-offload: off
> > udp-fragmentation-offload: off
> > generic-segmentation-offload: off
> > generic-receive-offload: off
> > large-receive-offload: off
> > ntuple-filters: off
> > receive-hashing: off
> > ip ro flush cache
> > 
> > 64 bytes from 192.168.223.84: icmp_req=16 ttl=64 time=42.6 ms
> > 64 bytes from 192.168.223.84: icmp_req=17 ttl=64 time=39.1 ms
> > 64 bytes from 192.168.223.84: icmp_req=18 ttl=64 time=45.5 ms
> > 64 bytes from 192.168.223.84: icmp_req=19 ttl=64 time=406 ms
> > 64 bytes from 192.168.223.84: icmp_req=20 ttl=64 time=919 ms
> > 64 bytes from 192.168.223.84: icmp_req=21 ttl=64 time=920 ms
> > 64 bytes from 192.168.223.84: icmp_req=22 ttl=64 time=1013 ms
> > 64 bytes from 192.168.223.84: icmp_req=23 ttl=64 time=1158 ms
> > 64 bytes from 192.168.223.84: icmp_req=24 ttl=64 time=1521 ms
> > 64 bytes from 192.168.223.84: icmp_req=25 ttl=64 time=1915 ms
> > 64 bytes from 192.168.223.84: icmp_req=26 ttl=64 time=2371 ms
> > 64 bytes from 192.168.223.84: icmp_req=27 ttl=64 time=2797 ms
> > 64 bytes from 192.168.223.84: icmp_req=28 ttl=64 time=3161 ms
> > 64 bytes from 192.168.223.84: icmp_req=29 ttl=64 time=3162 ms
> > 64 bytes from 192.168.223.84: icmp_req=30 ttl=64 time=3163 ms
> > 
> > Just in case something is amiss in my methodology, I have four ssh
> > sessions open to the test firewall; ssh is in a separate prioritized
> > queue.  In one session I run:
> > 	ping 192.168.223.84
> > Then, in another, I do:
> > 	nc 192.168.223.100 443 >/dev/null - this should go into a non-default,
> 
> So you _receive_ trafic ?
Yes
> 
> Are you aware you dont have SFQ in your ingress setup, only egress  ?
Yes.  This is a problem I have with netem on ingress traffic.  I use the
filter on ffff: to redirect to ifb0 for the ingress traffic shaping.  I
cannot figure out a way to redirect a second time to ifb1 for the netem
qdisc.  I tried putting two action mirred statements in the filter but
that did not work.  Unlike eth1, I cannot attach a filter further down
the ifb0 hfsc hierarchy because one can't redirect one ifb into another
ifb.  Thus, the only way I could figure out how to do inbound netem was
to replace the terminal qdisc with netem rather than SFQ.  I'd love to
be able to do that differently.  I tried attaching netem to the SFQ but
that failed (I assume because SFQ is classless) and I tried the other
way around, attaching SFQ to netem since you mentioned netem could take
a class but that did not work either.
> 
> > prioritized queue.
> > Pings are OK at this point.
> > Then, in a third, I do:
> > 	nc 192.168.223.100 80 >/dev/null - this goes into the default queue,
> 
> same here ?
Yes.
> 
> > the same as ping, and is when the trouble starts.
> > 
> > I did alter the queue lengths in a recommendation from Dave Taht.  Here
> > is my current script with netem:
> > 
> > tc qdisc add dev eth1 root handle 1: hfsc default 20
> > tc class add dev eth1 parent 1: classid 1:1 hfsc sc rate 1490kbit ul
> > rate 1490kbit
> > tc class add dev eth1 parent 1:1 classid 1:20 hfsc rt rate 400kbit ls
> > rate 200kbit
> > tc qdisc add dev eth1 parent 1:20 handle 1201 sfq perturb 60 limit 30
> > tc class add dev eth1 parent 1:1 classid 1:10 hfsc rt umax 16kbit dmax
> > 50ms rate 200kbit ls rate 1000kbit
> > tc qdisc add dev eth1 parent 1:10 handle 1101 sfq perturb 60 limit 30
> > tc class add dev eth1 parent 1:1 classid 1:30 hfsc rt umax 1514b dmax
> > 20ms rate 20kbit
> > tc qdisc add dev eth1 parent 1:30 handle 1301 sfq perturb 60 limit 30
> > iptables -t mangle -A POSTROUTING -p 6 --syn --dport 443 -j CONNMARK
> > --set-mark 0x10
> > iptables -t mangle -A PREROUTING -p 6 --syn --dport 822 -j CONNMARK
> > --set-mark 0x11
> > iptables -t mangle -A POSTROUTING -o eth1 -p 6 -j CONNMARK
> > --restore-mark
> > modprobe ifb
> > ifconfig ifb0 up
> > ifconfig ifb1 up
> > tc qdisc add dev ifb0 root handle 1: hfsc default 20
> > tc class add dev ifb0 parent 1: classid 1:1 hfsc sc rate 1490kbit ul
> > rate 1490kbit
> > tc class add dev ifb0 parent 1:1 classid 1:20 hfsc rt rate 400kbit ls
> > rate 200kbit
> > tc qdisc add dev ifb0 parent 1:20 handle 1201 netem delay 25ms 5ms
> > distribution normal loss 0.1% 30%
> > tc class add dev ifb0 parent 1:1 classid 1:10 hfsc rt umax 16kbit dmax
> > 50ms rate 200kbit ls rate 1000kbit
> > tc qdisc add dev ifb0 parent 1:10 handle 1101 netem delay 25ms 5ms
> > distribution normal loss 0.1% 30%
> > tc class add dev ifb0 parent 1:1 classid 1:30 hfsc rt umax 1514b dmax
> > 20ms rate 20kbit
> > tc qdisc add dev ifb0 parent 1:30 handle 1301 netem delay 25ms 5ms
> > distribution normal loss 0.1% 30%
> > tc filter add dev ifb0 parent 1:0 protocol ip prio 1 handle 6: u32
> > divisor 1
> > tc filter add dev ifb0 parent 1:0 protocol ip prio 1 u32 match ip
> > protocol 6 0xff link 6: offset at 0 mask 0x0f00 shift 6 plus 0 eat
> 
> 
> > tc filter add dev ifb0 parent 1:0 protocol ip prio 1 u32 ht 6:0 match
> > tcp src 443 0x00ff flowid 1:10
> 
> why "src 443 0x00ff" ? It should be "src 443 0xffff"
That's what I tried at first but nothing matched the filter.  I assumed
it was because it objected to a value in the dst field so I masked it
off and it worked.
> 
> > tc filter add dev ifb0 parent 1:0 protocol ip prio 1 u32 ht 6:0 match
> > tcp dst 822 0xff00 flowid 1:30
> 
> same here : "dst 822 0xffff"
Same as above.  No filter matches when using that mask.
> 
> > tc qdisc add dev ifb1 root handle 2 netem delay 25ms 5ms distribution
> > normal loss 0.1% 30%
> > tc qdisc add dev eth1 ingress
> > tc filter add dev eth1 parent ffff: protocol ip prio 50 u32 match u32 0
> > 0 action mirred egress redirect dev ifb0
> > tc filter add dev eth1 parent 1:1 protocol ip prio 1 handle 0x11 fw
> > flowid 1:30
> > tc filter add dev eth1 parent 1:1 protocol ip prio 1 handle 0x10 fw
> > flowid 1:10
> > tc filter add dev eth1 parent 1:1 protocol ip prio 2 u32 match u32 0 0
> > flowid 1:20
> > tc filter add dev eth1 parent 1:0 protocol ip prio 1 u32 match u32 0 0
> > flowid 1:1 action mirred egress redirect dev ifb1
> > ip link set eth1 txqueuelen 100
> > ip link set ifb1 txqueuelen 100
> > ip link set ifb0 txqueuelen 100
> > 
> > I'd love to solve this.  Just when I thought I was all finished having
> > cracked the multiple filter problem to add netem to hfsc, I hit this.
> > Thanks again - John
> > 
> 
> Add some SFQ to your ingress too...
<grin> how with netem?  Thanks very much again - John
> 
> 
> 
PS - I also manually disabled gro in case that was a problem even though
it was showing off already.  It made no difference.

^ permalink raw reply

* [PATCH net-next] sch_hfsc: report backlog information
From: Eric Dumazet @ 2011-12-23 15:19 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, John A. Sullivan III
In-Reply-To: <1324648822.2223.18.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>

Add backlog (byte count) information in hfsc classes and qdisc, so that
"tc -s" can report it to user, instead of 0 values :

qdisc hfsc 1: root refcnt 6 default 20 
 Sent 45141660 bytes 30545 pkt (dropped 0, overlimits 91751 requeues 0) 
 rate 1492Kbit 126pps backlog 103226b 74p requeues 0 
...
class hfsc 1:20 parent 1:1 leaf 1201: rt m1 0bit d 0us m2 400000bit ls m1 0bit d 0us m2 200000bit 
 Sent 49534912 bytes 33519 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 81822b 56p requeues 0 
 period 23 work 49451576 bytes rtwork 13277552 bytes level 0 
...

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: John A. Sullivan III <jsullivan@opensourcedevel.com>
---
 net/sched/sch_hfsc.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 6488e64..9bdca2e 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1368,6 +1368,7 @@ hfsc_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	struct tc_hfsc_stats xstats;
 
 	cl->qstats.qlen = cl->qdisc->q.qlen;
+	cl->qstats.backlog = cl->qdisc->qstats.backlog;
 	xstats.level   = cl->level;
 	xstats.period  = cl->cl_vtperiod;
 	xstats.work    = cl->cl_total;
@@ -1561,6 +1562,15 @@ hfsc_dump_qdisc(struct Qdisc *sch, struct sk_buff *skb)
 	struct hfsc_sched *q = qdisc_priv(sch);
 	unsigned char *b = skb_tail_pointer(skb);
 	struct tc_hfsc_qopt qopt;
+	struct hfsc_class *cl;
+	struct hlist_node *n;
+	unsigned int i;
+
+	sch->qstats.backlog = 0;
+	for (i = 0; i < q->clhash.hashsize; i++) {
+		hlist_for_each_entry(cl, n, &q->clhash.hash[i], cl_common.hnode)
+			sch->qstats.backlog += cl->qdisc->qstats.backlog;
+	}
 
 	qopt.defcls = q->defcls;
 	NLA_PUT(skb, TCA_OPTIONS, sizeof(qopt), &qopt);

^ permalink raw reply related

* RE: [PATCH 1/2] bna: Added flash sub-module and ethtool eeprom entry points.
From: Krishna Gudipati @ 2011-12-23 15:09 UTC (permalink / raw)
  To: 'Ben Hutchings'
  Cc: 'davem@davemloft.net', 'netdev@vger.kernel.org',
	Adapter Linux Open SRC Team, Rasesh Mody
In-Reply-To: <1324633462.3860.6.camel@deadeye>

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Ben Hutchings
Sent: Friday, December 23, 2011 1:44 AM
To: Krishna Gudipati
Cc: davem@davemloft.net; netdev@vger.kernel.org; Adapter Linux Open SRC Team; Rasesh Mody
Subject: Re: [PATCH 1/2] bna: Added flash sub-module and ethtool eeprom entry points.

On Thu, 2011-12-22 at 15:29 -0800, kgudipat@brocade.com wrote:
> From: Krishna Gudipati <kgudipat@brocade.com>
> 
> Change details:
> 	- The patch adds flash sub-module to the bna driver.
> 	- Added ethtool set_eeprom() and get_eeprom() entry points to
> 	  support flash partition read/write operations.
[...]

I'm not going to say this is wrong, but we didn't find the EEPROM
operations suitable for firmware upgrade.  I have a couple of questions:

1. How long can a single set_eeprom() operation take?
2. Have you considered implementing the flash_device() operation or an
MTD driver?

-----

Thanks Ben for reviewing.

For 1: Considering the max size of the flash image chunk passed to the set_eeprom() entry point
	 at a time is 4Kbytes, it takes a little around ~3min for the firmware upgrade as the
	 operation involves a flash write for each 4Kbytes of the total 461Kbytes which is our fwimg size.

	 Please note that we use the same entry point to do flash update for other flash partitions as well,
	 for which the flash image size is much lesser than the firmware image.

For 2: No, we noticed that flash_device() entry point is listed as obsolete in the RHEL6.2 kernel source,
	 so we did not want to implement using it.

Thanks,
Krishna.

^ permalink raw reply

* Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()
From: Xi Wang @ 2011-12-23 15:07 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Tom Herbert, David S. Miller, netdev
In-Reply-To: <1324652031.2223.28.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>

On Dec 23, 2011, at 9:53 AM, Eric Dumazet wrote:
> Unsigned arithmetics is well defined in C. Very well in fact.

Oversized shifts are undefined in C, no matter it is unsigned
or not; roundup_pow_of_two() is implemented by shifting.

>> BTW, (count > UINT_MAX) is shorter and easier to understand
>> than (count != (unsigned long)(u32)count).
> 
> You miss the point. UINT_MAX is too small for 64bit arches.

I am sorry why is it too small?  what's difference between
(count > UINT_MAX) and (count != (unsigned long)(u32)count)?

- xi

^ permalink raw reply

* Re: under-performing bonded interfaces
From: Simon Chen @ 2011-12-23 15:03 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Ben Hutchings, Ben Greear, netdev
In-Reply-To: <1324532593.2621.26.camel@edumazet-laptop>

It's a funny thing again... I left the bandwidth test running
over-night - I used 16 simple senders and receivers.

The bandwidth slowly climbed from around 16G to 19G, which is much
better. I suspect two causes: 1) regular tcp implementation that has
slow-start and aggressive back-off; 2) user-land application...

-Simon

On Thu, Dec 22, 2011 at 12:43 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mercredi 21 décembre 2011 à 20:26 -0500, Simon Chen a écrit :
>> Hi folks,
>>
>> I added an Intel X520 card to both the sender and receiver... Now I
>> have two 10G ports on a PCIe 2.0 x8 slot (5Gx8), so the bandwidth of
>> the PCI bus shouldn't be the bottleneck.
>>
>> Now the throughput test gives me around 16Gbps in aggregate. Any ideas
>> how I can push closer to 20G? I don't quite understand where the
>> bottleneck is now.
>
> Could you post some "perf top" or "perf record / report" numbers ?
>
>
>

^ permalink raw reply

* Re: SFQ on HFSC leaf does not seem to work
From: Eric Dumazet @ 2011-12-23 14:59 UTC (permalink / raw)
  To: John A. Sullivan III; +Cc: netdev
In-Reply-To: <1324651128.10184.586.camel@denise.theartistscloset.com>

Le vendredi 23 décembre 2011 à 09:38 -0500, John A. Sullivan III a
écrit :

> Thanks very much, Eric.  gso and gso only was enabled but disabling it
> does not seem to have solved the problem when I activate netem:
> 

And your kernel version is ?

> root@testswitch01:~# ./tcplay
> root@testswitch01:~# man ethtool
> root@testswitch01:~# ethtool -k eth1
> Offload parameters for eth1:
> rx-checksumming: on
> tx-checksumming: on
> scatter-gather: on
> tcp-segmentation-offload: off
> udp-fragmentation-offload: off
> generic-segmentation-offload: on
> generic-receive-offload: off
> large-receive-offload: off
> ntuple-filters: off
> receive-hashing: off
> root@testswitch01:~# ethtool -K eth1 gso off
> root@testswitch01:~# ethtool -k eth1
> Offload parameters for eth1:
> rx-checksumming: on
> tx-checksumming: on
> scatter-gather: on
> tcp-segmentation-offload: off
> udp-fragmentation-offload: off
> generic-segmentation-offload: off
> generic-receive-offload: off
> large-receive-offload: off
> ntuple-filters: off
> receive-hashing: off
> ip ro flush cache
> 
> 64 bytes from 192.168.223.84: icmp_req=16 ttl=64 time=42.6 ms
> 64 bytes from 192.168.223.84: icmp_req=17 ttl=64 time=39.1 ms
> 64 bytes from 192.168.223.84: icmp_req=18 ttl=64 time=45.5 ms
> 64 bytes from 192.168.223.84: icmp_req=19 ttl=64 time=406 ms
> 64 bytes from 192.168.223.84: icmp_req=20 ttl=64 time=919 ms
> 64 bytes from 192.168.223.84: icmp_req=21 ttl=64 time=920 ms
> 64 bytes from 192.168.223.84: icmp_req=22 ttl=64 time=1013 ms
> 64 bytes from 192.168.223.84: icmp_req=23 ttl=64 time=1158 ms
> 64 bytes from 192.168.223.84: icmp_req=24 ttl=64 time=1521 ms
> 64 bytes from 192.168.223.84: icmp_req=25 ttl=64 time=1915 ms
> 64 bytes from 192.168.223.84: icmp_req=26 ttl=64 time=2371 ms
> 64 bytes from 192.168.223.84: icmp_req=27 ttl=64 time=2797 ms
> 64 bytes from 192.168.223.84: icmp_req=28 ttl=64 time=3161 ms
> 64 bytes from 192.168.223.84: icmp_req=29 ttl=64 time=3162 ms
> 64 bytes from 192.168.223.84: icmp_req=30 ttl=64 time=3163 ms
> 
> Just in case something is amiss in my methodology, I have four ssh
> sessions open to the test firewall; ssh is in a separate prioritized
> queue.  In one session I run:
> 	ping 192.168.223.84
> Then, in another, I do:
> 	nc 192.168.223.100 443 >/dev/null - this should go into a non-default,

So you _receive_ trafic ?

Are you aware you dont have SFQ in your ingress setup, only egress  ?

> prioritized queue.
> Pings are OK at this point.
> Then, in a third, I do:
> 	nc 192.168.223.100 80 >/dev/null - this goes into the default queue,

same here ?

> the same as ping, and is when the trouble starts.
> 
> I did alter the queue lengths in a recommendation from Dave Taht.  Here
> is my current script with netem:
> 
> tc qdisc add dev eth1 root handle 1: hfsc default 20
> tc class add dev eth1 parent 1: classid 1:1 hfsc sc rate 1490kbit ul
> rate 1490kbit
> tc class add dev eth1 parent 1:1 classid 1:20 hfsc rt rate 400kbit ls
> rate 200kbit
> tc qdisc add dev eth1 parent 1:20 handle 1201 sfq perturb 60 limit 30
> tc class add dev eth1 parent 1:1 classid 1:10 hfsc rt umax 16kbit dmax
> 50ms rate 200kbit ls rate 1000kbit
> tc qdisc add dev eth1 parent 1:10 handle 1101 sfq perturb 60 limit 30
> tc class add dev eth1 parent 1:1 classid 1:30 hfsc rt umax 1514b dmax
> 20ms rate 20kbit
> tc qdisc add dev eth1 parent 1:30 handle 1301 sfq perturb 60 limit 30
> iptables -t mangle -A POSTROUTING -p 6 --syn --dport 443 -j CONNMARK
> --set-mark 0x10
> iptables -t mangle -A PREROUTING -p 6 --syn --dport 822 -j CONNMARK
> --set-mark 0x11
> iptables -t mangle -A POSTROUTING -o eth1 -p 6 -j CONNMARK
> --restore-mark
> modprobe ifb
> ifconfig ifb0 up
> ifconfig ifb1 up
> tc qdisc add dev ifb0 root handle 1: hfsc default 20
> tc class add dev ifb0 parent 1: classid 1:1 hfsc sc rate 1490kbit ul
> rate 1490kbit
> tc class add dev ifb0 parent 1:1 classid 1:20 hfsc rt rate 400kbit ls
> rate 200kbit
> tc qdisc add dev ifb0 parent 1:20 handle 1201 netem delay 25ms 5ms
> distribution normal loss 0.1% 30%
> tc class add dev ifb0 parent 1:1 classid 1:10 hfsc rt umax 16kbit dmax
> 50ms rate 200kbit ls rate 1000kbit
> tc qdisc add dev ifb0 parent 1:10 handle 1101 netem delay 25ms 5ms
> distribution normal loss 0.1% 30%
> tc class add dev ifb0 parent 1:1 classid 1:30 hfsc rt umax 1514b dmax
> 20ms rate 20kbit
> tc qdisc add dev ifb0 parent 1:30 handle 1301 netem delay 25ms 5ms
> distribution normal loss 0.1% 30%
> tc filter add dev ifb0 parent 1:0 protocol ip prio 1 handle 6: u32
> divisor 1
> tc filter add dev ifb0 parent 1:0 protocol ip prio 1 u32 match ip
> protocol 6 0xff link 6: offset at 0 mask 0x0f00 shift 6 plus 0 eat


> tc filter add dev ifb0 parent 1:0 protocol ip prio 1 u32 ht 6:0 match
> tcp src 443 0x00ff flowid 1:10

why "src 443 0x00ff" ? It should be "src 443 0xffff"

> tc filter add dev ifb0 parent 1:0 protocol ip prio 1 u32 ht 6:0 match
> tcp dst 822 0xff00 flowid 1:30

same here : "dst 822 0xffff"

> tc qdisc add dev ifb1 root handle 2 netem delay 25ms 5ms distribution
> normal loss 0.1% 30%
> tc qdisc add dev eth1 ingress
> tc filter add dev eth1 parent ffff: protocol ip prio 50 u32 match u32 0
> 0 action mirred egress redirect dev ifb0
> tc filter add dev eth1 parent 1:1 protocol ip prio 1 handle 0x11 fw
> flowid 1:30
> tc filter add dev eth1 parent 1:1 protocol ip prio 1 handle 0x10 fw
> flowid 1:10
> tc filter add dev eth1 parent 1:1 protocol ip prio 2 u32 match u32 0 0
> flowid 1:20
> tc filter add dev eth1 parent 1:0 protocol ip prio 1 u32 match u32 0 0
> flowid 1:1 action mirred egress redirect dev ifb1
> ip link set eth1 txqueuelen 100
> ip link set ifb1 txqueuelen 100
> ip link set ifb0 txqueuelen 100
> 
> I'd love to solve this.  Just when I thought I was all finished having
> cracked the multiple filter problem to add netem to hfsc, I hit this.
> Thanks again - John
> 

Add some SFQ to your ingress too...

^ permalink raw reply

* Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()
From: Eric Dumazet @ 2011-12-23 14:53 UTC (permalink / raw)
  To: Xi Wang; +Cc: Tom Herbert, David S. Miller, netdev
In-Reply-To: <EC59D18E-82F6-4409-BB29-D7434D63B0C9@gmail.com>

Le vendredi 23 décembre 2011 à 09:30 -0500, Xi Wang a écrit :
> On Dec 23, 2011, at 8:06 AM, Eric Dumazet wrote:
> 
> > I'll submit following patch for net-next, once your patch is in this
> > tree.
> 
> Thanks for doing this. ;-)
> 
> > 		count = roundup_pow_of_two(count);
> > +		if (!count ||
> > +		    count != (unsigned long)(u32)count)
> > +			return -EINVAL;
> > 		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
> > 				/ sizeof(struct rps_dev_flow)) {
> > 			/* Enforce a limit to prevent overflow */
> 
> I would rather avoid undefined behavior in C.
> 

Unsigned arithmetics is well defined in C. Very well in fact.


> Given count = ULONG_MAX on 64-bit systems, roundup_pow_of_two()
> would overflow, and the overflowed result is undefined, e.g.,
> on x86-64 it gives 1, not 0.  That's why I used INT_MAX.
> 

I'll check roundup_pow_of_two(), this seems a bug to me.

Problem is : INT_MAX doesnt allow the full range of rps : 2^32 flows.

> BTW, (count > UINT_MAX) is shorter and more easier to understand
> than (count != (unsigned long)(u32)count).

You miss the point. UINT_MAX is too small for 64bit arches.

if (count != (unsigned long)(u32)count)

is pretty common stuff in kernel.

Check _kstrtoul() for an example.

^ permalink raw reply

* Re: SFQ on HFSC leaf does not seem to work
From: John A. Sullivan III @ 2011-12-23 14:38 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1324648822.2223.18.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>

On Fri, 2011-12-23 at 15:00 +0100, Eric Dumazet wrote:
> Le vendredi 23 décembre 2011 à 14:45 +0100, Eric Dumazet a écrit :
> 
> > 1) What kernel version do you use ?
> > 
> > 2) How many concurrent flows are running (number of netperf/netcat)
> > 
> > 3) Remind that 'perturb xxx' introduces a temporary doubling of the
> > number of flows.
> > 
> > 4) Had you disabled tso on eth1 ?
> >    (If not, you might send 64Kbytes packets, and at 400kbit, they take a
> > lot of time to transmit : more than one second ...)
> > 
> > 
> 
> Using your script on net-next, (only using eth3 instead of eth1) and
> 
> ethtool -K eth3 tso off
> ethtool -K eth3 gso off
> ip ro flush cache
> 
> one ssh : dd if=/dev/zero | ssh 192.168.0.1 "dd of=/dev/null"
> my ping is quite good :
> 
> 
> $ ping -c 20 192.168.0.1
> PING 192.168.0.1 (192.168.0.1) 56(84) bytes of data.
> 2011/11/23 14:57:01.106 64 bytes from 192.168.0.1: icmp_seq=1 ttl=64 time=59.4 ms
> 2011/11/23 14:57:02.121 64 bytes from 192.168.0.1: icmp_seq=2 ttl=64 time=72.7 ms
> 2011/11/23 14:57:03.109 64 bytes from 192.168.0.1: icmp_seq=3 ttl=64 time=60.3 ms
> 2011/11/23 14:57:04.108 64 bytes from 192.168.0.1: icmp_seq=4 ttl=64 time=57.8 ms
> 2011/11/23 14:57:05.115 64 bytes from 192.168.0.1: icmp_seq=5 ttl=64 time=62.6 ms
> 2011/11/23 14:57:06.116 64 bytes from 192.168.0.1: icmp_seq=6 ttl=64 time=62.6 ms
> 2011/11/23 14:57:07.112 64 bytes from 192.168.0.1: icmp_seq=7 ttl=64 time=57.6 ms
> 2011/11/23 14:57:08.127 64 bytes from 192.168.0.1: icmp_seq=8 ttl=64 time=70.9 ms
> 2011/11/23 14:57:09.123 64 bytes from 192.168.0.1: icmp_seq=9 ttl=64 time=65.4 ms
> 2011/11/23 14:57:10.113 64 bytes from 192.168.0.1: icmp_seq=10 ttl=64 time=53.5 ms
> 2011/11/23 14:57:11.127 64 bytes from 192.168.0.1: icmp_seq=11 ttl=64 time=66.7 ms
> 2011/11/23 14:57:12.129 64 bytes from 192.168.0.1: icmp_seq=12 ttl=64 time=67.4 ms
> 2011/11/23 14:57:13.119 64 bytes from 192.168.0.1: icmp_seq=13 ttl=64 time=56.3 ms
> 2011/11/23 14:57:14.127 64 bytes from 192.168.0.1: icmp_seq=14 ttl=64 time=64.0 ms
> 2011/11/23 14:57:15.116 64 bytes from 192.168.0.1: icmp_seq=15 ttl=64 time=51.9 ms
> 2011/11/23 14:57:16.127 64 bytes from 192.168.0.1: icmp_seq=16 ttl=64 time=61.2 ms
> 2011/11/23 14:57:17.127 64 bytes from 192.168.0.1: icmp_seq=17 ttl=64 time=60.4 ms
> 2011/11/23 14:57:18.135 64 bytes from 192.168.0.1: icmp_seq=18 ttl=64 time=68.2 ms
> 2011/11/23 14:57:19.137 64 bytes from 192.168.0.1: icmp_seq=19 ttl=64 time=69.1 ms
> 2011/11/23 14:57:20.136 64 bytes from 192.168.0.1: icmp_seq=20 ttl=64 time=67.0 ms
> 
> --- 192.168.0.1 ping statistics ---
> 20 packets transmitted, 20 received, 0% packet loss, time 19022ms
> rtt min/avg/max/mdev = 51.909/62.796/72.751/5.579 ms
> 
> $ tc -s -d class show dev eth3
> class hfsc 1: root 
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
>  backlog 0b 0p requeues 0 
>  period 0 level 2 
> 
> class hfsc 1:1 parent 1: sc m1 0bit d 0us m2 1490Kbit ul m1 0bit d 0us m2 1490Kbit 
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
>  backlog 0b 0p requeues 0 
>  period 69 work 38559740 bytes level 1 
> 
> class hfsc 1:10 parent 1:1 leaf 1101: rt m1 327680bit d 50.0ms m2 200000bit ls m1 0bit d 0us m2 1000Kbit 
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
>  backlog 0b 0p requeues 0 
>  period 0 level 0 
> 
> class hfsc 1:20 parent 1:1 leaf 1201: rt m1 0bit d 0us m2 400000bit ls m1 0bit d 0us m2 200000bit 
>  Sent 38587058 bytes 27022 pkt (dropped 0, overlimits 0 requeues 0) 
>  backlog 0b 19p requeues 0 
>  period 69 work 38559740 bytes rtwork 10358780 bytes level 0 
> 
> class hfsc 1:30 parent 1:1 leaf 1301: rt m1 605600bit d 20.0ms m2 20000bit 
>  Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
>  backlog 0b 0p requeues 0 
>  period 0 level 0 
> 
> class sfq 1201:f7 parent 1201: 
>  (dropped 0, overlimits 0 requeues 0) 
>  backlog 25804b 18p requeues 0 
>  allot -1336 
> 
> 
> Hmm... we probably could fill hfsc class information with non null bytes backlog...
> I'll take a look.
> 
> 
> 
Thanks very much, Eric.  gso and gso only was enabled but disabling it
does not seem to have solved the problem when I activate netem:

root@testswitch01:~# ./tcplay
root@testswitch01:~# man ethtool
root@testswitch01:~# ethtool -k eth1
Offload parameters for eth1:
rx-checksumming: on
tx-checksumming: on
scatter-gather: on
tcp-segmentation-offload: off
udp-fragmentation-offload: off
generic-segmentation-offload: on
generic-receive-offload: off
large-receive-offload: off
ntuple-filters: off
receive-hashing: off
root@testswitch01:~# ethtool -K eth1 gso off
root@testswitch01:~# ethtool -k eth1
Offload parameters for eth1:
rx-checksumming: on
tx-checksumming: on
scatter-gather: on
tcp-segmentation-offload: off
udp-fragmentation-offload: off
generic-segmentation-offload: off
generic-receive-offload: off
large-receive-offload: off
ntuple-filters: off
receive-hashing: off
ip ro flush cache

64 bytes from 192.168.223.84: icmp_req=16 ttl=64 time=42.6 ms
64 bytes from 192.168.223.84: icmp_req=17 ttl=64 time=39.1 ms
64 bytes from 192.168.223.84: icmp_req=18 ttl=64 time=45.5 ms
64 bytes from 192.168.223.84: icmp_req=19 ttl=64 time=406 ms
64 bytes from 192.168.223.84: icmp_req=20 ttl=64 time=919 ms
64 bytes from 192.168.223.84: icmp_req=21 ttl=64 time=920 ms
64 bytes from 192.168.223.84: icmp_req=22 ttl=64 time=1013 ms
64 bytes from 192.168.223.84: icmp_req=23 ttl=64 time=1158 ms
64 bytes from 192.168.223.84: icmp_req=24 ttl=64 time=1521 ms
64 bytes from 192.168.223.84: icmp_req=25 ttl=64 time=1915 ms
64 bytes from 192.168.223.84: icmp_req=26 ttl=64 time=2371 ms
64 bytes from 192.168.223.84: icmp_req=27 ttl=64 time=2797 ms
64 bytes from 192.168.223.84: icmp_req=28 ttl=64 time=3161 ms
64 bytes from 192.168.223.84: icmp_req=29 ttl=64 time=3162 ms
64 bytes from 192.168.223.84: icmp_req=30 ttl=64 time=3163 ms

Just in case something is amiss in my methodology, I have four ssh
sessions open to the test firewall; ssh is in a separate prioritized
queue.  In one session I run:
	ping 192.168.223.84
Then, in another, I do:
	nc 192.168.223.100 443 >/dev/null - this should go into a non-default,
prioritized queue.
Pings are OK at this point.
Then, in a third, I do:
	nc 192.168.223.100 80 >/dev/null - this goes into the default queue,
the same as ping, and is when the trouble starts.

I did alter the queue lengths in a recommendation from Dave Taht.  Here
is my current script with netem:

tc qdisc add dev eth1 root handle 1: hfsc default 20
tc class add dev eth1 parent 1: classid 1:1 hfsc sc rate 1490kbit ul
rate 1490kbit
tc class add dev eth1 parent 1:1 classid 1:20 hfsc rt rate 400kbit ls
rate 200kbit
tc qdisc add dev eth1 parent 1:20 handle 1201 sfq perturb 60 limit 30
tc class add dev eth1 parent 1:1 classid 1:10 hfsc rt umax 16kbit dmax
50ms rate 200kbit ls rate 1000kbit
tc qdisc add dev eth1 parent 1:10 handle 1101 sfq perturb 60 limit 30
tc class add dev eth1 parent 1:1 classid 1:30 hfsc rt umax 1514b dmax
20ms rate 20kbit
tc qdisc add dev eth1 parent 1:30 handle 1301 sfq perturb 60 limit 30
iptables -t mangle -A POSTROUTING -p 6 --syn --dport 443 -j CONNMARK
--set-mark 0x10
iptables -t mangle -A PREROUTING -p 6 --syn --dport 822 -j CONNMARK
--set-mark 0x11
iptables -t mangle -A POSTROUTING -o eth1 -p 6 -j CONNMARK
--restore-mark
modprobe ifb
ifconfig ifb0 up
ifconfig ifb1 up
tc qdisc add dev ifb0 root handle 1: hfsc default 20
tc class add dev ifb0 parent 1: classid 1:1 hfsc sc rate 1490kbit ul
rate 1490kbit
tc class add dev ifb0 parent 1:1 classid 1:20 hfsc rt rate 400kbit ls
rate 200kbit
tc qdisc add dev ifb0 parent 1:20 handle 1201 netem delay 25ms 5ms
distribution normal loss 0.1% 30%
tc class add dev ifb0 parent 1:1 classid 1:10 hfsc rt umax 16kbit dmax
50ms rate 200kbit ls rate 1000kbit
tc qdisc add dev ifb0 parent 1:10 handle 1101 netem delay 25ms 5ms
distribution normal loss 0.1% 30%
tc class add dev ifb0 parent 1:1 classid 1:30 hfsc rt umax 1514b dmax
20ms rate 20kbit
tc qdisc add dev ifb0 parent 1:30 handle 1301 netem delay 25ms 5ms
distribution normal loss 0.1% 30%
tc filter add dev ifb0 parent 1:0 protocol ip prio 1 handle 6: u32
divisor 1
tc filter add dev ifb0 parent 1:0 protocol ip prio 1 u32 match ip
protocol 6 0xff link 6: offset at 0 mask 0x0f00 shift 6 plus 0 eat
tc filter add dev ifb0 parent 1:0 protocol ip prio 1 u32 ht 6:0 match
tcp src 443 0x00ff flowid 1:10
tc filter add dev ifb0 parent 1:0 protocol ip prio 1 u32 ht 6:0 match
tcp dst 822 0xff00 flowid 1:30
tc qdisc add dev ifb1 root handle 2 netem delay 25ms 5ms distribution
normal loss 0.1% 30%
tc qdisc add dev eth1 ingress
tc filter add dev eth1 parent ffff: protocol ip prio 50 u32 match u32 0
0 action mirred egress redirect dev ifb0
tc filter add dev eth1 parent 1:1 protocol ip prio 1 handle 0x11 fw
flowid 1:30
tc filter add dev eth1 parent 1:1 protocol ip prio 1 handle 0x10 fw
flowid 1:10
tc filter add dev eth1 parent 1:1 protocol ip prio 2 u32 match u32 0 0
flowid 1:20
tc filter add dev eth1 parent 1:0 protocol ip prio 1 u32 match u32 0 0
flowid 1:1 action mirred egress redirect dev ifb1
ip link set eth1 txqueuelen 100
ip link set ifb1 txqueuelen 100
ip link set ifb0 txqueuelen 100

I'd love to solve this.  Just when I thought I was all finished having
cracked the multiple filter problem to add netem to hfsc, I hit this.
Thanks again - John

^ permalink raw reply

* Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()
From: Xi Wang @ 2011-12-23 14:30 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Tom Herbert, David S. Miller, netdev
In-Reply-To: <1324645592.2223.9.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>

On Dec 23, 2011, at 8:06 AM, Eric Dumazet wrote:

> I'll submit following patch for net-next, once your patch is in this
> tree.

Thanks for doing this. ;-)

> 		count = roundup_pow_of_two(count);
> +		if (!count ||
> +		    count != (unsigned long)(u32)count)
> +			return -EINVAL;
> 		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
> 				/ sizeof(struct rps_dev_flow)) {
> 			/* Enforce a limit to prevent overflow */

I would rather avoid undefined behavior in C.

Given count = ULONG_MAX on 64-bit systems, roundup_pow_of_two()
would overflow, and the overflowed result is undefined, e.g.,
on x86-64 it gives 1, not 0.  That's why I used INT_MAX.

BTW, (count > UINT_MAX) is shorter and more easier to understand
than (count != (unsigned long)(u32)count).

- xi

^ permalink raw reply

* Re: SFQ on HFSC leaf does not seem to work
From: Eric Dumazet @ 2011-12-23 14:00 UTC (permalink / raw)
  To: John A. Sullivan III; +Cc: netdev
In-Reply-To: <1324647920.2223.14.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC>

Le vendredi 23 décembre 2011 à 14:45 +0100, Eric Dumazet a écrit :

> 1) What kernel version do you use ?
> 
> 2) How many concurrent flows are running (number of netperf/netcat)
> 
> 3) Remind that 'perturb xxx' introduces a temporary doubling of the
> number of flows.
> 
> 4) Had you disabled tso on eth1 ?
>    (If not, you might send 64Kbytes packets, and at 400kbit, they take a
> lot of time to transmit : more than one second ...)
> 
> 

Using your script on net-next, (only using eth3 instead of eth1) and

ethtool -K eth3 tso off
ethtool -K eth3 gso off
ip ro flush cache

one ssh : dd if=/dev/zero | ssh 192.168.0.1 "dd of=/dev/null"
my ping is quite good :


$ ping -c 20 192.168.0.1
PING 192.168.0.1 (192.168.0.1) 56(84) bytes of data.
2011/11/23 14:57:01.106 64 bytes from 192.168.0.1: icmp_seq=1 ttl=64 time=59.4 ms
2011/11/23 14:57:02.121 64 bytes from 192.168.0.1: icmp_seq=2 ttl=64 time=72.7 ms
2011/11/23 14:57:03.109 64 bytes from 192.168.0.1: icmp_seq=3 ttl=64 time=60.3 ms
2011/11/23 14:57:04.108 64 bytes from 192.168.0.1: icmp_seq=4 ttl=64 time=57.8 ms
2011/11/23 14:57:05.115 64 bytes from 192.168.0.1: icmp_seq=5 ttl=64 time=62.6 ms
2011/11/23 14:57:06.116 64 bytes from 192.168.0.1: icmp_seq=6 ttl=64 time=62.6 ms
2011/11/23 14:57:07.112 64 bytes from 192.168.0.1: icmp_seq=7 ttl=64 time=57.6 ms
2011/11/23 14:57:08.127 64 bytes from 192.168.0.1: icmp_seq=8 ttl=64 time=70.9 ms
2011/11/23 14:57:09.123 64 bytes from 192.168.0.1: icmp_seq=9 ttl=64 time=65.4 ms
2011/11/23 14:57:10.113 64 bytes from 192.168.0.1: icmp_seq=10 ttl=64 time=53.5 ms
2011/11/23 14:57:11.127 64 bytes from 192.168.0.1: icmp_seq=11 ttl=64 time=66.7 ms
2011/11/23 14:57:12.129 64 bytes from 192.168.0.1: icmp_seq=12 ttl=64 time=67.4 ms
2011/11/23 14:57:13.119 64 bytes from 192.168.0.1: icmp_seq=13 ttl=64 time=56.3 ms
2011/11/23 14:57:14.127 64 bytes from 192.168.0.1: icmp_seq=14 ttl=64 time=64.0 ms
2011/11/23 14:57:15.116 64 bytes from 192.168.0.1: icmp_seq=15 ttl=64 time=51.9 ms
2011/11/23 14:57:16.127 64 bytes from 192.168.0.1: icmp_seq=16 ttl=64 time=61.2 ms
2011/11/23 14:57:17.127 64 bytes from 192.168.0.1: icmp_seq=17 ttl=64 time=60.4 ms
2011/11/23 14:57:18.135 64 bytes from 192.168.0.1: icmp_seq=18 ttl=64 time=68.2 ms
2011/11/23 14:57:19.137 64 bytes from 192.168.0.1: icmp_seq=19 ttl=64 time=69.1 ms
2011/11/23 14:57:20.136 64 bytes from 192.168.0.1: icmp_seq=20 ttl=64 time=67.0 ms

--- 192.168.0.1 ping statistics ---
20 packets transmitted, 20 received, 0% packet loss, time 19022ms
rtt min/avg/max/mdev = 51.909/62.796/72.751/5.579 ms

$ tc -s -d class show dev eth3
class hfsc 1: root 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0 
 period 0 level 2 

class hfsc 1:1 parent 1: sc m1 0bit d 0us m2 1490Kbit ul m1 0bit d 0us m2 1490Kbit 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0 
 period 69 work 38559740 bytes level 1 

class hfsc 1:10 parent 1:1 leaf 1101: rt m1 327680bit d 50.0ms m2 200000bit ls m1 0bit d 0us m2 1000Kbit 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0 
 period 0 level 0 

class hfsc 1:20 parent 1:1 leaf 1201: rt m1 0bit d 0us m2 400000bit ls m1 0bit d 0us m2 200000bit 
 Sent 38587058 bytes 27022 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 19p requeues 0 
 period 69 work 38559740 bytes rtwork 10358780 bytes level 0 

class hfsc 1:30 parent 1:1 leaf 1301: rt m1 605600bit d 20.0ms m2 20000bit 
 Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) 
 backlog 0b 0p requeues 0 
 period 0 level 0 

class sfq 1201:f7 parent 1201: 
 (dropped 0, overlimits 0 requeues 0) 
 backlog 25804b 18p requeues 0 
 allot -1336 


Hmm... we probably could fill hfsc class information with non null bytes backlog...
I'll take a look.

^ permalink raw reply

* Re: SFQ on HFSC leaf does not seem to work
From: Eric Dumazet @ 2011-12-23 13:45 UTC (permalink / raw)
  To: John A. Sullivan III; +Cc: netdev
In-Reply-To: <1324645986.10184.571.camel@denise.theartistscloset.com>

Le vendredi 23 décembre 2011 à 08:13 -0500, John A. Sullivan III a
écrit :
> On Fri, 2011-12-23 at 09:10 +0100, Eric Dumazet wrote:
> > Le vendredi 23 décembre 2011 à 01:00 -0500, John A. Sullivan III a
> > écrit :
> > 
> > > Where might this 3000 ms delay be coming from?
> > > 
> > 
> > Certainly not from SFQ
> > 
> > You could use tcpdump to check if delay is at egress or ingress side.
> > 
> > 
> > 
> That's perplexing as well.  Tracing on the eth1 interface of the test
> devices, we see the packets going out with near perfect regularity at
> one second intervals and replies returning immediately.  They are also
> completely in sequence so it is not as if we are matching packet 4 with
> packet 7 and seeing an immediate reply that is really offset by three
> seconds.
> 
> Thus, the delay seems to be registered only in the ICMP application
> itself.  The CPU is virtually idle - very impressed with Debian's
> handling of interrupts on this old system would have been spending
> almost 60% on hardware interrupts.
> 
> Another weird characteristic is that the delay is introduced gradually
> over several seconds:
> 64 bytes from 192.168.223.84: icmp_req=4 ttl=64 time=58.0 ms
> 64 bytes from 192.168.223.84: icmp_req=5 ttl=64 time=52.4 ms
> 64 bytes from 192.168.223.84: icmp_req=6 ttl=64 time=48.7 ms
> 64 bytes from 192.168.223.84: icmp_req=7 ttl=64 time=118 ms
> 64 bytes from 192.168.223.84: icmp_req=8 ttl=64 time=834 ms
> 64 bytes from 192.168.223.84: icmp_req=9 ttl=64 time=896 ms
> 64 bytes from 192.168.223.84: icmp_req=10 ttl=64 time=897 ms
> 64 bytes from 192.168.223.84: icmp_req=11 ttl=64 time=1081 ms
> 64 bytes from 192.168.223.84: icmp_req=12 ttl=64 time=1257 ms
> 64 bytes from 192.168.223.84: icmp_req=13 ttl=64 time=1744 ms
> 64 bytes from 192.168.223.84: icmp_req=14 ttl=64 time=2107 ms
> 64 bytes from 192.168.223.84: icmp_req=15 ttl=64 time=2532 ms
> 64 bytes from 192.168.223.84: icmp_req=16 ttl=64 time=2948 ms
> 64 bytes from 192.168.223.84: icmp_req=17 ttl=64 time=3191 ms
> 64 bytes from 192.168.223.84: icmp_req=18 ttl=64 time=3163 ms
> 
> While the delay builds, we can see the replied noticeably delayed
> however, once we hit 3000ms, the display updates one packet per second.
> I would expect one packet per three seconds unless we were interleaving
> packets but we are not according to the packet trace.
> 
> So I am guessing an inbound queue but where? Ah, netem.  I pulled out
> netem and I seem vastly different results.  I get a very occasional lost
> packet but no impact to latency:
> 
> 64 bytes from 192.168.223.84: icmp_req=48 ttl=64 time=0.802 ms
> 64 bytes from 192.168.223.84: icmp_req=49 ttl=64 time=0.843 ms
> 64 bytes from 192.168.223.84: icmp_req=50 ttl=64 time=0.739 ms
> 64 bytes from 192.168.223.84: icmp_req=51 ttl=64 time=0.769 ms
> 64 bytes from 192.168.223.84: icmp_req=52 ttl=64 time=0.833 ms
> 64 bytes from 192.168.223.84: icmp_req=53 ttl=64 time=0.872 ms
> 64 bytes from 192.168.223.84: icmp_req=54 ttl=64 time=0.786 ms
> 64 bytes from 192.168.223.84: icmp_req=55 ttl=64 time=0.766 ms
> 64 bytes from 192.168.223.84: icmp_req=56 ttl=64 time=0.715 ms
> 64 bytes from 192.168.223.84: icmp_req=57 ttl=64 time=0.710 ms
> 64 bytes from 192.168.223.84: icmp_req=58 ttl=64 time=0.784 ms
> 64 bytes from 192.168.223.84: icmp_req=59 ttl=64 time=0.766 ms
> 64 bytes from 192.168.223.84: icmp_req=60 ttl=64 time=0.748 ms
> 
> 
> So netem has bufferbloat ;)  Thanks - John
> 

1) What kernel version do you use ?

2) How many concurrent flows are running (number of netperf/netcat)

3) Remind that 'perturb xxx' introduces a temporary doubling of the
number of flows.

4) Had you disabled tso on eth1 ?
   (If not, you might send 64Kbytes packets, and at 400kbit, they take a
lot of time to transmit : more than one second ...)

^ permalink raw reply

* Re: SFQ on HFSC leaf does not seem to work
From: John A. Sullivan III @ 2011-12-23 13:13 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1324627808.10854.6.camel@edumazet-laptop>

On Fri, 2011-12-23 at 09:10 +0100, Eric Dumazet wrote:
> Le vendredi 23 décembre 2011 à 01:00 -0500, John A. Sullivan III a
> écrit :
> 
> > Where might this 3000 ms delay be coming from?
> > 
> 
> Certainly not from SFQ
> 
> You could use tcpdump to check if delay is at egress or ingress side.
> 
> 
> 
That's perplexing as well.  Tracing on the eth1 interface of the test
devices, we see the packets going out with near perfect regularity at
one second intervals and replies returning immediately.  They are also
completely in sequence so it is not as if we are matching packet 4 with
packet 7 and seeing an immediate reply that is really offset by three
seconds.

Thus, the delay seems to be registered only in the ICMP application
itself.  The CPU is virtually idle - very impressed with Debian's
handling of interrupts on this old system would have been spending
almost 60% on hardware interrupts.

Another weird characteristic is that the delay is introduced gradually
over several seconds:
64 bytes from 192.168.223.84: icmp_req=4 ttl=64 time=58.0 ms
64 bytes from 192.168.223.84: icmp_req=5 ttl=64 time=52.4 ms
64 bytes from 192.168.223.84: icmp_req=6 ttl=64 time=48.7 ms
64 bytes from 192.168.223.84: icmp_req=7 ttl=64 time=118 ms
64 bytes from 192.168.223.84: icmp_req=8 ttl=64 time=834 ms
64 bytes from 192.168.223.84: icmp_req=9 ttl=64 time=896 ms
64 bytes from 192.168.223.84: icmp_req=10 ttl=64 time=897 ms
64 bytes from 192.168.223.84: icmp_req=11 ttl=64 time=1081 ms
64 bytes from 192.168.223.84: icmp_req=12 ttl=64 time=1257 ms
64 bytes from 192.168.223.84: icmp_req=13 ttl=64 time=1744 ms
64 bytes from 192.168.223.84: icmp_req=14 ttl=64 time=2107 ms
64 bytes from 192.168.223.84: icmp_req=15 ttl=64 time=2532 ms
64 bytes from 192.168.223.84: icmp_req=16 ttl=64 time=2948 ms
64 bytes from 192.168.223.84: icmp_req=17 ttl=64 time=3191 ms
64 bytes from 192.168.223.84: icmp_req=18 ttl=64 time=3163 ms

While the delay builds, we can see the replied noticeably delayed
however, once we hit 3000ms, the display updates one packet per second.
I would expect one packet per three seconds unless we were interleaving
packets but we are not according to the packet trace.

So I am guessing an inbound queue but where? Ah, netem.  I pulled out
netem and I seem vastly different results.  I get a very occasional lost
packet but no impact to latency:

64 bytes from 192.168.223.84: icmp_req=48 ttl=64 time=0.802 ms
64 bytes from 192.168.223.84: icmp_req=49 ttl=64 time=0.843 ms
64 bytes from 192.168.223.84: icmp_req=50 ttl=64 time=0.739 ms
64 bytes from 192.168.223.84: icmp_req=51 ttl=64 time=0.769 ms
64 bytes from 192.168.223.84: icmp_req=52 ttl=64 time=0.833 ms
64 bytes from 192.168.223.84: icmp_req=53 ttl=64 time=0.872 ms
64 bytes from 192.168.223.84: icmp_req=54 ttl=64 time=0.786 ms
64 bytes from 192.168.223.84: icmp_req=55 ttl=64 time=0.766 ms
64 bytes from 192.168.223.84: icmp_req=56 ttl=64 time=0.715 ms
64 bytes from 192.168.223.84: icmp_req=57 ttl=64 time=0.710 ms
64 bytes from 192.168.223.84: icmp_req=58 ttl=64 time=0.784 ms
64 bytes from 192.168.223.84: icmp_req=59 ttl=64 time=0.766 ms
64 bytes from 192.168.223.84: icmp_req=60 ttl=64 time=0.748 ms


So netem has bufferbloat ;)  Thanks - John

^ permalink raw reply

* Re: [net-next 8/9] ixgbe: add interface to export thermal data
From: Francois Romieu @ 2011-12-23 13:01 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: davem, Don Skidmore, netdev, gospo, sassmann,
	Peter P Waskiewicz Jr
In-Reply-To: <1324631357-31789-9-git-send-email-jeffrey.t.kirsher@intel.com>

Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
> From: Don Skidmore <donald.c.skidmore@intel.com>
> 
> Some of our adapters have thermal data available, this patch exports this
> data via a read-only sysfs interface.  More patches will follow that will
> contain additional information to be exported.
> 
> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/Makefile      |    2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h       |    4 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c |    2 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |    6 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |  305 ++++++++++++++++++++++++
[...]
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
> index 7720721..1a3810e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
> @@ -2137,6 +2137,8 @@ static struct ixgbe_mac_operations mac_ops_82599 = {
>  	.set_vlan_anti_spoofing = &ixgbe_set_vlan_anti_spoofing,
>  	.acquire_swfw_sync      = &ixgbe_acquire_swfw_sync,
>  	.release_swfw_sync      = &ixgbe_release_swfw_sync,
> +	.get_thermal_sensor_data = &ixgbe_get_thermal_sensor_data_generic,
> +	.init_thermal_sensor_thresh = &ixgbe_init_thermal_sensor_thresh_generic,

.get_thermal_sensor_data and .init_thermal_sensor_thresh do not seem to
be read anywhere.

>  
>  };
>  
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index e27e4d1..b5cef7e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7717,6 +7717,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  
>  	e_dev_info("Intel(R) 10 Gigabit Network Connection\n");
>  	cards_found++;
> +
> +	if (ixgbe_sysfs_init(adapter))
> +		e_err(probe, "failed to allocate sysfs resources\n");
> +
>  	return 0;
>  
>  err_register:
> @@ -7764,6 +7768,8 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
>  	}
>  
>  #endif
> +	ixgbe_sysfs_exit(adapter);
> +
>  #ifdef IXGBE_FCOE
>  	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
>  		ixgbe_cleanup_fcoe(adapter);
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
> new file mode 100644
> index 0000000..db818ae
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
[...]
> +static void ixgbe_del_adapter(struct ixgbe_adapter *adapter)

Please name it ixgbe_sysfs_del_adapter or anything better.

> +{
> +	int i;
> +
> +	if (adapter == NULL)
> +		return;
> +
> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
> +		if (adapter->therm_kobj[i] == NULL)
> +			continue;
> +		sysfs_remove_group(adapter->therm_kobj[i], &therm_attr_group);
> +		kobject_put(adapter->therm_kobj[i]);
> +	}
> +	if (adapter->info_kobj != NULL)
> +		kobject_put(adapter->info_kobj);
> +}
> +
> +/* called from ixgbe_main.c */
> +void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter)
> +{
> +	ixgbe_del_adapter(adapter);
> +}
> +
> +/* called from ixgbe_main.c */
> +int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
> +{
> +	struct net_device *netdev;
> +	int rc = 0;
> +	int i;
> +	char buf[16];
> +
> +	if (adapter == NULL)
> +		goto err;

ixgbe_sysfs_init is only used in ixgbe_probe at a place where adapter can
not be NULL.

> +	netdev = adapter->netdev;
> +	if (netdev == NULL)
> +		goto err;

netdev can not be NULL.

> +
> +	adapter->info_kobj = NULL;
> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++)
> +		adapter->therm_kobj[i] = NULL;
> +
> +	/* create info kobj and attribute listings in kobj */
> +	adapter->info_kobj = kobject_create_and_add("info", 
> +					&(netdev->dev.kobj));

Remove comment and parenthesis. Everything can fit in a single line.

> +	if (adapter->info_kobj == NULL)
> +		goto err;
> +
> +	/* Don't create thermal subkobjs if no data present */
> +	if (ixgbe_thermal_present(adapter->info_kobj) != true)
> +		goto exit;
> +
> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {

'buf' ought to be declared here.

> +
> +		/*
> +		 * Likewise only create individual kobjs that have
> +		 * meaningful data.
> +		 */
> +		if (adapter->hw.mac.thermal_sensor_data.sensor[i].location == 0)
> +			continue;
> +
> +		/* directory named after sensor offset */
> +		snprintf(buf, sizeof(buf), "sensor_%d", i);
> +		adapter->therm_kobj[i] =
> +			kobject_create_and_add(buf, adapter->info_kobj);
> +		if (adapter->therm_kobj[i] == NULL)
> +			goto err;
> +		if (sysfs_create_group(adapter->therm_kobj[i],
> +				       &therm_attr_group))
> +			goto err;
> +	}
> +
> +	goto exit;
> +
> +err:
> +	ixgbe_del_adapter(adapter);
> +	rc = -1;
> +exit:
> +	return rc;
> +}
> +
> -- 
> 1.7.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [net-next 7/9] ixgbe: add support functions for gathering thermal data sensor
From: Francois Romieu @ 2011-12-23 13:01 UTC (permalink / raw)
  To: Jeff Kirsher
  Cc: davem, Don Skidmore, netdev, gospo, sassmann,
	Peter P Waskiewicz Jr
In-Reply-To: <1324631357-31789-8-git-send-email-jeffrey.t.kirsher@intel.com>

Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
> From: Don Skidmore <donald.c.skidmore@intel.com>
> 
> Some 82599 adapters contain thermal data that we can get to via
> an i2c interface.  These functions provide support to get at that
> data.  A following patch will export this data.
> 
> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_common.c |  150 +++++++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_common.h |   13 ++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_type.h   |   38 ++++++
>  3 files changed, 201 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> index a3aa633..05aa156 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> @@ -3526,3 +3526,153 @@ void ixgbe_clear_tx_pending(struct ixgbe_hw *hw)
>  	IXGBE_WRITE_REG(hw, IXGBE_GCR_EXT, gcr_ext);
>  	IXGBE_WRITE_REG(hw, IXGBE_HLREG0, hlreg0);
>  }
> +
> +static const u8 ixgbe_emc_temp_data[4] = {
> +	IXGBE_EMC_INTERNAL_DATA,
> +	IXGBE_EMC_DIODE1_DATA,
> +	IXGBE_EMC_DIODE2_DATA,
> +	IXGBE_EMC_DIODE3_DATA
> +};
> +static const u8 ixgbe_emc_therm_limit[4] = {
> +	IXGBE_EMC_INTERNAL_THERM_LIMIT,
> +	IXGBE_EMC_DIODE1_THERM_LIMIT,
> +	IXGBE_EMC_DIODE2_THERM_LIMIT,
> +	IXGBE_EMC_DIODE3_THERM_LIMIT
> +};
> +
> +/**
> + *  ixgbe_get_thermal_sensor_data - Gathers thermal sensor data
> + *  @hw: pointer to hardware structure
> + *  @data: pointer to the thermal sensor data structure
> + *
> + *  Returns the thermal sensor data structure
> + **/
> +s32 ixgbe_get_thermal_sensor_data_generic(struct ixgbe_hw *hw)
> +{
> +	s32 status = 0;
> +	u16 ets_offset;
> +	u16 ets_cfg;
> +	u16 ets_sensor;
> +	u8  num_sensors;
> +	u8  sensor_index;
> +	u8  sensor_location;
> +	u8  i;
> +	struct ixgbe_thermal_sensor_data *data = &hw->mac.thermal_sensor_data;
> +
> +	/* Only support thermal sensors attached to 82599 physical port 0 */
> +	if ((hw->mac.type != ixgbe_mac_82599EB) ||
> +	     (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1)) {
> +		status = IXGBE_NOT_IMPLEMENTED;

While defined, IXGBE_NOT_IMPLEMENTED is currently unused in both davem-next
and the remaining patches of this series. Can't you remove it completely and
use a standard error code, say -EOPNOTSUPP ?

> +		goto out;
> +	}
> +
> +	status = hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
> +	if (status)
> +		goto out;
> +
> +	if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF)) {
> +		status = IXGBE_NOT_IMPLEMENTED;
> +		goto out;
> +	}
> +
> +	status = hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
> +	if (status)
> +		goto out;
> +
> +	if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
> +	    != IXGBE_ETS_TYPE_EMC) {
            ^^ -> should appear in the previous line

> +		status = IXGBE_NOT_IMPLEMENTED;
> +		goto out;
> +	}
> +
> +	num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
> +	if (num_sensors > IXGBE_MAX_SENSORS)
> +		num_sensors = IXGBE_MAX_SENSORS;

ets_cfg is not used beyond this point and most of this code is duplicated
in ixgbe_init_thermal_sensor_thresh_generic.

You may refactor the code with xyz_get_ets_offset() and xyz_get_num_sensors()
methods, add some eth_type local variable and avoid the duplication while
keeping the lines short.

> +
> +	for (i = 0; i < num_sensors; i++) {

sensor_index and sensor_location should be declared here.

> +		status = hw->eeprom.ops.read(hw, (ets_offset + 1 + i),
> +					     &ets_sensor);
> +		if (status)
> +			goto out;
> +
> +		sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
> +				IXGBE_ETS_DATA_INDEX_SHIFT);
> +		sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
> +				   IXGBE_ETS_DATA_LOC_SHIFT);
> +
> +		if (sensor_location != 0) {
> +			status = hw->phy.ops.read_i2c_byte(hw,
> +					ixgbe_emc_temp_data[sensor_index],
> +					IXGBE_I2C_THERMAL_SENSOR_ADDR,
> +					&data->sensor[i].temp);
> +		if (status)
> +			goto out;
> +		}

Broken indentation.

> +	}
> +out:
> +	return status;
> +}
> +
> +/**
> + * ixgbe_init_thermal_sensor_thresh_generic - Inits thermal sensor thresholds
> + * @hw: pointer to hardware structure
> + *
> + * Inits the thermal sensor thresholds according to the NVM map
> + * and save off the threshold and location values into mac.thermal_sensor_data
> + **/
> +s32 ixgbe_init_thermal_sensor_thresh_generic(struct ixgbe_hw *hw)
> +{
> +	s32 status = 0;
> +	u16 ets_offset;
> +	u16 ets_cfg;
> +	u16 ets_sensor;
> +	u8  low_thresh_delta;
> +	u8  num_sensors;
> +	u8  sensor_index;
> +	u8  sensor_location;
> +	u8  therm_limit;
> +	u8  i;
> +	struct ixgbe_thermal_sensor_data *data = &hw->mac.thermal_sensor_data;
> +
> +	memset(data, 0, sizeof(struct ixgbe_thermal_sensor_data));
> +
> +	/* Only support thermal sensors attached to 82599 physical port 0 */
> +	if ((hw->mac.type != ixgbe_mac_82599EB) ||
> +	    (IXGBE_READ_REG(hw, IXGBE_STATUS) & IXGBE_STATUS_LAN_ID_1))
> +		return IXGBE_NOT_IMPLEMENTED;
> +
> +	hw->eeprom.ops.read(hw, IXGBE_ETS_CFG, &ets_offset);
> +	if ((ets_offset == 0x0000) || (ets_offset == 0xFFFF))
> +		return IXGBE_NOT_IMPLEMENTED;
> +
> +	hw->eeprom.ops.read(hw, ets_offset, &ets_cfg);
> +	if (((ets_cfg & IXGBE_ETS_TYPE_MASK) >> IXGBE_ETS_TYPE_SHIFT)
> +	    != IXGBE_ETS_TYPE_EMC)
            ^^ sic

> +		return IXGBE_NOT_IMPLEMENTED;
> +
> +	low_thresh_delta = ((ets_cfg & IXGBE_ETS_LTHRES_DELTA_MASK) >>
> +			     IXGBE_ETS_LTHRES_DELTA_SHIFT);
> +	num_sensors = (ets_cfg & IXGBE_ETS_NUM_SENSORS_MASK);
> +
> +	for (i = 0; i < num_sensors; i++) {
> +		hw->eeprom.ops.read(hw, (ets_offset + 1 + i), &ets_sensor);
> +		sensor_index = ((ets_sensor & IXGBE_ETS_DATA_INDEX_MASK) >>
> +				IXGBE_ETS_DATA_INDEX_SHIFT);
> +		sensor_location = ((ets_sensor & IXGBE_ETS_DATA_LOC_MASK) >>
> +				   IXGBE_ETS_DATA_LOC_SHIFT);
> +		therm_limit = ets_sensor & IXGBE_ETS_DATA_HTHRESH_MASK;
> +
> +		hw->phy.ops.write_i2c_byte(hw,
> +			ixgbe_emc_therm_limit[sensor_index],
> +			IXGBE_I2C_THERMAL_SENSOR_ADDR, therm_limit);
> +
> +		if ((i < IXGBE_MAX_SENSORS) && (sensor_location != 0)) {

The patch would be easier to review if num_sensors was capped as in
ixgbe_get_thermal_sensor_data_generic...

> +			data->sensor[i].location = sensor_location;
> +			data->sensor[i].caution_thresh = therm_limit;
> +			data->sensor[i].max_op_thresh = therm_limit -
> +							low_thresh_delta;

... and the code would gain an extra tab level, thus avoiding the line
break if there was a 'continue' statement when sensor_location fails the
test.

-- 
Ueimor

^ permalink raw reply

* Re: [RFC PATCH 00/18] netfilter: IPv6 NAT
From: Pablo Neira Ayuso @ 2011-12-23 13:08 UTC (permalink / raw)
  To: kaber; +Cc: netfilter-devel, netdev, ulrich.weber
In-Reply-To: <1322153850-10533-1-git-send-email-kaber@trash.net>

Hi Patrick,

On Thu, Nov 24, 2011 at 05:57:11PM +0100, kaber@trash.net wrote:
> The following patches contain the updated IPv6 NAT patchset forward ported
> to 3.2-rc3.

I've applied patches from 1 to 7. They are independent of the
IPv6 NAT support so they are safe to be applied.

Please, rebase your tree once they have hit davem's tree. Make sure
you rebase your tree before final submission of the IPv6 NAT support.

Thanks!

^ permalink raw reply

* Re: [PATCH v2] rps: fix insufficient bounds checking in store_rps_dev_flow_table_cnt()
From: Eric Dumazet @ 2011-12-23 13:06 UTC (permalink / raw)
  To: Xi Wang; +Cc: Tom Herbert, David S. Miller, netdev
In-Reply-To: <4DA4756B-0654-49F4-B135-9A9F89BC7D21@gmail.com>

Le vendredi 23 décembre 2011 à 00:56 -0500, Xi Wang a écrit :
> On Dec 23, 2011, at 12:35 AM, Eric Dumazet wrote:
> > By the way, the theorical limit on number of flows on 64bit platform is
> > 2^32  (rxhash being an u32)
> > 
> > Not sure spending 32GB per table would be wise for typical machines :)
> 
> True, a large rps_flow_cnt is not that useful. ;-)
> 
> Anyway, my point is that if the patch looks confusing to you, then
> it is probably not good.  I am happy to see a more elegant fix.
> 

I'll submit following patch for net-next, once your patch is in this
tree.

- Full u32 range on 64bit arches (2^32 flows max)
- Use of kstrtoul() instead of simple_strtoul()

 net/core/net-sysfs.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 385aefe..63bd152 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -618,7 +618,7 @@ static ssize_t show_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 					   char *buf)
 {
 	struct rps_dev_flow_table *flow_table;
-	unsigned int val = 0;
+	unsigned long val = 0;
 
 	rcu_read_lock();
 	flow_table = rcu_dereference(queue->rps_flow_table);
@@ -626,7 +626,7 @@ static ssize_t show_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 		val = flow_table->mask + 1;
 	rcu_read_unlock();
 
-	return sprintf(buf, "%u\n", val);
+	return sprintf(buf, "%lu\n", val);
 }
 
 static void rps_dev_flow_table_release_work(struct work_struct *work)
@@ -650,24 +650,23 @@ static ssize_t store_rps_dev_flow_table_cnt(struct netdev_rx_queue *queue,
 				     struct rx_queue_attribute *attr,
 				     const char *buf, size_t len)
 {
-	unsigned int count;
-	char *endp;
+	unsigned long i, count;
 	struct rps_dev_flow_table *table, *old_table;
 	static DEFINE_SPINLOCK(rps_dev_flow_lock);
+	int rc;
 
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	count = simple_strtoul(buf, &endp, 0);
-	if (endp == buf)
-		return -EINVAL;
+	rc = kstrtoul(buf, 0, &count);
+	if (rc < 0)
+		return rc;
 
 	if (count) {
-		int i;
-
-		if (count > INT_MAX)
-			return -EINVAL;
 		count = roundup_pow_of_two(count);
+		if (!count ||
+		    count != (unsigned long)(u32)count)
+			return -EINVAL;
 		if (count > (ULONG_MAX - sizeof(struct rps_dev_flow_table))
 				/ sizeof(struct rps_dev_flow)) {
 			/* Enforce a limit to prevent overflow */

^ permalink raw reply related

* Re: [RFC] iproute2: split up ip man page?
From: Hagen Paul Pfeifer @ 2011-12-23 11:57 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: netdev
In-Reply-To: <20111222162959.167b9df0@nehalam.linuxnetplumber.net>

* Stephen Hemminger | 2011-12-22 16:29:59 [-0800]:

>I am fixing netem loss stuff now. Fixing broken gimodel.

Great news, I have some pending patches for the gimodel too.

Hagen

^ permalink raw reply

* Re: [net-next] stmmac: fix missing module license in the main.
From: Giuseppe CAVALLARO @ 2011-12-23 11:16 UTC (permalink / raw)
  To: linux-next; +Cc: netdev, Randy Dunlap, LKML, Stephen Rothwell, David S. Miller
In-Reply-To: <1324636267-25533-1-git-send-email-peppe.cavallaro@st.com>

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

Sorry I had forgotten to add you on copy.

Peppe

p.s. patch attached here too.

On 12/23/2011 11:31 AM, Giuseppe CAVALLARO wrote:
> This patch fixes the following warning raised
> when compile:
> 
> WARNING: modpost: missing MODULE_LICENSE()
> in drivers/net/ethernet/stmicro/stmmac/stmmac.o
> 
> Reported-by: Randy Dunlap <rdunlap@xenotime.net>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b314592..3738b47 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -2039,3 +2039,7 @@ err:
>  
>  __setup("stmmaceth=", stmmac_cmdline_opt);
>  #endif
> +
> +MODULE_DESCRIPTION("STMMAC 10/100/1000 Ethernet device driver");
> +MODULE_AUTHOR("Giuseppe Cavallaro <peppe.cavallaro@st.com>");
> +MODULE_LICENSE("GPL");



[-- Attachment #2: 0001-stmmac-fix-missing-module-license-in-the-main.patch --]
[-- Type: text/x-patch, Size: 1114 bytes --]

>From c24fa5e2784f85e6170a495bc42dea9a854cb3da Mon Sep 17 00:00:00 2001
From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Fri, 23 Dec 2011 11:25:13 +0100
Subject: [net-next] stmmac: fix missing module license in the main.

This patch fixes the following warning raised
when compile:

WARNING: modpost: missing MODULE_LICENSE()
in drivers/net/ethernet/stmicro/stmmac/stmmac.o

Reported-by: Randy Dunlap <rdunlap@xenotime.net>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b314592..3738b47 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2039,3 +2039,7 @@ err:
 
 __setup("stmmaceth=", stmmac_cmdline_opt);
 #endif
+
+MODULE_DESCRIPTION("STMMAC 10/100/1000 Ethernet device driver");
+MODULE_AUTHOR("Giuseppe Cavallaro <peppe.cavallaro@st.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.4.4


^ permalink raw reply related

* Re: igb + balance-rr + bridge + IPv6 = no go without promiscuous mode
From: Chris Boot @ 2011-12-23 10:56 UTC (permalink / raw)
  To: Nicolas de Pesloüan; +Cc: netdev
In-Reply-To: <4EF45C7D.8090409@gmail.com>

On 23/12/2011 10:48, Nicolas de Pesloüan wrote:
> [ Forwarded to netdev, because two previous e-mail erroneously sent in 
> HTML ]
>
> Le 23/12/2011 11:15, Chris Boot a écrit :
>> On 23/12/2011 09:52, Nicolas de Pesloüan wrote:
>>>
>>>
>>> Le 23 déc. 2011 10:42, "Chris Boot" <bootc@bootc.net 
>>> <mailto:bootc@bootc.net>> a écrit :
>>> >
>>> > Hi folks,
>>> >
>>> > As per Eric Dumazet and Dave Miller, I'm opening up a separate 
>>> thread on this issue.
>>> >
>>> > I have two identical servers in a cluster for running KVM virtual 
>>> machines. They each have a
>>> single connection to the Internet (irrelevant for this) and two 
>>> gigabit connections between each
>>> other for cluster replication, etc... These two connections are in a 
>>> balance-rr bonded connection,
>>> which is itself member of a bridge that the VMs attach to. I'm 
>>> running v3.2-rc6-140-gb9e26df on
>>> Debian Wheezy.
>>> >
>>> > When the bridge is brought up, IPv4 works fine but IPv6 does not. 
>>> I can use neither the
>>> automatic link-local on the brid ge nor the static global address I 
>>> assign. Neither machine can
>>> perform neighbour discovery over the link until I put the bond 
>>> members (eth0 and eth1) into
>>> promiscuous mode.  I can do this either with tcpdump or 'ip link set 
>>> dev ethX promisc on' and this
>>> is enough to make the link spring to life.
>>>
>>> For as far as I remember, setting bond0 to promisc should set the 
>>> bonding member to promisc too.
>>> And inserting bond0 into br0 should set bond0 to promisc... So 
>>> everything should be in promisc
>>> mode anyway... but you shoudn't have to do it by hand.
>>>
>>
>> Sorry, I should have added that I tried this. Setting bond0 or br0 to 
>> promisc has no effect. I
>> discovered this by running tcpdump on br0 first, then bond0, then 
>> eventually each bond member in
>> turn. Only at the last stage did things jump to life.
>>
>>> >
>>> > This cluster is not currently live so I can easily test patches 
>>> and various configurations.
>>>
>>> Can you try to remove the bonding part, connecting eth0 and eth1 
>>> directly to br0 and see if it
>>> works better? (This is a test ony. I perfectly understand that you 
>>> would loose balance-rr in this
>>> setup.)
>>>
>>
>> Good call. Let's see.
>>
>> I took br0 and bond0 apart, took eth0 and eth1 out of enforced 
>> promisc mode, then manually built a
>> br0 with eth0 in only so I didn't cause a network loop. Adding eth0 
>> to br0 did not make it go into
>> promisc mode, but IPv6 does work over this setup. I also made sure ip 
>> -6 neigh was empty on both
>> machines before I started.
>>
>> I then decided to try the test with just the bond0 in balance-rr 
>> mode. Once again I took everything
>> down and ensured no promisc mode and no ip -6 neigh. I noticed bond0 
>> wasn't getting a link-local and
>> I found out for some reason 
>> /proc/sys/net/ipv6/conf/bond0/disable_ipv6 was set on both servers so I
>> set it to 0. That brought things to life.
>>
>> So then I put it all back together again and it didn't work. I once 
>> again noticed disable_ipv6 was
>> set on the bond0 interfaces, now part of the bridge. Toggling this on 
>> the _bond_ interface made
>> things work again.
>>
>> What's setting disable_ipv6? Should this be having an impact if the 
>> port is part of a bridge?

Hmm, as a further update... I brought up my VMs on the bridge with 
disable_ipv6 turned off. The VMs on one host couldn't see what was on 
the other side of the bridge (on the other server) until I turned 
promisc back on manually. So it's not entirely disable_ipv6's fault.

Chris

-- 
Chris Boot
bootc@bootc.net

^ permalink raw reply

* Re: igb + balance-rr + bridge + IPv6 = no go without promiscuous mode
From: Nicolas de Pesloüan @ 2011-12-23 10:48 UTC (permalink / raw)
  To: netdev; +Cc: Chris Boot
In-Reply-To: <4EF454C7.8020305@bootc.net>

[ Forwarded to netdev, because two previous e-mail erroneously sent in HTML ]

Le 23/12/2011 11:15, Chris Boot a écrit :
> On 23/12/2011 09:52, Nicolas de Pesloüan wrote:
>>
>>
>> Le 23 déc. 2011 10:42, "Chris Boot" <bootc@bootc.net <mailto:bootc@bootc.net>> a écrit :
>> >
>> > Hi folks,
>> >
>> > As per Eric Dumazet and Dave Miller, I'm opening up a separate thread on this issue.
>> >
>> > I have two identical servers in a cluster for running KVM virtual machines. They each have a
>> single connection to the Internet (irrelevant for this) and two gigabit connections between each
>> other for cluster replication, etc... These two connections are in a balance-rr bonded connection,
>> which is itself member of a bridge that the VMs attach to. I'm running v3.2-rc6-140-gb9e26df on
>> Debian Wheezy.
>> >
>> > When the bridge is brought up, IPv4 works fine but IPv6 does not. I can use neither the
>> automatic link-local on the brid ge nor the static global address I assign. Neither machine can
>> perform neighbour discovery over the link until I put the bond members (eth0 and eth1) into
>> promiscuous mode.  I can do this either with tcpdump or 'ip link set dev ethX promisc on' and this
>> is enough to make the link spring to life.
>>
>> For as far as I remember, setting bond0 to promisc should set the bonding member to promisc too.
>> And inserting bond0 into br0 should set bond0 to promisc... So everything should be in promisc
>> mode anyway... but you shoudn't have to do it by hand.
>>
>
> Sorry, I should have added that I tried this. Setting bond0 or br0 to promisc has no effect. I
> discovered this by running tcpdump on br0 first, then bond0, then eventually each bond member in
> turn. Only at the last stage did things jump to life.
>
>> >
>> > This cluster is not currently live so I can easily test patches and various configurations.
>>
>> Can you try to remove the bonding part, connecting eth0 and eth1 directly to br0 and see if it
>> works better? (This is a test ony. I perfectly understand that you would loose balance-rr in this
>> setup.)
>>
>
> Good call. Let's see.
>
> I took br0 and bond0 apart, took eth0 and eth1 out of enforced promisc mode, then manually built a
> br0 with eth0 in only so I didn't cause a network loop. Adding eth0 to br0 did not make it go into
> promisc mode, but IPv6 does work over this setup. I also made sure ip -6 neigh was empty on both
> machines before I started.
>
> I then decided to try the test with just the bond0 in balance-rr mode. Once again I took everything
> down and ensured no promisc mode and no ip -6 neigh. I noticed bond0 wasn't getting a link-local and
> I found out for some reason /proc/sys/net/ipv6/conf/bond0/disable_ipv6 was set on both servers so I
> set it to 0. That brought things to life.
>
> So then I put it all back together again and it didn't work. I once again noticed disable_ipv6 was
> set on the bond0 interfaces, now part of the bridge. Toggling this on the _bond_ interface made
> things work again.
>
> What's setting disable_ipv6? Should this be having an impact if the port is part of a bridge?
>
> Chris
>
> --
> Chris Boot
> bootc@bootc.net
>

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox