* Re: [RFC] back on nf_tables (plus compatibility layer)
From: Jan Engelhardt @ 2012-10-31 13:50 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Netfilter Development Mailing list,
Linux Networking Developer Mailing List
In-Reply-To: <20121026110419.GA16629@1984>
On Friday 2012-10-26 13:04, Pablo Neira Ayuso wrote:
>>
>> Meanwhile, I am on xtables2 that actually reproduces the set of
>> _really important_ features that currently are in the setsockopt
>> iptables, like atomic table replace and atomic dump.
>>
>> I have updated to the newest tree, and the first set is
>> available in the git repository at:
>> git://git.inai.de/linux xt2-20121025
>
>If you think this feature is important, checkout nf_tables and think
>how to integrate this prototype code that provides atomic table
>replacement to it.
I'd rather tinker with xt2.
^ permalink raw reply
* Re: [uclinux-dist-devel] [PATCH RFC net-next 2/4] bfin_mac: replace sys time stamps with raw ones instead.
From: Richard Cochran @ 2012-10-31 12:46 UTC (permalink / raw)
To: Bob Liu
Cc: netdev, Miroslav Lichvar, John Ronciak, John Stultz, Jeff Kirsher,
device-drivers-devel, Jacob Keller, uclinux-dist-devel,
Patrick Ohly, David Miller
In-Reply-To: <CAA_GA1cQojiaJat8gGmp3XYZVGOtKi2xoFS08hsYTGssdbWcRg@mail.gmail.com>
On Wed, Oct 31, 2012 at 05:00:55PM +0800, Bob Liu wrote:
> On Wed, Oct 31, 2012 at 4:36 PM, Richard Cochran
> <richardcochran@gmail.com> wrote:
> > On Wed, Oct 31, 2012 at 02:29:23PM +0800, Bob Liu wrote:
> >
> >> Any guide how to use ptp4l to sync system time between two machines
> >> over ethernet?
> >
> > You can run ptp4l on two machines like this
> >
> > ptp4l -i eth0 -q -v # master
> > ptp4l -i eth0 -q -v -s # slave
> >
>
> Thank you.
> But i'm still failed to sync system time.
>
> On mater:
> -------------------------------
> root:/> date
> Wed Dec 20 01:31:02 UTC 2006
> root:/>
> root:/> ptp4l -i eth0 -q -v
> ptp4l[33.520]: selected /dev/ptp0 as PTP clock
> ptp4l[33.520]: m
> ptp4l[33.520]: driver rejected most general HWTSTAMP filter
> ptp4l[33.520]: port 1: INITIALIZING to LISTENING on INITIALIZE
> ptp4l[33.524]: port 0: INITIALIZING to LISTENING on INITIALIZE
> ptp4l[39.524]: port 1: LISTENING to MASTER on ANNOUNCE_RECEIPT_TIMEOUT_EXPIRES
> .....
>
> -----------------------------
> On slave:
> root:~> date
> Fri Jan 2 13:16:07 UTC 1970
>
> root:~> ptp4l -i eth0 -q -v -s
> ptp4l[39.624]: selected /dev/ptp0 as PTP clock
> ptp4l[39.628]: m
> ptp4l[39.628]: driver rejected most general HWTSTAMP filter
> ptp4l[39.628]: port 1: INITIALIZING to LISTENING on INITIALIZE
> ptp4l[39.632]: port 0: INITIALIZING to LISTENING on INITIALIZE
> ptp4l[39.664]: port 1: new foreign master 00e022.fffe.fe8529-1
> ptp4l[43.672]: selected best master clock 00e022.fffe.fe8529
> ptp4l[43.676]: port 1: LISTENING to UNCALIBRATED on RS_SLAVE
> ptp4l[44.688]: master offset -7968957480 s0 adj +0 path delay 8120
> ptp4l[45.692]: master offset -7968953124 s0 adj +0 path delay 9076
> ptp4l[46.696]: master offset -7968947340 s0 adj +0 path delay 8860
> ptp4l[47.700]: master offset -7968941884 s1 adj +0 path delay 8860
> ptp4l[48.704]: master offset 4564 s2 adj +4564 path delay 8860
> ptp4l[48.704]: port 1: UNCALIBRATED to SLAVE on MASTER_CLOCK_SELECTED
> ptp4l[49.708]: master offset 5154 s2 adj +6523 path delay 9086
> ptp4l[50.712]: master offset 3686 s2 adj +6601 path delay 9322
> ptp4l[51.716]: master offset 2582 s2 adj +6603 path delay 9322
> ptp4l[52.720]: master offset 1362 s2 adj +6158 path delay 9438
> ptp4l[53.724]: master offset 658 s2 adj +5862 path delay 9438
> ptp4l[54.728]: master offset -85 s2 adj +5317 path delay 9685
> ptp4l[55.732]: master offset -113 s2 adj +5263 path delay 9873
> ptp4l[56.748]: master offset 86 s2 adj +5428 path delay 9930
> ptp4l[57.740]: master offset 166 s2 adj +5534 path delay 9930
> ptp4l[58.744]: master offset -247 s2 adj +5171 path delay 10151
> ptp4l[59.748]: master offset -39 s2 adj +5305 path delay 10151
> ptp4l[60.752]: master offset 217 s2 adj +5549 path delay 10151
> ptp4l[61.756]: master offset 256 s2 adj +5653 path delay 10128
> .....
This looks perfect. Did you fixup the wrong time stamp shift?
>
> root:~> date
> Fri Jan 2 13:16:45 UTC 1970
> root:~>
>
> The system time in slave didn't change.
That is correct. Using RAW time stamping and PHC, the time in the PHC
changes, but not the system time. The system time may be slaved to
the PHC time using the phc2sys program.
Anyhow, it looks like this is working fine to me.
Thanks,
Richard
^ permalink raw reply
* [net PATCH V2] net: fix divide by zero in tcp algorithm illinois
From: Jesper Dangaard Brouer @ 2012-10-31 12:45 UTC (permalink / raw)
To: David S. Miller
Cc: Jesper Dangaard Brouer, netdev, Petr Matousek, Stephen Hemminger,
Eric Dumazet
Reading TCP stats when using TCP Illinois congestion control algorithm
can cause a divide by zero kernel oops.
The division by zero occur in tcp_illinois_info() at:
do_div(t, ca->cnt_rtt);
where ca->cnt_rtt can become zero (when rtt_reset is called)
Steps to Reproduce:
1. Register tcp_illinois:
# sysctl -w net.ipv4.tcp_congestion_control=illinois
2. Monitor internal TCP information via command "ss -i"
# watch -d ss -i
3. Establish new TCP conn to machine
Either it fails at the initial conn, or else it needs to wait
for a loss or a reset.
This is only related to reading stats. The function avg_delay() also
performs the same divide, but is guarded with a (ca->cnt_rtt > 0) at its
calling point in update_params(). Thus, simply fix tcp_illinois_info().
Function tcp_illinois_info() / get_info() is called without
socket lock. Thus, eliminate any race condition on ca->cnt_rtt
by using a local stack variable. Simply reuse info.tcpv_rttcnt,
as its already set to ca->cnt_rtt.
Function avg_delay() is not affected by this race condition, as
its called with the socket lock.
Cc: Petr Matousek <pmatouse@redhat.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
V2:
Address Eric Dumazets input:
- Save 2 bytes of stack, by using info.tcpv_rttcnt.
- Help compiler, and define "u64 t" inside if() lexical scope.
net/ipv4/tcp_illinois.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/tcp_illinois.c b/net/ipv4/tcp_illinois.c
index 813b43a..834857f 100644
--- a/net/ipv4/tcp_illinois.c
+++ b/net/ipv4/tcp_illinois.c
@@ -313,11 +313,13 @@ static void tcp_illinois_info(struct sock *sk, u32 ext,
.tcpv_rttcnt = ca->cnt_rtt,
.tcpv_minrtt = ca->base_rtt,
};
- u64 t = ca->sum_rtt;
- do_div(t, ca->cnt_rtt);
- info.tcpv_rtt = t;
+ if (info.tcpv_rttcnt > 0) {
+ u64 t = ca->sum_rtt;
+ do_div(t, info.tcpv_rttcnt);
+ info.tcpv_rtt = t;
+ }
nla_put(skb, INET_DIAG_VEGASINFO, sizeof(info), &info);
}
}
^ permalink raw reply related
* Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc
From: David Woodhouse @ 2012-10-31 11:52 UTC (permalink / raw)
To: Krzysztof Mazur; +Cc: davem, netdev, linux-kernel
In-Reply-To: <20121031113021.GA24093@shrek.podlesie.net>
[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]
On Wed, 2012-10-31 at 12:30 +0100, Krzysztof Mazur wrote:
> Yes, original patch had also the same problem with sock_owned_by_user(),
> so I just incorrectly assumed that we can do "goto nospace" after
> pppoatm_may_send(), but ppooatm_may_send() must be the last test.
>
> So I just moved all other tests earlier and and now pppoatm_may_send()
> is also protected by ATM socket lock as you suggested earlier.
I don't think that's sufficient. When we return zero from
pppoatm_send(), the generic PPP code considers the channel to be
blocked, and it won't send any more data to it, ever, until we call
ppp_output_wakeup(). Which we do from a tasklet, triggered in
pppoatm_pop() *iff* the BLOCKED flag is set.
So we play silly buggers in pppoatm_may_send() to ensure that *if* we're
going to return zero, we make damn sure the BLOCKED flag is set and that
pppoatm_pop() is going to see that it's set. There are extensive
comments in pppoatm_pop() and pppoatm_may_send() which try to explain
this. It works because there's *always* going to be packet in flight if
we say that the sk_wmem is full, so of course there's *always* going to
be a later call to pppoatm_pop() to wake things up.
However, if you're going to return zero from pppoatm_send() when
sock_owned_by_user() is true, what guarantees that ppp_output_wakeup()
will ever be called?
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/3] pppoatm: fix race condition with destroying of vcc
From: Krzysztof Mazur @ 2012-10-31 11:30 UTC (permalink / raw)
To: David Woodhouse; +Cc: davem, netdev, linux-kernel
In-Reply-To: <1351678578.8774.8.camel@shinybook.infradead.org>
On Wed, Oct 31, 2012 at 10:16:18AM +0000, David Woodhouse wrote:
>
> Does this break the pvcc->blocked handling that coordinates with
> pppoatm_pop()?
>
> If we have one packet in flight, so pppoatm_may_send() permits a new one
> to be queued... but they're *large* packets to sk_wmem_alloc doesn't
> permit it. Immediately after the check, pppoatm_pop() runs and leaves
> the queue empty. We return zero, blocking the queue??? which never gets
> woken because we didn't set the BLOCKED flag and thus the tasklet never
> runs.
>
> In fact, I think we need the BLOCKED handling for the
> sock_owned_by_user() case too? When the VCC is actually closed, I
> suppose that's not recoverable and we don't care about waking the queue
> anyway? But any time we end up returning zero from pppoatm_send(), we
> *need* to ensure that a wakeup will happen in future unless the socket
> is actually dead.
>
Yes, original patch had also the same problem with sock_owned_by_user(),
so I just incorrectly assumed that we can do "goto nospace" after
pppoatm_may_send(), but ppooatm_may_send() must be the last test.
So I just moved all other tests earlier and and now pppoatm_may_send()
is also protected by ATM socket lock as you suggested earlier.
Krzysiek
-- >8 --
Subject: [PATCH] pppoatm: fix race condition with destroying of vcc
The pppoatm_send() calls vcc->send() and now also checks for
some vcc flags that indicate destroyed vcc without proper locking.
The vcc_sendmsg() uses lock_sock(sk). This lock is used by
vcc_release(), so vcc_destroy_socket() will not be called between
check and during ->send(). The vcc_release_async() sets ATM_VF_CLOSE,
but it should be safe to call ->send() after it, because
vcc->dev->ops->close() is not called.
The pppoatm_send() is called with BH disabled, so bh_lock_sock()
should be used instead of lock_sock().
Signed-off-by: Krzysztof Mazur <krzysiek@podlesie.net>
---
net/atm/pppoatm.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 0dcb5dc..eb76bd3 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -270,11 +270,22 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
{
struct pppoatm_vcc *pvcc = chan_to_pvcc(chan);
struct atm_vcc *vcc;
+ int ret;
ATM_SKB(skb)->vcc = pvcc->atmvcc;
pr_debug("(skb=0x%p, vcc=0x%p)\n", skb, pvcc->atmvcc);
if (skb->data[0] == '\0' && (pvcc->flags & SC_COMP_PROT))
(void) skb_pull(skb, 1);
+
+ vcc = ATM_SKB(skb)->vcc;
+ bh_lock_sock(sk_atm(vcc));
+ if (sock_owned_by_user(sk_atm(vcc)))
+ goto nospace;
+ if (test_bit(ATM_VF_RELEASED, &vcc->flags)
+ || test_bit(ATM_VF_CLOSE, &vcc->flags)
+ || !test_bit(ATM_VF_READY, &vcc->flags))
+ goto nospace;
+
switch (pvcc->encaps) { /* LLC encapsulation needed */
case e_llc:
if (skb_headroom(skb) < LLC_LEN) {
@@ -287,8 +298,10 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
}
consume_skb(skb);
skb = n;
- if (skb == NULL)
+ if (skb == NULL) {
+ bh_unlock_sock(sk_atm(vcc));
return DROP_PACKET;
+ }
} else if (!pppoatm_may_send(pvcc, skb->truesize))
goto nospace;
memcpy(skb_push(skb, LLC_LEN), pppllc, LLC_LEN);
@@ -298,24 +311,22 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
goto nospace;
break;
case e_autodetect:
+ bh_unlock_sock(sk_atm(vcc));
pr_debug("Trying to send without setting encaps!\n");
kfree_skb(skb);
return 1;
}
- vcc = ATM_SKB(skb)->vcc;
- if (test_bit(ATM_VF_RELEASED, &vcc->flags)
- || test_bit(ATM_VF_CLOSE, &vcc->flags)
- || !test_bit(ATM_VF_READY, &vcc->flags))
- goto nospace;
-
atomic_add(skb->truesize, &sk_atm(ATM_SKB(skb)->vcc)->sk_wmem_alloc);
ATM_SKB(skb)->atm_options = ATM_SKB(skb)->vcc->atm_options;
pr_debug("atm_skb(%p)->vcc(%p)->dev(%p)\n",
skb, ATM_SKB(skb)->vcc, ATM_SKB(skb)->vcc->dev);
- return ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb)
+ ret = ATM_SKB(skb)->vcc->send(ATM_SKB(skb)->vcc, skb)
? DROP_PACKET : 1;
+ bh_unlock_sock(sk_atm(vcc));
+ return ret;
nospace:
+ bh_unlock_sock(sk_atm(vcc));
/*
* We don't have space to send this SKB now, but we might have
* already applied SC_COMP_PROT compression, so may need to undo
--
1.8.0.172.g62af90c
^ permalink raw reply related
* [PATCH 4/9] net: openvswitch: use this_cpu_ptr per-cpu helper
From: Shan Wei @ 2012-10-31 11:22 UTC (permalink / raw)
To: jesse-l0M0P4e3n4LQT0dZR+AlfA, dev-yBygre7rU0TnMu66kgdUjQ, NetDev,
Kernel-Maillist, David Miller,
cl-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b
From: Shan Wei <davidshan-1Nz4purKYjRBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Shan Wei <davidshan-1Nz4purKYjRBDgjK7y7TUQ@public.gmane.org>
---
net/openvswitch/datapath.c | 4 ++--
net/openvswitch/vport.c | 5 ++---
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 4c4b62c..77d16a5 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -208,7 +208,7 @@ void ovs_dp_process_received_packet(struct vport *p, struct sk_buff *skb)
int error;
int key_len;
- stats = per_cpu_ptr(dp->stats_percpu, smp_processor_id());
+ stats = this_cpu_ptr(dp->stats_percpu);
/* Extract flow from 'skb' into 'key'. */
error = ovs_flow_extract(skb, p->port_no, &key, &key_len);
@@ -282,7 +282,7 @@ int ovs_dp_upcall(struct datapath *dp, struct sk_buff *skb,
return 0;
err:
- stats = per_cpu_ptr(dp->stats_percpu, smp_processor_id());
+ stats = this_cpu_ptr(dp->stats_percpu);
u64_stats_update_begin(&stats->sync);
stats->n_lost++;
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 03779e8..70af0be 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -333,8 +333,7 @@ void ovs_vport_receive(struct vport *vport, struct sk_buff *skb)
{
struct vport_percpu_stats *stats;
- stats = per_cpu_ptr(vport->percpu_stats, smp_processor_id());
-
+ stats = this_cpu_ptr(vport->percpu_stats);
u64_stats_update_begin(&stats->sync);
stats->rx_packets++;
stats->rx_bytes += skb->len;
@@ -359,7 +358,7 @@ int ovs_vport_send(struct vport *vport, struct sk_buff *skb)
if (likely(sent)) {
struct vport_percpu_stats *stats;
- stats = per_cpu_ptr(vport->percpu_stats, smp_processor_id());
+ stats = this_cpu_ptr(vport->percpu_stats);
u64_stats_update_begin(&stats->sync);
stats->tx_packets++;
--
1.7.1
^ permalink raw reply related
* [PATCH 3/9] net: xfrm: use this_cpu_ptr per-cpu helper
From: Shan Wei @ 2012-10-31 11:22 UTC (permalink / raw)
To: steffen.klassert, David Miller, NetDev, Herbert Xu,
Kernel-Maillist, cl
From: Shan Wei <davidshan@tencent.com>
Signed-off-by: Shan Wei <davidshan@tencent.com>
---
net/xfrm/xfrm_ipcomp.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index e5246fb..af6c78a 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c
@@ -276,14 +276,13 @@ static struct crypto_comp * __percpu *ipcomp_alloc_tfms(const char *alg_name)
struct crypto_comp * __percpu *tfms;
int cpu;
- /* This can be any valid CPU ID so we don't need locking. */
- cpu = raw_smp_processor_id();
-
list_for_each_entry(pos, &ipcomp_tfms_list, list) {
struct crypto_comp *tfm;
tfms = pos->tfms;
- tfm = *per_cpu_ptr(tfms, cpu);
+
+ /* This can be any valid CPU ID so we don't need locking. */
+ tfm = *this_cpu_ptr(tfms);
if (!strcmp(crypto_comp_name(tfm), alg_name)) {
pos->users++;
--
1.7.1
^ permalink raw reply related
* [PATCH 2/9] net: rds: use this_cpu_ptr per-cpu helper
From: Shan Wei @ 2012-10-31 11:22 UTC (permalink / raw)
To: venkat.x.venkatsubra, David Miller, rds-devel, NetDev,
Kernel-Maillist, cl
From: Shan Wei <davidshan@tencent.com>
Signed-off-by: Shan Wei <davidshan@tencent.com>
---
net/rds/ib_recv.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index 8d19491..a4a5064 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -423,7 +423,7 @@ static void rds_ib_recv_cache_put(struct list_head *new_item,
local_irq_save(flags);
- chp = per_cpu_ptr(cache->percpu, smp_processor_id());
+ chp = this_cpu_ptr(cache->percpu);
if (!chp->first)
INIT_LIST_HEAD(new_item);
else /* put on front */
--
1.7.1
^ permalink raw reply related
* [PATCH 1/9] net: core: use this_cpu_ptr per-cpu helper
From: Shan Wei @ 2012-10-31 11:22 UTC (permalink / raw)
To: David Miller, timo.teras, steffen.klassert, NetDev,
Kernel-Maillist, cl
From: Shan Wei <davidshan@tencent.com>
Signed-off-by: Shan Wei <davidshan@tencent.com>
---
net/core/flow.c | 4 +---
1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/net/core/flow.c b/net/core/flow.c
index e318c7e..3bad824 100644
--- a/net/core/flow.c
+++ b/net/core/flow.c
@@ -327,11 +327,9 @@ static void flow_cache_flush_tasklet(unsigned long data)
static void flow_cache_flush_per_cpu(void *data)
{
struct flow_flush_info *info = data;
- int cpu;
struct tasklet_struct *tasklet;
- cpu = smp_processor_id();
- tasklet = &per_cpu_ptr(info->cache->percpu, cpu)->flush_tasklet;
+ tasklet = &this_cpu_ptr(info->cache->percpu)->flush_tasklet;
tasklet->data = (unsigned long)info;
tasklet_schedule(tasklet);
}
--
1.7.1
^ permalink raw reply related
* [PATCH 0/9] use this_cpu_ptr instead of per_cpu_ptr(p, smp_processor_id())
From: Shan Wei @ 2012-10-31 11:21 UTC (permalink / raw)
To: cl, David Miller, NetDev, Kernel-Maillist
this_cpu_ptr is faster than per_cpu_ptr(p, smp_processor_id()).
The latter helper needs to find the offset for current cpu,
and needs more assembler instructions which objdump shows in following.
per_cpu_ptr(p, smp_processor_id())：
1e: 65 8b 04 25 00 00 00 00 mov %gs:0x0,%eax
26: 48 98 cltq
28: 31 f6 xor %esi,%esi
2a: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
31: 48 8b 04 c5 00 00 00 00 mov 0x0(,%rax,8),%rax
39: c7 44 10 04 14 00 00 00 movl $0x14,0x4(%rax,%rdx,1)
this_cpu_ptr(p)
1e: 65 48 03 14 25 00 00 00 00 add %gs:0x0,%rdx
27: 31 f6 xor %esi,%esi
29: c7 42 04 14 00 00 00 movl $0x14,0x4(%rdx)
30: 48 c7 c7 00 00 00 00 mov $0x0,%rdi
$ git diff --stat a932657f51eadb8280166e82dc7034dfbff3985a..
drivers/clocksource/arm_generic.c | 2 +-
include/trace/ftrace.h | 4 +---
kernel/padata.c | 2 +-
kernel/rcutree.c | 2 +-
kernel/trace/blktrace.c | 2 +-
kernel/trace/trace.c | 2 +-
net/core/flow.c | 4 +---
net/openvswitch/datapath.c | 4 ++--
net/openvswitch/vport.c | 5 ++---
net/rds/ib_recv.c | 2 +-
net/xfrm/xfrm_ipcomp.c | 7 +++----
11 files changed, 15 insertions(+), 21 deletions(-)
^ permalink raw reply
* Re: [Xen-devel] [PATCH] net: allow configuration of the size of page in __netdev_alloc_frag
From: Eric Dumazet @ 2012-10-31 11:19 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Konrad Rzeszutek Wilk, netdev@vger.kernel.org, Eric Dumazet,
Ian Campbell, xen-devel@lists.xen.org
In-Reply-To: <20121031110149.GA6376@localhost.localdomain>
On Wed, 2012-10-31 at 07:01 -0400, Konrad Rzeszutek Wilk wrote:
> Argh. Please disregard this test. I added an extra patch in the kernel
> tree to track the SWIOTLB bounce usage and print it.. and the printk was
> going out to the FB, which was not too fast - so the whole test was being
> slowed down by FB drivers :-)
>
> Will re-run this test without the offending patch.
Anyway, I must confess I didnt understand what you did ;)
^ permalink raw reply
* Re: [PATCH net] net: fix divide by zero in tcp algorithm illinois
From: Eric Dumazet @ 2012-10-31 11:17 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: David S. Miller, netdev, Petr Matousek, Stephen Hemminger
In-Reply-To: <20121031103630.18756.15685.stgit@dragon>
On Wed, 2012-10-31 at 11:37 +0100, Jesper Dangaard Brouer wrote:
> Reading TCP stats when using TCP Illinois congestion control algorithm
> can cause a divide by zero kernel oops.
>
> The division by zero occur in tcp_illinois_info() at:
> do_div(t, ca->cnt_rtt);
> where ca->cnt_rtt can become zero (when rtt_reset is called)
>
> Steps to Reproduce:
> 1. Register tcp_illinois:
> # sysctl -w net.ipv4.tcp_congestion_control=illinois
> 2. Monitor internal TCP information via command "ss -i"
> # watch -d ss -i
> 3. Establish new TCP conn to machine
>
> Either it fails at the initial conn, or else it needs to wait
> for a loss or a reset.
>
> This is only related to reading stats. The function avg_delay() also
> performs the same divide, but is guarded with a (ca->cnt_rtt > 0) at its
> calling point in update_params(). Thus, simply fix tcp_illinois_info().
>
avg_delay() is called with socket locked so it is safe.
While get_info() is called with socket not locked.
> To be on the safe side, I use a local stack variable in tcp_illinois_info()
> to eliminate any race conditions. I'm not sure this is needed, as this
> would also affect avg_delay(), if this race exists. (Although this is likely
> already "fix" by compiler optimization and kept in a local register)
Hmm, this is certainly not a valid reason.
Compiler could do the reverse actually, even with a local var.
Could you please use info.tcpv_rttcnt to be on the safe side ?
diff --git a/net/ipv4/tcp_illinois.c b/net/ipv4/tcp_illinois.c
index 813b43a..d92ae7e 100644
--- a/net/ipv4/tcp_illinois.c
+++ b/net/ipv4/tcp_illinois.c
@@ -313,11 +313,13 @@ static void tcp_illinois_info(struct sock *sk, u32 ext,
.tcpv_rttcnt = ca->cnt_rtt,
.tcpv_minrtt = ca->base_rtt,
};
- u64 t = ca->sum_rtt;
- do_div(t, ca->cnt_rtt);
- info.tcpv_rtt = t;
+ if (info.tcpv_rttcnt) {
+ u64 t = ca->sum_rtt;
+ do_div(t, info.tcpv_rttcnt);
+ info.tcpv_rtt = t;
+ }
nla_put(skb, INET_DIAG_VEGASINFO, sizeof(info), &info);
}
}
^ permalink raw reply related
* Re: [PATCH 1/4] net: mvneta: driver for Marvell Armada 370/XP network unit
From: Florian Fainelli @ 2012-10-31 11:12 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: David S. Miller, Lennert Buytenhek, netdev, linux-arm-kernel,
Jason Cooper, Andrew Lunn, Gregory Clement, Lior Amsalem,
Maen Suleiman
In-Reply-To: <1351245804-31478-2-git-send-email-thomas.petazzoni@free-electrons.com>
Hello Thomas,
On Friday 26 October 2012 12:03:21 Thomas Petazzoni wrote:
> This patch contains a new network driver for the network unit of the
> ARM Marvell Armada 370 and the Armada XP. Both SoCs use the PJ4B
> processor, a Marvell-developed ARM core that implements the ARMv7
> instruction set.
>
> Compared to previous ARM Marvell SoCs (Kirkwood, Orion, Discovery),
> the network unit in Armada 370 and Armada XP is highly different. This
> is the reason why this new 'mvneta' driver is needed, while the older
> ARM Marvell SoCs use the 'mv643xx_eth' driver.
>
> Here is an overview of the most important hardware changes that
> require a new, specific, driver for the network unit of Armada 370/XP:
>
> - The new network unit has a completely different design and layout
> for the RX and TX descriptors. They are now organized as a simple
> array (each RX and TX queue has base address and size of this
> array) rather than a linked list as in the old SoCs.
>
> - The new network unit has a different RXQ and TXQ management: this
> management is done using special read/write counter registers,
> while in the Old SocS, it was done using the Ownership bit in RX
> and TX descriptors.
>
> - The new network unit has different interrupt registers
>
> - The new network unit way of cleaning of interrupts is not done by
> writing to the cause register, but by updating per-queue counters
>
> - The new network unit has different GMAC registers (link, speed,
> duplex configuration) and different WRR registers.
>
> - The new network unit has lots of new units like PnC (Parser and
> Classifier), PMT, BM (Memory Buffer Management), xPON, and more.
>
> The driver proposed in the current patch only handles the basic
> features. Additional hardware features will progressively be supported
> as needed.
>
> This code has originally been written by Rami Rosen
> <rosenr@marvell.com>, and then reviewed and cleaned up by Thomas
> Petazzoni <thomas.petazzoni@free-electrons.com>.
This is looking good from a PHY lib point of view now, thanks for doing this!
--
Florian
^ permalink raw reply
* Re: [Xen-devel] [PATCH] net: allow configuration of the size of page in __netdev_alloc_frag
From: Konrad Rzeszutek Wilk @ 2012-10-31 11:01 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk
Cc: Eric Dumazet, netdev@vger.kernel.org, Eric Dumazet, Ian Campbell,
xen-devel@lists.xen.org
In-Reply-To: <20121030172352.GA31997@phenom.dumpdata.com>
On Tue, Oct 30, 2012 at 01:23:52PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 30, 2012 at 12:53:09PM -0400, Konrad Rzeszutek Wilk wrote:
> > On Wed, Oct 24, 2012 at 06:43:20PM +0200, Eric Dumazet wrote:
> > > On Wed, 2012-10-24 at 17:22 +0100, Ian Campbell wrote:
> > > > On Wed, 2012-10-24 at 16:21 +0100, Eric Dumazet wrote:
> > >
> > > > > If you really have such problems, why locally generated TCP traffic
> > > > > doesnt also have it ?
> > > >
> > > > I think it does. The reason I noticed the original problem was that ssh
> > > > to the machine was virtually (no pun intended) unusable.
> > > >
> > > > > Your patch doesnt touch sk_page_frag_refill(), does it ?
> > > >
> > > > That's right. It doesn't. When is (sk->sk_allocation & __GFP_WAIT) true?
> > > > Is it possible I'm just not hitting that case?
> > > >
> > >
> > > I hope not. GFP_KERNEL has __GFP_WAIT.
> > >
> > > > Is it possible that this only affects certain traffic patterns (I only
> > > > really tried ssh/scp and ping)? Or perhaps its just that the swiotlb is
> > > > only broken in one corner case and not the other.
> > >
> > > Could you try a netperf -t TCP_STREAM ?
> >
> > For fun I did a couple of tests - I setup two machines (one r8168, the other
> > e1000e) and tried to do netperf/netserver. Both of them are running a baremetal
> > kernel and one of them has 'iommu=soft swiotlb=force' to simulate the worst
> > case. This is using v3.7-rc3.
>
> I also did a test with the patch at the top, with the same setup and ... it
> does look like it fixes some issues, but not the underlaying one.
>
> The same test, with net.core.netdev_frag_page_max_order=0, the e1000e->r8169
> gets ~124, but then on subsequent runs it picks up to ~933. If I let the
> machine stay a bit idle and then do this again, it does around ~124 again.
>
> Thoughts?
Argh. Please disregard this test. I added an extra patch in the kernel
tree to track the SWIOTLB bounce usage and print it.. and the printk was
going out to the FB, which was not too fast - so the whole test was being
slowed down by FB drivers :-)
Will re-run this test without the offending patch.
^ permalink raw reply
* Re: [PATCH v3 10/10] net/macb: add pinctrl consumer support
From: Nicolas Ferre @ 2012-10-31 10:42 UTC (permalink / raw)
To: Joachim Eastwood
Cc: netdev, davem, havard, bhutchings, linux-arm-kernel, plagnioj,
patrice.vilchez, linux-kernel
In-Reply-To: <CAGhQ9VxJ58ej3j9eEx1rex5h-ua_C53=Z4WwGk5YdjfaYTdHpw@mail.gmail.com>
On 10/30/2012 06:37 PM, Joachim Eastwood :
> On 30 October 2012 11:18, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
>> From: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>>
>> If no pinctrl available just report a warning as some architecture may not
>> need to do anything.
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> [nicolas.ferre@atmel.com: adapt the error path]
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Cc: netdev@vger.kernel.org
>> ---
>> drivers/net/ethernet/cadence/macb.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index 4d51877..eae3d74 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -26,6 +26,9 @@
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> #include <linux/of_net.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/gpio.h>
> Why are these two headers added?
> I don't see anything from them used in the code added.
>
> Wouldn't the pinctrl header by itself be sufficient?
It is: I will remove the two unneeded headers in my v4 patch series.
Thanks,
>
>> +#include <linux/pinctrl/consumer.h>
>>
>> #include "macb.h"
>>
>> @@ -1472,6 +1475,7 @@ static int __init macb_probe(struct platform_device *pdev)
>> struct phy_device *phydev;
>> u32 config;
>> int err = -ENXIO;
>> + struct pinctrl *pinctrl;
>>
>> regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> if (!regs) {
>> @@ -1479,6 +1483,15 @@ static int __init macb_probe(struct platform_device *pdev)
>> goto err_out;
>> }
>>
>> + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>> + if (IS_ERR(pinctrl)) {
>> + err = PTR_ERR(pinctrl);
>> + if (err == -EPROBE_DEFER)
>> + goto err_out;
>> +
>> + dev_warn(&pdev->dev, "No pinctrl provided\n");
>> + }
>> +
>> err = -ENOMEM;
>> dev = alloc_etherdev(sizeof(*bp));
>> if (!dev)
Best regards,
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH v3 06/10] net/macb: clean up ring buffer logic
From: Nicolas Ferre @ 2012-10-31 10:39 UTC (permalink / raw)
To: Håvard Skinnemoen, David Laight
Cc: netdev, manabian, patrice.vilchez, linux-kernel, bhutchings,
plagnioj, davem, linux-arm-kernel
In-Reply-To: <5090F67C.9020104@atmel.com>
On 10/31/2012 10:59 AM, Nicolas Ferre :
> On 10/30/2012 07:22 PM, Håvard Skinnemoen :
>> On Tue, Oct 30, 2012 at 4:12 AM, David Laight <David.Laight@aculab.com> wrote:
>>>> Instead of masking head and tail every time we increment them, just let them
>>>> wrap through UINT_MAX and mask them when subscripting. Add simple accessor
>>>> functions to do the subscripting properly to minimize the chances of messing
>>>> this up.
>>> ...
>>>> +static unsigned int macb_tx_ring_avail(struct macb *bp)
>>>> +{
>>>> + return TX_RING_SIZE - (bp->tx_head - bp->tx_tail);
>>>> +}
>>>
>>> That one doesn't look quite right to me.
>>> Surely it should be masking with 'TX_RING_SIZE - 1'
>>
>> Why is that? head and tail can never be more than TX_RING_SIZE apart,
>> so it shouldn't make any difference.
>
> Absolutely.
Well not so absolute, after having thinking twice ;-)
We should move to:
static unsigned int macb_tx_ring_avail(struct macb *bp)
{
return (TX_RING_SIZE - (bp->tx_head - bp->tx_tail) & (TX_RING_SIZE - 1));
}
Thanks David!
(sorry for the noise) Bye,
--
Nicolas Ferre
^ permalink raw reply
* [PATCH net] net: fix divide by zero in tcp algorithm illinois
From: Jesper Dangaard Brouer @ 2012-10-31 10:37 UTC (permalink / raw)
To: David S. Miller
Cc: Jesper Dangaard Brouer, netdev, Petr Matousek, Stephen Hemminger
Reading TCP stats when using TCP Illinois congestion control algorithm
can cause a divide by zero kernel oops.
The division by zero occur in tcp_illinois_info() at:
do_div(t, ca->cnt_rtt);
where ca->cnt_rtt can become zero (when rtt_reset is called)
Steps to Reproduce:
1. Register tcp_illinois:
# sysctl -w net.ipv4.tcp_congestion_control=illinois
2. Monitor internal TCP information via command "ss -i"
# watch -d ss -i
3. Establish new TCP conn to machine
Either it fails at the initial conn, or else it needs to wait
for a loss or a reset.
This is only related to reading stats. The function avg_delay() also
performs the same divide, but is guarded with a (ca->cnt_rtt > 0) at its
calling point in update_params(). Thus, simply fix tcp_illinois_info().
To be on the safe side, I use a local stack variable in tcp_illinois_info()
to eliminate any race conditions. I'm not sure this is needed, as this
would also affect avg_delay(), if this race exists. (Although this is likely
already "fix" by compiler optimization and kept in a local register)
Cc: Petr Matousek <pmatouse@redhat.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
net/ipv4/tcp_illinois.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/tcp_illinois.c b/net/ipv4/tcp_illinois.c
index 813b43a..343f160 100644
--- a/net/ipv4/tcp_illinois.c
+++ b/net/ipv4/tcp_illinois.c
@@ -306,6 +306,7 @@ static void tcp_illinois_info(struct sock *sk, u32 ext,
struct sk_buff *skb)
{
const struct illinois *ca = inet_csk_ca(sk);
+ u16 cnt_rtt;
if (ext & (1 << (INET_DIAG_VEGASINFO - 1))) {
struct tcpvegas_info info = {
@@ -315,8 +316,11 @@ static void tcp_illinois_info(struct sock *sk, u32 ext,
};
u64 t = ca->sum_rtt;
- do_div(t, ca->cnt_rtt);
- info.tcpv_rtt = t;
+ cnt_rtt = ca->cnt_rtt;
+ if (cnt_rtt > 0) {
+ do_div(t, cnt_rtt);
+ info.tcpv_rtt = t;
+ }
nla_put(skb, INET_DIAG_VEGASINFO, sizeof(info), &info);
}
^ permalink raw reply related
* Re: [rfc net-next v6 0/3] Multiqueue virtio-net
From: Jason Wang @ 2012-10-31 10:33 UTC (permalink / raw)
To: Rick Jones
Cc: krkumar2, kvm, mst, netdev, linux-kernel, virtualization, davem
In-Reply-To: <509024F4.8080408@hp.com>
On 10/31/2012 03:05 AM, Rick Jones wrote:
> On 10/30/2012 03:03 AM, Jason Wang wrote:
>> Hi all:
>>
>> This series is an update version of multiqueue virtio-net driver
>> based on
>> Krishna Kumar's work to let virtio-net use multiple rx/tx queues to
>> do the
>> packets reception and transmission. Please review and comments.
>>
>> Changes from v5:
>> - Align the implementation with the RFC spec update v4
>> - Switch the mode between single mode and multiqueue mode without reset
>> - Remove the 256 limitation of queues
>> - Use helpers to do the mapping between virtqueues and tx/rx queues
>> - Use commbined channels instead of separated rx/tx queus when do the
>> queue
>> number configuartion
>> - Other coding style comments from Michael
>>
>> Reference:
>> - A protype implementation of qemu-kvm support could by found in
>> git://github.com/jasowang/qemu-kvm-mq.git
>> - V5 could be found at http://lwn.net/Articles/505388/
>> - V4 could be found at https://lkml.org/lkml/2012/6/25/120
>> - V2 could be found at http://lwn.net/Articles/467283/
>> - Michael virtio-spec:
>> http://www.spinics.net/lists/netdev/msg209986.html
>>
>> Perf Numbers:
>>
>> - Pktgen test shows the receiving capability of the multiqueue
>> virtio-net were
>> dramatically improved.
>> - Netperf result shows latency were greately improved according to
>> the test
>> result.
>
> I suppose it is technically correct to say that latency was improved,
> but usually for aggregate request/response tests I tend to talk about
> the aggregate transactions per second.
Sure.
>
> Do you have a hypothesis as to why the improvement dropped going to 20
> concurrent sessions from 10?
>
> rick jones
I'm investigating this issuse currently, but with no much ideas. The
aggregate transactions per second scales pretty well even with 20
cocurrent sessions when doing test between a local host and a local vm.
Looks like some bottleneck were reached when doing testing over 10gb or
vms as even if I increase the number of sessions, the result would not
increase.
^ permalink raw reply
* [PATCHv2 net-next 7/8] vhost-net: select tx zero copy dynamically
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
Even when vhost-net is in zero-copy transmit mode,
net core might still decide to copy the skb later
which is somewhat slower than a copy in user
context: data copy overhead is added to the cost of
page pin/unpin. The result is that enabling tx zero copy
option leads to higher CPU utilization for guest to guest
and guest to host traffic.
To fix this, suppress zero copy tx after a given number of
packets triggered late data copy. Re-enable periodically
to detect workload changes.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 50 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 532fc88..8e9de79 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -42,6 +42,21 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Experimental Zero Copy TX");
#define VHOST_MAX_PEND 128
#define VHOST_GOODCOPY_LEN 256
+/*
+ * For transmit, used buffer len is unused; we override it to track buffer
+ * status internally; used for zerocopy tx only.
+ */
+/* Lower device DMA failed */
+#define VHOST_DMA_FAILED_LEN 3
+/* Lower device DMA done */
+#define VHOST_DMA_DONE_LEN 2
+/* Lower device DMA in progress */
+#define VHOST_DMA_IN_PROGRESS 1
+/* Buffer unused */
+#define VHOST_DMA_CLEAR_LEN 0
+
+#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
+
enum {
VHOST_NET_VQ_RX = 0,
VHOST_NET_VQ_TX = 1,
@@ -62,8 +77,33 @@ struct vhost_net {
* We only do this when socket buffer fills up.
* Protected by tx vq lock. */
enum vhost_net_poll_state tx_poll_state;
+ /* Number of TX recently submitted.
+ * Protected by tx vq lock. */
+ unsigned tx_packets;
+ /* Number of times zerocopy TX recently failed.
+ * Protected by tx vq lock. */
+ unsigned tx_zcopy_err;
};
+static void vhost_net_tx_packet(struct vhost_net *net)
+{
+ ++net->tx_packets;
+ if (net->tx_packets < 1024)
+ return;
+ net->tx_packets = 0;
+ net->tx_zcopy_err = 0;
+}
+
+static void vhost_net_tx_err(struct vhost_net *net)
+{
+ ++net->tx_zcopy_err;
+}
+
+static bool vhost_net_tx_select_zcopy(struct vhost_net *net)
+{
+ return net->tx_packets / 64 >= net->tx_zcopy_err;
+}
+
static bool vhost_sock_zcopy(struct socket *sock)
{
return unlikely(experimental_zcopytx) &&
@@ -131,12 +171,15 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock)
* of used idx. Once lower device DMA done contiguously, we will signal KVM
* guest used idx.
*/
-int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq)
+static int vhost_zerocopy_signal_used(struct vhost_net *net,
+ struct vhost_virtqueue *vq)
{
int i;
int j = 0;
for (i = vq->done_idx; i != vq->upend_idx; i = (i + 1) % UIO_MAXIOV) {
+ if (vq->heads[i].len == VHOST_DMA_FAILED_LEN)
+ vhost_net_tx_err(net);
if (VHOST_DMA_IS_DONE(vq->heads[i].len)) {
vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
vhost_add_used_and_signal(vq->dev, vq,
@@ -208,7 +251,7 @@ static void handle_tx(struct vhost_net *net)
for (;;) {
/* Release DMAs done buffers first */
if (zcopy)
- vhost_zerocopy_signal_used(vq);
+ vhost_zerocopy_signal_used(net, vq);
head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
ARRAY_SIZE(vq->iov),
@@ -263,7 +306,8 @@ static void handle_tx(struct vhost_net *net)
/* use msg_control to pass vhost zerocopy ubuf info to skb */
if (zcopy) {
vq->heads[vq->upend_idx].id = head;
- if (len < VHOST_GOODCOPY_LEN) {
+ if (!vhost_net_tx_select_zcopy(net) ||
+ len < VHOST_GOODCOPY_LEN) {
/* copy don't need to wait for DMA done */
vq->heads[vq->upend_idx].len =
VHOST_DMA_DONE_LEN;
@@ -305,8 +349,9 @@ static void handle_tx(struct vhost_net *net)
if (!zcopy)
vhost_add_used_and_signal(&net->dev, vq, head, 0);
else
- vhost_zerocopy_signal_used(vq);
+ vhost_zerocopy_signal_used(net, vq);
total_len += len;
+ vhost_net_tx_packet(net);
if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
vhost_poll_queue(&vq->poll);
break;
@@ -774,7 +819,7 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
if (oldubufs) {
vhost_ubuf_put_and_wait(oldubufs);
mutex_lock(&vq->mutex);
- vhost_zerocopy_signal_used(vq);
+ vhost_zerocopy_signal_used(n, vq);
mutex_unlock(&vq->mutex);
}
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 6/8] vhost: move -net specific code out
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
Zerocopy handling code is vhost-net specific.
Move it from vhost.c/vhost.h out to net.c
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 45 ++++++++++++++++++++++++++++++++++++++++
drivers/vhost/tcm_vhost.c | 1 +
drivers/vhost/vhost.c | 53 +++++++----------------------------------------
drivers/vhost/vhost.h | 21 +++----------------
4 files changed, 56 insertions(+), 64 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index f80ae5f..532fc88 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -126,6 +126,42 @@ static void tx_poll_start(struct vhost_net *net, struct socket *sock)
net->tx_poll_state = VHOST_NET_POLL_STARTED;
}
+/* In case of DMA done not in order in lower device driver for some reason.
+ * upend_idx is used to track end of used idx, done_idx is used to track head
+ * of used idx. Once lower device DMA done contiguously, we will signal KVM
+ * guest used idx.
+ */
+int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq)
+{
+ int i;
+ int j = 0;
+
+ for (i = vq->done_idx; i != vq->upend_idx; i = (i + 1) % UIO_MAXIOV) {
+ if (VHOST_DMA_IS_DONE(vq->heads[i].len)) {
+ vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
+ vhost_add_used_and_signal(vq->dev, vq,
+ vq->heads[i].id, 0);
+ ++j;
+ } else
+ break;
+ }
+ if (j)
+ vq->done_idx = i;
+ return j;
+}
+
+static void vhost_zerocopy_callback(struct ubuf_info *ubuf, int status)
+{
+ struct vhost_ubuf_ref *ubufs = ubuf->ctx;
+ struct vhost_virtqueue *vq = ubufs->vq;
+
+ vhost_poll_queue(&vq->poll);
+ /* set len to mark this desc buffers done DMA */
+ vq->heads[ubuf->desc].len = status ?
+ VHOST_DMA_FAILED_LEN : VHOST_DMA_DONE_LEN;
+ vhost_ubuf_put(ubufs);
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_tx(struct vhost_net *net)
@@ -594,9 +630,18 @@ static int vhost_net_release(struct inode *inode, struct file *f)
struct vhost_net *n = f->private_data;
struct socket *tx_sock;
struct socket *rx_sock;
+ int i;
vhost_net_stop(n, &tx_sock, &rx_sock);
vhost_net_flush(n);
+ vhost_dev_stop(&n->dev);
+ for (i = 0; i < n->dev.nvqs; ++i) {
+ /* Wait for all lower device DMAs done. */
+ if (n->dev.vqs[i].ubufs)
+ vhost_ubuf_put_and_wait(n->dev.vqs[i].ubufs);
+
+ vhost_zerocopy_signal_used(n, &n->dev.vqs[i]);
+ }
vhost_dev_cleanup(&n->dev, false);
if (tx_sock)
fput(tx_sock->file);
diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index aa31692..23c138f 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -895,6 +895,7 @@ static int vhost_scsi_release(struct inode *inode, struct file *f)
vhost_scsi_clear_endpoint(s, &backend);
}
+ vhost_dev_stop(&s->dev);
vhost_dev_cleanup(&s->dev, false);
kfree(s);
return 0;
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 5affce3..ef8f598 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -26,10 +26,6 @@
#include <linux/kthread.h>
#include <linux/cgroup.h>
-#include <linux/net.h>
-#include <linux/if_packet.h>
-#include <linux/if_arp.h>
-
#include "vhost.h"
enum {
@@ -414,28 +410,16 @@ long vhost_dev_reset_owner(struct vhost_dev *dev)
return 0;
}
-/* In case of DMA done not in order in lower device driver for some reason.
- * upend_idx is used to track end of used idx, done_idx is used to track head
- * of used idx. Once lower device DMA done contiguously, we will signal KVM
- * guest used idx.
- */
-int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq)
+void vhost_dev_stop(struct vhost_dev *dev)
{
int i;
- int j = 0;
-
- for (i = vq->done_idx; i != vq->upend_idx; i = (i + 1) % UIO_MAXIOV) {
- if (VHOST_DMA_IS_DONE(vq->heads[i].len)) {
- vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
- vhost_add_used_and_signal(vq->dev, vq,
- vq->heads[i].id, 0);
- ++j;
- } else
- break;
+
+ for (i = 0; i < dev->nvqs; ++i) {
+ if (dev->vqs[i].kick && dev->vqs[i].handle_kick) {
+ vhost_poll_stop(&dev->vqs[i].poll);
+ vhost_poll_flush(&dev->vqs[i].poll);
+ }
}
- if (j)
- vq->done_idx = i;
- return j;
}
/* Caller should have device mutex if and only if locked is set */
@@ -444,17 +428,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
int i;
for (i = 0; i < dev->nvqs; ++i) {
- if (dev->vqs[i].kick && dev->vqs[i].handle_kick) {
- vhost_poll_stop(&dev->vqs[i].poll);
- vhost_poll_flush(&dev->vqs[i].poll);
- }
- /* Wait for all lower device DMAs done. */
- if (dev->vqs[i].ubufs)
- vhost_ubuf_put_and_wait(dev->vqs[i].ubufs);
-
- /* Signal guest as appropriate. */
- vhost_zerocopy_signal_used(&dev->vqs[i]);
-
if (dev->vqs[i].error_ctx)
eventfd_ctx_put(dev->vqs[i].error_ctx);
if (dev->vqs[i].error)
@@ -1599,15 +1572,3 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs)
wait_event(ubufs->wait, !atomic_read(&ubufs->kref.refcount));
kfree(ubufs);
}
-
-void vhost_zerocopy_callback(struct ubuf_info *ubuf, int status)
-{
- struct vhost_ubuf_ref *ubufs = ubuf->ctx;
- struct vhost_virtqueue *vq = ubufs->vq;
-
- vhost_poll_queue(&vq->poll);
- /* set len to mark this desc buffers done DMA */
- vq->heads[ubuf->desc].len = status ?
- VHOST_DMA_FAILED_LEN : VHOST_DMA_DONE_LEN;
- kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
-}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 6fdf31d..5e19e3d 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -7,27 +7,11 @@
#include <linux/mutex.h>
#include <linux/poll.h>
#include <linux/file.h>
-#include <linux/skbuff.h>
#include <linux/uio.h>
#include <linux/virtio_config.h>
#include <linux/virtio_ring.h>
#include <linux/atomic.h>
-/*
- * For transmit, used buffer len is unused; we override it to track buffer
- * status internally; used for zerocopy tx only.
- */
-/* Lower device DMA failed */
-#define VHOST_DMA_FAILED_LEN 3
-/* Lower device DMA done */
-#define VHOST_DMA_DONE_LEN 2
-/* Lower device DMA in progress */
-#define VHOST_DMA_IN_PROGRESS 1
-/* Buffer unused */
-#define VHOST_DMA_CLEAR_LEN 0
-
-#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
-
struct vhost_device;
struct vhost_work;
@@ -80,6 +64,8 @@ struct vhost_ubuf_ref *vhost_ubuf_alloc(struct vhost_virtqueue *, bool zcopy);
void vhost_ubuf_put(struct vhost_ubuf_ref *);
void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *);
+struct ubuf_info;
+
/* The virtqueue structure describes a queue attached to a device. */
struct vhost_virtqueue {
struct vhost_dev *dev;
@@ -177,6 +163,7 @@ long vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue *vqs, int nvqs);
long vhost_dev_check_owner(struct vhost_dev *);
long vhost_dev_reset_owner(struct vhost_dev *);
void vhost_dev_cleanup(struct vhost_dev *, bool locked);
+void vhost_dev_stop(struct vhost_dev *);
long vhost_dev_ioctl(struct vhost_dev *, unsigned int ioctl, unsigned long arg);
int vhost_vq_access_ok(struct vhost_virtqueue *vq);
int vhost_log_access_ok(struct vhost_dev *);
@@ -201,8 +188,6 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
-void vhost_zerocopy_callback(struct ubuf_info *, int);
-int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq);
#define vq_err(vq, fmt, ...) do { \
pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 5/8] vhost: track zero copy failures using DMA length
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, David S. Miller, Eric Dumazet, Andrew Morton,
Alexander Duyck, Ian Campbell, kvm, virtualization, netdev,
linux-kernel
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
This will be used to disable zerocopy when error rate
is high.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 7 ++++---
drivers/vhost/vhost.h | 4 ++++
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 906fd9f..5affce3 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -425,7 +425,7 @@ int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq)
int j = 0;
for (i = vq->done_idx; i != vq->upend_idx; i = (i + 1) % UIO_MAXIOV) {
- if ((vq->heads[i].len == VHOST_DMA_DONE_LEN)) {
+ if (VHOST_DMA_IS_DONE(vq->heads[i].len)) {
vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
vhost_add_used_and_signal(vq->dev, vq,
vq->heads[i].id, 0);
@@ -1600,13 +1600,14 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs)
kfree(ubufs);
}
-void vhost_zerocopy_callback(struct ubuf_info *ubuf, int zerocopy_status)
+void vhost_zerocopy_callback(struct ubuf_info *ubuf, int status)
{
struct vhost_ubuf_ref *ubufs = ubuf->ctx;
struct vhost_virtqueue *vq = ubufs->vq;
vhost_poll_queue(&vq->poll);
/* set len to mark this desc buffers done DMA */
- vq->heads[ubuf->desc].len = VHOST_DMA_DONE_LEN;
+ vq->heads[ubuf->desc].len = status ?
+ VHOST_DMA_FAILED_LEN : VHOST_DMA_DONE_LEN;
kref_put(&ubufs->kref, vhost_zerocopy_done_signal);
}
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index ad72a1f..6fdf31d 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -17,6 +17,8 @@
* For transmit, used buffer len is unused; we override it to track buffer
* status internally; used for zerocopy tx only.
*/
+/* Lower device DMA failed */
+#define VHOST_DMA_FAILED_LEN 3
/* Lower device DMA done */
#define VHOST_DMA_DONE_LEN 2
/* Lower device DMA in progress */
@@ -24,6 +26,8 @@
/* Buffer unused */
#define VHOST_DMA_CLEAR_LEN 0
+#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
+
struct vhost_device;
struct vhost_work;
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 8/8] vhost-net: reduce vq polling on tx zerocopy
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
It seems that to avoid deadlocks it is enough to poll vq before
we are going to use the last buffer. This is faster than
c70aa540c7a9f67add11ad3161096fb95233aa2e.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 8e9de79..88beedb 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -197,8 +197,18 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, int status)
{
struct vhost_ubuf_ref *ubufs = ubuf->ctx;
struct vhost_virtqueue *vq = ubufs->vq;
-
- vhost_poll_queue(&vq->poll);
+ int cnt = atomic_read(&ubufs->kref.refcount);
+
+ /*
+ * Trigger polling thread if guest stopped submitting new buffers:
+ * in this case, the refcount after decrement will eventually reach 1
+ * so here it is 2.
+ * We also trigger polling periodically after each 16 packets
+ * (the value 16 here is more or less arbitrary, it's tuned to trigger
+ * less than 10% of times).
+ */
+ if (cnt <= 2 || !(cnt % 16))
+ vhost_poll_queue(&vq->poll);
/* set len to mark this desc buffers done DMA */
vq->heads[ubuf->desc].len = status ?
VHOST_DMA_FAILED_LEN : VHOST_DMA_DONE_LEN;
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 3/8] tun: report orphan frags errors to zero copy callback
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
When tun transmits a zero copy skb, it orphans the frags
which might need to allocate extra memory, in atomic context.
If that fails, notify ubufs callback before freeing the skb
as a hint that device should disable zerocopy mode.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/tun.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 3157519..613f826 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -433,6 +433,7 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
drop:
dev->stats.tx_dropped++;
+ skb_tx_error(skb, -ENOMEM);
kfree_skb(skb);
return NETDEV_TX_OK;
}
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 2/8] skb: api to report errors for zero copy skbs
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
Orphaning frags for zero copy skbs needs to allocate data in atomic
context so is has a chance to fail. If it does we currently discard
the skb which is safe, but we don't report anything to the caller,
so it can not recover by e.g. disabling zero copy.
Add an API to free skb reporting such errors: this is used
by tun in case orphaning frags fails.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/skbuff.h | 1 +
net/core/skbuff.c | 20 ++++++++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 8bac11b..0644432 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -568,6 +568,7 @@ static inline struct rtable *skb_rtable(const struct sk_buff *skb)
}
extern void kfree_skb(struct sk_buff *skb);
+extern void skb_tx_error(struct sk_buff *skb, int err);
extern void consume_skb(struct sk_buff *skb);
extern void __kfree_skb(struct sk_buff *skb);
extern struct kmem_cache *skbuff_head_cache;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index eb31f6e..2e7a1fd 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -635,6 +635,26 @@ void kfree_skb(struct sk_buff *skb)
EXPORT_SYMBOL(kfree_skb);
/**
+ * skb_tx_error - report an sk_buff xmit error
+ * @skb: buffer that triggered an error
+ *
+ * Report xmit error if a device callback is tracking this skb.
+ * skb must be freed afterwards.
+ */
+void skb_tx_error(struct sk_buff *skb, int err)
+{
+ if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
+ struct ubuf_info *uarg;
+
+ uarg = skb_shinfo(skb)->destructor_arg;
+ if (uarg->callback)
+ uarg->callback(uarg, err);
+ skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
+ }
+}
+EXPORT_SYMBOL(skb_tx_error);
+
+/**
* consume_skb - free an skbuff
* @skb: buffer to free
*
--
MST
^ permalink raw reply related
* [PATCHv2 net-next 1/8] skb: report completion status for zero copy skbs
From: Michael S. Tsirkin @ 2012-10-31 10:31 UTC (permalink / raw)
Cc: Vlad Yasevich, Alexander Duyck, Ian Campbell, kvm, netdev,
linux-kernel, virtualization, Eric Dumazet, Andrew Morton,
David S. Miller
In-Reply-To: <cover.1351679008.git.mst@redhat.com>
Even if skb is marked for zero copy, net core might still decide
to copy it later which is somewhat slower than a copy in user context:
besides copying the data we need to pin/unpin the pages.
Add a parameter reporting such cases through zero copy callback:
if this happens a lot, device can take this into account
and switch to copying in user context.
This patch updates all users but ignores the passed value for now:
it will be used by follow-up patches.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/vhost.c | 2 +-
drivers/vhost/vhost.h | 2 +-
include/linux/skbuff.h | 4 +++-
net/core/skbuff.c | 4 ++--
4 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 99ac2cb..92308b6 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1600,7 +1600,7 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs)
kfree(ubufs);
}
-void vhost_zerocopy_callback(struct ubuf_info *ubuf)
+void vhost_zerocopy_callback(struct ubuf_info *ubuf, int zerocopy_status)
{
struct vhost_ubuf_ref *ubufs = ubuf->ctx;
struct vhost_virtqueue *vq = ubufs->vq;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 1125af3..eb7263c3 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -191,7 +191,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
-void vhost_zerocopy_callback(struct ubuf_info *);
+void vhost_zerocopy_callback(struct ubuf_info *, int);
int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq);
#define vq_err(vq, fmt, ...) do { \
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6a2c34e..8bac11b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -235,11 +235,13 @@ enum {
/*
* The callback notifies userspace to release buffers when skb DMA is done in
* lower device, the skb last reference should be 0 when calling this.
+ * The zerocopy_status argument is 0 if zero copy transmit occurred,
+ * 1 on successful data copy; < 0 on out of memory error.
* The ctx field is used to track device context.
* The desc field is used to track userspace buffer index.
*/
struct ubuf_info {
- void (*callback)(struct ubuf_info *);
+ void (*callback)(struct ubuf_info *, int zerocopy_status);
void *ctx;
unsigned long desc;
};
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 6e04b1f..eb31f6e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -519,7 +519,7 @@ static void skb_release_data(struct sk_buff *skb)
uarg = skb_shinfo(skb)->destructor_arg;
if (uarg->callback)
- uarg->callback(uarg);
+ uarg->callback(uarg, 0);
}
if (skb_has_frag_list(skb))
@@ -797,7 +797,7 @@ int skb_copy_ubufs(struct sk_buff *skb, gfp_t gfp_mask)
for (i = 0; i < num_frags; i++)
skb_frag_unref(skb, i);
- uarg->callback(uarg);
+ uarg->callback(uarg, 1);
/* skb frags point to kernel buffers */
for (i = num_frags - 1; i >= 0; i--) {
--
MST
^ permalink raw reply related
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