* Re: [PATCH 0/2] adding tracepoints to vhost
From: Stefan Hajnoczi @ 2012-04-10 12:54 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, netdev, linux-kernel, virtualization
In-Reply-To: <20120410124250.GB29808@redhat.com>
On Tue, Apr 10, 2012 at 1:42 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Apr 10, 2012 at 12:40:50PM +0100, Stefan Hajnoczi wrote:
>> On Tue, Apr 10, 2012 at 3:58 AM, Jason Wang <jasowang@redhat.com> wrote:
>> > To help in vhost analyzing, the following series adding basic tracepoints to
>> > vhost. Operations of both virtqueues and vhost works were traced in current
>> > implementation, net code were untouched. A top-like satistics displaying script
>> > were introduced to help the troubleshooting.
>> >
>> > TODO:
>> > - net specific tracepoints?
>> >
>> > ---
>> >
>> > Jason Wang (2):
>> > vhost: basic tracepoints
>> > tools: virtio: add a top-like utility for displaying vhost satistics
>> >
>> >
>> > drivers/vhost/trace.h | 153 ++++++++++++++++++++
>> > drivers/vhost/vhost.c | 17 ++
>> > tools/virtio/vhost_stat | 360 +++++++++++++++++++++++++++++++++++++++++++++++
>> > 3 files changed, 528 insertions(+), 2 deletions(-)
>> > create mode 100644 drivers/vhost/trace.h
>> > create mode 100755 tools/virtio/vhost_stat
>>
>> Perhaps this can replace the vhost log feature? I'm not sure if
>> tracepoints support the right data types but it seems like vhost
>> debugging could be done using tracing with less code.
>>
>> Stefan
>
> vhost log is not a debugging tool, it logs memory accesses for
> migration.
Thanks. I totally misunderstood its purpose.
Stefan
^ permalink raw reply
* [PATCH 5/5] netfilter: nf_conntrack: fix incorrect logic in nf_conntrack_init_net
From: pablo @ 2012-04-10 12:49 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1334062141-3962-1-git-send-email-pablo@netfilter.org>
From: Gao feng <gaofeng@cn.fujitsu.com>
in function nf_conntrack_init_net,when nf_conntrack_timeout_init falied,
we should call nf_conntrack_ecache_fini to do rollback.
but the current code calls nf_conntrack_timeout_fini.
Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_core.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 3cc4487..729f157 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1592,7 +1592,7 @@ static int nf_conntrack_init_net(struct net *net)
return 0;
err_timeout:
- nf_conntrack_timeout_fini(net);
+ nf_conntrack_ecache_fini(net);
err_ecache:
nf_conntrack_tstamp_fini(net);
err_tstamp:
--
1.7.2.5
^ permalink raw reply related
* [PATCH 2/5] netfilter: ip6_tables: ip6t_ext_hdr is now static inline
From: pablo @ 2012-04-10 12:48 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1334062141-3962-1-git-send-email-pablo@netfilter.org>
From: Pablo Neira Ayuso <pablo@netfilter.org>
We may hit this in xt_LOG:
net/built-in.o:xt_LOG.c:function dump_ipv6_packet:
error: undefined reference to 'ip6t_ext_hdr'
happens with these config options:
CONFIG_NETFILTER_XT_TARGET_LOG=y
CONFIG_IP6_NF_IPTABLES=m
ip6t_ext_hdr is fairly small and it is called in the packet path.
Make it static inline.
Reported-by: Simon Kirby <sim@netnation.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
include/linux/netfilter_ipv6/ip6_tables.h | 12 +++++++++++-
net/ipv6/netfilter/ip6_tables.c | 14 --------------
2 files changed, 11 insertions(+), 15 deletions(-)
diff --git a/include/linux/netfilter_ipv6/ip6_tables.h b/include/linux/netfilter_ipv6/ip6_tables.h
index f549adc..1bc898b 100644
--- a/include/linux/netfilter_ipv6/ip6_tables.h
+++ b/include/linux/netfilter_ipv6/ip6_tables.h
@@ -287,7 +287,17 @@ extern unsigned int ip6t_do_table(struct sk_buff *skb,
struct xt_table *table);
/* Check for an extension */
-extern int ip6t_ext_hdr(u8 nexthdr);
+static inline int
+ip6t_ext_hdr(u8 nexthdr)
+{ return (nexthdr == IPPROTO_HOPOPTS) ||
+ (nexthdr == IPPROTO_ROUTING) ||
+ (nexthdr == IPPROTO_FRAGMENT) ||
+ (nexthdr == IPPROTO_ESP) ||
+ (nexthdr == IPPROTO_AH) ||
+ (nexthdr == IPPROTO_NONE) ||
+ (nexthdr == IPPROTO_DSTOPTS);
+}
+
/* find specified header and get offset to it */
extern int ipv6_find_hdr(const struct sk_buff *skb, unsigned int *offset,
int target, unsigned short *fragoff);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 94874b0..9d4e155 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -78,19 +78,6 @@ EXPORT_SYMBOL_GPL(ip6t_alloc_initial_table);
Hence the start of any table is given by get_table() below. */
-/* Check for an extension */
-int
-ip6t_ext_hdr(u8 nexthdr)
-{
- return (nexthdr == IPPROTO_HOPOPTS) ||
- (nexthdr == IPPROTO_ROUTING) ||
- (nexthdr == IPPROTO_FRAGMENT) ||
- (nexthdr == IPPROTO_ESP) ||
- (nexthdr == IPPROTO_AH) ||
- (nexthdr == IPPROTO_NONE) ||
- (nexthdr == IPPROTO_DSTOPTS);
-}
-
/* Returns whether matches rule or not. */
/* Performance critical - called for every packet */
static inline bool
@@ -2366,7 +2353,6 @@ int ipv6_find_hdr(const struct sk_buff *skb, unsigned int *offset,
EXPORT_SYMBOL(ip6t_register_table);
EXPORT_SYMBOL(ip6t_unregister_table);
EXPORT_SYMBOL(ip6t_do_table);
-EXPORT_SYMBOL(ip6t_ext_hdr);
EXPORT_SYMBOL(ipv6_find_hdr);
module_init(ip6_tables_init);
--
1.7.2.5
^ permalink raw reply related
* [PATCH 4/5] netfilter: nf_ct_ipv4: packets with wrong ihl are invalid
From: pablo @ 2012-04-10 12:49 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1334062141-3962-1-git-send-email-pablo@netfilter.org>
From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
It was reported that the Linux kernel sometimes logs:
klogd: [2629147.402413] kernel BUG at net / netfilter /
nf_conntrack_proto_tcp.c: 447!
klogd: [1072212.887368] kernel BUG at net / netfilter /
nf_conntrack_proto_tcp.c: 392
ipv4_get_l4proto() in nf_conntrack_l3proto_ipv4.c and tcp_error() in
nf_conntrack_proto_tcp.c should catch malformed packets, so the errors
at the indicated lines - TCP options parsing - should not happen.
However, tcp_error() relies on the "dataoff" offset to the TCP header,
calculated by ipv4_get_l4proto(). But ipv4_get_l4proto() does not check
bogus ihl values in IPv4 packets, which then can slip through tcp_error()
and get caught at the TCP options parsing routines.
The patch fixes ipv4_get_l4proto() by invalidating packets with bogus
ihl value.
The patch closes netfilter bugzilla id 771.
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index 750b06a..cf73cc7 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -84,6 +84,14 @@ static int ipv4_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
*dataoff = nhoff + (iph->ihl << 2);
*protonum = iph->protocol;
+ /* Check bogus IP headers */
+ if (*dataoff > skb->len) {
+ pr_debug("nf_conntrack_ipv4: bogus IPv4 packet: "
+ "nhoff %u, ihl %u, skblen %u\n",
+ nhoff, iph->ihl << 2, skb->len);
+ return -NF_ACCEPT;
+ }
+
return NF_ACCEPT;
}
--
1.7.2.5
^ permalink raw reply related
* [PATCH 3/5] netfilter: nf_ct_ipv4: handle invalid IPv4 and IPv6 packets consistently
From: pablo @ 2012-04-10 12:48 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1334062141-3962-1-git-send-email-pablo@netfilter.org>
From: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
IPv6 conntrack marked invalid packets as INVALID and let the user
drop those by an explicit rule, while IPv4 conntrack dropped such
packets itself.
IPv4 conntrack is changed so that it marks INVALID packets and let
the user to drop them.
Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index de9da21..750b06a 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -74,12 +74,12 @@ static int ipv4_get_l4proto(const struct sk_buff *skb, unsigned int nhoff,
iph = skb_header_pointer(skb, nhoff, sizeof(_iph), &_iph);
if (iph == NULL)
- return -NF_DROP;
+ return -NF_ACCEPT;
/* Conntrack defragments packets, we might still see fragments
* inside ICMP packets though. */
if (iph->frag_off & htons(IP_OFFSET))
- return -NF_DROP;
+ return -NF_ACCEPT;
*dataoff = nhoff + (iph->ihl << 2);
*protonum = iph->protocol;
--
1.7.2.5
^ permalink raw reply related
* [PATCH 1/5] netfilter: nf_ct_tcp: don't scale the size of the window up twice
From: pablo @ 2012-04-10 12:48 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
In-Reply-To: <1334062141-3962-1-git-send-email-pablo@netfilter.org>
From: Changli Gao <xiaosuo@gmail.com>
For a picked up connection, the window win is scaled twice: one is by the
initialization code, and the other is by the sender updating code.
I use the temporary variable swin instead of modifying the variable win.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
net/netfilter/nf_conntrack_proto_tcp.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 361eade..0d07a1d 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -584,8 +584,8 @@ static bool tcp_in_window(const struct nf_conn *ct,
* Let's try to use the data from the packet.
*/
sender->td_end = end;
- win <<= sender->td_scale;
- sender->td_maxwin = (win == 0 ? 1 : win);
+ swin = win << sender->td_scale;
+ sender->td_maxwin = (swin == 0 ? 1 : swin);
sender->td_maxend = end + sender->td_maxwin;
/*
* We haven't seen traffic in the other direction yet
--
1.7.2.5
^ permalink raw reply related
* [PATCH 0/5] netfilter fixes for 3.4-rc2
From: pablo @ 2012-04-10 12:48 UTC (permalink / raw)
To: netfilter-devel; +Cc: davem, netdev
From: Pablo Neira Ayuso <pablo@netfilter.org>
Hi David,
The following patchset includes netfilter fixes for 3.4-rc2, they are:
* A couple of fixes for the IPv4 connection tracker from Jozsef. One
to behave consistently with IPv6 and to follow the conntrack policy
(ie. don't drop, the user controls what to do by dropping invalid
packet via iptables). The other one checks for invalid IPv4 ihl
values that go further the packet boundary.
* Fix missing ip6t_ext_hdr symbol if ip6tables is compiled xt_LOG
is compiled built-in and ip6tables as module by myself.
* One fix for the error path of nf_conntrack_init_net introduced by
the recently added nf_conntrack_timeout infrastructure from Gao Feng.
* We don't want to scale the window twice for picked up connection in
the nf_ct_tcp code, from Changli Gao.
You can pull changes these from:
git://1984.lsi.us.es/net master
Changli Gao (1):
netfilter: nf_ct_tcp: don't scale the size of the window up twice
Gao feng (1):
netfilter: nf_conntrack: fix incorrect logic in nf_conntrack_init_net
Jozsef Kadlecsik (2):
netfilter: nf_ct_ipv4: handle invalid IPv4 and IPv6 packets consistently
netfilter: nf_ct_ipv4: packets with wrong ihl are invalid
Pablo Neira Ayuso (1):
netfilter: ip6_tables: ip6t_ext_hdr is now static inline
include/linux/netfilter_ipv6/ip6_tables.h | 12 +++++++++++-
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c | 12 ++++++++++--
net/ipv6/netfilter/ip6_tables.c | 14 --------------
net/netfilter/nf_conntrack_core.c | 2 +-
net/netfilter/nf_conntrack_proto_tcp.c | 4 ++--
5 files changed, 24 insertions(+), 20 deletions(-)
--
1.7.2.5
^ permalink raw reply
* Re: [PATCH 0/2] adding tracepoints to vhost
From: Michael S. Tsirkin @ 2012-04-10 12:42 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <CAJSP0QWA1z6_YQe9hhpG7bCyZQarmU5NXbHsLGTL_zPTk3dOUA@mail.gmail.com>
On Tue, Apr 10, 2012 at 12:40:50PM +0100, Stefan Hajnoczi wrote:
> On Tue, Apr 10, 2012 at 3:58 AM, Jason Wang <jasowang@redhat.com> wrote:
> > To help in vhost analyzing, the following series adding basic tracepoints to
> > vhost. Operations of both virtqueues and vhost works were traced in current
> > implementation, net code were untouched. A top-like satistics displaying script
> > were introduced to help the troubleshooting.
> >
> > TODO:
> > - net specific tracepoints?
> >
> > ---
> >
> > Jason Wang (2):
> > vhost: basic tracepoints
> > tools: virtio: add a top-like utility for displaying vhost satistics
> >
> >
> > drivers/vhost/trace.h | 153 ++++++++++++++++++++
> > drivers/vhost/vhost.c | 17 ++
> > tools/virtio/vhost_stat | 360 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 528 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/vhost/trace.h
> > create mode 100755 tools/virtio/vhost_stat
>
> Perhaps this can replace the vhost log feature? I'm not sure if
> tracepoints support the right data types but it seems like vhost
> debugging could be done using tracing with less code.
>
> Stefan
vhost log is not a debugging tool, it logs memory accesses for
migration.
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Michael S. Tsirkin @ 2012-04-10 12:41 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Michał Mirosław, Ben Hutchings,
Herbert Xu
In-Reply-To: <1334058300.3126.99.camel@edumazet-glaptop>
On Tue, Apr 10, 2012 at 01:45:00PM +0200, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 14:25 +0300, Michael S. Tsirkin wrote:
> > On Tue, Apr 10, 2012 at 12:04:19PM +0200, Eric Dumazet wrote:
> > > On Tue, 2012-04-10 at 12:31 +0300, Michael S. Tsirkin wrote:
> > >
> > > > True. Still this is the only interface we have for controlling
> > > > the internal queue length so it seems safe to assume someone
> > > > is using it for this purpose.
> > > >
> > >
> > > So to workaround a problem in tun, you want to hack net/core/dev.c :(
> >
> > Sorry about being unclear, I'm just saying that your patch assumes
> > tx_queue_len == 0 since you set it that way at device init but we can't
> > rely on this as existing users might have changed that value.
> > One way to fix would be a patch at the bottom: then we
> > can leave tun to treat tx_queue_len like it always did.
>
>
> > ----
> >
> > We don't want a queue for tun since it can stall forever, but userspace
> > might tweak it's tx_queue_len as a way to control RX queue depth,
> > and we don't want to break userspace. Use a private flag to disable queue.
> >
> > Warning: untested.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index 27883d1..644ca53 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -695,7 +692,7 @@ static void attach_one_default_qdisc(struct net_device *dev,
> > {
> > struct Qdisc *qdisc = &noqueue_qdisc;
> >
> > - if (dev->tx_queue_len) {
> > + if (dev->tx_queue_len && !(dev->priv_flags & IFF_TX_CAN_STALL)) {
> > qdisc = qdisc_create_dflt(dev_queue,
> > &pfifo_fast_ops, TC_H_ROOT);
> > if (!qdisc) {
>
>
> Thing is this function is called before userspace can tweak tx_queue_len
>
> So if you create a vlan device (this sets tx_queue_len to 0), no qdisc
> is attached.
>
> If later userspace changes tx_queue_len to this device, qdisc wont
> automatically be created/attached.
True. But there's another place where this can happen - after
dev_change_net_namespace, no?
This calls dev_shutdown.
> Really, tx_queue_len is private to net/sched layer, it should not be
> used by tun device to control a receive queue limit.
>
> Please try to not hack net/sched or net/core for your needs.
>
> Its not because tun abused tx_queue_len in the past we must keep this
> hack forever.
>
> In ethernet drivers, TX ring size is controlled by ethtool -g
>
> Why tun driver would use another way ?
>
I think it's a bad interface too but it's in a userspace ABI
now so I suspect we are stuck with it for now. We can try deprecating
but we can't just drop it.
--
MST
^ permalink raw reply
* Re: [PATCH] netfilter: fix incorrect logic in nf_conntrack_init_net
From: Pablo Neira Ayuso @ 2012-04-10 12:36 UTC (permalink / raw)
To: Gao feng; +Cc: netfilter-devel, netdev
In-Reply-To: <1333850908-3108-1-git-send-email-gaofeng@cn.fujitsu.com>
On Sun, Apr 08, 2012 at 10:08:28AM +0800, Gao feng wrote:
> in function nf_conntrack_init_net,when nf_conntrack_timeout_init falied,
> we should call nf_conntrack_ecache_fini to do rollback.
> but the current code calls nf_conntrack_timeout_fini.
>
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] netfilter: don't scale the size of the window up twice
From: Pablo Neira Ayuso @ 2012-04-10 12:35 UTC (permalink / raw)
To: Jozsef Kadlecsik
Cc: Patrick McHardy, David S. Miller, netfilter-devel, netdev,
Changli Gao
In-Reply-To: <alpine.DEB.2.00.1204042100390.21828@blackhole.kfki.hu>
On Wed, Apr 04, 2012 at 09:05:26PM +0200, Jozsef Kadlecsik wrote:
> On Wed, 4 Apr 2012, Pablo Neira Ayuso wrote:
>
> > On Mon, Apr 02, 2012 at 11:25:06AM +0800, Changli Gao wrote:
> > > For a picked up connection, the window win is scaled twice: one is by the
> > > initialization code, and the other is by the sender updating code.
> > >
> > > I use the temporary variable swin instead of modifying the variable win.
> > >
> > > Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> > > ---
> > > net/netfilter/nf_conntrack_proto_tcp.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
> > > index 361eade..0d07a1d 100644
> > > --- a/net/netfilter/nf_conntrack_proto_tcp.c
> > > +++ b/net/netfilter/nf_conntrack_proto_tcp.c
> > > @@ -584,8 +584,8 @@ static bool tcp_in_window(const struct nf_conn *ct,
> > > * Let's try to use the data from the packet.
> > > */
> > > sender->td_end = end;
> > > - win <<= sender->td_scale;
> > > - sender->td_maxwin = (win == 0 ? 1 : win);
> > > + swin = win << sender->td_scale;
> > > + sender->td_maxwin = (swin == 0 ? 1 : swin);
> > > sender->td_maxend = end + sender->td_maxwin;
> > > /*
> > > * We haven't seen traffic in the other direction yet
> >
> > Jozsef, do you remember if this is intentional?
>
> No, it's a good spotting.
>
> Acked-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Applied, thanks.
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Michal Simek @ 2012-04-10 12:12 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, John Williams, David Miller
In-Reply-To: <1334058625.3126.101.camel@edumazet-glaptop>
On 04/10/2012 01:50 PM, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 13:38 +0200, Michal Simek wrote:
>> On 04/10/2012 01:32 PM, Eric Dumazet wrote:
>>>
>>> ll_temac allocates XTE_MAX_JUMBO_FRAME_SIZE frames, even for MTU=1500 ?
>>
>> yes.
>>
>>>
>>> Trying to allocate smaller packets would definitely help your tcp
>>> receiver performance and overall reliability (order-0 allocations
>>> instead of order-2)
>>
>> There is long history for ll_temac and axi emac drivers. and if you compare
>> them they are very similar with similar bugs.
>> In our distribution we are using older ll_temac driver where this is fixed.
>> Performance is much higher if you allocate buffers for actual mtu.
>>
>> Thanks,
>> Michal
>>
>> P.S.: Our plan is to start to use mainline drivers and fixing them to get
>> the same performance as we have with old one. It is one my todo list.
>
> Ah good, because this 9000+pad versus 1500+pad allocation is way more
> problematic than the (small) change on skb->truesize you bisected ;)
yeah I know. Currently for me is the most important thing to have stable
driver which is ancient one and start to work on mainline to add there all features
and do more testing before we will add it to customers.
I have found it because I have compared the same driver with different kernel
versions and there was performance regression. Comparing with old one will show
different results. :-)
I will keep you in the loop when I start to fix mainline driver.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Eric Dumazet @ 2012-04-10 11:50 UTC (permalink / raw)
To: monstr; +Cc: netdev, John Williams, David Miller
In-Reply-To: <4F841BB9.2030209@monstr.eu>
On Tue, 2012-04-10 at 13:38 +0200, Michal Simek wrote:
> On 04/10/2012 01:32 PM, Eric Dumazet wrote:
> >
> > ll_temac allocates XTE_MAX_JUMBO_FRAME_SIZE frames, even for MTU=1500 ?
>
> yes.
>
> >
> > Trying to allocate smaller packets would definitely help your tcp
> > receiver performance and overall reliability (order-0 allocations
> > instead of order-2)
>
> There is long history for ll_temac and axi emac drivers. and if you compare
> them they are very similar with similar bugs.
> In our distribution we are using older ll_temac driver where this is fixed.
> Performance is much higher if you allocate buffers for actual mtu.
>
> Thanks,
> Michal
>
> P.S.: Our plan is to start to use mainline drivers and fixing them to get
> the same performance as we have with old one. It is one my todo list.
Ah good, because this 9000+pad versus 1500+pad allocation is way more
problematic than the (small) change on skb->truesize you bisected ;)
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Eric Dumazet @ 2012-04-10 11:45 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Michał Mirosław, Ben Hutchings,
Herbert Xu
In-Reply-To: <20120410112459.GA28825@redhat.com>
On Tue, 2012-04-10 at 14:25 +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 12:04:19PM +0200, Eric Dumazet wrote:
> > On Tue, 2012-04-10 at 12:31 +0300, Michael S. Tsirkin wrote:
> >
> > > True. Still this is the only interface we have for controlling
> > > the internal queue length so it seems safe to assume someone
> > > is using it for this purpose.
> > >
> >
> > So to workaround a problem in tun, you want to hack net/core/dev.c :(
>
> Sorry about being unclear, I'm just saying that your patch assumes
> tx_queue_len == 0 since you set it that way at device init but we can't
> rely on this as existing users might have changed that value.
> One way to fix would be a patch at the bottom: then we
> can leave tun to treat tx_queue_len like it always did.
> ----
>
> We don't want a queue for tun since it can stall forever, but userspace
> might tweak it's tx_queue_len as a way to control RX queue depth,
> and we don't want to break userspace. Use a private flag to disable queue.
>
> Warning: untested.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 27883d1..644ca53 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -695,7 +692,7 @@ static void attach_one_default_qdisc(struct net_device *dev,
> {
> struct Qdisc *qdisc = &noqueue_qdisc;
>
> - if (dev->tx_queue_len) {
> + if (dev->tx_queue_len && !(dev->priv_flags & IFF_TX_CAN_STALL)) {
> qdisc = qdisc_create_dflt(dev_queue,
> &pfifo_fast_ops, TC_H_ROOT);
> if (!qdisc) {
Thing is this function is called before userspace can tweak tx_queue_len
So if you create a vlan device (this sets tx_queue_len to 0), no qdisc
is attached.
If later userspace changes tx_queue_len to this device, qdisc wont
automatically be created/attached.
Really, tx_queue_len is private to net/sched layer, it should not be
used by tun device to control a receive queue limit.
Please try to not hack net/sched or net/core for your needs.
Its not because tun abused tx_queue_len in the past we must keep this
hack forever.
In ethernet drivers, TX ring size is controlled by ethtool -g
Why tun driver would use another way ?
^ permalink raw reply
* Re: [PATCH 0/2] adding tracepoints to vhost
From: Stefan Hajnoczi @ 2012-04-10 11:40 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, mst, linux-kernel, kvm, virtualization
In-Reply-To: <20120410025327.49693.93562.stgit@amd-6168-8-1.englab.nay.redhat.com>
On Tue, Apr 10, 2012 at 3:58 AM, Jason Wang <jasowang@redhat.com> wrote:
> To help in vhost analyzing, the following series adding basic tracepoints to
> vhost. Operations of both virtqueues and vhost works were traced in current
> implementation, net code were untouched. A top-like satistics displaying script
> were introduced to help the troubleshooting.
>
> TODO:
> - net specific tracepoints?
>
> ---
>
> Jason Wang (2):
> vhost: basic tracepoints
> tools: virtio: add a top-like utility for displaying vhost satistics
>
>
> drivers/vhost/trace.h | 153 ++++++++++++++++++++
> drivers/vhost/vhost.c | 17 ++
> tools/virtio/vhost_stat | 360 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 528 insertions(+), 2 deletions(-)
> create mode 100644 drivers/vhost/trace.h
> create mode 100755 tools/virtio/vhost_stat
Perhaps this can replace the vhost log feature? I'm not sure if
tracepoints support the right data types but it seems like vhost
debugging could be done using tracing with less code.
Stefan
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Michal Simek @ 2012-04-10 11:38 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, John Williams, David Miller
In-Reply-To: <1334057529.3126.90.camel@edumazet-glaptop>
On 04/10/2012 01:32 PM, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 10:37 +0200, Michal Simek wrote:
>> On 04/10/2012 10:32 AM, Eric Dumazet wrote:
>>> On Tue, 2012-04-10 at 10:27 +0200, Eric Dumazet wrote:
>>>
>>>> BTW, some NIC drivers are known to provide fat skb in their rx path, and
>>>> need to be fixed as well. (Some others just lie about skb->truesize to
>>>> avoid the tcp slowdown, see my previous iwlwifi patch)
>>>>
>>>>
>>>
>>> What is the driver you currently use on your platform ?
>>
>> Using Xilinx ll_temac(in mainline) and axi_emac.
>>
>
> ll_temac allocates XTE_MAX_JUMBO_FRAME_SIZE frames, even for MTU=1500 ?
yes.
>
> Trying to allocate smaller packets would definitely help your tcp
> receiver performance and overall reliability (order-0 allocations
> instead of order-2)
There is long history for ll_temac and axi emac drivers. and if you compare
them they are very similar with similar bugs.
In our distribution we are using older ll_temac driver where this is fixed.
Performance is much higher if you allocate buffers for actual mtu.
Thanks,
Michal
P.S.: Our plan is to start to use mainline drivers and fixing them to get
the same performance as we have with old one. It is one my todo list.
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* Re: net: more accurate skb truesize - regression on Microblaze
From: Eric Dumazet @ 2012-04-10 11:32 UTC (permalink / raw)
To: monstr; +Cc: netdev, John Williams, David Miller
In-Reply-To: <4F83F166.4010208@monstr.eu>
On Tue, 2012-04-10 at 10:37 +0200, Michal Simek wrote:
> On 04/10/2012 10:32 AM, Eric Dumazet wrote:
> > On Tue, 2012-04-10 at 10:27 +0200, Eric Dumazet wrote:
> >
> >> BTW, some NIC drivers are known to provide fat skb in their rx path, and
> >> need to be fixed as well. (Some others just lie about skb->truesize to
> >> avoid the tcp slowdown, see my previous iwlwifi patch)
> >>
> >>
> >
> > What is the driver you currently use on your platform ?
>
> Using Xilinx ll_temac(in mainline) and axi_emac.
>
ll_temac allocates XTE_MAX_JUMBO_FRAME_SIZE frames, even for MTU=1500 ?
Trying to allocate smaller packets would definitely help your tcp
receiver performance and overall reliability (order-0 allocations
instead of order-2)
^ permalink raw reply
* Re: axienet driver copyrights
From: Daniel Borkmann @ 2012-04-10 11:27 UTC (permalink / raw)
To: monstr; +Cc: Ariane Keller, netdev, John Williams, John Linn, David Miller
In-Reply-To: <4F84164C.6000702@monstr.eu>
On Tue, Apr 10, 2012 at 1:15 PM, Michal Simek <monstr@monstr.eu> wrote:
> Dear Ariane and Daniel, [cc: John Linn, John Williams, David Miller and
> netdev]
>
> I have several questions about one patch which was added to mainline Linux
> tree.
> Below is information about the patch:
>
> drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver
> (sha1: 8a3b7a252dca9fb28c23b5bf76c49180a2b60d3b)
>
> The whole driver contains your copyright in 3 files but I don't believe
> that you are authors of significant changes to be polite to write there your
> copyright.
> I really appreciate that someone invested time to add this driver to the
> mainline
> but I am not comfortable that you will be identify as authors of this driver
> because you are not!
>
> Maintainer for this driver should be Xilinx which has done this driver. I
> also
> see that you have integrated all changes I have done to it. We have done
> these
> changes on Xilinx request to improve the driver stability and performance.
>
> I have spent some time to list your changes you have done to be sure that my
> first feeling was wrong but it is not.
>
> There are changes which do nothing with functionality.
> 1. Clean the driver by checkpatch.pl script.
> 2. Copy axienet_ior/iow to header file
> 3. axienet_device_reset resort.
>
> Can you tell me what significant changes you have done in this driver?
>
> I know the whole history of this driver and I have never heard about you
> and your changes pointing me that you have just took the latest version
> from PetaLogix or Xilinx git repository and use checkpatch.pl script
> to fix coding style violations.
>
> I don't think that using checkpatch + 2 simple changes is not enough
> to be identified as author.
>
> Can you please correct me?
Yes, we mainly spent time to refactor this patch (in particular
removing duplicate code parts), make it work under the latest kernel,
make it kernel coding convention compliant and included it into
Kconfig.
But feel free to remove us from the header, sure, that should be no
problem at all.
I guess we should have CC'ed you when submitting. Sorry that we have
forgotten that.
> Thanks,
> Michal
>
> David: Would it be possible to ping me everytime when there is any patch
> around xilinx ethernet controller. I have all equipment for testing it
> and it is my day job to take care about microblaze/xilinx drivers.
>
>
>
>
> --
> Michal Simek, Ing. (M.Eng)
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
> Microblaze U-BOOT custodian
> --
> 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: [PATCH] net: orphan queued skbs if device tx can stall
From: Michael S. Tsirkin @ 2012-04-10 11:25 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Michał Mirosław, Ben Hutchings,
Herbert Xu
In-Reply-To: <1334052259.3126.68.camel@edumazet-glaptop>
On Tue, Apr 10, 2012 at 12:04:19PM +0200, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 12:31 +0300, Michael S. Tsirkin wrote:
>
> > True. Still this is the only interface we have for controlling
> > the internal queue length so it seems safe to assume someone
> > is using it for this purpose.
> >
>
> So to workaround a problem in tun, you want to hack net/core/dev.c :(
Sorry about being unclear, I'm just saying that your patch assumes
tx_queue_len == 0 since you set it that way at device init but we can't
rely on this as existing users might have changed that value.
One way to fix would be a patch at the bottom: then we
can leave tun to treat tx_queue_len like it always did.
> Packets in qdisc should not be orphaned.
>
> If you think about it, why do we attach skb to socket in the first
> place ?
Good point. The answer is to avoid skb drops for local sockets by
stopping them, right?
> If its not needed for tun, why should it be needed for other devices ?
Maybe needed but it's already broken for tun since the skbs in the
private queue are orphaned by skb_orphan_try?
> If TUN has a problem being stopped forever, maybe it should take
> appropriate action to flush all packets in qdisc queue after a while, as
> this makes no sense to delay packets forever.
>
Well arbitrary timers aren't a solid protection, right?
We get out of stalling transmitters forever but a bad VM can still
degrade performance significantly for others...
----
We don't want a queue for tun since it can stall forever, but userspace
might tweak it's tx_queue_len as a way to control RX queue depth,
and we don't want to break userspace. Use a private flag to disable queue.
Warning: untested.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 27883d1..644ca53 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -695,7 +692,7 @@ static void attach_one_default_qdisc(struct net_device *dev,
{
struct Qdisc *qdisc = &noqueue_qdisc;
- if (dev->tx_queue_len) {
+ if (dev->tx_queue_len && !(dev->priv_flags & IFF_TX_CAN_STALL)) {
qdisc = qdisc_create_dflt(dev_queue,
&pfifo_fast_ops, TC_H_ROOT);
if (!qdisc) {
--
MST
^ permalink raw reply related
* axienet driver copyrights
From: Michal Simek @ 2012-04-10 11:15 UTC (permalink / raw)
To: Ariane Keller, Daniel Borkmann
Cc: netdev, John Williams, John Linn, David Miller
Dear Ariane and Daniel, [cc: John Linn, John Williams, David Miller and netdev]
I have several questions about one patch which was added to mainline Linux tree.
Below is information about the patch:
drivers/net/ethernet/xilinx: added Xilinx AXI Ethernet driver
(sha1: 8a3b7a252dca9fb28c23b5bf76c49180a2b60d3b)
The whole driver contains your copyright in 3 files but I don't believe
that you are authors of significant changes to be polite to write there your
copyright.
I really appreciate that someone invested time to add this driver to the mainline
but I am not comfortable that you will be identify as authors of this driver because you are not!
Maintainer for this driver should be Xilinx which has done this driver. I also
see that you have integrated all changes I have done to it. We have done these
changes on Xilinx request to improve the driver stability and performance.
I have spent some time to list your changes you have done to be sure that my
first feeling was wrong but it is not.
There are changes which do nothing with functionality.
1. Clean the driver by checkpatch.pl script.
2. Copy axienet_ior/iow to header file
3. axienet_device_reset resort.
Can you tell me what significant changes you have done in this driver?
I know the whole history of this driver and I have never heard about you
and your changes pointing me that you have just took the latest version
from PetaLogix or Xilinx git repository and use checkpatch.pl script
to fix coding style violations.
I don't think that using checkpatch + 2 simple changes is not enough
to be identified as author.
Can you please correct me?
Thanks,
Michal
David: Would it be possible to ping me everytime when there is any patch
around xilinx ethernet controller. I have all equipment for testing it
and it is my day job to take care about microblaze/xilinx drivers.
--
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian
^ permalink raw reply
* [PATCH v2] tcp: restore correct limit
From: Eric Dumazet @ 2012-04-10 10:56 UTC (permalink / raw)
To: Jason Wang; +Cc: monstr, netdev, John Williams, David Miller, Glauber Costa
In-Reply-To: <1334054436.3126.80.camel@edumazet-glaptop>
Commit c43b874d5d714f (tcp: properly initialize tcp memory limits) tried
to fix a regression added in commits 4acb4190 & 3dc43e3,
but still get it wrong.
Result is machines with low amount of memory have too small tcp_rmem[2]
value and slow tcp receives : Per socket limit being 1/1024 of memory
instead of 1/128 in old kernels, so rcv window is capped to small
values.
Fix this to match comment and previous behavior.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: Glauber Costa <glommer@parallels.com>
---
v2: remove one useless "limit = max(limit, 128UL);" line
net/ipv4/tcp.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 5d54ed3..7758a83 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -3302,8 +3302,7 @@ void __init tcp_init(void)
tcp_init_mem(&init_net);
/* Set per-socket limits to no more than 1/128 the pressure threshold */
- limit = nr_free_buffer_pages() << (PAGE_SHIFT - 10);
- limit = max(limit, 128UL);
+ limit = nr_free_buffer_pages() << (PAGE_SHIFT - 7);
max_share = min(4UL*1024*1024, limit);
sysctl_tcp_wmem[0] = SK_MEM_QUANTUM;
^ permalink raw reply related
* Re: [v2] Re: [RFC] l2tp/ipv6: support for L2TPv2 over UDP over IPv6
From: James Chapman @ 2012-04-10 10:47 UTC (permalink / raw)
To: Benjamin LaHaise; +Cc: Eric Dumazet, netdev
In-Reply-To: <20120319032807.GD11293@kvack.org>
On 19/03/12 03:28, Benjamin LaHaise wrote:
> On Fri, Mar 16, 2012 at 12:19:02PM +0000, James Chapman wrote:
>> Hi Ben,
>>
>> Do you have an updated patch to test?
>
> Yes, here's an updated version.
We've tested this in several L2TP environments - looks good. Will you be
submitting a version soon?
We have additional patches ready for review which add IPv6 for L2TPv3,
applying on top of your L2TPv2 patch. We also have a patch for iproute2
to add support for unmanaged L2TPv3 ethernet pseudowires over IPv6. They
can be posted when your L2TPv2 patch is submitted, or as an RFC if you
aren't ready to submit your patch yet.
James
^ permalink raw reply
* Re: [PATCH 2/2] phonet: Sort out initiailziation and cleanup code.
From: Sasha Levin @ 2012-04-10 10:47 UTC (permalink / raw)
To: Eric W. Biederman
Cc: David Miller, Eric Dumazet, Eric Van Hensbergen, Dave Jones,
linux-kernel, netdev, Remi Denis-Courmont
In-Reply-To: <m162dctjbo.fsf_-_@fess.ebiederm.org>
On Sat, Apr 7, 2012 at 3:35 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Recently an oops was reported in phonet if there was a failure during
> network namespace creation.
>
Looks like these two patches fix the problem I've reported.
^ permalink raw reply
* Re: [PATCH 6/6] tilegx network driver: initial support
From: Arnd Bergmann @ 2012-04-10 10:42 UTC (permalink / raw)
To: Chris Metcalf; +Cc: linux-kernel, netdev
In-Reply-To: <4F835510.4060100@tilera.com>
On Monday 09 April 2012, Chris Metcalf wrote:
> On 4/9/2012 9:49 AM, Arnd Bergmann wrote:
> > On Friday 06 April 2012, Chris Metcalf wrote:
> >> This change adds support for the tilegx network driver based on the
> >> GXIO IORPC support in the tilegx software stack, using the on-chip
> >> mPIPE packet processing engine.
> >>
> >> Signed-off-by: Chris Metcalf <cmetcalf@tilera.com>
> >> ---
> >> drivers/net/ethernet/tile/Kconfig | 1 +
> >> drivers/net/ethernet/tile/Makefile | 4 +-
> >> drivers/net/ethernet/tile/tilegx.c | 2045 ++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 2048 insertions(+), 2 deletions(-)
> >> create mode 100644 drivers/net/ethernet/tile/tilegx.c
> > I think the directory name should be the company, not the architecture here, so make
> > it drivers/net/ethernet/tilera/tilegx.c instead.
>
> This path was picked back when Jeff Kirsher did the initial move into
> drivers/net/ethernet/ for the tilepro driver. I don't have too strong an
> opinion on this; at this point I'm mostly just concerned that it seems like
> potentially not worth the churn to move the files for 3.2, then again for
> 3.5. But if folks agree we should do it, it's fine with me.
Ah, I didn't realize that the directory already exists. It's probably better
not to move it then.
> The actual author would rather not publish his name (I just double-checked
> with him).
Hmm, it doesn't look all that bad actually, the comments I had are just for
small details.
> >> +/* The actual devices. */
> >> +static struct net_device *tile_net_devs[TILE_NET_DEVS];
> >> +
> >> +/* The device for a given channel. HACK: We use "32", not
> >> + * TILE_NET_CHANNELS, because it is fairly subtle that the 5 bit
> >> + * "idesc.channel" field never exceeds TILE_NET_CHANNELS.
> >> + */
> >> +static struct net_device *tile_net_devs_for_channel[32];
> > When you need to keep a list or array of device structures in a driver, you're
> > usually doing something very wrong. The convention is to just pass the pointer
> > around to where you need it.
>
> We need "tile_net_devs_for_channel" because we share a single hardware
> queue for all devices, and each packet's metadata contains a "channel"
> value which indicates the device.
Ok, but please remove tile_net_devs then.
I think a better abstraction for tile_net_devs_for_channel would be
some interface that lets you add private data to a channel so when
you get data from a channel, you can extract that pointer from the driver
using the channel.
Don't you already have a per-channel data structure?
>
> /*
> * The on-chip I/O hardware on tilegx is configured with VA=PA for the
> * kernel's PA range. The low-level APIs and field names use "va" and
> * "void *" nomenclature, to be consistent with the general notion
> * that the addresses in question are virtualizable, but in the kernel
> * context we are actually manipulating PA values. To allow readers
> * of the code to understand what's happening, we direct their
> * attention to this comment by using the following two no-op functions.
> */
> static inline unsigned long pa_to_tile_io_addr(phys_addr_t pa)
> {
> BUILD_BUG_ON(sizeof(phys_addr_t) != sizeof(unsigned long));
> return pa;
> }
> static inline phys_addr_t tile_io_addr_to_pa(unsigned long tile_io_addr)
> {
> return tile_io_addr;
> }
>
> Then the individual uses in the network driver are just things like
> "edesc_head.va = pa_to_tile_io_addr(__pa(va))" or "va =
> __va(tile_io_addr_to_pa((unsigned long)gxio_mpipe_idesc_get_va(idesc)))"
> which I think is a little clearer.
Yes, although I would probably add a typedef for tile_io_addr and pass
the virtual address in and out these helper functions.
For added clarity, you could make the interface look like dma_map_single(),
which requires adding an empty unmap() function as well -- that would
make it obvious where that data is actually used. Why do you require
the reverse map anyway? Normally you only need to pass a bus address to
the device but don't need to translate that back into a virtual address
because you already had that in the beginning.
> >> +/* Allocate and push a buffer. */
> >> +static bool tile_net_provide_buffer(bool small)
> >> +{
> >> [...]
> >> +
> >> + /* Save a back-pointer to 'skb'. */
> >> + *(struct sk_buff **)(skb->data - sizeof(struct sk_buff **)) = skb;
> > This looks very wrong: why would you put the pointer to the skb into the
> > skb itself?
>
> Because we create skbuffs, and then feed the raw underlying buffer storage
> to our hardware, and later, we get back this raw pointer from hardware,
> from which we need to be able to extract the actual skbuff.
Hmm, this sounds very unusual, but I don't really have a better suggestion
here.
> >> + /* Compute the "ip checksum". */
> >> + jsum = isum_hack + htons(s_len - eh_len) + htons(id);
> >> + jsum = __insn_v2sadu(jsum, 0);
> >> + jsum = __insn_v2sadu(jsum, 0);
> >> + jsum = (0xFFFF ^ jsum);
> >> + jh->check = jsum;
> >> +
> >> + /* Update the tcp "seq". */
> >> + uh->seq = htonl(seq);
> >> +
> >> + /* Update some flags. */
> >> + if (!final)
> >> + uh->fin = uh->psh = 0;
> >> +
> >> + /* Compute the tcp pseudo-header checksum. */
> >> + usum = tsum_hack + htons(s_len);
> >> + usum = __insn_v2sadu(usum, 0);
> >> + usum = __insn_v2sadu(usum, 0);
> >> + uh->check = usum;
> > Why to you open-code the ip checksum functions here? Normally the stack takes
> > care of this by calling the functions you already provide in
> > arch/tile/lib/checksum.c
>
> If there is a way to do TSO without this, we'd be happy to hear it, but
> it's not clear how it would be possible. We are only computing a PARTIAL
> checksum here, and letting the hardware compute the "full" checksum.
Sounds like you're looking for csum_partial() ;-)
Arnd
^ permalink raw reply
* Re: [PATCH] tcp: restore correct limit
From: Eric Dumazet @ 2012-04-10 10:40 UTC (permalink / raw)
To: Jason Wang; +Cc: monstr, netdev, John Williams, David Miller, Glauber Costa
In-Reply-To: <4F840B99.8040409@redhat.com>
On Tue, 2012-04-10 at 18:29 +0800, Jason Wang wrote:
> On 04/10/2012 06:14 PM, Michal Simek wrote:
> > On 04/10/2012 12:03 PM, Eric Dumazet wrote:
> >> Commit c43b874d5d714f (tcp: properly initialize tcp memory limits)
> >> added a regression on machines with low amount of memory, since sockets
> >> cant use 1/128 of memory but 1/1024
> >>
> >> Fix this to match comment and previous behavior.
> >>
> >> Signed-off-by: Eric Dumazet<eric.dumazet@gmail.com>
> >> Cc: Jason Wang<jasowang@redhat.com>
> >> Cc: Glauber Costa<glommer@parallels.com>
> >> ---
> >> net/ipv4/tcp.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> >> index 5d54ed3..67d726e 100644
> >> --- a/net/ipv4/tcp.c
> >> +++ b/net/ipv4/tcp.c
> >> @@ -3302,7 +3302,7 @@ void __init tcp_init(void)
> >>
> >> tcp_init_mem(&init_net);
> >> /* Set per-socket limits to no more than 1/128 the pressure
> >> threshold */
> >> - limit = nr_free_buffer_pages()<< (PAGE_SHIFT - 10);
> >> + limit = nr_free_buffer_pages()<< (PAGE_SHIFT - 7);
> >> limit = max(limit, 128UL);
> >> max_share = min(4UL*1024*1024, limit);
> >>
> >
> > hw design with csum is also much better.
> > Tested-by: Michal Simek <monstr@monstr.eu>
> >
> > Thanks for help,
> > Michal
> >
> >
> >
> >
> >
> Hi Michal and Eric:
>
> Which version of kernel did you test, did you try the newest kernel? The
> reason I use (PAGE_SHIFT - 10) is in the commit before 3dc43e3, the
> limit were calculated with:
>
> limit = nr_free_buffer_pages() / 8;
> limit = max(limit, 128UL);
> ...
> limit = ((unsigned long)sysctl_tcp_mem[1]) << (PAGE_SHIFT - 7);
>
> So the rmem should be ok. But there's a defect (which I think does
> affect the regression) of my patch would could cause limit that we
> should shift after comparing with 128UL like:
>
> limit = nr_free_buffer_pages() / 8;
> limit = max(limit, 128UL) << (PAGE_SHIFT - 7);
>
> Is anything I miss?
>
Yes, probably.
Maybe you should check what was the situation on 2.6 kernels.
Your commit did not completely fix the 4acb41903b2 one
I dont feel its necessary to put in the changelog the complete bug
history, since your commit does the needed tracking.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox