* Re: [PATCHv2 net-next 01/15] net: add skb_dst_set_unref
From: Julian Anastasov @ 2013-03-10 22:05 UTC (permalink / raw)
To: David Miller; +Cc: horms, lvs-devel, netdev
In-Reply-To: <20130310.170004.2239870728229481365.davem@davemloft.net>
Hello,
On Sun, 10 Mar 2013, David Miller wrote:
> From: Julian Anastasov <ja@ssi.bg>
> Date: Sun, 10 Mar 2013 15:37:34 +0200 (EET)
>
> > The idea looks good, here is the implementation.
> > Can I use it in this form for next patchset versions?
>
> Yes, but with one minor change:
>
> > - if (unlikely(dst->flags & DST_NOCACHE)) {
> > + if (unlikely(dst->flags & DST_NOCACHE && !force)) {
>
> I keep forgetting the && vs. & operator precedence, and so
> do many other people, so please put parenthesis around
> the "dst->flags & DST_NOCACHE" expression, thanks.
Done, I'll wait a week for more feedback...
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply
* Fwd: [PATCH] phydev: Add sysctl variable for polling interval of phy
From: EUNBONG SONG @ 2013-03-10 23:44 UTC (permalink / raw)
To: netdev
From d55a22be52e5a768409aa0999d6636cdfc369676 Mon Sep 17 00:00:00 2001
From: eunbonsong
Date: Sun, 10 Mar 2013 04:57:39 -0700
Subject: [PATCH] phydev: Add sysctl variable for polling interval of phy state
This adds a dev.phy.phy_poll_interval sysctl variable. This value is represented in milliseconds.
And phy_state_machine() is scheduled as this variable.
I think HZ is enough for PC. But sometimes especially in network devices
such as switches,routers, needs more granularity for detecting phy state change.
---
drivers/net/phy/phy.c | 4 +++-
drivers/net/phy/phy_device.c | 43 ++++++++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 1 -
3 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ef9ea92..126a69f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -42,6 +42,8 @@
#include
#include
+extern unsigned long sysctl_phy_poll_interval;
+
/**
* phy_print_status - Convenience function to print out the current phy status
* @phydev: the phy_device struct
@@ -966,7 +968,7 @@ void phy_state_machine(struct work_struct *work)
if (err < 0)
phy_error(phydev);
- schedule_delayed_work(&phydev->state_queue, PHY_STATE_TIME * HZ);
+ schedule_delayed_work(&phydev->state_queue, msecs_to_jiffies(sysctl_phy_poll_interval));
}
static inline void mmd_phy_indirect(struct mii_bus *bus, int prtad, int devad,
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3657b4a..c2697e2 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
@@ -42,6 +43,45 @@ MODULE_DESCRIPTION("PHY library");
MODULE_AUTHOR("Andy Fleming");
MODULE_LICENSE("GPL");
+unsigned long sysctl_phy_poll_interval = 1000;
+static unsigned long min_phy_poll_interval = 1;
+static unsigned long max_phy_poll_interval = 10000;
+
+static struct ctl_table_header *phy_table_header;
+
+static ctl_table phy_table[] = {
+ {
+ .procname = "phy_poll_interval",
+ .data = &sysctl_phy_poll_interval,
+ .maxlen = sizeof(long),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &min_phy_poll_interval,
+ .extra2 = &max_phy_poll_interval,
+ },
+ { }
+};
+
+static ctl_table phy_dir_table[] = {
+ {
+ .procname = "phy",
+ .maxlen = 0,
+ .mode = 0555,
+ .child = phy_table,
+ },
+ {}
+};
+
+static ctl_table phy_root_table[] = {
+ {
+ .procname = "dev",
+ .maxlen = 0,
+ .mode = 0555,
+ .child = phy_dir_table,
+ },
+ {}
+};
+
void phy_device_free(struct phy_device *phydev)
{
put_device(&phydev->dev);
@@ -1134,6 +1174,8 @@ static int __init phy_init(void)
if (rc)
mdio_bus_exit();
+ phy_table_header = register_sysctl_table(phy_root_table);
+
return rc;
}
@@ -1141,6 +1183,7 @@ static void __exit phy_exit(void)
{
phy_driver_unregister(&genphy_driver);
mdio_bus_exit();
+ unregister_sysctl_table(phy_table_header);
}
subsys_initcall(phy_init);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 33999ad..0cbc1fe 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -68,7 +68,6 @@ typedef enum {
#define PHY_INIT_TIMEOUT 100000
-#define PHY_STATE_TIME 1
#define PHY_FORCE_TIMEOUT 10
#define PHY_AN_TIMEOUT 10
--
1.7.10.4
^ permalink raw reply related
* Re: r8169 regression: UDP packets dropped intermittantly
From: Francois Romieu @ 2013-03-11 0:02 UTC (permalink / raw)
To: Jonathan Woithe; +Cc: netdev
In-Reply-To: <20130309064647.GC4984@marvin.atrad.com.au>
Jonathan Woithe <jwoithe@atrad.com.au> :
[...]
> While we can rely on the retry system to paper over this regression, the
> behaviour of earlier kernels suggests that there is a real issue at play.
> Is there any chance that this can be looked at ?
Yes.
You'll have to be a bit patient since I still have to spend a pair of non
worked hours on a compromised system. :o/
Take it for what it's worth but I don't test much with PREEMPT, NOHZ or
SLUB and I never tried all those at the same time.
--
Ueimor
^ permalink raw reply
* Re: r8169 regression: UDP packets dropped intermittantly
From: Jonathan Woithe @ 2013-03-11 0:17 UTC (permalink / raw)
To: Francois Romieu; +Cc: netdev, Jonathan Woithe
In-Reply-To: <20130311000216.GA31808@zoreil.com>
On Mon, Mar 11, 2013 at 01:02:16AM +0100, Francois Romieu wrote:
> Jonathan Woithe <jwoithe@atrad.com.au> :
> [...]
> > While we can rely on the retry system to paper over this regression, the
> > behaviour of earlier kernels suggests that there is a real issue at play.
> > Is there any chance that this can be looked at ?
>
> Yes.
>
> You'll have to be a bit patient since I still have to spend a pair of non
> worked hours on a compromised system. :o/
That's fine. Do I wait for you to ping me about this?
> Take it for what it's worth but I don't test much with PREEMPT, NOHZ or
> SLUB and I never tried all those at the same time.
Fair enough. I'm not using CONFIG_PREEMPT but NOHZ and SLUB are both in the
mix. In the meantime I can easily test to see if turning off
CONFIG_PREEMPT_VOLUNTARY, CONFIG_NO_HZ and/or moving to SLAB changes things.
I'll report the findings.
Regards
jonathan
^ permalink raw reply
* Re: hitting lockdep warning as of too early VF probe with 3.9-rc1
From: Ming Lei @ 2013-03-11 1:26 UTC (permalink / raw)
To: Jack Morgenstein
Cc: Or Gerlitz, Or Gerlitz, Greg Kroah-Hartman, David Miller,
Roland Dreier, netdev, Yan Burman, Liran Liss
In-Reply-To: <201303101728.50883.jackm@dev.mellanox.co.il>
On Sun, Mar 10, 2013 at 11:28 PM, Jack Morgenstein
<jackm@dev.mellanox.co.il> wrote:
> Hello, Ming, Greg, Roland, Dave, all...
>
> From a quick scan of ethernet drivers in Dave Miller's net-next git, I
> notice that the following drivers (apart from the Mellanox mlx4 driver)
> enable SRIOV during the PF probe:
> cisco enic (function "enic_probe")
> neterion vxge driver(function "vxge_probe")
> Solarflare efx driver (function "efx_pci_probe", which invokes "efx_sriov_init")
> emulex driver (function "be_probe" --> be_setup --> be_vf_setup)
>
> It would seem that these drivers are susceptible to the nested probe/deadlock
> race condition as well.
>
> I believe that it is healthiest for everyone if the probe code in the kernel itself
> would avoid such nested probe calls (rather than forcing vendors to deal
> with this issue). The kernel code is certainly aware
> (or could easily track) that it is invoking the a driver's probe function
> while that same probe function has already been invoked and has not yet returned!
Generally nested probe() should be fine if your driver knows how to end
the nesting, also USB drivers use nested probe too, which is healthy. This
report is only related with PCI driver probe, which is scheduled as work
function, so flush_work() around nested probe() may cause deadlock.
IMO, fixing the problem generally might need to touch PCI driver probe
code(pci_call_probe).
Thanks,
--
Ming Lei
^ permalink raw reply
* Re: [Patch net] bridge: do not expire mdb entry when bridge still uses it
From: Cong Wang @ 2013-03-11 1:44 UTC (permalink / raw)
To: Herbert Xu; +Cc: Stephen Hemminger, netdev, bridge, David S. Miller, Adam Baker
In-Reply-To: <20130308112438.GA29924@gondor.apana.org.au>
On Fri, 2013-03-08 at 19:24 +0800, Herbert Xu wrote:
> On Fri, Mar 08, 2013 at 03:26:21PM +0800, Cong Wang wrote:
> >
> > Hmm, what the user experiences is the multicast traffic between the
> > bridge and some port is broken. Isn't this expected to work by default?
>
> Oh I'm not arguing that this isn't a bug. I'm trying to ensure
> that we come up with the right fix.
Do you have any idea of a better fix?
>
> > Or we can just change the default value of br->multicast_router to 2?
>
> No because that will cause all sorts of stuff to be thrown at
> the host.
>
OK.
Thanks!
^ permalink raw reply
* Re: be2net failed to initialize regression
From: CAI Qian @ 2013-03-11 2:07 UTC (permalink / raw)
To: Sathya Perla; +Cc: Ivan Vecera, LKML, netdev
In-Reply-To: <CF9D1877D81D214CB0CA0669EFAE020C0F259965@CMEXMB1.ad.emulex.com>
----- Original Message -----
> From: "Sathya Perla" <Sathya.Perla@Emulex.Com>
> To: "CAI Qian" <caiqian@redhat.com>, netdev@vger.kernel.org
> Cc: "Ivan Vecera" <ivecera@redhat.com>, "LKML" <linux-kernel@vger.kernel.org>
> Sent: Friday, March 8, 2013 6:22:24 PM
> Subject: RE: be2net failed to initialize regression
>
> > -----Original Message-----
> > From: netdev-owner@vger.kernel.org
> > [mailto:netdev-owner@vger.kernel.org]
> > On Behalf Of CAI Qian
> >
> > Emulex CNA card failed to initialize using 3.8 and the latest
> > upstream kernel,
> >
> > [ 87.479859] be2net 0000:04:00.0: POST timeout; stage=0xc911
> > [ 87.515978] be2net 0000:04:00.0: Emulex OneConnect
> > initialization failed
> > [ 87.557130] be2net: probe of 0000:04:00.0 failed with error -1
> >
> > lspci output,
> >
> > 04:00.0 Ethernet controller: Emulex Corporation OneConnect 10Gb NIC
> > (rev 02)
> > 04:00.1 Ethernet controller: Emulex Corporation OneConnect 10Gb NIC
> > (rev 02)
> >
> > 04:00.1 Ethernet controller: Emulex Corporation OneConnect 10Gb NIC
> > (rev 02)
> > Subsystem: Hewlett-Packard Company NC551i Dual Port
> > FlexFabric 10Gb
> > Adapter
> >
> > Confirmed no such problem using 3.7 kernel. Reproduced every time
> > and still
> > bisecting. Just want to give an early head-up to see if anyone saw
> > sometime
> > obvious.
>
> Could you give me the FW version (ethtool -i) of the adapter (after
> be2net successfully
> probes in a 3.7 kernel.)
firmware-version: 2.104.281.0
>
> If the FW version is as old as 2.x, then the culprit commit that
> broke compatibility with old FW versions
> on some (BE2) chips I is:
> commit 1bc8e7e4f36c0c19dd7dea29e7c248b7c6ef3a15
> be2net: fix access to SEMAPHORE reg
>
> The fix for this is (still on David's net tree I guess):
> commit c5b3ad4c67989c778e4753be4f91dc7193a04d21
> be2net: use CSR-BAR SEMAPHORE reg for BE2/BE3
>
> Dave, can these 2 commits be queued for stable too:
> commit1: c5b3ad4c67989c778e4753be4f91dc7193a04d21
> commit2: 1bc8e7e4f36c0c19dd7dea29e7c248b7c6ef3a15
>
> thanks,
> -Sathya
>
^ permalink raw reply
* Re: [PATCH] sctp: don't break the loop while meeting the active_path so as to find the matched transport
From: Xufeng Zhang @ 2013-03-11 2:14 UTC (permalink / raw)
To: Neil Horman
Cc: Xufeng Zhang, vyasevich, davem, linux-sctp, netdev, linux-kernel
In-Reply-To: <20130308142720.GA9873@hmsreliant.think-freely.org>
On 3/8/13, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Fri, Mar 08, 2013 at 03:39:37PM +0800, Xufeng Zhang wrote:
>> sctp_assoc_lookup_tsn() function searchs which transport a certain TSN
>> was sent on, if not found in the active_path transport, then go search
>> all the other transports in the peer's transport_addr_list, however, we
>> should continue to the next entry rather than break the loop when meet
>> the active_path transport.
>>
>> Signed-off-by: Xufeng Zhang <xufeng.zhang@windriver.com>
>> ---
>> net/sctp/associola.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>> index 43cd0dd..d2709e2 100644
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -1079,7 +1079,7 @@ struct sctp_transport *sctp_assoc_lookup_tsn(struct
>> sctp_association *asoc,
>> transports) {
>>
>> if (transport == active)
>> - break;
>> + continue;
>> list_for_each_entry(chunk, &transport->transmitted,
>> transmitted_list) {
>> if (key == chunk->subh.data_hdr->tsn) {
>> --
>> 1.7.0.2
>>
>>
>
> This works, but what might be better would be if we did a move to front
> heuristic in sctp_assoc_set_primary. E.g. when we set the active_path, move
> the
> requisite transport to the front of the transport_addr_list. If we did
> that,
> then we could just do one for loop in sctp_assoc_lookup_tsn and wind up
> implicitly check the active path first without having to check it seprately
> and
> skip it in the second for loop.
Thanks for your review, Neil!
I know what you mean, yes, it's most probably that the searched TSN was
transmitted in the currently active_path, but what should we do if not.
Check the comment in sctp_assoc_lookup_tsn() function:
/* Let's be hopeful and check the active_path first. */
/* If not found, go search all the other transports. */
It has checked the active_path first and then traverse all the other
transports in
the transport_addr_list except the active_path.
So what I want to fix here is the inconsistency between the function
should do and
the code actually does.
Thanks,
Xufeng
>
> Neil
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply
* Re: [PATCH net-next] cxgb4: Allow for backward compatibility with new VPD scheme.
From: Vipul Pandya @ 2013-03-11 5:51 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, Divy Le Ray, Dimitrios Michailidis,
Abhishek Agrawal, Santosh Rastapur
In-Reply-To: <20130308.124411.445253877641244372.davem@davemloft.net>
On 08-03-2013 23:14, David Miller wrote:
> From: Vipul Pandya <vipul@chelsio.com>
> Date: Fri, 8 Mar 2013 19:05:29 +0530
>
>> From: Santosh Rastapur <santosh@chelsio.com>
>>
>> New scheme calls for 3rd party VPD at offset 0x0 and Chelsio VPD at offset
>> 0x400 of the function. If no 3rd party VPD is present, then a copy of
>> Chelsio's VPD will be at offset 0x0 to keep in line with PCI spec which
>> requires the VPD to be present at offset 0x0.
>>
>> Signed-off-by: Santosh Rastapur <santosh@chelsio.com>
>> Signed-off-by: Vipul Pandya <vipul@chelsio.com>
>
> Isn't this a fix which is better targetted at 'net' than 'net-next'?
>
We have very soon new patch series coming up for net-next. This patch
requires for that patch series to work correctly. So, we would like to
get this merge in net-next tree.
Thanks,
Vipul
^ permalink raw reply
* Re: [PATCH net-next 2/2] bnx2x: use the default NAPI weight
From: Eric Dumazet @ 2013-03-11 6:14 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, netdev, eilong, jhs, Tom Herbert
In-Reply-To: <20130310083800.GC10448@gondor.apana.org.au>
On Sun, 2013-03-10 at 16:38 +0800, Herbert Xu wrote:
> On Wed, Mar 06, 2013 at 01:52:57PM -0800, Eric Dumazet wrote:
> >
> > - BQL (incurring more TX completion rounds and possibility to
> > block/unblock a qdisc)
> > - ticket spinlocks, and even with the guard of qdisc busylock
> >
> > -> we can have a starvation problem.
>
> This only happens in cases where we aren't using multiqueue or
> we're using it incorrectly, resulting in TX work from being split
> over CPUs.
>
Or using qdisc like HTB ;)
> In that case it's not clear that it is starvation if we keep the
> TX processing on one CPU.
Thats not always the case. TX path has interesting features like XPS ...
We probably could add instrumentation and keep track of the maximum time
we are spending in this dark area.
^ permalink raw reply
* Re: TCP small packets throughput and multiqueue virtio-net
From: Jason Wang @ 2013-03-11 6:17 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Michael S. Tsirkin, KVM, David Miller, Stephen Hemminger,
jpirko
In-Reply-To: <1362755115.15793.236.camel@edumazet-glaptop>
On 03/08/2013 11:05 PM, Eric Dumazet wrote:
> On Fri, 2013-03-08 at 14:24 +0800, Jason Wang wrote:
>> Hello all:
>>
>> I meet an issue when testing multiqueue virtio-net. When I testing guest
>> small packets stream sending performance with netperf. I find an
>> regression of multiqueue. When I run 2 sessions of TCP_STREAM test with
>> 1024 byte from guest to local host, I get following result:
>>
>> 1q result: 3457.64
>> 2q result: 7781.45
>>
>> Statistics shows that: compared with one queue, multiqueue tends to send
>> much more but smaller packets. Tcpdump shows single queue has a much
>> higher possibility to produce a 64K gso packet compared to multiqueue.
>> More but smaller packets will cause more vmexits and interrupts which
>> lead a degradation on throughput.
>>
>> Then problem only exist for small packets sending. When I test with
>> larger size, multiqueue will gradually outperfrom single queue. And
>> multiqueue also outperfrom in both TCP_RR and pktgen test (even with
>> small packets). The problem disappear when I turn off both gso and tso.
>>
> This makes little sense to me : TCP_RR workload (assuming one byte
> payload) cannot use GSO or TSO anyway. Same for pktgen as it uses UDP.
>
>> I'm not sure whether it's a bug or expected since anyway we get
>> improvement on latency. And if it's a bug, I suspect it was related to
>> TCP GSO batching algorithm who tends to batch less in this situation. (
>> Jiri Pirko suspect it was the defect of virtio-net driver, but I didn't
>> find any obvious clue on this). After some experiments, I find the it
>> maybe related to tcp_tso_should_defer(), after
>> 1) change the tcp_tso_win_divisor to 1
>> 2) the following changes
>> the throughput were almost the same (but still a little worse) as single
>> queue:
>>
>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>> index fd0cea1..dedd2a6 100644
>> --- a/net/ipv4/tcp_output.c
>> +++ b/net/ipv4/tcp_output.c
>> @@ -1777,10 +1777,12 @@ static bool tcp_tso_should_defer(struct sock
>> *sk, struct sk_buff *skb)
>>
>> limit = min(send_win, cong_win);
>>
>> +#if 0
>> /* If a full-sized TSO skb can be sent, do it. */
>> if (limit >= min_t(unsigned int, sk->sk_gso_max_size,
>> sk->sk_gso_max_segs * tp->mss_cache))
>> goto send_now;
>> +#endif
>>
>> /* Middle in queue won't get any more data, full sendable
>> already? */
>> if ((skb != tcp_write_queue_tail(sk)) && (limit >= skb->len))
>>
>> Git history shows this check were added for both westwood and fasttcp,
>> I'm not familiar with tcp but looks like we can easily hit this check
>> under when multiqueue is enabled for virtio-net. Maybe I was wrong but I
>> wonder whether we can still do some batching here.
>>
>> Any comments, thoughts are welcomed.
>>
> Well, the point is : if your app does write(1024) bytes, thats probably
> because it wants small packets from the very beginning. (See the TCP
> PUSH flag ?)
Didn't fully understand the question, but according to the tcpdump, TCP
PUSH flag were seen in very few packets.
> If the transport is slow, TCP stack will automatically collapse several
> write into single skbs (assuming TSO or GSO is on), and you'll see big
> GSO packets with tcpdump [1]. So TCP will help you to get less overhead
> in this case.
>
> But if transport is fast, you'll see small packets, and thats good for
> latency.
>
> So my opinion is : its exactly behaving as expected.
>
> If you want bigger packets either :
> - Make the application doing big write()
> - Slow the vmexit ;)
Good to know this, thanks for the explanation.
> [1] GSO fools tcpdump : Actual packets sent to the wire are not 'big
> packets', but they hit dev_hard_start_xmit() as GSO packets.
>
>
>
^ permalink raw reply
* Re: TCP small packets throughput and multiqueue virtio-net
From: Jason Wang @ 2013-03-11 6:21 UTC (permalink / raw)
To: Rick Jones
Cc: Eric Dumazet, netdev, Michael S. Tsirkin, KVM, David Miller,
Stephen Hemminger, jpirko
In-Reply-To: <513A1F36.5020401@hp.com>
On 03/09/2013 01:26 AM, Rick Jones wrote:
>
>>
>> Well, the point is : if your app does write(1024) bytes, thats probably
>> because it wants small packets from the very beginning. (See the TCP
>> PUSH flag ?)
>
> I think that raises the question of whether or not Jason was setting
> the test-specific -D option on his TCP_STREAM tests, to have netperf
> make a setsockopt(TCP_NODELAY) call.
I didn't set the -D option to disable nagle. But I get almost the almost
same result with -D (1024 byte TCP_STREAM from guest to local host).
>
> happy benchmarking,
>
> rick jones
>
>> If the transport is slow, TCP stack will automatically collapse several
>> write into single skbs (assuming TSO or GSO is on), and you'll see big
>> GSO packets with tcpdump [1]. So TCP will help you to get less overhead
>> in this case.
>>
>> But if transport is fast, you'll see small packets, and thats good for
>> latency.
>>
>> So my opinion is : its exactly behaving as expected.
>>
>> If you want bigger packets either :
>> - Make the application doing big write()
>> - Slow the vmexit ;)
>>
>> [1] GSO fools tcpdump : Actual packets sent to the wire are not 'big
>> packets', but they hit dev_hard_start_xmit() as GSO packets.
>>
>>
>>
>> --
>> 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-next] cxgb4: Allow for backward compatibility with new VPD scheme.
From: David Miller @ 2013-03-11 6:30 UTC (permalink / raw)
To: vipul; +Cc: netdev, divy, dm, abhishek, santosh
In-Reply-To: <513D70F7.4090708@chelsio.com>
From: Vipul Pandya <vipul@chelsio.com>
Date: Mon, 11 Mar 2013 11:21:51 +0530
>
>
> On 08-03-2013 23:14, David Miller wrote:
>> From: Vipul Pandya <vipul@chelsio.com>
>> Date: Fri, 8 Mar 2013 19:05:29 +0530
>>
>>> From: Santosh Rastapur <santosh@chelsio.com>
>>>
>>> New scheme calls for 3rd party VPD at offset 0x0 and Chelsio VPD at offset
>>> 0x400 of the function. If no 3rd party VPD is present, then a copy of
>>> Chelsio's VPD will be at offset 0x0 to keep in line with PCI spec which
>>> requires the VPD to be present at offset 0x0.
>>>
>>> Signed-off-by: Santosh Rastapur <santosh@chelsio.com>
>>> Signed-off-by: Vipul Pandya <vipul@chelsio.com>
>>
>> Isn't this a fix which is better targetted at 'net' than 'net-next'?
>>
>
> We have very soon new patch series coming up for net-next. This patch
> requires for that patch series to work correctly. So, we would like to
> get this merge in net-next tree.
But you can achieve that by asking me to apply this to 'net' and then
when the patch series that depends upon it is posted, you tell me about
this dependency.
^ permalink raw reply
* Re: [net-next.git 6/9] stmmac: add missing supported filters to get_ts_info
From: Giuseppe CAVALLARO @ 2013-03-11 6:39 UTC (permalink / raw)
To: Richard Cochran; +Cc: netdev, bh74.an, ayagond
In-Reply-To: <20130310123632.GC6407@netboy.at.omicron.at>
On 3/10/2013 1:36 PM, Richard Cochran wrote:
> On Thu, Mar 07, 2013 at 11:50:16AM +0100, Giuseppe CAVALLARO wrote:
>> This patch updates the filters for ethtool's get_ts_info to return support for
>> all filters which can be supported after having added the PTP support.
>
> This code should ideally appear in the same patch where the filtering
> support first appears.
Ok, I'll add it inside the patch that adds the support.
peppe
>
> Thanks,
> Richard
> --
> 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-next] cxgb4: Allow for backward compatibility with new VPD scheme.
From: Vipul Pandya @ 2013-03-11 6:53 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, Divy Le Ray, Dimitrios Michailidis,
Abhishek Agrawal, Santosh Rastapur
In-Reply-To: <20130311.023009.1276623167785151916.davem@davemloft.net>
On 11-03-2013 12:00, David Miller wrote:
> From: Vipul Pandya <vipul@chelsio.com>
> Date: Mon, 11 Mar 2013 11:21:51 +0530
>
>>
>>
>> On 08-03-2013 23:14, David Miller wrote:
>>> From: Vipul Pandya <vipul@chelsio.com>
>>> Date: Fri, 8 Mar 2013 19:05:29 +0530
>>>
>>>> From: Santosh Rastapur <santosh@chelsio.com>
>>>>
>>>> New scheme calls for 3rd party VPD at offset 0x0 and Chelsio VPD at offset
>>>> 0x400 of the function. If no 3rd party VPD is present, then a copy of
>>>> Chelsio's VPD will be at offset 0x0 to keep in line with PCI spec which
>>>> requires the VPD to be present at offset 0x0.
>>>>
>>>> Signed-off-by: Santosh Rastapur <santosh@chelsio.com>
>>>> Signed-off-by: Vipul Pandya <vipul@chelsio.com>
>>>
>>> Isn't this a fix which is better targetted at 'net' than 'net-next'?
>>>
>>
>> We have very soon new patch series coming up for net-next. This patch
>> requires for that patch series to work correctly. So, we would like to
>> get this merge in net-next tree.
>
> But you can achieve that by asking me to apply this to 'net' and then
> when the patch series that depends upon it is posted, you tell me about
> this dependency.
>
Ok. I was not aware of this. However there can be a merge conflict at
the time of merging 'net' and 'net-next' tree if this patch gets applied
in 'net' tree and our new patch series gets applied in 'net-next'. To
avoid it we recommended the same. The merge conflict wont be a major and
if it is alright to resolve it we can apply this patch to 'net' tree.
Thanks,
Vipul
^ permalink raw reply
* Re: [PATCHv2] net: can: af_can.c: Fix checkpatch warnings
From: Oliver Hartkopp @ 2013-03-11 7:02 UTC (permalink / raw)
To: Valentin Ilie; +Cc: davem, linux-can, netdev, linux-kernel
In-Reply-To: <1362950113-25515-1-git-send-email-valentin.ilie@gmail.com>
On 10.03.2013 22:15, Valentin Ilie wrote:
> Replace printk(KERN_ERR with pr_err
> Add space before {
> Removed OOM messages
>
> Signed-off-by: Valentin Ilie <valentin.ilie@gmail.com>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Thanks Valentin
> ---
> net/can/af_can.c | 21 ++++++++-------------
> 1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index c48e522..8bacf28 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -525,7 +525,7 @@ void can_rx_unregister(struct net_device *dev, canid_t can_id, canid_t mask,
>
> d = find_dev_rcv_lists(dev);
> if (!d) {
> - printk(KERN_ERR "BUG: receive list not found for "
> + pr_err("BUG: receive list not found for "
> "dev %s, id %03X, mask %03X\n",
> DNAME(dev), can_id, mask);
> goto out;
> @@ -552,7 +552,7 @@ void can_rx_unregister(struct net_device *dev, canid_t can_id, canid_t mask,
> */
>
> if (!r) {
> - printk(KERN_ERR "BUG: receive list entry not found for "
> + pr_err("BUG: receive list entry not found for "
> "dev %s, id %03X, mask %03X\n",
> DNAME(dev), can_id, mask);
> r = NULL;
> @@ -749,8 +749,7 @@ int can_proto_register(const struct can_proto *cp)
> int err = 0;
>
> if (proto < 0 || proto >= CAN_NPROTO) {
> - printk(KERN_ERR "can: protocol number %d out of range\n",
> - proto);
> + pr_err("can: protocol number %d out of range\n", proto);
> return -EINVAL;
> }
>
> @@ -761,8 +760,7 @@ int can_proto_register(const struct can_proto *cp)
> mutex_lock(&proto_tab_lock);
>
> if (proto_tab[proto]) {
> - printk(KERN_ERR "can: protocol %d already registered\n",
> - proto);
> + pr_err("can: protocol %d already registered\n", proto);
> err = -EBUSY;
> } else
> RCU_INIT_POINTER(proto_tab[proto], cp);
> @@ -816,11 +814,8 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
>
> /* create new dev_rcv_lists for this device */
> d = kzalloc(sizeof(*d), GFP_KERNEL);
> - if (!d) {
> - printk(KERN_ERR
> - "can: allocation of receive list failed\n");
> + if (!d)
> return NOTIFY_DONE;
> - }
> BUG_ON(dev->ml_priv);
> dev->ml_priv = d;
>
> @@ -838,8 +833,8 @@ static int can_notifier(struct notifier_block *nb, unsigned long msg,
> dev->ml_priv = NULL;
> }
> } else
> - printk(KERN_ERR "can: notifier: receive list not "
> - "found for dev %s\n", dev->name);
> + pr_err("can: notifier: receive list not found for dev "
> + "%s\n", dev->name);
>
> spin_unlock(&can_rcvlists_lock);
>
> @@ -927,7 +922,7 @@ static __exit void can_exit(void)
> /* remove created dev_rcv_lists from still registered CAN devices */
> rcu_read_lock();
> for_each_netdev_rcu(&init_net, dev) {
> - if (dev->type == ARPHRD_CAN && dev->ml_priv){
> + if (dev->type == ARPHRD_CAN && dev->ml_priv) {
>
> struct dev_rcv_lists *d = dev->ml_priv;
>
^ permalink raw reply
* Re: [PATCH] vhost_net: remove tx polling state
From: Jason Wang @ 2013-03-11 7:09 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20130310165030.GA13687@redhat.com>
On 03/11/2013 12:50 AM, Michael S. Tsirkin wrote:
> On Thu, Mar 07, 2013 at 12:31:56PM +0800, Jason Wang wrote:
>> After commit 2b8b328b61c799957a456a5a8dab8cc7dea68575 (vhost_net: handle polling
>> errors when setting backend), we in fact track the polling state through
>> poll->wqh, so there's no need to duplicate the work with an extra
>> vhost_net_polling_state. So this patch removes this and make the code simpler.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> I'd prefer a more radical approach, since I think it can be even
> simpler: tap and macvtap backends both only send events when tx queue
> overruns which should almost never happen.
>
> So let's just start polling when VQ is enabled
> drop all poll_start/stop calls on data path.
I think then it may damage the performance at least for tx. We
conditionally enable the tx polling in the past to reduce the
unnecessary wakeups of vhost thead when zerocopy or sndbuf is used. If
we don't stop the polling, after each packet were transmitted,
tap/macvtap will try to wakeup the vhost thread.
Actually, I'm considering the reverse direction. Looks like we can
disable the rx polling like what we do for tx in handle_tx() to reduce
the uncessary wakeups.
> The optimization was written for packet socket backend but I know of no
> one caring about performance of that one that much.
> Needs a bit of perf testing to make sure I didn't miss anything though
> but it's not 3.9 material anyway so there's no rush.
>
>> ---
>> drivers/vhost/net.c | 60 ++++++++----------------------------------------
>> drivers/vhost/vhost.c | 3 ++
>> 2 files changed, 13 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 959b1cd..d1a03dd 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -64,20 +64,10 @@ enum {
>> VHOST_NET_VQ_MAX = 2,
>> };
>>
>> -enum vhost_net_poll_state {
>> - VHOST_NET_POLL_DISABLED = 0,
>> - VHOST_NET_POLL_STARTED = 1,
>> - VHOST_NET_POLL_STOPPED = 2,
>> -};
>> -
>> struct vhost_net {
>> struct vhost_dev dev;
>> struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX];
>> struct vhost_poll poll[VHOST_NET_VQ_MAX];
>> - /* Tells us whether we are polling a socket for TX.
>> - * 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;
>> @@ -155,28 +145,6 @@ static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
>> }
>> }
>>
>> -/* Caller must have TX VQ lock */
>> -static void tx_poll_stop(struct vhost_net *net)
>> -{
>> - if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
>> - return;
>> - vhost_poll_stop(net->poll + VHOST_NET_VQ_TX);
>> - net->tx_poll_state = VHOST_NET_POLL_STOPPED;
>> -}
>> -
>> -/* Caller must have TX VQ lock */
>> -static int tx_poll_start(struct vhost_net *net, struct socket *sock)
>> -{
>> - int ret;
>> -
>> - if (unlikely(net->tx_poll_state != VHOST_NET_POLL_STOPPED))
>> - return 0;
>> - ret = vhost_poll_start(net->poll + VHOST_NET_VQ_TX, sock->file);
>> - if (!ret)
>> - net->tx_poll_state = VHOST_NET_POLL_STARTED;
>> - return ret;
>> -}
>> -
>> /* 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
>> @@ -231,6 +199,7 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
>> static void handle_tx(struct vhost_net *net)
>> {
>> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
>> + struct vhost_poll *poll = net->poll + VHOST_NET_VQ_TX;
>> unsigned out, in, s;
>> int head;
>> struct msghdr msg = {
>> @@ -256,7 +225,7 @@ static void handle_tx(struct vhost_net *net)
>> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
>> if (wmem >= sock->sk->sk_sndbuf) {
>> mutex_lock(&vq->mutex);
>> - tx_poll_start(net, sock);
>> + vhost_poll_start(poll, sock->file);
>> mutex_unlock(&vq->mutex);
>> return;
>> }
>> @@ -265,7 +234,7 @@ static void handle_tx(struct vhost_net *net)
>> vhost_disable_notify(&net->dev, vq);
>>
>> if (wmem < sock->sk->sk_sndbuf / 2)
>> - tx_poll_stop(net);
>> + vhost_poll_stop(poll);
>> hdr_size = vq->vhost_hlen;
>> zcopy = vq->ubufs;
>>
>> @@ -287,7 +256,7 @@ static void handle_tx(struct vhost_net *net)
>>
>> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
>> if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
>> - tx_poll_start(net, sock);
>> + vhost_poll_start(poll, sock->file);
>> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
>> break;
>> }
>> @@ -298,7 +267,7 @@ static void handle_tx(struct vhost_net *net)
>> (vq->upend_idx - vq->done_idx) :
>> (vq->upend_idx + UIO_MAXIOV - vq->done_idx);
>> if (unlikely(num_pends > VHOST_MAX_PEND)) {
>> - tx_poll_start(net, sock);
>> + vhost_poll_start(poll, sock->file);
>> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
>> break;
>> }
>> @@ -364,7 +333,7 @@ static void handle_tx(struct vhost_net *net)
>> }
>> vhost_discard_vq_desc(vq, 1);
>> if (err == -EAGAIN || err == -ENOBUFS)
>> - tx_poll_start(net, sock);
>> + vhost_poll_start(poll, sock->file);
>> break;
>> }
>> if (err != len)
>> @@ -627,7 +596,6 @@ static int vhost_net_open(struct inode *inode, struct file *f)
>>
>> vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT, dev);
>> vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN, dev);
>> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
>>
>> f->private_data = n;
>>
>> @@ -637,32 +605,24 @@ static int vhost_net_open(struct inode *inode, struct file *f)
>> static void vhost_net_disable_vq(struct vhost_net *n,
>> struct vhost_virtqueue *vq)
>> {
>> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
>> if (!vq->private_data)
>> return;
>> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
>> - tx_poll_stop(n);
>> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
>> - } else
>> - vhost_poll_stop(n->poll + VHOST_NET_VQ_RX);
>> + vhost_poll_stop(poll);
>> }
>>
>> static int vhost_net_enable_vq(struct vhost_net *n,
>> struct vhost_virtqueue *vq)
>> {
>> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
>> struct socket *sock;
>> - int ret;
>>
>> sock = rcu_dereference_protected(vq->private_data,
>> lockdep_is_held(&vq->mutex));
>> if (!sock)
>> return 0;
>> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
>> - n->tx_poll_state = VHOST_NET_POLL_STOPPED;
>> - ret = tx_poll_start(n, sock);
>> - } else
>> - ret = vhost_poll_start(n->poll + VHOST_NET_VQ_RX, sock->file);
>>
>> - return ret;
>> + return vhost_poll_start(poll, sock->file);
>> }
>>
>> static struct socket *vhost_net_stop_vq(struct vhost_net *n,
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 9759249..4eecdb8 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -89,6 +89,9 @@ int vhost_poll_start(struct vhost_poll *poll, struct file *file)
>> unsigned long mask;
>> int ret = 0;
>>
>> + if (poll->wqh)
>> + return 0;
>> +
>> mask = file->f_op->poll(file, &poll->table);
>> if (mask)
>> vhost_poll_wakeup(&poll->wait, 0, 0, (void *)mask);
>> --
>> 1.7.1
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [net-next.git 0/9] stmmac: update to March_2013 (adding PTP & RGMII/SGMII)
From: Giuseppe CAVALLARO @ 2013-03-11 7:11 UTC (permalink / raw)
To: David Miller; +Cc: netdev, bh74.an, rayagond
In-Reply-To: <20130308.112219.1999774630969953487.davem@davemloft.net>
On 3/8/2013 5:22 PM, David Miller wrote:
> From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
> Date: Fri, 08 Mar 2013 08:16:24 +0100
>
>> Hello David
>>
>> On 3/7/2013 9:34 PM, David Miller wrote:
>>> From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
>>> Date: Thu, 7 Mar 2013 11:50:10 +0100
>>>
>>>> The PTP support is quite intrusive because it needs to support the
>>>> extended
>>>> descriptors used for saving the HW timestamps.
>>>> These are available in new chip generations, only.
>>>> So we have actually found useful to use some Kconfig options to
>>>> surround PTP and extended descriptor support. This approach helped on
>>>> old platform (embeeded system) where PTP is not supported and where we
>>>> do not want to pay extra code and check in critical rx/tx paths.
>>>
>>> I would prefer run time handling of all of this.
>>>
>>> You do not need to put extra checks in the fast paths, instead you
>>> have different methods that get hooked up into the netdev_ops
>>> based upon chip capabilities/features/mode.
> ...
>> Welcome advice.
>
> Exactly what I told you above. You have two sets of routines,
> one that deal with the older descriptors and one set that deals
> with the new descriptors that can do PTP.
>
> And you hook up different interrupt handler, NAPI poll, and
> netdev_ops based upon the chip's capabilities.
>
> You need no runtime testing, only a test a probe time to setup
> things up properly.
> What is so hard about this to understand?
This is exactly what the driver does to manage different chip versions
w/o using any ifdef. So fortunately I had already understood this :-).
Unfortunately, just in this case, I've met some problems on
implementing the code because We have three cases:
1- normal desc
2- enhanced desc.
3- enhanced desc + extended desc.
-------------
|_ Also used for PTPv2
To support extended desc (3) we have to modify the main descriptor
structure and it is not good for the case (2) where the DMA will expect
to use these descriptors:
-------------
| des0 | status / conf
-------------
| des1 | status / conf
-------------
| des2 | Buffer 1 Address [31:0]
-------------
| des3 | Buffer 2 Address [31:0] or Next Descriptor Addr
-------------
but we have added other 4 fields:
struct des {
unsigned int des0;
unsigned int des1;
unsigned int des2;
unsigned int des3;
unsigned int des4; |
unsigned int des5; |_ extended
unsigned int des6; |
unsigned int des7; |
}
and this structure cannot be used.
In fact, if the HW supports the extended desc we have to program a
register to allow the DMA to walk through the new structure.
At any case, I'll arrange to change the code removing the Koption and
continuing (as done in the past) to setup at probe time the right
configuration. This will be in the next version of the patches.
Thx
Peppe
^ permalink raw reply
* Re: [PATCH] vhost_net: remove tx polling state
From: Jason Wang @ 2013-03-11 7:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <513D8316.90907@redhat.com>
On 03/11/2013 03:09 PM, Jason Wang wrote:
> On 03/11/2013 12:50 AM, Michael S. Tsirkin wrote:
>> On Thu, Mar 07, 2013 at 12:31:56PM +0800, Jason Wang wrote:
>>> After commit 2b8b328b61c799957a456a5a8dab8cc7dea68575 (vhost_net: handle polling
>>> errors when setting backend), we in fact track the polling state through
>>> poll->wqh, so there's no need to duplicate the work with an extra
>>> vhost_net_polling_state. So this patch removes this and make the code simpler.
>>>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> I'd prefer a more radical approach, since I think it can be even
>> simpler: tap and macvtap backends both only send events when tx queue
>> overruns which should almost never happen.
>>
>> So let's just start polling when VQ is enabled
>> drop all poll_start/stop calls on data path.
> I think then it may damage the performance at least for tx. We
> conditionally enable the tx polling in the past to reduce the
> unnecessary wakeups of vhost thead when zerocopy or sndbuf is used. If
> we don't stop the polling, after each packet were transmitted,
> tap/macvtap will try to wakeup the vhost thread.
Or for zerocopy, looks like we can in fact just depends on
ubuf->callback in stead of tx polling to wakeup tx thread to do tx
completion, so we can even drop the num_pends and VHOST_MAX_PEND stuffs.
> Actually, I'm considering the reverse direction. Looks like we can
> disable the rx polling like what we do for tx in handle_tx() to reduce
> the uncessary wakeups.
>> The optimization was written for packet socket backend but I know of no
>> one caring about performance of that one that much.
>> Needs a bit of perf testing to make sure I didn't miss anything though
>> but it's not 3.9 material anyway so there's no rush.
>>
>>> ---
>>> drivers/vhost/net.c | 60 ++++++++----------------------------------------
>>> drivers/vhost/vhost.c | 3 ++
>>> 2 files changed, 13 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>> index 959b1cd..d1a03dd 100644
>>> --- a/drivers/vhost/net.c
>>> +++ b/drivers/vhost/net.c
>>> @@ -64,20 +64,10 @@ enum {
>>> VHOST_NET_VQ_MAX = 2,
>>> };
>>>
>>> -enum vhost_net_poll_state {
>>> - VHOST_NET_POLL_DISABLED = 0,
>>> - VHOST_NET_POLL_STARTED = 1,
>>> - VHOST_NET_POLL_STOPPED = 2,
>>> -};
>>> -
>>> struct vhost_net {
>>> struct vhost_dev dev;
>>> struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX];
>>> struct vhost_poll poll[VHOST_NET_VQ_MAX];
>>> - /* Tells us whether we are polling a socket for TX.
>>> - * 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;
>>> @@ -155,28 +145,6 @@ static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
>>> }
>>> }
>>>
>>> -/* Caller must have TX VQ lock */
>>> -static void tx_poll_stop(struct vhost_net *net)
>>> -{
>>> - if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
>>> - return;
>>> - vhost_poll_stop(net->poll + VHOST_NET_VQ_TX);
>>> - net->tx_poll_state = VHOST_NET_POLL_STOPPED;
>>> -}
>>> -
>>> -/* Caller must have TX VQ lock */
>>> -static int tx_poll_start(struct vhost_net *net, struct socket *sock)
>>> -{
>>> - int ret;
>>> -
>>> - if (unlikely(net->tx_poll_state != VHOST_NET_POLL_STOPPED))
>>> - return 0;
>>> - ret = vhost_poll_start(net->poll + VHOST_NET_VQ_TX, sock->file);
>>> - if (!ret)
>>> - net->tx_poll_state = VHOST_NET_POLL_STARTED;
>>> - return ret;
>>> -}
>>> -
>>> /* 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
>>> @@ -231,6 +199,7 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
>>> static void handle_tx(struct vhost_net *net)
>>> {
>>> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
>>> + struct vhost_poll *poll = net->poll + VHOST_NET_VQ_TX;
>>> unsigned out, in, s;
>>> int head;
>>> struct msghdr msg = {
>>> @@ -256,7 +225,7 @@ static void handle_tx(struct vhost_net *net)
>>> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
>>> if (wmem >= sock->sk->sk_sndbuf) {
>>> mutex_lock(&vq->mutex);
>>> - tx_poll_start(net, sock);
>>> + vhost_poll_start(poll, sock->file);
>>> mutex_unlock(&vq->mutex);
>>> return;
>>> }
>>> @@ -265,7 +234,7 @@ static void handle_tx(struct vhost_net *net)
>>> vhost_disable_notify(&net->dev, vq);
>>>
>>> if (wmem < sock->sk->sk_sndbuf / 2)
>>> - tx_poll_stop(net);
>>> + vhost_poll_stop(poll);
>>> hdr_size = vq->vhost_hlen;
>>> zcopy = vq->ubufs;
>>>
>>> @@ -287,7 +256,7 @@ static void handle_tx(struct vhost_net *net)
>>>
>>> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
>>> if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
>>> - tx_poll_start(net, sock);
>>> + vhost_poll_start(poll, sock->file);
>>> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
>>> break;
>>> }
>>> @@ -298,7 +267,7 @@ static void handle_tx(struct vhost_net *net)
>>> (vq->upend_idx - vq->done_idx) :
>>> (vq->upend_idx + UIO_MAXIOV - vq->done_idx);
>>> if (unlikely(num_pends > VHOST_MAX_PEND)) {
>>> - tx_poll_start(net, sock);
>>> + vhost_poll_start(poll, sock->file);
>>> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
>>> break;
>>> }
>>> @@ -364,7 +333,7 @@ static void handle_tx(struct vhost_net *net)
>>> }
>>> vhost_discard_vq_desc(vq, 1);
>>> if (err == -EAGAIN || err == -ENOBUFS)
>>> - tx_poll_start(net, sock);
>>> + vhost_poll_start(poll, sock->file);
>>> break;
>>> }
>>> if (err != len)
>>> @@ -627,7 +596,6 @@ static int vhost_net_open(struct inode *inode, struct file *f)
>>>
>>> vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT, dev);
>>> vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN, dev);
>>> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
>>>
>>> f->private_data = n;
>>>
>>> @@ -637,32 +605,24 @@ static int vhost_net_open(struct inode *inode, struct file *f)
>>> static void vhost_net_disable_vq(struct vhost_net *n,
>>> struct vhost_virtqueue *vq)
>>> {
>>> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
>>> if (!vq->private_data)
>>> return;
>>> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
>>> - tx_poll_stop(n);
>>> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
>>> - } else
>>> - vhost_poll_stop(n->poll + VHOST_NET_VQ_RX);
>>> + vhost_poll_stop(poll);
>>> }
>>>
>>> static int vhost_net_enable_vq(struct vhost_net *n,
>>> struct vhost_virtqueue *vq)
>>> {
>>> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
>>> struct socket *sock;
>>> - int ret;
>>>
>>> sock = rcu_dereference_protected(vq->private_data,
>>> lockdep_is_held(&vq->mutex));
>>> if (!sock)
>>> return 0;
>>> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
>>> - n->tx_poll_state = VHOST_NET_POLL_STOPPED;
>>> - ret = tx_poll_start(n, sock);
>>> - } else
>>> - ret = vhost_poll_start(n->poll + VHOST_NET_VQ_RX, sock->file);
>>>
>>> - return ret;
>>> + return vhost_poll_start(poll, sock->file);
>>> }
>>>
>>> static struct socket *vhost_net_stop_vq(struct vhost_net *n,
>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>> index 9759249..4eecdb8 100644
>>> --- a/drivers/vhost/vhost.c
>>> +++ b/drivers/vhost/vhost.c
>>> @@ -89,6 +89,9 @@ int vhost_poll_start(struct vhost_poll *poll, struct file *file)
>>> unsigned long mask;
>>> int ret = 0;
>>>
>>> + if (poll->wqh)
>>> + return 0;
>>> +
>>> mask = file->f_op->poll(file, &poll->table);
>>> if (mask)
>>> vhost_poll_wakeup(&poll->wait, 0, 0, (void *)mask);
>>> --
>>> 1.7.1
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH] vhost_net: remove tx polling state
From: Michael S. Tsirkin @ 2013-03-11 8:26 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <513D88BC.4010108@redhat.com>
On Mon, Mar 11, 2013 at 03:33:16PM +0800, Jason Wang wrote:
> On 03/11/2013 03:09 PM, Jason Wang wrote:
> > On 03/11/2013 12:50 AM, Michael S. Tsirkin wrote:
> >> On Thu, Mar 07, 2013 at 12:31:56PM +0800, Jason Wang wrote:
> >>> After commit 2b8b328b61c799957a456a5a8dab8cc7dea68575 (vhost_net: handle polling
> >>> errors when setting backend), we in fact track the polling state through
> >>> poll->wqh, so there's no need to duplicate the work with an extra
> >>> vhost_net_polling_state. So this patch removes this and make the code simpler.
> >>>
> >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> I'd prefer a more radical approach, since I think it can be even
> >> simpler: tap and macvtap backends both only send events when tx queue
> >> overruns which should almost never happen.
> >>
> >> So let's just start polling when VQ is enabled
> >> drop all poll_start/stop calls on data path.
> > I think then it may damage the performance at least for tx. We
> > conditionally enable the tx polling in the past to reduce the
> > unnecessary wakeups of vhost thead when zerocopy or sndbuf is used. If
> > we don't stop the polling, after each packet were transmitted,
> > tap/macvtap will try to wakeup the vhost thread.
>
> Or for zerocopy, looks like we can in fact just depends on
> ubuf->callback in stead of tx polling to wakeup tx thread to do tx
> completion, so we can even drop the num_pends and VHOST_MAX_PEND stuffs.
I think we still need to limit the number of outstanding buffers so
VHOST_MAX_PEND needs to stay. I'm not against switching to callback
for this but let's make this a separate change.
> > Actually, I'm considering the reverse direction. Looks like we can
> > disable the rx polling like what we do for tx in handle_tx() to reduce
> > the uncessary wakeups.
> >> The optimization was written for packet socket backend but I know of no
> >> one caring about performance of that one that much.
> >> Needs a bit of perf testing to make sure I didn't miss anything though
> >> but it's not 3.9 material anyway so there's no rush.
> >>
> >>> ---
> >>> drivers/vhost/net.c | 60 ++++++++----------------------------------------
> >>> drivers/vhost/vhost.c | 3 ++
> >>> 2 files changed, 13 insertions(+), 50 deletions(-)
> >>>
> >>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> >>> index 959b1cd..d1a03dd 100644
> >>> --- a/drivers/vhost/net.c
> >>> +++ b/drivers/vhost/net.c
> >>> @@ -64,20 +64,10 @@ enum {
> >>> VHOST_NET_VQ_MAX = 2,
> >>> };
> >>>
> >>> -enum vhost_net_poll_state {
> >>> - VHOST_NET_POLL_DISABLED = 0,
> >>> - VHOST_NET_POLL_STARTED = 1,
> >>> - VHOST_NET_POLL_STOPPED = 2,
> >>> -};
> >>> -
> >>> struct vhost_net {
> >>> struct vhost_dev dev;
> >>> struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX];
> >>> struct vhost_poll poll[VHOST_NET_VQ_MAX];
> >>> - /* Tells us whether we are polling a socket for TX.
> >>> - * 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;
> >>> @@ -155,28 +145,6 @@ static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
> >>> }
> >>> }
> >>>
> >>> -/* Caller must have TX VQ lock */
> >>> -static void tx_poll_stop(struct vhost_net *net)
> >>> -{
> >>> - if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> >>> - return;
> >>> - vhost_poll_stop(net->poll + VHOST_NET_VQ_TX);
> >>> - net->tx_poll_state = VHOST_NET_POLL_STOPPED;
> >>> -}
> >>> -
> >>> -/* Caller must have TX VQ lock */
> >>> -static int tx_poll_start(struct vhost_net *net, struct socket *sock)
> >>> -{
> >>> - int ret;
> >>> -
> >>> - if (unlikely(net->tx_poll_state != VHOST_NET_POLL_STOPPED))
> >>> - return 0;
> >>> - ret = vhost_poll_start(net->poll + VHOST_NET_VQ_TX, sock->file);
> >>> - if (!ret)
> >>> - net->tx_poll_state = VHOST_NET_POLL_STARTED;
> >>> - return ret;
> >>> -}
> >>> -
> >>> /* 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
> >>> @@ -231,6 +199,7 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> >>> static void handle_tx(struct vhost_net *net)
> >>> {
> >>> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> >>> + struct vhost_poll *poll = net->poll + VHOST_NET_VQ_TX;
> >>> unsigned out, in, s;
> >>> int head;
> >>> struct msghdr msg = {
> >>> @@ -256,7 +225,7 @@ static void handle_tx(struct vhost_net *net)
> >>> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> >>> if (wmem >= sock->sk->sk_sndbuf) {
> >>> mutex_lock(&vq->mutex);
> >>> - tx_poll_start(net, sock);
> >>> + vhost_poll_start(poll, sock->file);
> >>> mutex_unlock(&vq->mutex);
> >>> return;
> >>> }
> >>> @@ -265,7 +234,7 @@ static void handle_tx(struct vhost_net *net)
> >>> vhost_disable_notify(&net->dev, vq);
> >>>
> >>> if (wmem < sock->sk->sk_sndbuf / 2)
> >>> - tx_poll_stop(net);
> >>> + vhost_poll_stop(poll);
> >>> hdr_size = vq->vhost_hlen;
> >>> zcopy = vq->ubufs;
> >>>
> >>> @@ -287,7 +256,7 @@ static void handle_tx(struct vhost_net *net)
> >>>
> >>> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> >>> if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
> >>> - tx_poll_start(net, sock);
> >>> + vhost_poll_start(poll, sock->file);
> >>> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> >>> break;
> >>> }
> >>> @@ -298,7 +267,7 @@ static void handle_tx(struct vhost_net *net)
> >>> (vq->upend_idx - vq->done_idx) :
> >>> (vq->upend_idx + UIO_MAXIOV - vq->done_idx);
> >>> if (unlikely(num_pends > VHOST_MAX_PEND)) {
> >>> - tx_poll_start(net, sock);
> >>> + vhost_poll_start(poll, sock->file);
> >>> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> >>> break;
> >>> }
> >>> @@ -364,7 +333,7 @@ static void handle_tx(struct vhost_net *net)
> >>> }
> >>> vhost_discard_vq_desc(vq, 1);
> >>> if (err == -EAGAIN || err == -ENOBUFS)
> >>> - tx_poll_start(net, sock);
> >>> + vhost_poll_start(poll, sock->file);
> >>> break;
> >>> }
> >>> if (err != len)
> >>> @@ -627,7 +596,6 @@ static int vhost_net_open(struct inode *inode, struct file *f)
> >>>
> >>> vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT, dev);
> >>> vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN, dev);
> >>> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
> >>>
> >>> f->private_data = n;
> >>>
> >>> @@ -637,32 +605,24 @@ static int vhost_net_open(struct inode *inode, struct file *f)
> >>> static void vhost_net_disable_vq(struct vhost_net *n,
> >>> struct vhost_virtqueue *vq)
> >>> {
> >>> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
> >>> if (!vq->private_data)
> >>> return;
> >>> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
> >>> - tx_poll_stop(n);
> >>> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
> >>> - } else
> >>> - vhost_poll_stop(n->poll + VHOST_NET_VQ_RX);
> >>> + vhost_poll_stop(poll);
> >>> }
> >>>
> >>> static int vhost_net_enable_vq(struct vhost_net *n,
> >>> struct vhost_virtqueue *vq)
> >>> {
> >>> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
> >>> struct socket *sock;
> >>> - int ret;
> >>>
> >>> sock = rcu_dereference_protected(vq->private_data,
> >>> lockdep_is_held(&vq->mutex));
> >>> if (!sock)
> >>> return 0;
> >>> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
> >>> - n->tx_poll_state = VHOST_NET_POLL_STOPPED;
> >>> - ret = tx_poll_start(n, sock);
> >>> - } else
> >>> - ret = vhost_poll_start(n->poll + VHOST_NET_VQ_RX, sock->file);
> >>>
> >>> - return ret;
> >>> + return vhost_poll_start(poll, sock->file);
> >>> }
> >>>
> >>> static struct socket *vhost_net_stop_vq(struct vhost_net *n,
> >>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >>> index 9759249..4eecdb8 100644
> >>> --- a/drivers/vhost/vhost.c
> >>> +++ b/drivers/vhost/vhost.c
> >>> @@ -89,6 +89,9 @@ int vhost_poll_start(struct vhost_poll *poll, struct file *file)
> >>> unsigned long mask;
> >>> int ret = 0;
> >>>
> >>> + if (poll->wqh)
> >>> + return 0;
> >>> +
> >>> mask = file->f_op->poll(file, &poll->table);
> >>> if (mask)
> >>> vhost_poll_wakeup(&poll->wait, 0, 0, (void *)mask);
> >>> --
> >>> 1.7.1
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe kvm" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH] vhost_net: remove tx polling state
From: Michael S. Tsirkin @ 2013-03-11 8:29 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <513D8316.90907@redhat.com>
On Mon, Mar 11, 2013 at 03:09:10PM +0800, Jason Wang wrote:
> On 03/11/2013 12:50 AM, Michael S. Tsirkin wrote:
> > On Thu, Mar 07, 2013 at 12:31:56PM +0800, Jason Wang wrote:
> >> After commit 2b8b328b61c799957a456a5a8dab8cc7dea68575 (vhost_net: handle polling
> >> errors when setting backend), we in fact track the polling state through
> >> poll->wqh, so there's no need to duplicate the work with an extra
> >> vhost_net_polling_state. So this patch removes this and make the code simpler.
> >>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > I'd prefer a more radical approach, since I think it can be even
> > simpler: tap and macvtap backends both only send events when tx queue
> > overruns which should almost never happen.
> >
> > So let's just start polling when VQ is enabled
> > drop all poll_start/stop calls on data path.
>
> I think then it may damage the performance at least for tx. We
> conditionally enable the tx polling in the past to reduce the
> unnecessary wakeups of vhost thead when zerocopy or sndbuf is used. If
> we don't stop the polling, after each packet were transmitted,
> tap/macvtap will try to wakeup the vhost thread.
It won't actually.
static void macvtap_sock_write_space(struct sock *sk)
{
wait_queue_head_t *wqueue;
if (!sock_writeable(sk) ||
!test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sk->sk_socket->flags))
return;
wqueue = sk_sleep(sk);
if (wqueue && waitqueue_active(wqueue))
wake_up_interruptible_poll(wqueue, POLLOUT | POLLWRNORM | POLLWRBAND);
}
As long as SOCK_ASYNC_NOSPACE is not set, there's no wakeup.
> Actually, I'm considering the reverse direction. Looks like we can
> disable the rx polling like what we do for tx in handle_tx() to reduce
> the uncessary wakeups.
> > The optimization was written for packet socket backend but I know of no
> > one caring about performance of that one that much.
> > Needs a bit of perf testing to make sure I didn't miss anything though
> > but it's not 3.9 material anyway so there's no rush.
> >
> >> ---
> >> drivers/vhost/net.c | 60 ++++++++----------------------------------------
> >> drivers/vhost/vhost.c | 3 ++
> >> 2 files changed, 13 insertions(+), 50 deletions(-)
> >>
> >> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> >> index 959b1cd..d1a03dd 100644
> >> --- a/drivers/vhost/net.c
> >> +++ b/drivers/vhost/net.c
> >> @@ -64,20 +64,10 @@ enum {
> >> VHOST_NET_VQ_MAX = 2,
> >> };
> >>
> >> -enum vhost_net_poll_state {
> >> - VHOST_NET_POLL_DISABLED = 0,
> >> - VHOST_NET_POLL_STARTED = 1,
> >> - VHOST_NET_POLL_STOPPED = 2,
> >> -};
> >> -
> >> struct vhost_net {
> >> struct vhost_dev dev;
> >> struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX];
> >> struct vhost_poll poll[VHOST_NET_VQ_MAX];
> >> - /* Tells us whether we are polling a socket for TX.
> >> - * 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;
> >> @@ -155,28 +145,6 @@ static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
> >> }
> >> }
> >>
> >> -/* Caller must have TX VQ lock */
> >> -static void tx_poll_stop(struct vhost_net *net)
> >> -{
> >> - if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> >> - return;
> >> - vhost_poll_stop(net->poll + VHOST_NET_VQ_TX);
> >> - net->tx_poll_state = VHOST_NET_POLL_STOPPED;
> >> -}
> >> -
> >> -/* Caller must have TX VQ lock */
> >> -static int tx_poll_start(struct vhost_net *net, struct socket *sock)
> >> -{
> >> - int ret;
> >> -
> >> - if (unlikely(net->tx_poll_state != VHOST_NET_POLL_STOPPED))
> >> - return 0;
> >> - ret = vhost_poll_start(net->poll + VHOST_NET_VQ_TX, sock->file);
> >> - if (!ret)
> >> - net->tx_poll_state = VHOST_NET_POLL_STARTED;
> >> - return ret;
> >> -}
> >> -
> >> /* 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
> >> @@ -231,6 +199,7 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> >> static void handle_tx(struct vhost_net *net)
> >> {
> >> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> >> + struct vhost_poll *poll = net->poll + VHOST_NET_VQ_TX;
> >> unsigned out, in, s;
> >> int head;
> >> struct msghdr msg = {
> >> @@ -256,7 +225,7 @@ static void handle_tx(struct vhost_net *net)
> >> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> >> if (wmem >= sock->sk->sk_sndbuf) {
> >> mutex_lock(&vq->mutex);
> >> - tx_poll_start(net, sock);
> >> + vhost_poll_start(poll, sock->file);
> >> mutex_unlock(&vq->mutex);
> >> return;
> >> }
> >> @@ -265,7 +234,7 @@ static void handle_tx(struct vhost_net *net)
> >> vhost_disable_notify(&net->dev, vq);
> >>
> >> if (wmem < sock->sk->sk_sndbuf / 2)
> >> - tx_poll_stop(net);
> >> + vhost_poll_stop(poll);
> >> hdr_size = vq->vhost_hlen;
> >> zcopy = vq->ubufs;
> >>
> >> @@ -287,7 +256,7 @@ static void handle_tx(struct vhost_net *net)
> >>
> >> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> >> if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
> >> - tx_poll_start(net, sock);
> >> + vhost_poll_start(poll, sock->file);
> >> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> >> break;
> >> }
> >> @@ -298,7 +267,7 @@ static void handle_tx(struct vhost_net *net)
> >> (vq->upend_idx - vq->done_idx) :
> >> (vq->upend_idx + UIO_MAXIOV - vq->done_idx);
> >> if (unlikely(num_pends > VHOST_MAX_PEND)) {
> >> - tx_poll_start(net, sock);
> >> + vhost_poll_start(poll, sock->file);
> >> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> >> break;
> >> }
> >> @@ -364,7 +333,7 @@ static void handle_tx(struct vhost_net *net)
> >> }
> >> vhost_discard_vq_desc(vq, 1);
> >> if (err == -EAGAIN || err == -ENOBUFS)
> >> - tx_poll_start(net, sock);
> >> + vhost_poll_start(poll, sock->file);
> >> break;
> >> }
> >> if (err != len)
> >> @@ -627,7 +596,6 @@ static int vhost_net_open(struct inode *inode, struct file *f)
> >>
> >> vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT, dev);
> >> vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN, dev);
> >> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
> >>
> >> f->private_data = n;
> >>
> >> @@ -637,32 +605,24 @@ static int vhost_net_open(struct inode *inode, struct file *f)
> >> static void vhost_net_disable_vq(struct vhost_net *n,
> >> struct vhost_virtqueue *vq)
> >> {
> >> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
> >> if (!vq->private_data)
> >> return;
> >> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
> >> - tx_poll_stop(n);
> >> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
> >> - } else
> >> - vhost_poll_stop(n->poll + VHOST_NET_VQ_RX);
> >> + vhost_poll_stop(poll);
> >> }
> >>
> >> static int vhost_net_enable_vq(struct vhost_net *n,
> >> struct vhost_virtqueue *vq)
> >> {
> >> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
> >> struct socket *sock;
> >> - int ret;
> >>
> >> sock = rcu_dereference_protected(vq->private_data,
> >> lockdep_is_held(&vq->mutex));
> >> if (!sock)
> >> return 0;
> >> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
> >> - n->tx_poll_state = VHOST_NET_POLL_STOPPED;
> >> - ret = tx_poll_start(n, sock);
> >> - } else
> >> - ret = vhost_poll_start(n->poll + VHOST_NET_VQ_RX, sock->file);
> >>
> >> - return ret;
> >> + return vhost_poll_start(poll, sock->file);
> >> }
> >>
> >> static struct socket *vhost_net_stop_vq(struct vhost_net *n,
> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> >> index 9759249..4eecdb8 100644
> >> --- a/drivers/vhost/vhost.c
> >> +++ b/drivers/vhost/vhost.c
> >> @@ -89,6 +89,9 @@ int vhost_poll_start(struct vhost_poll *poll, struct file *file)
> >> unsigned long mask;
> >> int ret = 0;
> >>
> >> + if (poll->wqh)
> >> + return 0;
> >> +
> >> mask = file->f_op->poll(file, &poll->table);
> >> if (mask)
> >> vhost_poll_wakeup(&poll->wait, 0, 0, (void *)mask);
> >> --
> >> 1.7.1
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" 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] vhost_net: remove tx polling state
From: Jason Wang @ 2013-03-11 8:45 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20130311082923.GB21086@redhat.com>
On 03/11/2013 04:29 PM, Michael S. Tsirkin wrote:
> On Mon, Mar 11, 2013 at 03:09:10PM +0800, Jason Wang wrote:
>> On 03/11/2013 12:50 AM, Michael S. Tsirkin wrote:
>>> On Thu, Mar 07, 2013 at 12:31:56PM +0800, Jason Wang wrote:
>>>> After commit 2b8b328b61c799957a456a5a8dab8cc7dea68575 (vhost_net: handle polling
>>>> errors when setting backend), we in fact track the polling state through
>>>> poll->wqh, so there's no need to duplicate the work with an extra
>>>> vhost_net_polling_state. So this patch removes this and make the code simpler.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> I'd prefer a more radical approach, since I think it can be even
>>> simpler: tap and macvtap backends both only send events when tx queue
>>> overruns which should almost never happen.
>>>
>>> So let's just start polling when VQ is enabled
>>> drop all poll_start/stop calls on data path.
>> I think then it may damage the performance at least for tx. We
>> conditionally enable the tx polling in the past to reduce the
>> unnecessary wakeups of vhost thead when zerocopy or sndbuf is used. If
>> we don't stop the polling, after each packet were transmitted,
>> tap/macvtap will try to wakeup the vhost thread.
> It won't actually.
> static void macvtap_sock_write_space(struct sock *sk)
> {
> wait_queue_head_t *wqueue;
>
> if (!sock_writeable(sk) ||
> !test_and_clear_bit(SOCK_ASYNC_NOSPACE, &sk->sk_socket->flags))
> return;
>
> wqueue = sk_sleep(sk);
> if (wqueue && waitqueue_active(wqueue))
> wake_up_interruptible_poll(wqueue, POLLOUT | POLLWRNORM | POLLWRBAND);
> }
>
> As long as SOCK_ASYNC_NOSPACE is not set, there's no wakeup.
>
Ok, will send V2 which removes all the polling start/stop in datapath.
Thanks
^ permalink raw reply
* Re: [PATCH] vhost_net: remove tx polling state
From: Michael S. Tsirkin @ 2013-03-11 8:47 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <1362630716-57674-1-git-send-email-jasowang@redhat.com>
On Thu, Mar 07, 2013 at 12:31:56PM +0800, Jason Wang wrote:
> After commit 2b8b328b61c799957a456a5a8dab8cc7dea68575 (vhost_net: handle polling
> errors when setting backend), we in fact track the polling state through
> poll->wqh, so there's no need to duplicate the work with an extra
> vhost_net_polling_state. So this patch removes this and make the code simpler.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
OK NACK as Jason said he wants to rework this patch.
> ---
> drivers/vhost/net.c | 60 ++++++++----------------------------------------
> drivers/vhost/vhost.c | 3 ++
> 2 files changed, 13 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 959b1cd..d1a03dd 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -64,20 +64,10 @@ enum {
> VHOST_NET_VQ_MAX = 2,
> };
>
> -enum vhost_net_poll_state {
> - VHOST_NET_POLL_DISABLED = 0,
> - VHOST_NET_POLL_STARTED = 1,
> - VHOST_NET_POLL_STOPPED = 2,
> -};
> -
> struct vhost_net {
> struct vhost_dev dev;
> struct vhost_virtqueue vqs[VHOST_NET_VQ_MAX];
> struct vhost_poll poll[VHOST_NET_VQ_MAX];
> - /* Tells us whether we are polling a socket for TX.
> - * 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;
> @@ -155,28 +145,6 @@ static void copy_iovec_hdr(const struct iovec *from, struct iovec *to,
> }
> }
>
> -/* Caller must have TX VQ lock */
> -static void tx_poll_stop(struct vhost_net *net)
> -{
> - if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
> - return;
> - vhost_poll_stop(net->poll + VHOST_NET_VQ_TX);
> - net->tx_poll_state = VHOST_NET_POLL_STOPPED;
> -}
> -
> -/* Caller must have TX VQ lock */
> -static int tx_poll_start(struct vhost_net *net, struct socket *sock)
> -{
> - int ret;
> -
> - if (unlikely(net->tx_poll_state != VHOST_NET_POLL_STOPPED))
> - return 0;
> - ret = vhost_poll_start(net->poll + VHOST_NET_VQ_TX, sock->file);
> - if (!ret)
> - net->tx_poll_state = VHOST_NET_POLL_STARTED;
> - return ret;
> -}
> -
> /* 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
> @@ -231,6 +199,7 @@ static void vhost_zerocopy_callback(struct ubuf_info *ubuf, bool success)
> static void handle_tx(struct vhost_net *net)
> {
> struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> + struct vhost_poll *poll = net->poll + VHOST_NET_VQ_TX;
> unsigned out, in, s;
> int head;
> struct msghdr msg = {
> @@ -256,7 +225,7 @@ static void handle_tx(struct vhost_net *net)
> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> if (wmem >= sock->sk->sk_sndbuf) {
> mutex_lock(&vq->mutex);
> - tx_poll_start(net, sock);
> + vhost_poll_start(poll, sock->file);
> mutex_unlock(&vq->mutex);
> return;
> }
> @@ -265,7 +234,7 @@ static void handle_tx(struct vhost_net *net)
> vhost_disable_notify(&net->dev, vq);
>
> if (wmem < sock->sk->sk_sndbuf / 2)
> - tx_poll_stop(net);
> + vhost_poll_stop(poll);
> hdr_size = vq->vhost_hlen;
> zcopy = vq->ubufs;
>
> @@ -287,7 +256,7 @@ static void handle_tx(struct vhost_net *net)
>
> wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> if (wmem >= sock->sk->sk_sndbuf * 3 / 4) {
> - tx_poll_start(net, sock);
> + vhost_poll_start(poll, sock->file);
> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> break;
> }
> @@ -298,7 +267,7 @@ static void handle_tx(struct vhost_net *net)
> (vq->upend_idx - vq->done_idx) :
> (vq->upend_idx + UIO_MAXIOV - vq->done_idx);
> if (unlikely(num_pends > VHOST_MAX_PEND)) {
> - tx_poll_start(net, sock);
> + vhost_poll_start(poll, sock->file);
> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> break;
> }
> @@ -364,7 +333,7 @@ static void handle_tx(struct vhost_net *net)
> }
> vhost_discard_vq_desc(vq, 1);
> if (err == -EAGAIN || err == -ENOBUFS)
> - tx_poll_start(net, sock);
> + vhost_poll_start(poll, sock->file);
> break;
> }
> if (err != len)
> @@ -627,7 +596,6 @@ static int vhost_net_open(struct inode *inode, struct file *f)
>
> vhost_poll_init(n->poll + VHOST_NET_VQ_TX, handle_tx_net, POLLOUT, dev);
> vhost_poll_init(n->poll + VHOST_NET_VQ_RX, handle_rx_net, POLLIN, dev);
> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
>
> f->private_data = n;
>
> @@ -637,32 +605,24 @@ static int vhost_net_open(struct inode *inode, struct file *f)
> static void vhost_net_disable_vq(struct vhost_net *n,
> struct vhost_virtqueue *vq)
> {
> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
> if (!vq->private_data)
> return;
> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
> - tx_poll_stop(n);
> - n->tx_poll_state = VHOST_NET_POLL_DISABLED;
> - } else
> - vhost_poll_stop(n->poll + VHOST_NET_VQ_RX);
> + vhost_poll_stop(poll);
> }
>
> static int vhost_net_enable_vq(struct vhost_net *n,
> struct vhost_virtqueue *vq)
> {
> + struct vhost_poll *poll = n->poll + (vq - n->vqs);
> struct socket *sock;
> - int ret;
>
> sock = rcu_dereference_protected(vq->private_data,
> lockdep_is_held(&vq->mutex));
> if (!sock)
> return 0;
> - if (vq == n->vqs + VHOST_NET_VQ_TX) {
> - n->tx_poll_state = VHOST_NET_POLL_STOPPED;
> - ret = tx_poll_start(n, sock);
> - } else
> - ret = vhost_poll_start(n->poll + VHOST_NET_VQ_RX, sock->file);
>
> - return ret;
> + return vhost_poll_start(poll, sock->file);
> }
>
> static struct socket *vhost_net_stop_vq(struct vhost_net *n,
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 9759249..4eecdb8 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -89,6 +89,9 @@ int vhost_poll_start(struct vhost_poll *poll, struct file *file)
> unsigned long mask;
> int ret = 0;
>
> + if (poll->wqh)
> + return 0;
> +
> mask = file->f_op->poll(file, &poll->table);
> if (mask)
> vhost_poll_wakeup(&poll->wait, 0, 0, (void *)mask);
> --
> 1.7.1
^ permalink raw reply
* [PATCH 1/2] phy/micrel: Add support for KSZ8031
From: Hector Palacios @ 2013-03-11 8:50 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, marex, davem, Hector Palacios
Micrel PHY KSZ8031 is similar to KSZ8021 and also requires the special
initialization of "Operation Mode Strap Override" in reg 0x16
introduced in 212ea99 (phy/micrel: Implement support for KSZ8021).
Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
drivers/net/phy/micrel.c | 14 ++++++++++++++
include/linux/micrel_phy.h | 1 +
2 files changed, 15 insertions(+)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index b983596..1ab6743 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -187,6 +187,19 @@ static struct phy_driver ksphy_driver[] = {
.config_intr = kszphy_config_intr,
.driver = { .owner = THIS_MODULE,},
}, {
+ .phy_id = PHY_ID_KSZ8031,
+ .phy_id_mask = 0x00ffffff,
+ .name = "Micrel KSZ8031",
+ .features = (PHY_BASIC_FEATURES | SUPPORTED_Pause |
+ SUPPORTED_Asym_Pause),
+ .flags = PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
+ .config_init = ksz8021_config_init,
+ .config_aneg = genphy_config_aneg,
+ .read_status = genphy_read_status,
+ .ack_interrupt = kszphy_ack_interrupt,
+ .config_intr = kszphy_config_intr,
+ .driver = { .owner = THIS_MODULE,},
+}, {
.phy_id = PHY_ID_KSZ8041,
.phy_id_mask = 0x00fffff0,
.name = "Micrel KSZ8041",
@@ -273,6 +286,7 @@ static struct mdio_device_id __maybe_unused micrel_tbl[] = {
{ PHY_ID_KSZ8001, 0x00ffffff },
{ PHY_ID_KS8737, 0x00fffff0 },
{ PHY_ID_KSZ8021, 0x00ffffff },
+ { PHY_ID_KSZ8031, 0x00ffffff },
{ PHY_ID_KSZ8041, 0x00fffff0 },
{ PHY_ID_KSZ8051, 0x00fffff0 },
{ PHY_ID_KSZ8873MLL, 0x00fffff0 },
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index adfe8c0..b1ffaa4 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -19,6 +19,7 @@
#define PHY_ID_KSZ9021 0x00221610
#define PHY_ID_KS8737 0x00221720
#define PHY_ID_KSZ8021 0x00221555
+#define PHY_ID_KSZ8031 0x00221556
#define PHY_ID_KSZ8041 0x00221510
#define PHY_ID_KSZ8051 0x00221550
/* both for ks8001 Rev. A/B, and for ks8721 Rev 3. */
--
1.7.9.5
^ permalink raw reply related
* [PATCH 2/2] phy/micrel: move flag handling to function for common use
From: Hector Palacios @ 2013-03-11 8:50 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, marex, davem, Hector Palacios
In-Reply-To: <1362991803-4610-1-git-send-email-hector.palacios@digi.com>
The flag MICREL_PHY_50MHZ_CLK is not of exclusive use of KSZ8051
model. At least KSZ8021 and KSZ8031 models also use it.
This patch moves the handling of this and future flags to a
separate function so that the different PHY models can call it on
their init function, if needed.
Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
drivers/net/phy/micrel.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 1ab6743..6a0834b 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -48,6 +48,18 @@
#define KS8737_CTRL_INT_ACTIVE_HIGH (1 << 14)
#define KSZ8051_RMII_50MHZ_CLK (1 << 7)
+static int ksz_config_flags(struct phy_device *phydev)
+{
+ int regval;
+
+ if (phydev->dev_flags & MICREL_PHY_50MHZ_CLK) {
+ regval = phy_read(phydev, MII_KSZPHY_CTRL);
+ regval |= KSZ8051_RMII_50MHZ_CLK;
+ return phy_write(phydev, MII_KSZPHY_CTRL, regval);
+ }
+ return 0;
+}
+
static int kszphy_ack_interrupt(struct phy_device *phydev)
{
/* bit[7..0] int status, which is a read and clear register. */
@@ -109,22 +121,19 @@ static int kszphy_config_init(struct phy_device *phydev)
static int ksz8021_config_init(struct phy_device *phydev)
{
+ int rc;
const u16 val = KSZPHY_OMSO_B_CAST_OFF | KSZPHY_OMSO_RMII_OVERRIDE;
phy_write(phydev, MII_KSZPHY_OMSO, val);
- return 0;
+ rc = ksz_config_flags(phydev);
+ return rc < 0 ? rc : 0;
}
static int ks8051_config_init(struct phy_device *phydev)
{
- int regval;
-
- if (phydev->dev_flags & MICREL_PHY_50MHZ_CLK) {
- regval = phy_read(phydev, MII_KSZPHY_CTRL);
- regval |= KSZ8051_RMII_50MHZ_CLK;
- phy_write(phydev, MII_KSZPHY_CTRL, regval);
- }
+ int rc;
- return 0;
+ rc = ksz_config_flags(phydev);
+ return rc < 0 ? rc : 0;
}
#define KSZ8873MLL_GLOBAL_CONTROL_4 0x06
--
1.7.9.5
^ 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