* Re: [net-next PATCH 2/2] ixgbe: deleting dfwd stations out of order can cause null ptr deref
From: Jeff Kirsher @ 2013-11-08 12:36 UTC (permalink / raw)
To: John Fastabend; +Cc: netdev, davem, nhorman
In-Reply-To: <20131108085109.6935.99661.stgit@nitbit.x32>
[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]
On Fri, 2013-11-08 at 00:51 -0800, John Fastabend wrote:
> The number of stations in use is kept in the num_rx_pools counter
> in the ixgbe_adapter structure. This is in turn used by the queue
> allocation scheme to determine how many queues are needed to support
> the number of pools in use with the current feature set.
>
> This works as long as the pools are added and destroyed in order
> because (num_rx_pools * queues_per_pool) is equal to the last
> queue in use by a pool. But as soon as you delete a pool out of
> order this is no longer the case. So the above multiplication
> allocates to few queues and a pool may reference a ring that has
> not been allocated/initialized.
>
> To resolve use the bit mask of in use pools to determine the final
> pool being used and allocate enough queues so that we don't
> inadvertently remove its queues.
>
> # ip link add link eth2 \
> numtxqueues 4 numrxqueues 4 txqueuelen 50 type macvlan
> # ip link set dev macvlan0 up
> # ip link add link eth2 \
> numtxqueues 4 numrxqueues 4 txqueuelen 50 type macvlan
> # ip link set dev macvlan1 up
> # for i in {0..100}; do
> ip link set dev macvlan0 down; ip link set dev macvlan0 up;
> done;
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [net-next PATCH 1/2] ixgbe: fix build err, num_rx_queues is only available with CONFIG_RPS
From: Jeff Kirsher @ 2013-11-08 12:35 UTC (permalink / raw)
To: John Fastabend; +Cc: netdev, davem, nhorman
In-Reply-To: <20131108085031.6935.61765.stgit@nitbit.x32>
[-- Attachment #1: Type: text/plain, Size: 939 bytes --]
On Fri, 2013-11-08 at 00:50 -0800, John Fastabend wrote:
> In the recent support for layer 2 hardware acceleration, I added a
> few references to real_num_rx_queues and num_rx_queues which are
> only available with CONFIG_RPS.
>
> The fix is first to remove unnecessary references to num_rx_queues.
> Because the hardware offload case is limited to cases where RX queues
> and TX queues are equal we only need a single check. Then wrap the
> single case in an ifdef.
>
> The patch that introduce this is here,
>
> commit a6cc0cfa72e0b6d9f2c8fd858aacc32313c4f272
> Author: John Fastabend <john.r.fastabend@intel.com>
> Date: Wed Nov 6 09:54:46 2013 -0800
>
> net: Add layer 2 hardware acceleration operations for macvlan
> devices
>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Acked-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 2/3] ipv6: increase maximum lifetime of flow labels
From: Hannes Frederic Sowa @ 2013-11-08 12:34 UTC (permalink / raw)
To: Florent Fourcot; +Cc: netdev
In-Reply-To: <1383843194-22945-2-git-send-email-florent.fourcot@enst-bretagne.fr>
On Thu, Nov 07, 2013 at 05:53:13PM +0100, Florent Fourcot wrote:
> If the last RFC 6437 does not give any constraints
> for lifetime of flow labels, the previous RFC 3697
> spoke of a minimum of 120 seconds between
> reattribution of a flow label.
>
> The maximum linger is currently set to 60 seconds
> and does not allow this configuration without
> CAP_NET_ADMIN right.
>
> This patch increase the maximum linger to 150
> seconds, allowing more flexibility to standard
> users.
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
^ permalink raw reply
* Re: [PATCH net-next v2 0/4] sctp: do some clean ups
From: Neil Horman @ 2013-11-08 12:31 UTC (permalink / raw)
To: Wang Weidong; +Cc: vyasevich, davem, dingtianhong, linux-sctp, netdev
In-Reply-To: <1383891471-20892-1-git-send-email-wangweidong1@huawei.com>
On Fri, Nov 08, 2013 at 02:17:47PM +0800, Wang Weidong wrote:
> This patch series include: remove the duplicate initialize,
> make code simple, convert func to boolean, fix some typos.
>
> v1 -> v2:
> - patch2: make the code more simplification, as suggested
> by Joe Perches.
>
> Wang Weidong (4):
> sctp: remove the duplicate initialize
> sctp: remove the else path
> sctp: convert sctp_peer_needs_update to boolean
> sctp: fix some comments in associola.c
>
> net/sctp/associola.c | 65 ++++++++--------------------------------------------
> 1 file changed, 10 insertions(+), 55 deletions(-)
>
> --
> 1.7.12
>
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [net-next PATCH 2/2] ixgbe: deleting dfwd stations out of order can cause null ptr deref
From: Neil Horman @ 2013-11-08 12:31 UTC (permalink / raw)
To: John Fastabend; +Cc: jeffrey.t.kirsher, netdev, davem
In-Reply-To: <20131108085109.6935.99661.stgit@nitbit.x32>
On Fri, Nov 08, 2013 at 12:51:10AM -0800, John Fastabend wrote:
> The number of stations in use is kept in the num_rx_pools counter
> in the ixgbe_adapter structure. This is in turn used by the queue
> allocation scheme to determine how many queues are needed to support
> the number of pools in use with the current feature set.
>
> This works as long as the pools are added and destroyed in order
> because (num_rx_pools * queues_per_pool) is equal to the last
> queue in use by a pool. But as soon as you delete a pool out of
> order this is no longer the case. So the above multiplication
> allocates to few queues and a pool may reference a ring that has
> not been allocated/initialized.
>
> To resolve use the bit mask of in use pools to determine the final
> pool being used and allocate enough queues so that we don't
> inadvertently remove its queues.
>
> # ip link add link eth2 \
> numtxqueues 4 numrxqueues 4 txqueuelen 50 type macvlan
> # ip link set dev macvlan0 up
> # ip link add link eth2 \
> numtxqueues 4 numrxqueues 4 txqueuelen 50 type macvlan
> # ip link set dev macvlan1 up
> # for i in {0..100}; do
> ip link set dev macvlan0 down; ip link set dev macvlan0 up;
> done;
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [net-next PATCH 1/2] ixgbe: fix build err, num_rx_queues is only available with CONFIG_RPS
From: Neil Horman @ 2013-11-08 12:29 UTC (permalink / raw)
To: John Fastabend; +Cc: jeffrey.t.kirsher, netdev, davem
In-Reply-To: <20131108085031.6935.61765.stgit@nitbit.x32>
On Fri, Nov 08, 2013 at 12:50:32AM -0800, John Fastabend wrote:
> In the recent support for layer 2 hardware acceleration, I added a
> few references to real_num_rx_queues and num_rx_queues which are
> only available with CONFIG_RPS.
>
> The fix is first to remove unnecessary references to num_rx_queues.
> Because the hardware offload case is limited to cases where RX queues
> and TX queues are equal we only need a single check. Then wrap the
> single case in an ifdef.
>
> The patch that introduce this is here,
>
> commit a6cc0cfa72e0b6d9f2c8fd858aacc32313c4f272
> Author: John Fastabend <john.r.fastabend@intel.com>
> Date: Wed Nov 6 09:54:46 2013 -0800
>
> net: Add layer 2 hardware acceleration operations for macvlan devices
>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] ipv6: protect flow label renew against GC
From: Hannes Frederic Sowa @ 2013-11-08 12:29 UTC (permalink / raw)
To: Florent Fourcot; +Cc: netdev
In-Reply-To: <1383843194-22945-3-git-send-email-florent.fourcot@enst-bretagne.fr>
On Thu, Nov 07, 2013 at 05:53:14PM +0100, Florent Fourcot wrote:
> Take ip6_fl_lock before to read and update
> a label.
>
> v2: protect only the relevant code
>
> Reported-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Florent Fourcot <florent.fourcot@enst-bretagne.fr>
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Thanks for fixing this,
Hannes
^ permalink raw reply
* Re: [PATCH net-next v3 1/3] ipv6: enable IPV6_FLOWLABEL_MGR for getsockopt
From: Hannes Frederic Sowa @ 2013-11-08 12:27 UTC (permalink / raw)
To: Florent Fourcot; +Cc: netdev
In-Reply-To: <1383843194-22945-1-git-send-email-florent.fourcot@enst-bretagne.fr>
On Thu, Nov 07, 2013 at 05:53:12PM +0100, Florent Fourcot wrote:
> It is already possible to set/put/renew a label
> with IPV6_FLOWLABEL_MGR and setsockopt. This patch
> add the possibility to get information about this
> label (current value, time before expiration, etc).
>
> It helps application to take decision for a renew
> or a release of the label.
>
> v2:
> * Add spin_lock to prevent race condition
> * return -ENOENT if no result found
> * check if flr_action is GET
>
> v3:
> * move the spin_lock to protect only the
> relevant code
>
> Signed-off-by: Florent Fourcot <florent.fourcot@enst-bretagne.fr>
There still are some locking anomalies in ip6_flowlabel. But none
has something to do with your patches. E.g. mem_check runs without
rcu_read_lock.
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Thanks,
Hannes
^ permalink raw reply
* Re: [PATCH v2 net-next] inet: fix a UFO regression
From: Eric Dumazet @ 2013-11-08 12:18 UTC (permalink / raw)
To: Sridhar Samudrala
Cc: Alexei Starovoitov, David Miller, netdev, Hannes Frederic Sowa
In-Reply-To: <527C877D.3080309@gmail.com>
On Thu, 2013-11-07 at 22:41 -0800, Sridhar Samudrala wrote:
> Does this fix also help vxlan performance between 2 VMs on 2 different
> physical m/cs across 10G NIC?
> What is the throughput you are seeing via 10G nics?
> With linux 3.12, i am only seeing around 2Gbps (iperf TCP_STREAM with
> 16K messages) between 2 VMs when using vxlan across a 10G nic.
> Is this due to the overhead of software GSO and not doing GRO at the
> receiver?
What NIC is used at sender, what NIC is used at receiver ?
If you use GRE instead of VXLAN, do you get any difference in speed ?
^ permalink raw reply
* Re: [ethtool] ethtool: ixgbe DCB registers dump for 82599 and x540
From: Jeff Kirsher @ 2013-11-08 12:01 UTC (permalink / raw)
To: bhutchings; +Cc: Leonardo Potenza, netdev, gospo, sassmann, Maryam Tahhan
In-Reply-To: <1380787887-6878-1-git-send-email-jeffrey.t.kirsher@intel.com>
[-- Attachment #1: Type: text/plain, Size: 865 bytes --]
On Thu, 2013-10-03 at 01:11 -0700, Jeff Kirsher wrote:
> From: Leonardo Potenza <leonardo.potenza@intel.com>
>
> Added support for DCB registers dump using ethtool -d option both for
> 82599 and x540 ethernet controllers
>
> Signed-off-by: Leonardo Potenza <leonardo.potenza@intel.com>
> Signed-off-by: Maryam Tahhan <maryam.tahhan@intel.com>
> Acked-by: John Fastabend <john.r.fastabend@intel.com>
> Tested-by: Phil Schmitt <phillip.j.schmitt@intel.com>
> Tested-by: Jack Morgan <jack.morgan@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
> ixgbe.c | 154
> ++++++++++++++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 125 insertions(+), 29 deletions(-)
Ben-
I see you released 3.12 and I do not see this patch in your tree. I
sent it originally on October 3rd, do need me to resend it?
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCHv2 net-next 0/3] Small IPsec fix
From: Steffen Klassert @ 2013-11-08 11:54 UTC (permalink / raw)
To: Fan Du; +Cc: herbert, davem, netdev
In-Reply-To: <1383817670-5783-1-git-send-email-fan.du@windriver.com>
On Thu, Nov 07, 2013 at 05:47:47PM +0800, Fan Du wrote:
> Hi
>
> This patchset made below modifications:
> - Fix setting policy index with "ip xfrm ... index=xx"
> which is currently broken
>
> - Using correct name space for home agent when migrating
> key info.
>
> - Put xfrm locks into per namespace to improve scalability.
>
> Changelog:
> v2:
> xfrm: Namespacify xfrm state/policy locks
> -Fix compile error when CONFIG_NET_NS is unset, thanks for the lovely build robot.
>
> Fan Du (3):
> xfrm: Try to honor policy index if it's supplied by user
> xfrm: Using the right namespace to migrate key info
> xfrm: Namespacify xfrm state/policy locks
Your patches are currently in the ipsec-next testing branch.
linux-next should not contain v3.14 material at the moment,
so I'll wait until after the merge window before I'm pushing
them to the master branch. No further action from your side
required.
Thanks, Fan!
^ permalink raw reply
* Re: [PATCH 2/2 net-next] openvswitch: Use skb_zerocopy() for upcall
From: Daniel Borkmann @ 2013-11-08 11:24 UTC (permalink / raw)
To: Thomas Graf
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <9369d871e13fe5b6e468b269450b9f2032a970aa.1383906944.git.tgraf-G/eBtMaohhA@public.gmane.org>
On 11/08/2013 11:47 AM, Thomas Graf wrote:
> Use of skb_zerocopy() avoids the expensive call to memcpy() when
> copying the packet data into the Netlink skb. Completes checksum
> through skb_checksum_help() if needed.
>
> Netlink messaged must be properly padded and aligned to meet
> sanity checks of the user space counterpart.
>
> Cost of memcpy is significantly reduced from:
> + 7.48% vhost-8471 [k] memcpy
> + 5.57% ovs-vswitchd [k] memcpy
> + 2.81% vhost-8471 [k] csum_partial_copy_generic
>
> to:
> + 5.72% ovs-vswitchd [k] memcpy
> + 3.32% vhost-5153 [k] memcpy
> + 0.68% vhost-5153 [k] skb_zerocopy
>
> (megaflows disabled)
>
> Signed-off-by: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
> Cc: Jesse Gross <jesse-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
> Cc: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: David Laight @ 2013-11-08 11:07 UTC (permalink / raw)
To: David Laight, netdev, linux-usb, Sarah Sharp
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B73EF@saturn3.aculab.com>
> While this change improves things a lot (it runs for 20 minutes rather than
> a few seconds), there is still something else wrong.
Almost certainly caused by an unrelated local change.
David
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net_sched: fix some checkpatch errors
From: Bjørn Mork @ 2013-11-08 11:04 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Yang Yingliang, davem, netdev, eric.dumazet, jhs, stephen
In-Reply-To: <527B5160.5070700@redhat.com>
Daniel Borkmann <dborkman@redhat.com> writes:
> On 11/07/2013 03:13 AM, Yang Yingliang wrote:
>
>> @@ -263,7 +264,7 @@ void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo)
>> }
>> EXPORT_SYMBOL(tcf_hash_insert);
>>
>> -static struct tc_action_ops *act_base = NULL;
>> +static struct tc_action_ops *act_base;
>
> From a readability point of view, I think this makes it worse, also the other places
> where you change globals vars like that.
Then you should probably argue for a change to checkpatch. It will make
a lot of noise about such unnecessary initialization.
Bjørn
^ permalink raw reply
* Re: [PATCH 1/2 net-next] net: Export skb_zerocopy() to zerocopy from one skb to another
From: Daniel Borkmann @ 2013-11-08 11:06 UTC (permalink / raw)
To: Thomas Graf
Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <107fca409db2405bf3c650a03ea2254d3d6b63dc.1383906944.git.tgraf-G/eBtMaohhA@public.gmane.org>
On 11/08/2013 11:47 AM, Thomas Graf wrote:
> Make the skb zerocopy logic written for nfnetlink queue available for
> use by other modules.
>
> Signed-off-by: Thomas Graf <tgraf-G/eBtMaohhA@public.gmane.org>
Reviewed-by: Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
^ permalink raw reply
* Re: [PATCH net v3] vti: fix spd lookup: match plaintext pkt, not ipsec pkt
From: Steffen Klassert @ 2013-11-08 11:01 UTC (permalink / raw)
To: Christophe Gouault
Cc: David S. Miller, Herbert Xu, netdev, Saurabh Mohan,
Sergei Shtylyov, Eric Dumazet
In-Reply-To: <527B8DC5.6080702@6wind.com>
On Thu, Nov 07, 2013 at 01:55:33PM +0100, Christophe Gouault wrote:
> Hello Steffen,
>
> I am also interested in knowing Saurabh's intentions regarding the
> behavior of policies bound to vti interfaces.
>
> However, please note that setting a policy with a wildcard selector
> works in both cases (before or after this patch), so a common test
> case can be defined.
Yes, I looked at the Cisco vti documents but all examples I found use
wildcard selectors which work for both. So I'm still not sure which
version is the right one. Let's wait on Saurabh's explaination.
>
> Actually the *previous* patch on vti (7263a5187f9e vti: get rid of
> nf mark rule in prerouting) introduced significant changes, and
> implies behaviors dependant on the kernel version, but it seemed to
> meet Saurabh's agreement, as the following thread witnesses:
>
> http://www.spinics.net/lists/netdev/msg253134.html
I've just noticed that this went to the stable trees. People who
update a stable kernel want (security) fixes in the first place,
they don't want to change their configuration on the IPsec gateways.
So I think patches that require a configuration change should better
go to net-next, unless it's a urgent fix.
I was not on Cc and it looks like I've overlooked this on the list.
The vti interfaces are pure IPsec interfaces, so perhaps we should
add them to the IPsec section in the maintainers file (maybe together
with the main IPsec protocols esp, ah and ipcomp, which are also not
listed there).
David, would you agree with such a patch?
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: David Laight @ 2013-11-08 10:47 UTC (permalink / raw)
To: Sarah Sharp; +Cc: netdev, linux-usb
In-Reply-To: <20131108001838.GB11529@xanatos>
> From: Sarah Sharp [mailto:sarah.a.sharp@linux.intel.com]
> On Thu, Nov 07, 2013 at 05:20:49PM -0000, David Laight wrote:
> >
> > Section 4.11.7.1 of rev 1.0 of the xhci specification states that a link TRB
> > can only occur at a boundary between underlying USB frames (512 bytes for 480M).
>
> Which version of the spec are you looking at? I'm looking at the
> updated version from 08/2012 and I don't see any such requirement.
The copy I downloaded last week is dated 5/21/10, seems the copy on the
web site hasn't been updated.
> I do see that section says "A TD Fragment shall not span Transfer Ring
> Segments" where a TD fragment is defined as exact multiples of Max Burst
> Size * Max Packet Size bytes for the endpoint. Is that what you mean
> about USB frames?
That is the clause, it needs some understanding.
Figure 24 shows a correctly aligned link TRB - one where the TRB boundary
is aligned with a packet boundary (512 bytes at 480M, 1k at 5G etc).
> > If this isn't done the USB frames aren't formatted correctly and, for example,
> > the USB3 ethernet ax88179_178a card will stop sending (while still receiving)
> > when running a netperf tcp transmit test with (say) and 8k buffer.
>
> Which driver does that use? Is it using the scatter gather list
> mechanism for URBs (i.e. urb->sg)? Or is it submitting multiple URBs
> for fragmented buffers? Or is it submitting isochronous URBs with
> multiple frames? Or is it submitting bulk URBs with one transfer buffer
> that crosses the 64KB boundary?
>
> I'm trying to understand what the USB device driver is doing in order to
> create multi-TRB TDs that would violate the TD fragment rules.
The usb setup is done by net/usbnet.c, the ax88179_178a driver just adds
a header to the ethernet frame. Since the hardware support TCP segment
offload the urb->sg list is used, the first fragment seems to be 1578
bytes and is followed by two or three fragments (split on an 0x8000
boundary) making up the rest of the 65234 bytes.
(I haven't yet seen a buffer that crossed a 64k boundary).
I'm running netperf - which is doing a burst of 8k sends into
a TCP socket.
> > While this change improves things a lot (it runs for 20 minutes rather than
> > a few seconds), there is still something else wrong.
> >
> > This should be a candidate for stable, the ax88179_178a driver defaults to
> > gso and tso enabled so it passes a lot of fragmented skb to the USB stack.
>
> I want to understand what the larger issue is here before pushing
> bandaid fixes.
>
> I don't think this is the right approach, because prepare_ring() can be
> called for isochronous URBs that get mapped to multiple TDs. The TD
> fragment rules only apply to one TD, so failing URB submission because
> many isochronous TDs span more than one ring segment doesn't seem like
> the right approach. Inserting no-op TRBs will also result in odd
> isochronous behavior, since a no-op TRB means the xHC should not send or
> request an isochronous packet for that service interval.
I wasn't aware of that effect of NOP TRB on isoc data.
Three obvious options:
1) Pass an extra parameter indicating that the caller will handle an
TRB data alignment issues at the link TRB.
2) Add a test for bulk endpoints (ok until the isoc code gets modified
to support SG data).
3) Copy the LINK TRB instead of adding NOPs (would require the rest of
the code use the ring length instead of reading the TRB type).
The second is probably the easiest way to isolate the change.
However there could be problems on an isoc endpoint if the data
crosses a 64k boundary.
> The patch also doesn't address the underlying issue of constructing
> multi-TRB TDs so that it fits the TD fragment rules. Your patch ensures
> there are no link TRBs in the middle of a TD, but it doesn't ensure that
> the TRBs in the TD are exact multiples of the Max Burst Size * Max
> Packet Size bytes for the endpoint.
That doesn't matter, look at figure 25. A TD can be any number of USB
packets and any number or TRB. The constraint is that a TD cannot
contain a LINK TRB.
> Instead, new code in count_sg_trbs_needed() should break the TD into
> proper TD fragments. The queueing code for all endpoints would also
> have to follow those rules. This would have to happen while still
> respecting the 64KB boundary rule.
>
> The driver would also have to make sure that TD fragments didn't have
> link TRBs in them. That's an issue, since the spec decidedly unclear
> about what exactly comprises a TD fragment. Is the xHC host greedy, and
> will lump all multiples into one TD fragment? Or will it assume the TD
> fragment has ended once it consumes one multiple of Max Burst Size * Max
> Packet Size bytes?
It only matters at a LINK TRB (unless you want to request an interrupt).
The constraint seems to be that the data TRB before a link TRB must end
on a MBP boundary.
> This ambiguity means it's basically impossible to figure out whether a
> TD with a link TRB in the middle is constructed properly. The xHCI spec
> architect didn't have a good solution to this problem, so I punted and
> never implemented TD fragments.
The problem is that the hardware does implement them:-)
> If this is really an issue, it's going to be pretty complex to solve.
I thought about solutions and then decided it was easiest to skip to
the next ring segment. The data could be scanned for an earlier
boundary that was aligned, but it really didn't seem worth the effort.
If all of the TRB reference buffers that are larger than MBP then,
when the LINK is encountered, the previous TRB can be split on its
last MBP boundary, if not a bounce buffer would have to be used
(and the code doesn't really know if this a tx or rx transaction).
I've a tx that is split 1576/328/32768/30560 where the 328 could
not be split - so this does happen.
>
> > Signed-off-by: David Laight <david.laight@aculab.com>
> > ---
> >
> > Although I've got a USB2 analyser its trigger and trace stuff is almost
> > unusable! In any case this fails much faster on USB3 (Intel i7 cpu).
>
> I thought you were using a big endian system?
No - I was just reading the code and spotted the endianness bug.
David
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> > index 5480215..23abc9b 100644
> > --- a/drivers/usb/host/xhci-ring.c
> > +++ b/drivers/usb/host/xhci-ring.c
> > @@ -2927,8 +2932,43 @@ static int prepare_ring(struct xhci_hcd *xhci, struct xhci_ring *ep_ring,
> > }
> >
> > while (1) {
> > - if (room_on_ring(xhci, ep_ring, num_trbs))
> > - break;
> > + if (room_on_ring(xhci, ep_ring, num_trbs)) {
> > + union xhci_trb *trb = ep_ring->enqueue;
> > + unsigned int usable = ep_ring->enq_seg->trbs +
> > + TRBS_PER_SEGMENT - 1 - trb;
> > + u32 nop_cmd;
> > +
> > + /*
> > + * Section 4.11.7.1 TD Fragments states that a link
> > + * TRB must only occur at the boundary between
> > + * data bursts (eg 512 bytes for 480M).
> > + * While it is possible to split a large fragment
> > + * we don't know the size yet.
> > + * Simplest solution is to fill the trb before the
> > + * LINK with nop commands.
> > + */
> > + if (num_trbs == 1 || num_trbs <= usable || usable == 0)
> > + break;
> > +
> > + if (num_trbs >= TRBS_PER_SEGMENT) {
> > + xhci_err(xhci, "Too many fragments %d, max %d\n",
> > + num_trbs, TRBS_PER_SEGMENT - 1);
> > + return -ENOMEM;
> > + }
> > +
> > + nop_cmd = cpu_to_le32(TRB_TYPE(TRB_TR_NOOP) |
> > + ep_ring->cycle_state);
> > + for (; !TRB_TYPE_LINK_LE32(trb->link.control); trb++) {
> > + trb->generic.field[0] = 0;
> > + trb->generic.field[1] = 0;
> > + trb->generic.field[2] = 0;
> > + trb->generic.field[3] = nop_cmd;
> > + ep_ring->num_trbs_free--;
> > + }
> > + ep_ring->enqueue = trb;
> > + if (room_on_ring(xhci, ep_ring, num_trbs))
> > + break;
> > + }
> >
> > if (ep_ring == xhci->cmd_ring) {
> > xhci_err(xhci, "Do not support expand command ring\n");
>
> Sarah Sharp
^ permalink raw reply
* [PATCH 1/2 net-next] net: Export skb_zerocopy() to zerocopy from one skb to another
From: Thomas Graf @ 2013-11-08 10:47 UTC (permalink / raw)
To: jesse, davem; +Cc: dev, netdev, eric.dumazet
In-Reply-To: <cover.1383906944.git.tgraf@suug.ch>
Make the skb zerocopy logic written for nfnetlink queue available for
use by other modules.
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
include/linux/skbuff.h | 3 ++
net/core/skbuff.c | 85 ++++++++++++++++++++++++++++++++++++
net/netfilter/nfnetlink_queue_core.c | 59 ++-----------------------
3 files changed, 92 insertions(+), 55 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 036ec7d..9e6a293 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2372,6 +2372,9 @@ int skb_splice_bits(struct sk_buff *skb, unsigned int offset,
struct pipe_inode_info *pipe, unsigned int len,
unsigned int flags);
void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to);
+unsigned int skb_zerocopy_headlen(const struct sk_buff *from);
+void skb_zerocopy(struct sk_buff *to, const struct sk_buff *from,
+ int len, int hlen);
void skb_split(struct sk_buff *skb, struct sk_buff *skb1, const u32 len);
int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen);
void skb_scrub_packet(struct sk_buff *skb, bool xnet);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 8c5197f..c5cefbe 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2125,6 +2125,91 @@ __wsum skb_copy_and_csum_bits(const struct sk_buff *skb, int offset,
}
EXPORT_SYMBOL(skb_copy_and_csum_bits);
+ /**
+ * skb_zerocopy_headlen - Calculate headroom needed for skb_zerocopy()
+ * @from: source buffer
+ *
+ * Calculates the amount of linear headroom needed in the 'to' skb passed
+ * into skb_zerocopy().
+ */
+unsigned int
+skb_zerocopy_headlen(const struct sk_buff *from)
+{
+ unsigned int hlen = 0;
+
+ if (!from->head_frag ||
+ skb_headlen(from) < L1_CACHE_BYTES ||
+ skb_shinfo(from)->nr_frags >= MAX_SKB_FRAGS)
+ hlen = skb_headlen(from);
+
+ if (skb_has_frag_list(from))
+ hlen = from->len;
+
+ return hlen;
+}
+EXPORT_SYMBOL_GPL(skb_zerocopy_headlen);
+
+/**
+ * skb_zerocopy - Zero copy skb to skb
+ * @to: destination buffer
+ * @source: source buffer
+ * @len: number of bytes to copy from source buffer
+ * @hlen: size of linear headroom in destination buffer
+ *
+ * Copies up to `len` bytes from `from` to `to` by creating references
+ * to the frags in the source buffer.
+ *
+ * The `hlen` as calculated by skb_zerocopy_headlen() specifies the
+ * headroom in the `to` buffer.
+ */
+void
+skb_zerocopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
+{
+ int i, j = 0;
+ int plen = 0; /* length of skb->head fragment */
+ struct page *page;
+ unsigned int offset;
+
+ BUG_ON(!from->head_frag && !hlen);
+
+ /* dont bother with small payloads */
+ if (len <= skb_tailroom(to)) {
+ skb_copy_bits(from, 0, skb_put(to, len), len);
+ return;
+ }
+
+ if (hlen) {
+ skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
+ len -= hlen;
+ } else {
+ plen = min_t(int, skb_headlen(from), len);
+ if (plen) {
+ page = virt_to_head_page(from->head);
+ offset = from->data - (unsigned char *)page_address(page);
+ __skb_fill_page_desc(to, 0, page, offset, plen);
+ get_page(page);
+ j = 1;
+ len -= plen;
+ }
+ }
+
+ to->truesize += len + plen;
+ to->len += len + plen;
+ to->data_len += len + plen;
+
+ for (i = 0; i < skb_shinfo(from)->nr_frags; i++) {
+ if (!len)
+ break;
+ skb_shinfo(to)->frags[j] = skb_shinfo(from)->frags[i];
+ skb_shinfo(to)->frags[j].size = min_t(int, skb_shinfo(to)->frags[j].size, len);
+ len -= skb_shinfo(to)->frags[j].size;
+ skb_frag_ref(to, j);
+ j++;
+ }
+ skb_shinfo(to)->nr_frags = j;
+}
+EXPORT_SYMBOL_GPL(skb_zerocopy);
+
void skb_copy_and_csum_dev(const struct sk_buff *skb, u8 *to)
{
__wsum csum;
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 21258cf..615ee12 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -235,51 +235,6 @@ nfqnl_flush(struct nfqnl_instance *queue, nfqnl_cmpfn cmpfn, unsigned long data)
spin_unlock_bh(&queue->lock);
}
-static void
-nfqnl_zcopy(struct sk_buff *to, const struct sk_buff *from, int len, int hlen)
-{
- int i, j = 0;
- int plen = 0; /* length of skb->head fragment */
- struct page *page;
- unsigned int offset;
-
- /* dont bother with small payloads */
- if (len <= skb_tailroom(to)) {
- skb_copy_bits(from, 0, skb_put(to, len), len);
- return;
- }
-
- if (hlen) {
- skb_copy_bits(from, 0, skb_put(to, hlen), hlen);
- len -= hlen;
- } else {
- plen = min_t(int, skb_headlen(from), len);
- if (plen) {
- page = virt_to_head_page(from->head);
- offset = from->data - (unsigned char *)page_address(page);
- __skb_fill_page_desc(to, 0, page, offset, plen);
- get_page(page);
- j = 1;
- len -= plen;
- }
- }
-
- to->truesize += len + plen;
- to->len += len + plen;
- to->data_len += len + plen;
-
- for (i = 0; i < skb_shinfo(from)->nr_frags; i++) {
- if (!len)
- break;
- skb_shinfo(to)->frags[j] = skb_shinfo(from)->frags[i];
- skb_shinfo(to)->frags[j].size = min_t(int, skb_shinfo(to)->frags[j].size, len);
- len -= skb_shinfo(to)->frags[j].size;
- skb_frag_ref(to, j);
- j++;
- }
- skb_shinfo(to)->nr_frags = j;
-}
-
static int
nfqnl_put_packet_info(struct sk_buff *nlskb, struct sk_buff *packet,
bool csum_verify)
@@ -304,7 +259,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
{
size_t size;
size_t data_len = 0, cap_len = 0;
- int hlen = 0;
+ unsigned int hlen = 0;
struct sk_buff *skb;
struct nlattr *nla;
struct nfqnl_msg_packet_hdr *pmsg;
@@ -356,14 +311,8 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
if (data_len > entskb->len)
data_len = entskb->len;
- if (!entskb->head_frag ||
- skb_headlen(entskb) < L1_CACHE_BYTES ||
- skb_shinfo(entskb)->nr_frags >= MAX_SKB_FRAGS)
- hlen = skb_headlen(entskb);
-
- if (skb_has_frag_list(entskb))
- hlen = entskb->len;
- hlen = min_t(int, data_len, hlen);
+ hlen = skb_zerocopy_headlen(entskb);
+ hlen = min_t(unsigned int, hlen, data_len);
size += sizeof(struct nlattr) + hlen;
cap_len = entskb->len;
break;
@@ -504,7 +453,7 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
nla->nla_type = NFQA_PAYLOAD;
nla->nla_len = nla_attr_size(data_len);
- nfqnl_zcopy(skb, entskb, data_len, hlen);
+ skb_zerocopy(skb, entskb, data_len, hlen);
}
nlh->nlmsg_len = skb->len;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/2 net-next] openvswitch: Use skb_zerocopy() for upcall
From: Thomas Graf @ 2013-11-08 10:47 UTC (permalink / raw)
To: jesse, davem; +Cc: dev, netdev, eric.dumazet
In-Reply-To: <cover.1383906944.git.tgraf@suug.ch>
Use of skb_zerocopy() avoids the expensive call to memcpy() when
copying the packet data into the Netlink skb. Completes checksum
through skb_checksum_help() if needed.
Netlink messaged must be properly padded and aligned to meet
sanity checks of the user space counterpart.
Cost of memcpy is significantly reduced from:
+ 7.48% vhost-8471 [k] memcpy
+ 5.57% ovs-vswitchd [k] memcpy
+ 2.81% vhost-8471 [k] csum_partial_copy_generic
to:
+ 5.72% ovs-vswitchd [k] memcpy
+ 3.32% vhost-5153 [k] memcpy
+ 0.68% vhost-5153 [k] skb_zerocopy
(megaflows disabled)
Signed-off-by: Thomas Graf <tgraf@suug.ch>
Cc: Jesse Gross <jesse@nicira.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---
net/openvswitch/datapath.c | 52 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 45 insertions(+), 7 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 1408adc..3f170e3 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -381,10 +381,11 @@ static size_t key_attr_size(void)
}
static size_t upcall_msg_size(const struct sk_buff *skb,
- const struct nlattr *userdata)
+ const struct nlattr *userdata,
+ unsigned int hdrlen)
{
size_t size = NLMSG_ALIGN(sizeof(struct ovs_header))
- + nla_total_size(skb->len) /* OVS_PACKET_ATTR_PACKET */
+ + nla_total_size(hdrlen) /* OVS_PACKET_ATTR_PACKET */
+ nla_total_size(key_attr_size()); /* OVS_PACKET_ATTR_KEY */
/* OVS_PACKET_ATTR_USERDATA */
@@ -402,6 +403,7 @@ static int queue_userspace_packet(struct net *net, int dp_ifindex,
struct sk_buff *nskb = NULL;
struct sk_buff *user_skb; /* to be queued to userspace */
struct nlattr *nla;
+ unsigned int plen, hlen;
int err;
if (vlan_tx_tag_present(skb)) {
@@ -422,7 +424,13 @@ static int queue_userspace_packet(struct net *net, int dp_ifindex,
goto out;
}
- user_skb = genlmsg_new(upcall_msg_size(skb, upcall_info->userdata), GFP_ATOMIC);
+ /* Complete checksum if needed */
+ if (skb->ip_summed == CHECKSUM_PARTIAL &&
+ (err = skb_checksum_help(skb)))
+ goto out;
+
+ hlen = skb_zerocopy_headlen(skb);
+ user_skb = genlmsg_new(upcall_msg_size(skb, upcall_info->userdata, hlen), GFP_ATOMIC);
if (!user_skb) {
err = -ENOMEM;
goto out;
@@ -441,13 +449,43 @@ static int queue_userspace_packet(struct net *net, int dp_ifindex,
nla_len(upcall_info->userdata),
nla_data(upcall_info->userdata));
- nla = __nla_reserve(user_skb, OVS_PACKET_ATTR_PACKET, skb->len);
+ /* Only reserve room for attribute header, packet data is added
+ * in skb_zerocopy() */
+ if (!(nla = nla_reserve(user_skb, OVS_PACKET_ATTR_PACKET, 0)))
+ goto out;
+ nla->nla_len = nla_attr_size(skb->len);
- skb_copy_and_csum_dev(skb, nla_data(nla));
+ skb_zerocopy(user_skb, skb, skb->len, hlen);
- genlmsg_end(user_skb, upcall);
- err = genlmsg_unicast(net, user_skb, upcall_info->portid);
+ /* OVS user space expects the size of the message to be aligned to
+ * NLA_ALIGNTO. Aligning nlmsg_len is not enough, the actual bytes
+ * read must match nlmsg_len.
+ */
+ plen = NLA_ALIGN(user_skb->len) - user_skb->len;
+ if (plen > 0) {
+ int nr_frags = skb_shinfo(user_skb)->nr_frags;
+
+ if (nr_frags) {
+ skb_frag_t *frag;
+
+ frag = &skb_shinfo(user_skb)->frags[nr_frags -1];
+ skb_frag_size_add(frag, plen);
+ BUG_ON(frag->size > PAGE_SIZE);
+
+ user_skb->truesize += plen;
+ user_skb->len += plen;
+ user_skb->data_len += plen;
+ } else {
+ /* The linear headroom is aligned to NLMSG_ALIGN by
+ * genlmsg_new(), room must be available.
+ */
+ memset(skb_put(user_skb, plen), 0, plen);
+ }
+ }
+ ((struct nlmsghdr *) user_skb->data)->nlmsg_len = user_skb->len;
+
+ err = genlmsg_unicast(net, user_skb, upcall_info->portid);
out:
kfree_skb(nskb);
return err;
--
1.8.3.1
^ permalink raw reply related
* [PATCH 0/2 v4] Open vSwitch zerocopy upcall
From: Thomas Graf @ 2013-11-08 10:47 UTC (permalink / raw)
To: jesse, davem; +Cc: dev, netdev, eric.dumazet
Respin of the zerocopy patches for the openvswitch upcall.
V4: - Daniel Borkmann pointed out that the style in skbuff.h has changed
in net-next, adapted to no longer using extern in headers.
V3: - Removed unneeded alignment of nlmsg_len after padding
V2: - Added skb_zerocopy_headlen() to calculate headroom of destination
buffer. This also takes care of the from->head_frag issue.
- Attribute alignment for frag_list case
- API documentation
- performance data for CHECKSUM_PARTIAL tx case
Thomas Graf (2):
net: Export skb_zerocopy() to zerocopy from one skb to another
openvswitch: Use skb_zerocopy() for upcall
include/linux/skbuff.h | 3 ++
net/core/skbuff.c | 85 ++++++++++++++++++++++++++++++++++++
net/netfilter/nfnetlink_queue_core.c | 59 ++-----------------------
net/openvswitch/datapath.c | 52 +++++++++++++++++++---
4 files changed, 137 insertions(+), 62 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH linux-2.6.32.y] sctp: unbalanced rcu lock in ip_queue_xmit()
From: Nicolas Dichtel @ 2013-11-08 10:13 UTC (permalink / raw)
To: netdev, davem; +Cc: eric.dumazet, ben, w, stable, Nicolas Dichtel
The bug was introduced by commit b8710128e201 ("inet: add RCU protection to
inet->opt") (it's a backport of upstream commit f6d8bd051c39).
In SCTP case, packet is already routed, hence we jump to the label
'packet_routed', but without rcu_read_lock(). After this label,
rcu_read_unlock() is called unconditionally.
Spotted-by: Guo Fengtian <fengtian.guo@6wind.com>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
net/ipv4/ip_output.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 7dde039374e4..2cd69e3497ad 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -320,13 +320,13 @@ int ip_queue_xmit(struct sk_buff *skb, int ipfragok)
/* Skip all of this if the packet is already routed,
* f.e. by something like SCTP.
*/
+ rcu_read_lock();
rt = skb_rtable(skb);
if (rt != NULL)
goto packet_routed;
/* Make sure we can route this packet. */
rt = (struct rtable *)__sk_dst_check(sk, 0);
- rcu_read_lock();
inet_opt = rcu_dereference(inet->inet_opt);
if (rt == NULL) {
__be32 daddr;
--
1.8.4.1
^ permalink raw reply related
* Re: ipv6: a question about ECMP
From: Nicolas Dichtel @ 2013-11-08 10:09 UTC (permalink / raw)
To: sowmini varadhan, Duan Jiong, David Miller, netdev,
Hannes Frederic Sowa
In-Reply-To: <CACP96tRh345x4D1BqpOK2eWfoeVbqMnu-+jGfF7eyFC1dbQoug@mail.gmail.com>
Le 07/11/2013 19:32, sowmini varadhan a écrit :
> On Thu, Nov 7, 2013 at 7:16 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> Hi Duan!
>>
>> On Thu, Nov 07, 2013 at 06:33:20PM +0800, Duan Jiong wrote:
>>> After reading the ip6_pol_route(), i have a question about ECMP. Why we call
>>> the rt6_multipath_select() after calling rt6_select()?
>>> In my opinion, the route returned by rt6_select() has a highest score, but the route
>>> returned by rt6_multipath_select() may has a lower score than the former, because the
>>> ECMP don't take the route preference into consideration. That means that the kernel will
>>> choose a less-desirable route.
>>
>> ECMP routes only differ in the gateway the specify, so I doubt there will be
>> any change in the score they woud receive. rt6_multipath_select does merly
>> make sure we don't select the same route again and again.
>
> rt6_multipath_select() -> rt6_socre_route() seems to require that the
> interface *must* matchi, which is consistent with your assertion above that
> "ECMP routes differ in gw only".
In fact, ECMP routes have the same metric/weight and destination but not the
same next hop (ie gw + oif).
>
> But for IPv6, the gw addr is a a link-local, which is only required to be
> unique on the link. Thus, e.g., you can have fe80::1 as the gw on both eth0 and
> eth1.
Yes, oif can be different.
Note that gw can also be a global address.
>
> What is the assumption around "cost" for ECMP here- are we assuming some
> form of link bundling (Section 6 of rfc 2991) here? or is the "multiple parallel
> links" case handled somewhere else, that I am missing?
rt6_score_route() is called to check requested oif (see 52bd4c0c1551 "ipv6: fix
ecmp lookup when oif is specified").
Regards,
Nicolas
>
> --Sowmini
>
>>
>> Please note, the rt6_info's siblings fields were added for the solely purpose
>> of ECMP and the insertion only updates the siblings list if the above criteria
>> did hold. They make sure the routes lookup up do differ on each lookup, so it
>> does actually do multipath and does not depend on the order the routes where
>> inserted.
>>
>> Hope that helps,
>>
>> Hannes
>>
>> --
>> 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: make ndev->irq signed for error handling
From: Mugunthan V N @ 2013-11-08 9:18 UTC (permalink / raw)
To: Dan Carpenter; +Cc: David S. Miller, netdev, linux-kernel, kernel-janitors
In-Reply-To: <20131108084136.GY26669@mwanda>
On Friday 08 November 2013 02:11 PM, Dan Carpenter wrote:
> On Thu, Nov 07, 2013 at 10:03:19PM +0530, Mugunthan V N wrote:
>> On Thursday 07 November 2013 05:52 PM, Dan Carpenter wrote:
>>> On Thu, Nov 07, 2013 at 05:44:38PM +0530, Mugunthan V N wrote:
>>>> On Thursday 07 November 2013 01:18 PM, Dan Carpenter wrote:
>>>>> There is a bug in cpsw_probe() where we do:
>>>>>
>>>>> ndev->irq = platform_get_irq(pdev, 0);
>>>>> if (ndev->irq < 0) {
>>>>>
>>>>> The problem is that "ndev->irq" is unsigned so the error handling
>>>>> doesn't work. I have changed it to a regular int.
>>>>>
>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>> ndev->irq is never used any where in the driver, I think its better to
>>>> remove this part of code from probe. If every one is ok, I can send a
>>>> patch to remove the code.
>>> It seems like cpsw_ndo_poll_controller() uses it.
>>>
>>>
>> That can be changed to pass one of the interrupt numbers from priv as
>> the irq number is not used in interrupt service routine.
> I don't know anything about this driver, so I'm fine with your idea.
>
Anyhow David has applied your patch, I will send a driver cleanup patch
seperately.
Regards
Mugunthan V N
^ permalink raw reply
* Re: [PATCH v3] can: add Renesas R-Car CAN driver
From: Marc Kleine-Budde @ 2013-11-08 9:17 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: netdev, wg, linux-can, linux-sh, vksavl
In-Reply-To: <527C28DE.7070402@cogentembedded.com>
[-- Attachment #1: Type: text/plain, Size: 7598 bytes --]
On 11/08/2013 12:57 AM, Sergei Shtylyov wrote:
>>> Index: linux-can-next/drivers/net/can/rcar_can.c
>>> ===================================================================
>>> --- /dev/null
>>> +++ linux-can-next/drivers/net/can/rcar_can.c
>>> @@ -0,0 +1,893 @@
>>> +/*
>>> + * Renesas R-Car CAN device driver
>>> + *
>>> + * Copyright (C) 2013 Cogent Embedded, Inc. <source@cogentembedded.com>
>>> + * Copyright (C) 2013 Renesas Solutions Corp.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> modify it
>>> + * under the terms of the GNU General Public License as published
>>> by the
>>> + * Free Software Foundation; either version 2 of the License, or
>>> (at your
>>> + * option) any later version.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/types.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/netdevice.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/can/led.h>
>>> +#include <linux/can/dev.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/can/platform/rcar_can.h>
>>> +
>>> +#define DRV_NAME "rcar_can"
>>> +
>>> +/* Mailbox configuration:
>>> + * mailbox 0 - not used
>>> + * mailbox 1-31 - Rx
>>> + * mailbox 32-63 - Tx
>>> + * no FIFO mailboxes
>>> + */
>>> +#define N_MBX 64
>>> +#define FIRST_TX_MB 32
>>> +#define N_TX_MB (N_MBX - FIRST_TX_MB)
>>> +#define RX_MBX_MASK 0xFFFFFFFE
>
>> Please use a common prefix for all defines.
>
> OK, done now. Could you however explain why the file-local #define's
> should be prefixed? It's not quite obvious to me...
It's about readability and maintainability. If you don't know the
driver, but all driver local defines and functions have a common prefix,
it's much easier to read if you are not the author of the driver IMHO.
>
> [...]
>>> +static netdev_tx_t rcar_can_start_xmit(struct sk_buff *skb,
>>> + struct net_device *ndev)
>>> +{
>>> + struct rcar_can_priv *priv = netdev_priv(ndev);
>>> + struct can_frame *cf = (struct can_frame *)skb->data;
>>> + u32 data, mier1, mbxno, i;
>>> + unsigned long flags;
>>> + u8 mctl = 0;
>>> +
>>> + if (can_dropped_invalid_skb(ndev, skb))
>>> + return NETDEV_TX_OK;
>>> +
>>> + spin_lock_irqsave(&priv->mier_lock, flags);
>>> + mier1 = readl(&priv->regs->mier1);
>>> + if (mier1) {
>>> + i = __builtin_clz(mier1);
>>> + mbxno = i ? N_MBX - i : FIRST_TX_MB;
>>> + } else {
>>> + mbxno = FIRST_TX_MB;
>>> + }
>
>> Can you explain how the hardware arbitration works, and you do you
>> guarantee the CAN frames are send by the hardware in the same order you
>> put them into the hardware.
>
> Tx mailbox with the smallest mailbox number has the highest priority.
> The other possible Tx mailbox selection rule (not used by the driver
> now) is ID priority transmit mode (as defined in the ISO 11898-1 specs).
> The algorithm used guarantees the mailboxes are filled sequentially.
I see. You are using mier1 to track the used mailboxes....correct?
> + if (unlikely(mier1 == 0xffffffff))
> + netif_stop_queue(ndev);
Then you have a race condition in your tx-complete handler
rcar_can_tx_done(), as you call netif_wake_queue() unconditionally. If
mier1 == 0xffffffff you must wait until _all_ mailboxes are transmitted
until you are allowed to reenable the mailboxes. Have a look at the
at91_can driver, it's using a similar scheme. The lowest mailbox is
transmitted first, but there are three additional bits that indicate the
priority.
> I've used 'canfdtest' as suggested by Wolfgang Grandegger to verify, see
> the log below:
>
> root@am335x-evm:~# ./canfdtest -v -g can0
> interface = can0, family = 29, type = 3, proto = 1
> ...............................................................................C
>
> Test messages sent and received: 483203
> Exiting...
> root@am335x-evm:~#
>
> [...]
>>> +static int rcar_can_rx_poll(struct napi_struct *napi, int quota)
>>> +{
>>> + struct rcar_can_priv *priv = container_of(napi,
>>> + struct rcar_can_priv, napi);
>>> + int num_pkts = 0;
>>> +
>>> + /* Find mailbox */
>>> + while (num_pkts < quota) {
>>> + u8 mctl, mbx;
>>> +
>>> + mbx = readb(&priv->regs->mssr);
>
>> How does the RX work? Is it a hardware FIFO?
>
> In short, the MSSR register provides the smallest Rx mailbox number
> that is looked up in the Rx search mode. We read MSSR until no search
> results can be obtained, so it is some sort of FIFO.
This looks racy....
> And there is separate FIFO operation mode: some mailboxes can be
> configured as FIFO and serviced by special registers but this operation
> mode is not supported by the driver.
if you hardware supports a real FIFO then I strongly suggest to make use
of it.
> [...]
>>> +static int rcar_can_probe(struct platform_device *pdev)
>>> +{
>>> + struct rcar_can_platform_data *pdata;
>>> + struct rcar_can_priv *priv;
>>> + struct net_device *ndev;
>>> + struct resource *mem;
>>> + void __iomem *addr;
>>> + int err = -ENODEV;
>>> + int irq;
>>> +
>>> + pdata = dev_get_platdata(&pdev->dev);
>>> + if (!pdata) {
>>> + dev_err(&pdev->dev, "No platform data provided!\n");
>>> + goto fail;
>>> + }
>>> +
>>> + irq = platform_get_irq(pdev, 0);
>>> + if (!irq) {
>>> + dev_err(&pdev->dev, "No IRQ resource\n");
>>> + goto fail;
>>> + }
>>> +
>>> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + addr = devm_ioremap_resource(&pdev->dev, mem);
>>> + if (IS_ERR(addr)) {
>>> + err = PTR_ERR(addr);
>>> + goto fail;
>>> + }
>>> +
>>> + ndev = alloc_candev(sizeof(struct rcar_can_priv), N_MBX -
>>> FIRST_TX_MB);
>>> + if (!ndev) {
>>> + dev_err(&pdev->dev, "alloc_candev failed\n");
>>> + err = -ENOMEM;
>>> + goto fail;
>>> + }
>>> +
>>> + priv = netdev_priv(ndev);
>>> +
>>> + priv->clk = devm_clk_get(&pdev->dev, NULL);
>>> + if (IS_ERR(priv->clk)) {
>>> + err = PTR_ERR(priv->clk);
>>> + dev_err(&pdev->dev, "cannot get clock: %d\n", err);
>>> + goto fail_clk;
>>> + }
>>> +
>>> + ndev->netdev_ops = &rcar_can_netdev_ops;
>>> + ndev->irq = irq;
>>> + ndev->flags |= IFF_ECHO;
>>> + priv->ndev = ndev;
>>> + priv->regs = addr;
>>> + priv->clock_select = pdata->clock_select;
>>> + priv->can.clock.freq = clk_get_rate(priv->clk);
>>> + priv->can.bittiming_const = &rcar_can_bittiming_const;
>>> + priv->can.do_set_bittiming = rcar_can_set_bittiming;
>
>> Please call this function directly during the open() function.
>
> OK, done, and the method installation was removed. However, I'm not
> sure why you requested this as many drivers are still using the method.
The callback was there from the beginning, but then we figured out that
we don't need it in the driver, but no one has cleaned up the drivers
yet. So don't use it in new drivers. I know it's not documented anywhere :(
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]
^ permalink raw reply
* [PATCH 2/2 net-next] openvswitch: Use skb_zerocopy() for upcall
From: Thomas Graf @ 2013-11-08 9:15 UTC (permalink / raw)
To: jesse, davem; +Cc: dev, netdev, eric.dumazet
In-Reply-To: <cover.1383901577.git.tgraf@suug.ch>
Use of skb_zerocopy() avoids the expensive call to memcpy() when
copying the packet data into the Netlink skb. Completes checksum
through skb_checksum_help() if needed.
Netlink messaged must be properly padded and aligned to meet
sanity checks of the user space counterpart.
Cost of memcpy is significantly reduced from:
+ 7.48% vhost-8471 [k] memcpy
+ 5.57% ovs-vswitchd [k] memcpy
+ 2.81% vhost-8471 [k] csum_partial_copy_generic
to:
+ 5.72% ovs-vswitchd [k] memcpy
+ 3.32% vhost-5153 [k] memcpy
+ 0.68% vhost-5153 [k] skb_zerocopy
(megaflows disabled)
Signed-off-by: Thomas Graf <tgraf@suug.ch>
---
net/openvswitch/datapath.c | 52 +++++++++++++++++++++++++++++++++++++++-------
1 file changed, 45 insertions(+), 7 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 1408adc..3f170e3 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -381,10 +381,11 @@ static size_t key_attr_size(void)
}
static size_t upcall_msg_size(const struct sk_buff *skb,
- const struct nlattr *userdata)
+ const struct nlattr *userdata,
+ unsigned int hdrlen)
{
size_t size = NLMSG_ALIGN(sizeof(struct ovs_header))
- + nla_total_size(skb->len) /* OVS_PACKET_ATTR_PACKET */
+ + nla_total_size(hdrlen) /* OVS_PACKET_ATTR_PACKET */
+ nla_total_size(key_attr_size()); /* OVS_PACKET_ATTR_KEY */
/* OVS_PACKET_ATTR_USERDATA */
@@ -402,6 +403,7 @@ static int queue_userspace_packet(struct net *net, int dp_ifindex,
struct sk_buff *nskb = NULL;
struct sk_buff *user_skb; /* to be queued to userspace */
struct nlattr *nla;
+ unsigned int plen, hlen;
int err;
if (vlan_tx_tag_present(skb)) {
@@ -422,7 +424,13 @@ static int queue_userspace_packet(struct net *net, int dp_ifindex,
goto out;
}
- user_skb = genlmsg_new(upcall_msg_size(skb, upcall_info->userdata), GFP_ATOMIC);
+ /* Complete checksum if needed */
+ if (skb->ip_summed == CHECKSUM_PARTIAL &&
+ (err = skb_checksum_help(skb)))
+ goto out;
+
+ hlen = skb_zerocopy_headlen(skb);
+ user_skb = genlmsg_new(upcall_msg_size(skb, upcall_info->userdata, hlen), GFP_ATOMIC);
if (!user_skb) {
err = -ENOMEM;
goto out;
@@ -441,13 +449,43 @@ static int queue_userspace_packet(struct net *net, int dp_ifindex,
nla_len(upcall_info->userdata),
nla_data(upcall_info->userdata));
- nla = __nla_reserve(user_skb, OVS_PACKET_ATTR_PACKET, skb->len);
+ /* Only reserve room for attribute header, packet data is added
+ * in skb_zerocopy() */
+ if (!(nla = nla_reserve(user_skb, OVS_PACKET_ATTR_PACKET, 0)))
+ goto out;
+ nla->nla_len = nla_attr_size(skb->len);
- skb_copy_and_csum_dev(skb, nla_data(nla));
+ skb_zerocopy(user_skb, skb, skb->len, hlen);
- genlmsg_end(user_skb, upcall);
- err = genlmsg_unicast(net, user_skb, upcall_info->portid);
+ /* OVS user space expects the size of the message to be aligned to
+ * NLA_ALIGNTO. Aligning nlmsg_len is not enough, the actual bytes
+ * read must match nlmsg_len.
+ */
+ plen = NLA_ALIGN(user_skb->len) - user_skb->len;
+ if (plen > 0) {
+ int nr_frags = skb_shinfo(user_skb)->nr_frags;
+
+ if (nr_frags) {
+ skb_frag_t *frag;
+
+ frag = &skb_shinfo(user_skb)->frags[nr_frags -1];
+ skb_frag_size_add(frag, plen);
+ BUG_ON(frag->size > PAGE_SIZE);
+
+ user_skb->truesize += plen;
+ user_skb->len += plen;
+ user_skb->data_len += plen;
+ } else {
+ /* The linear headroom is aligned to NLMSG_ALIGN by
+ * genlmsg_new(), room must be available.
+ */
+ memset(skb_put(user_skb, plen), 0, plen);
+ }
+ }
+ ((struct nlmsghdr *) user_skb->data)->nlmsg_len = user_skb->len;
+
+ err = genlmsg_unicast(net, user_skb, upcall_info->portid);
out:
kfree_skb(nskb);
return err;
--
1.8.3.1
^ 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