* [PATCH v2 17/20] rtl8xxxu: constify usb_device_id
From: Arvind Yadav @ 2017-08-09 16:30 UTC (permalink / raw)
To: kvalo, Jes.Sorensen; +Cc: linux-kernel, netdev, linux-wireless
usb_device_id are not supposed to change at runtime. All functions
working with usb_device_id provided by <linux/usb.h> work with
const usb_device_id. So mark the non-const structs as const.
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
changes in v2:
Re-submitting wireless separately.
drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
index 21e5ef0..7806a4d 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
@@ -6190,7 +6190,7 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
ieee80211_free_hw(hw);
}
-static struct usb_device_id dev_table[] = {
+static const struct usb_device_id dev_table[] = {
{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDOR_ID_REALTEK, 0x8724, 0xff, 0xff, 0xff),
.driver_info = (unsigned long)&rtl8723au_fops},
{USB_DEVICE_AND_INTERFACE_INFO(USB_VENDOR_ID_REALTEK, 0x1724, 0xff, 0xff, 0xff),
--
2.7.4
^ permalink raw reply related
* [PATCH v2 18/20] rtl8192cu: constify usb_device_id
From: Arvind Yadav @ 2017-08-09 16:31 UTC (permalink / raw)
To: kvalo, Larry.Finger, chaoming_li; +Cc: linux-kernel, netdev, linux-wireless
usb_device_id are not supposed to change at runtime. All functions
working with usb_device_id provided by <linux/usb.h> work with
const usb_device_id. So mark the non-const structs as const.
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
changes in v2:
Re-submitting wireless separately.
drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c
index 96c923b..e673bc2 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192cu/sw.c
@@ -275,7 +275,7 @@ static struct rtl_hal_cfg rtl92cu_hal_cfg = {
#define USB_VENDER_ID_REALTEK 0x0bda
/* 2010-10-19 DID_USB_V3.4 */
-static struct usb_device_id rtl8192c_usb_ids[] = {
+static const struct usb_device_id rtl8192c_usb_ids[] = {
/*=== Realtek demoboard ===*/
/* Default ID */
--
2.7.4
^ permalink raw reply related
* [PATCH v2 19/20] zd1201: constify usb_device_id
From: Arvind Yadav @ 2017-08-09 16:31 UTC (permalink / raw)
To: kvalo; +Cc: linux-kernel, netdev, linux-wireless
usb_device_id are not supposed to change at runtime. All functions
working with usb_device_id provided by <linux/usb.h> work with
const usb_device_id. So mark the non-const structs as const.
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
changes in v2:
Re-submitting wireless separately.
drivers/net/wireless/zydas/zd1201.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/zydas/zd1201.c b/drivers/net/wireless/zydas/zd1201.c
index 7f586d7..581e857 100644
--- a/drivers/net/wireless/zydas/zd1201.c
+++ b/drivers/net/wireless/zydas/zd1201.c
@@ -25,7 +25,7 @@
#include <linux/firmware.h>
#include "zd1201.h"
-static struct usb_device_id zd1201_table[] = {
+static const struct usb_device_id zd1201_table[] = {
{USB_DEVICE(0x0586, 0x3400)}, /* Peabird Wireless USB Adapter */
{USB_DEVICE(0x0ace, 0x1201)}, /* ZyDAS ZD1201 Wireless USB Adapter */
{USB_DEVICE(0x050d, 0x6051)}, /* Belkin F5D6051 usb adapter */
--
2.7.4
^ permalink raw reply related
* [PATCH v2 20/20] zd1211rw: constify usb_device_id
From: Arvind Yadav @ 2017-08-09 16:31 UTC (permalink / raw)
To: kvalo, dsd, kune; +Cc: linux-kernel, netdev, linux-wireless
usb_device_id are not supposed to change at runtime. All functions
working with usb_device_id provided by <linux/usb.h> work with
const usb_device_id. So mark the non-const structs as const.
Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
changes in v2:
Re-submitting wireless separately.
drivers/net/wireless/zydas/zd1211rw/zd_usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
index 01ca1d5..c30bf11 100644
--- a/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
+++ b/drivers/net/wireless/zydas/zd1211rw/zd_usb.c
@@ -35,7 +35,7 @@
#include "zd_mac.h"
#include "zd_usb.h"
-static struct usb_device_id usb_ids[] = {
+static const struct usb_device_id usb_ids[] = {
/* ZD1211 */
{ USB_DEVICE(0x0105, 0x145f), .driver_info = DEVICE_ZD1211 },
{ USB_DEVICE(0x0586, 0x3401), .driver_info = DEVICE_ZD1211 },
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v9 1/4] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING
From: Casey Leedom @ 2017-08-09 16:36 UTC (permalink / raw)
To: Ding Tianhong, Bjorn Helgaas
Cc: mark.rutland@arm.com, gabriele.paoloni@huawei.com,
asit.k.mallick@intel.com, catalin.marinas@arm.com,
will.deacon@arm.com, linuxarm@huawei.com,
alexander.duyck@gmail.com, ashok.raj@intel.com,
jeffrey.t.kirsher@intel.com, linux-pci@vger.kernel.org, Ganesh GR,
Bob.Shaw@amd.com, patrick.j.cramer@intel.com, bhelgaas@google.com,
Michael Werner, linux-arm-kernel@lists.infradead.org,
amira@mellanox.com, "netdev@vger.kerne
In-Reply-To: <d168687d-f849-4d09-1a2d-c9c6718b3318@huawei.com>
| From: Ding Tianhong <dingtianhong@huawei.com>
| Sent: Wednesday, August 9, 2017 5:17 AM
|
| On 2017/8/9 11:02, Bjorn Helgaas wrote:
| >
| > On Wed, Aug 09, 2017 at 01:40:01AM +0000, Casey Leedom wrote:
| > >
| >> | From: Bjorn Helgaas <helgaas@kernel.org>
| >> | Sent: Tuesday, August 8, 2017 4:22 PM
| >> | ...
| >> | It should also include a pointer to the AMD erratum, if available, or
| >> | at least some reference to how we know it doesn't obey the rules.
| >>
| >> Getting an ACK from AMD seems like a forlorn cause at this point. My
| >> contact was Bob Shaw <Bob.Shaw@amd.com> and he stopped responding to me
| >> messages almost a year ago saying that all of AMD's energies were being
| >> redirected towards upcoming x86 products (likely Ryzen as we now know).
| >> As far as I can tell AMD has walked away from their A1100 (AKA
| >> "Seattle") ARM SoC.
| >>
| >> On the specific issue, I can certainly write up somthing even more
| >> extensive than I wrote up for the comment in drivers/pci/quirks.c.
| >> Please review the comment I wrote up and tell me if you'd like
| >> something even more detailed -- I'm usually acused of writing comments
| >> which are too long, so this would be a new one on me ... :-)
| >
| > If you have any bug reports with info about how you debugged it and
| > concluded that Seattle is broken, you could include a link (probably
| > in the changelog). But if there isn't anything, there isn't anything.
| ...
| OK, I could reorganize it, but still need the Casey to give me the link
| for the Seattle, otherwise I could remove the AMD part and wait until
| someone show it. Thanks
There are no links and I was never given an internal bug number at AMD. As
I said, they stopped responding to my notes about a years ago saying that
they were moving the focus of all their people and no longer had resources
to pursue the issue. Hopefully for them, Ryzen doesn't have the same
Data Corruption problem ...
As for how we diagnosed it, with our Ingress Packet delivery, we have the
Ingress Packet Data delivered (DMA Write) into Free List Buffers, and then
then a small message (DMA Write) to a "Response Queue" indicating delivery
of the Ingress Packet Data into the Free List Buffers. The Transaction
Layer Packets which convey the Ingress Packet Data all have the Relaxed
Ordering Attribute set, while the following TLP carring the Ingress Data
delivery notification into the Response Queue does not have the Relaxed
Ordering Attribute set.
The rules for processing TLPs with and without the Relaxed Ordering
Attribute set are covered in Section 2.4.1 of the PCIe 3.0 specification
(Revision 3.0 November 10, 2010). Table 2-34 "Ordering Rules Summary"
covers the cases where one TLP may "pass" (be proccessed earlier) than a
preceding TLP. In the case we're talking about, we have a sequence of one
or more Posted DMA Write TLPs with the Relaxed Ordering Attribute set and a
following Posted DMA Write TLP without the Relaxed Ordering Attribute set.
Thus we need to look at the Row A, Column 2 cell of Table 2-34 governing
when a Posted Request may "pass" a preceeding Posted Request. In that cell
we have:
a) No
b) Y/N
with the explanatory text:
A2a A Posted Request must not pass another Posted Request
unless A2b applies.
A2b A Posted Request with RO[23] Set is permitted to pass
another Posted Request[24]. A Posted Request with IDO
Set is permitted to pass another Posted Request if the
two Requester IDs are different.
[23] In this section, "RO" is an abbreviation for the Relaxed
Ordering Attribute field.
[24] Some usages are enabled by not implementing this passing
(see the No RO-enabled PR-PR Passing bit in Section
7.8.15).
In our case, we were getting notifications of Ingress Packet Delivery in our
Response Queues, but not all of the Ingress Packet Data Posted DMA Write
TLPs had been processed yet by the Root Complex. As a result, we were
picking up old stale memory data before those lagging Ingress Packet Data
TLPs could be processed. This is a clear violation of the PCIe 3.0 TLP
processing rules outlined above.
Does that help?
Casey
^ permalink raw reply
* Re: [PATCH V4 net 0/2] ipv6: fix flowlabel issue for reset packet
From: Tom Herbert @ 2017-08-09 16:40 UTC (permalink / raw)
To: Shaohua Li; +Cc: Linux Kernel Network Developers, David S. Miller, Shaohua Li
In-Reply-To: <cover.1501539150.git.shli@fb.com>
On Mon, Jul 31, 2017 at 3:19 PM, Shaohua Li <shli@kernel.org> wrote:
> From: Shaohua Li <shli@fb.com>
>
> Please see below tcpdump output:
> 21:00:48.109122 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 40) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [S], cksum 0x0529 (incorrect -> 0xf56c), seq 3282214508, win 43690, options [mss 65476,sackOK,TS val 2500903437 ecr 0,nop,wscale 7], length 0
> 21:00:48.109381 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 40) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [S.], cksum 0x0529 (incorrect -> 0x49ad), seq 1923801573, ack 3282214509, win 43690, options [mss 65476,sackOK,TS val 2500903437 ecr 2500903437,nop,wscale 7], length 0
> 21:00:48.109548 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1bdf), seq 1, ack 1, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 0
> 21:00:48.109823 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 62) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [P.], cksum 0x053f (incorrect -> 0xb8b1), seq 1:31, ack 1, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 30
> 21:00:48.109910 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [.], cksum 0x0521 (incorrect -> 0x1bc1), seq 1, ack 31, win 342, options [nop,nop,TS val 2500903437 ecr 2500903437], length 0
> 21:00:48.110043 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 56) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [P.], cksum 0x0539 (incorrect -> 0xb726), seq 1:25, ack 31, win 342, options [nop,nop,TS val 2500903438 ecr 2500903437], length 24
> 21:00:48.110173 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1ba7), seq 31, ack 25, win 342, options [nop,nop,TS val 2500903438 ecr 2500903438], length 0
> 21:00:48.110211 IP6 (flowlabel 0xd827f, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [F.], cksum 0x0521 (incorrect -> 0x1ba7), seq 25, ack 31, win 342, options [nop,nop,TS val 2500903438 ecr 2500903437], length 0
> 21:00:48.151099 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 32) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [.], cksum 0x0521 (incorrect -> 0x1ba6), seq 31, ack 26, win 342, options [nop,nop,TS val 2500903438 ecr 2500903438], length 0
> 21:00:49.110524 IP6 (flowlabel 0x43304, hlim 64, next-header TCP (6) payload length: 56) fec0::5054:ff:fe12:3456.55804 > fec0::5054:ff:fe12:3456.5555: Flags [P.], cksum 0x0539 (incorrect -> 0xb324), seq 31:55, ack 26, win 342, options [nop,nop,TS val 2500904438 ecr 2500903438], length 24
> 21:00:49.110637 IP6 (flowlabel 0xb34d5, hlim 64, next-header TCP (6) payload length: 20) fec0::5054:ff:fe12:3456.5555 > fec0::5054:ff:fe12:3456.55804: Flags [R], cksum 0x0515 (incorrect -> 0x668c), seq 1923801599, win 0, length 0
>
> The flowlabel of reset packet (0xb34d5) and flowlabel of normal packet
> (0xd827f) are different. This causes our router doesn't correctly close tcp
> connection. The patches try to fix the issue.
>
Shaohua,
Can you give some more detail about what the router doesn't close the
TCP connection means? I'm guessing the problem is either: 1) the
router is maintaining connection state that includes the flow label in
a connection tuple. 2) some router in the path is maintaining
connection state, but when the flow label changes the flow's packet
are routed through a different router that doesn't have a state for
the flow it drops the packet. #1 should be easily fix in the router,
flow labels cannot be used as state. #2 is the known problem that
stateful firewalls have killed our ability to use multihoming.
Another consideration is that sk_txhash is also used in routing
decisions by the local host (flow label is normally derived from
txhash). If you want to ensure that connections are routed
consistently for timewait state you might need sk_txhash saved also.
Tom
> Thanks,
> Shaohua
>
> Shaohua Li (2):
> net: remove unnecessary rotation
> net: fix tcp reset packet flowlabel for ipv6
>
> include/net/ipv6.h | 15 ++++-----------
> net/ipv4/tcp_minisocks.c | 8 +++++++-
> net/ipv6/ip6_gre.c | 2 +-
> net/ipv6/ip6_output.c | 4 ++--
> net/ipv6/ip6_tunnel.c | 2 +-
> net/ipv6/tcp_ipv6.c | 18 +++++++++++++++++-
> 6 files changed, 32 insertions(+), 17 deletions(-)
>
> --
> 2.9.3
>
^ permalink raw reply
* Re: [PATCH net-next 0/7] rtnetlink: allow to run selected handlers without rtnl
From: David Miller @ 2017-08-09 16:41 UTC (permalink / raw)
To: fw; +Cc: netdev
In-Reply-To: <20170809081928.GA26899@breakpoint.cc>
From: Florian Westphal <fw@strlen.de>
Date: Wed, 9 Aug 2017 10:19:28 +0200
> David Miller <davem@davemloft.net> wrote:
>> From: Florian Westphal <fw@strlen.de>
>> Date: Tue, 8 Aug 2017 18:02:29 +0200
>>
>> > Unfortunately RTNL mutex is a performance issue, e.g. a cpu adding
>> > an ip address prevents other cpus from seemingly unrelated tasks
>> > such as dumping tc classifiers.
>>
>> It is related if somehow the TC entries refer to IP addresses.
>>
>> Someone could create something like that.
>
> Actually I am not following. Why would read-only accesses need rtnl
> locking wrt. any other operation (provided of course rtnl lock doesn't
> protect the data structure)?
If the validity of change X depends upon another value Y meeting some
criteria, we test Y and then must be sure that Y doesn't change while
we go about making the change to X.
For example if I need to make sure a path to an IPV4 address exists
configured to an interface when adding a route, I must be sure that
someone can't remove that IPV4 address while I'm adding the route,
after I've checked that it does in fact exist.
This is the kind of stuff that the RTNL mutex ensures.
^ permalink raw reply
* Re: [PATCH net-next 0/7] rtnetlink: allow to run selected handlers without rtnl
From: David Miller @ 2017-08-09 16:42 UTC (permalink / raw)
To: fw; +Cc: netdev
In-Reply-To: <20170809081928.GA26899@breakpoint.cc>
From: Florian Westphal <fw@strlen.de>
Date: Wed, 9 Aug 2017 10:19:28 +0200
> Would you accept a v2 if i don't touch ipv6 routes for the time being?
>
> I would then audit those again. At the very least inet6_rtm_getroute should
> be able to work without rtnl lock (i.e., use a different lock if
> needed to protect vs. concurrent modifications).
Generally speaking 'get' operations should be fine.
^ permalink raw reply
* Re: skb allocation from interrupt handler?
From: Murali Karicheri @ 2017-08-09 16:36 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20170808.160022.550877862496200067.davem@davemloft.net>
Hi David,
On 08/08/2017 07:00 PM, David Miller wrote:
> From: Murali Karicheri <m-karicheri2@ti.com>
> Date: Tue, 8 Aug 2017 18:17:52 -0400
>
>> Is there an skb_alloc function that can be used from interrupt handler? Looks like netdev_alloc_skb()
>> can't be used since I see following trace with kernel hack debug options enabled.
>>
>> [ 652.481713] [<c021007c>] (unwind_backtrace) from [<c020bdcc>] (show_stack+0x10/0x14)
>> [ 652.481725] [<c020bdcc>] (show_stack) from [<c0517780>] (dump_stack+0x98/0xc4)
>> [ 652.481736] [<c0517780>] (dump_stack) from [<c0256a70>] (___might_sleep+0x1b8/0x2a4)
>> [ 652.481746] [<c0256a70>] (___might_sleep) from [<c0939e80>] (rt_spin_lock+0x24/0x5c)
>> [ 652.481755] [<c0939e80>] (rt_spin_lock) from [<c07d827c>] (__netdev_alloc_skb+0xd0/0x254)
>> [ 652.481774] [<c07d827c>] (__netdev_alloc_skb) from [<bf23a544>] (emac_rx_hardirq+0x374/0x554 [prueth])
>> [ 652.481793] [<bf23a544>] (emac_rx_hardirq [prueth]) from [<c02925dc>] (__handle_irq_event_percpu+0x9c/0x128)
>>
>> This is running under RT kernel off 4.9.y
>
> Your receive handler should be running from a NAPI poll, which is in
> software interrupt. You should not be doing packet processing in
> hardware interrupt context as hardware interrupts should be as short
> as possible, and with NAPI polling packet input processing can be
> properly distributed amongst several devices, and if the system is
> overloaded such processing can be deferred to a kernel thread.
>
Thanks for responding! I appreciate your feedback.
Our NetCP and CPSW device drivers do use NAPI poll to process receive packets.
However these hardwares have capability to use ring buffers or descriptors setup
in DDR to enqueue the received packets to the CPU. However the specific hardware
(in fact a firmware running in the ICSS PRU that is available on our industrial
IDK SoCs) have limited internal memory that is shared between the ARM and PRU to
enqueue the received packets to the CPU for processing. This is using a 100Mbps
Ethernet link. As per NAPI documentation, at
https://wiki.linuxfoundation.org/networking/napi
two of the conditions mentioned there for using NAPI are
====== Quote from the above link ================================================
DMA ring or enough RAM to store packets in software devices.
Ability to turn off interrupts or maybe events that send packets up the stack.
==================================================================================
The internal memory or FIFO can store only up to 3 MTU sized packets. So that has to
be processed before PRU gets another packets to send to CPU. So per above,
it is not ideal to run NAPI for this scenario, right? Also for NetCP we use
about 128 descriptors with MTU size buffers to handle 1Gbps Ethernet link.
Based on that roughly we would need at least 10-12 buffers in the FIFO.
Currently we have a NAPI implementation in use that gives throughput of 95Mbps for
MTU sized packets, but our UDP iperf tests shows less than 1% packet loss for an
offered traffic of 95Mbps with MTU sized packets. This is not good for industrial
network using HSR/PRP protocol for network redundancy. We need to have zero packet
loss for MTU sized packets at 95Mbps throughput. That is the problem description.
As an experiment, I have moved the packet processing to irq handler to see if we
can take advantage of CPU cycle to processing the packet instead of NAPI
and to check if the firmware encounters buffer overflow. The result is positive
with no buffer overflow seen at the firmware and no packet loss in the iperf test.
But we want to do more testing as an experiment and ran into a uart console locks
up after running traffic for about 2 minutes. So I tried enabling the DEBUG HACK
options to get some clue on what is happening and ran into the trace I shared
earlier. So what function can I use to allocate SKB from interrupt handler?
Also wondering what is the best way to implement the packet processing in this
case to avoid the packet loss.
> NAPI polling has a large number of other advantages as well, more
> streamlined GRO support, automatic support for busypolling... the
> list goes on and on and on.
>
> I could show you how to do an SKB allocation in a hardware interrupt,
> but instead I'd rather teach you how to fish properly, and encourage
> you to convert your driver to NAPI polling instead.
>
Would love to use NAPI if we can overcome the packet loss in some way.
Thanks and regards,
Murali
> Thanks.
>
--
Murali Karicheri
Linux Kernel, Keystone
^ permalink raw reply
* Re: [PATCH v9 1/4] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING
From: Casey Leedom @ 2017-08-09 16:46 UTC (permalink / raw)
To: Raj, Ashok, Bjorn Helgaas
Cc: Ding Tianhong, bhelgaas@google.com, Michael Werner, Ganesh GR,
asit.k.mallick@intel.com, patrick.j.cramer@intel.com,
Suravee.Suthikulpanit@amd.com, Bob.Shaw@amd.com,
l.stach@pengutronix.de, amira@mellanox.com,
gabriele.paoloni@huawei.com, David.Laight@aculab.com,
jeffrey.t.kirsher@intel.com, catalin.marinas@arm.com,
will.deacon@arm.com,
"mark.rutland@arm.com" <mark.rutla
In-Reply-To: <20170809155841.GA8675@otc-nc-03>
| From: Raj, Ashok <ashok.raj@intel.com>
| Sent: Wednesday, August 9, 2017 8:58 AM
| ...
| As Casey pointed out in an earlier thread, we choose the heavy hammer
| approach because there are some that can lead to data-corruption as opposed
| to perf degradation.
Careful. As far as I'm aware, there is no Data Corruption problem
whatsoever with Intel Root Ports and processing of Transaction Layer Packets
with and without the Relaxed Ordering Attribute set.
The only issue which we've discovered with relatively recent Intel Root Port
implementations and the use of the Relaxed Ordering Attribute is a
performance issue. To the best of our ability to analyze the PCIe traces,
it appeared that the Intel Root Complex delayed returning Link Flow Control
Credits resulting in lowered performance (total bandwidth). When we used
Relaxed Ordering for Ingress Packet Data delivery on a 100Gb/s Ethernet
link with 1500-byte MTU, we were pegged at ~75Gb/s. Once we disabled
Relaxed Ordering, we were able to deliver Ingress Packet Data to Host Memory
at the full link rate.
Casey
^ permalink raw reply
* Re: [PATCH net-next 1/9] bpf: add BPF_J{LT,LE,SLT,SLE} instructions
From: David Miller @ 2017-08-09 16:55 UTC (permalink / raw)
To: daniel; +Cc: ast, holzheu, naveen.n.rao, jakub.kicinski, netdev
In-Reply-To: <63b41a38e0d58536700c999ade7c2c267bd2613d.1502272924.git.daniel@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 9 Aug 2017 12:23:53 +0200
> [1] https://github.com/borkmann/llvm/tree/bpf-insns
How is this "backwards compatible"?
If someone takes a new LLVM and tries to load those programs
into an older kernel they will be rejected.
There appears to be no effort to make things work cleanly in
that situation at all.
^ permalink raw reply
* Re: [PATCH net-next 1/9] bpf: add BPF_J{LT,LE,SLT,SLE} instructions
From: Daniel Borkmann @ 2017-08-09 17:00 UTC (permalink / raw)
To: David Miller; +Cc: ast, holzheu, naveen.n.rao, jakub.kicinski, netdev
In-Reply-To: <20170809.095553.268665355274669790.davem@davemloft.net>
On 08/09/2017 06:55 PM, David Miller wrote:
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Wed, 9 Aug 2017 12:23:53 +0200
>
>> [1] https://github.com/borkmann/llvm/tree/bpf-insns
>
> How is this "backwards compatible"?
>
> If someone takes a new LLVM and tries to load those programs
> into an older kernel they will be rejected.
>
> There appears to be no effort to make things work cleanly in
> that situation at all.
No, that was just the patch I used for LLVM to enable the
insns, so not the final one that will be submitted there
officially where we have a switch to enable/disable this
functionality.
^ permalink raw reply
* Re: [PATCH net-next 1/9] bpf: add BPF_J{LT,LE,SLT,SLE} instructions
From: Daniel Borkmann @ 2017-08-09 17:03 UTC (permalink / raw)
To: David Miller; +Cc: ast, holzheu, naveen.n.rao, jakub.kicinski, netdev
In-Reply-To: <598B3FCA.7060904@iogearbox.net>
On 08/09/2017 07:00 PM, Daniel Borkmann wrote:
> On 08/09/2017 06:55 PM, David Miller wrote:
>> From: Daniel Borkmann <daniel@iogearbox.net>
>> Date: Wed, 9 Aug 2017 12:23:53 +0200
>>
>>> [1] https://github.com/borkmann/llvm/tree/bpf-insns
>>
>> How is this "backwards compatible"?
>>
>> If someone takes a new LLVM and tries to load those programs
>> into an older kernel they will be rejected.
>>
>> There appears to be no effort to make things work cleanly in
>> that situation at all.
>
> No, that was just the patch I used for LLVM to enable the
> insns, so not the final one that will be submitted there
> officially where we have a switch to enable/disable this
> functionality.
(In other words the one I used for doing the measurements.)
^ permalink raw reply
* Re: [PATCH] ibmvnic: Fix unused variable warning
From: Tyrel Datwyler @ 2017-08-09 17:03 UTC (permalink / raw)
To: Michal Suchanek, Thomas Falcon, John Allen,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, netdev,
linuxppc-dev, linux-kernel
In-Reply-To: <20170809111602.12170-1-msuchanek@suse.de>
On 08/09/2017 04:16 AM, Michal Suchanek wrote:
> Fixes: a248878d7a1d ("ibmvnic: Check for transport event on driver resume")
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
Reviewed-by: Tyrel Datwyler <tyreld@linux.vnet.ibm.com>
^ permalink raw reply
* [PATCH net-next] net: xfrm: support setting an output mark.
From: Lorenzo Colitti @ 2017-08-09 17:08 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs, steffen.klassert, herbert, Lorenzo Colitti
On systems that use mark-based routing it may be necessary for
routing lookups to use marks in order for packets to be routed
correctly. An example of such a system is Android, which uses
socket marks to route packets via different networks.
Currently, routing lookups in tunnel mode always use a mark of
zero, making routing incorrect on such systems.
This patch adds a new output_mark element to the xfrm state and
a corresponding XFRMA_OUTPUT_MARK netlink attribute. The output
mark differs from the existing xfrm mark in two ways:
1. The xfrm mark is used to match xfrm policies and states, while
the xfrm output mark is used to set the mark (and influence
the routing) of the packets emitted by those states.
2. The existing mark is constrained to be a subset of the bits of
the originating socket or transformed packet, but the output
mark is arbitrary and depends only on the state.
The use of a separate mark provides additional flexibility. For
example:
- A packet subject to two transforms (e.g., transport mode inside
tunnel mode) can have two different output marks applied to it,
one for the transport mode SA and one for the tunnel mode SA.
- On a system where socket marks determine routing, the packets
emitted by an IPsec tunnel can be routed based on a mark that
is determined by the tunnel, not by the marks of the
unencrypted packets.
- Support for setting the output marks can be introduced without
breaking any existing setups that employ both mark-based
routing and xfrm tunnel mode. Simply changing the code to use
the xfrm mark for routing output packets could xfrm mark could
change behaviour in a way that breaks these setups.
If the output mark is unspecified or set to zero, the mark is not
set or changed.
Tested: make allyesconfig; make -j64
Tested: https://android-review.googlesource.com/452776
Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
---
include/net/xfrm.h | 9 ++++++---
include/uapi/linux/xfrm.h | 1 +
net/ipv4/xfrm4_policy.c | 14 +++++++++-----
net/ipv6/xfrm6_policy.c | 9 ++++++---
net/xfrm/xfrm_device.c | 3 ++-
net/xfrm/xfrm_output.c | 3 +++
net/xfrm/xfrm_policy.c | 17 +++++++++--------
net/xfrm/xfrm_user.c | 11 +++++++++++
8 files changed, 47 insertions(+), 20 deletions(-)
diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index afb4929d72..6b1ddb790d 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -163,6 +163,7 @@ struct xfrm_state {
int header_len;
int trailer_len;
u32 extra_flags;
+ u32 output_mark;
} props;
struct xfrm_lifetime_cfg lft;
@@ -296,10 +297,12 @@ struct xfrm_policy_afinfo {
struct dst_entry *(*dst_lookup)(struct net *net,
int tos, int oif,
const xfrm_address_t *saddr,
- const xfrm_address_t *daddr);
+ const xfrm_address_t *daddr,
+ u32 mark);
int (*get_saddr)(struct net *net, int oif,
xfrm_address_t *saddr,
- xfrm_address_t *daddr);
+ xfrm_address_t *daddr,
+ u32 mark);
void (*decode_session)(struct sk_buff *skb,
struct flowi *fl,
int reverse);
@@ -1638,7 +1641,7 @@ static inline int xfrm4_udp_encap_rcv(struct sock *sk, struct sk_buff *skb)
struct dst_entry *__xfrm_dst_lookup(struct net *net, int tos, int oif,
const xfrm_address_t *saddr,
const xfrm_address_t *daddr,
- int family);
+ int family, u32 mark);
struct xfrm_policy *xfrm_policy_alloc(struct net *net, gfp_t gfp);
diff --git a/include/uapi/linux/xfrm.h b/include/uapi/linux/xfrm.h
index 2b384ff09f..5fe7370a2b 100644
--- a/include/uapi/linux/xfrm.h
+++ b/include/uapi/linux/xfrm.h
@@ -304,6 +304,7 @@ enum xfrm_attr_type_t {
XFRMA_ADDRESS_FILTER, /* struct xfrm_address_filter */
XFRMA_PAD,
XFRMA_OFFLOAD_DEV, /* struct xfrm_state_offload */
+ XFRMA_OUTPUT_MARK, /* __u32 */
__XFRMA_MAX
#define XFRMA_MAX (__XFRMA_MAX - 1)
diff --git a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
index 4aefb149fe..d7bf0b0418 100644
--- a/net/ipv4/xfrm4_policy.c
+++ b/net/ipv4/xfrm4_policy.c
@@ -20,7 +20,8 @@
static struct dst_entry *__xfrm4_dst_lookup(struct net *net, struct flowi4 *fl4,
int tos, int oif,
const xfrm_address_t *saddr,
- const xfrm_address_t *daddr)
+ const xfrm_address_t *daddr,
+ u32 mark)
{
struct rtable *rt;
@@ -28,6 +29,7 @@ static struct dst_entry *__xfrm4_dst_lookup(struct net *net, struct flowi4 *fl4,
fl4->daddr = daddr->a4;
fl4->flowi4_tos = tos;
fl4->flowi4_oif = l3mdev_master_ifindex_by_index(net, oif);
+ fl4->flowi4_mark = mark;
if (saddr)
fl4->saddr = saddr->a4;
@@ -42,20 +44,22 @@ static struct dst_entry *__xfrm4_dst_lookup(struct net *net, struct flowi4 *fl4,
static struct dst_entry *xfrm4_dst_lookup(struct net *net, int tos, int oif,
const xfrm_address_t *saddr,
- const xfrm_address_t *daddr)
+ const xfrm_address_t *daddr,
+ u32 mark)
{
struct flowi4 fl4;
- return __xfrm4_dst_lookup(net, &fl4, tos, oif, saddr, daddr);
+ return __xfrm4_dst_lookup(net, &fl4, tos, oif, saddr, daddr, mark);
}
static int xfrm4_get_saddr(struct net *net, int oif,
- xfrm_address_t *saddr, xfrm_address_t *daddr)
+ xfrm_address_t *saddr, xfrm_address_t *daddr,
+ u32 mark)
{
struct dst_entry *dst;
struct flowi4 fl4;
- dst = __xfrm4_dst_lookup(net, &fl4, 0, oif, NULL, daddr);
+ dst = __xfrm4_dst_lookup(net, &fl4, 0, oif, NULL, daddr, mark);
if (IS_ERR(dst))
return -EHOSTUNREACH;
diff --git a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
index f44b25a484..11d1314ab6 100644
--- a/net/ipv6/xfrm6_policy.c
+++ b/net/ipv6/xfrm6_policy.c
@@ -27,7 +27,8 @@
static struct dst_entry *xfrm6_dst_lookup(struct net *net, int tos, int oif,
const xfrm_address_t *saddr,
- const xfrm_address_t *daddr)
+ const xfrm_address_t *daddr,
+ u32 mark)
{
struct flowi6 fl6;
struct dst_entry *dst;
@@ -36,6 +37,7 @@ static struct dst_entry *xfrm6_dst_lookup(struct net *net, int tos, int oif,
memset(&fl6, 0, sizeof(fl6));
fl6.flowi6_oif = l3mdev_master_ifindex_by_index(net, oif);
fl6.flowi6_flags = FLOWI_FLAG_SKIP_NH_OIF;
+ fl6.flowi6_mark = mark;
memcpy(&fl6.daddr, daddr, sizeof(fl6.daddr));
if (saddr)
memcpy(&fl6.saddr, saddr, sizeof(fl6.saddr));
@@ -52,12 +54,13 @@ static struct dst_entry *xfrm6_dst_lookup(struct net *net, int tos, int oif,
}
static int xfrm6_get_saddr(struct net *net, int oif,
- xfrm_address_t *saddr, xfrm_address_t *daddr)
+ xfrm_address_t *saddr, xfrm_address_t *daddr,
+ u32 mark)
{
struct dst_entry *dst;
struct net_device *dev;
- dst = xfrm6_dst_lookup(net, 0, oif, NULL, daddr);
+ dst = xfrm6_dst_lookup(net, 0, oif, NULL, daddr, mark);
if (IS_ERR(dst))
return -EHOSTUNREACH;
diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
index 5cd7a244e8..dc9840e366 100644
--- a/net/xfrm/xfrm_device.c
+++ b/net/xfrm/xfrm_device.c
@@ -79,7 +79,8 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
daddr = &x->props.saddr;
}
- dst = __xfrm_dst_lookup(net, 0, 0, saddr, daddr, x->props.family);
+ dst = __xfrm_dst_lookup(net, 0, 0, saddr, daddr,
+ x->props.family, x->props.output_mark);
if (IS_ERR(dst))
return 0;
diff --git a/net/xfrm/xfrm_output.c b/net/xfrm/xfrm_output.c
index 8c0b6722aa..31a2e6d34d 100644
--- a/net/xfrm/xfrm_output.c
+++ b/net/xfrm/xfrm_output.c
@@ -66,6 +66,9 @@ static int xfrm_output_one(struct sk_buff *skb, int err)
goto error_nolock;
}
+ if (x->props.output_mark)
+ skb->mark = x->props.output_mark;
+
err = x->outer_mode->output(x, skb);
if (err) {
XFRM_INC_STATS(net, LINUX_MIB_XFRMOUTSTATEMODEERROR);
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 8da428f56a..cc0d783ccb 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -122,7 +122,7 @@ static const struct xfrm_policy_afinfo *xfrm_policy_get_afinfo(unsigned short fa
struct dst_entry *__xfrm_dst_lookup(struct net *net, int tos, int oif,
const xfrm_address_t *saddr,
const xfrm_address_t *daddr,
- int family)
+ int family, u32 mark)
{
const struct xfrm_policy_afinfo *afinfo;
struct dst_entry *dst;
@@ -131,7 +131,7 @@ struct dst_entry *__xfrm_dst_lookup(struct net *net, int tos, int oif,
if (unlikely(afinfo == NULL))
return ERR_PTR(-EAFNOSUPPORT);
- dst = afinfo->dst_lookup(net, tos, oif, saddr, daddr);
+ dst = afinfo->dst_lookup(net, tos, oif, saddr, daddr, mark);
rcu_read_unlock();
@@ -143,7 +143,7 @@ static inline struct dst_entry *xfrm_dst_lookup(struct xfrm_state *x,
int tos, int oif,
xfrm_address_t *prev_saddr,
xfrm_address_t *prev_daddr,
- int family)
+ int family, u32 mark)
{
struct net *net = xs_net(x);
xfrm_address_t *saddr = &x->props.saddr;
@@ -159,7 +159,7 @@ static inline struct dst_entry *xfrm_dst_lookup(struct xfrm_state *x,
daddr = x->coaddr;
}
- dst = __xfrm_dst_lookup(net, tos, oif, saddr, daddr, family);
+ dst = __xfrm_dst_lookup(net, tos, oif, saddr, daddr, family, mark);
if (!IS_ERR(dst)) {
if (prev_saddr != saddr)
@@ -1340,14 +1340,14 @@ int __xfrm_sk_clone_policy(struct sock *sk, const struct sock *osk)
static int
xfrm_get_saddr(struct net *net, int oif, xfrm_address_t *local,
- xfrm_address_t *remote, unsigned short family)
+ xfrm_address_t *remote, unsigned short family, u32 mark)
{
int err;
const struct xfrm_policy_afinfo *afinfo = xfrm_policy_get_afinfo(family);
if (unlikely(afinfo == NULL))
return -EINVAL;
- err = afinfo->get_saddr(net, oif, local, remote);
+ err = afinfo->get_saddr(net, oif, local, remote, mark);
rcu_read_unlock();
return err;
}
@@ -1378,7 +1378,7 @@ xfrm_tmpl_resolve_one(struct xfrm_policy *policy, const struct flowi *fl,
if (xfrm_addr_any(local, tmpl->encap_family)) {
error = xfrm_get_saddr(net, fl->flowi_oif,
&tmp, remote,
- tmpl->encap_family);
+ tmpl->encap_family, 0);
if (error)
goto fail;
local = &tmp;
@@ -1598,7 +1598,8 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
if (xfrm[i]->props.mode != XFRM_MODE_TRANSPORT) {
family = xfrm[i]->props.family;
dst = xfrm_dst_lookup(xfrm[i], tos, fl->flowi_oif,
- &saddr, &daddr, family);
+ &saddr, &daddr, family,
+ xfrm[i]->props.output_mark);
err = PTR_ERR(dst);
if (IS_ERR(dst))
goto put_states;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 1b539b7dcf..15dd0023ed 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -584,6 +584,9 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
xfrm_mark_get(attrs, &x->mark);
+ if (attrs[XFRMA_OUTPUT_MARK])
+ x->props.output_mark = nla_get_u32(attrs[XFRMA_OUTPUT_MARK]);
+
err = __xfrm_init_state(x, false);
if (err)
goto error;
@@ -899,6 +902,11 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
goto out;
if (x->security)
ret = copy_sec_ctx(x->security, skb);
+ if (x->props.output_mark) {
+ ret = nla_put_u32(skb, XFRMA_OUTPUT_MARK, x->props.output_mark);
+ if (ret)
+ goto out;
+ }
out:
return ret;
}
@@ -2454,6 +2462,7 @@ static const struct nla_policy xfrma_policy[XFRMA_MAX+1] = {
[XFRMA_PROTO] = { .type = NLA_U8 },
[XFRMA_ADDRESS_FILTER] = { .len = sizeof(struct xfrm_address_filter) },
[XFRMA_OFFLOAD_DEV] = { .len = sizeof(struct xfrm_user_offload) },
+ [XFRMA_OUTPUT_MARK] = { .len = NLA_U32 },
};
static const struct nla_policy xfrma_spd_policy[XFRMA_SPD_MAX+1] = {
@@ -2673,6 +2682,8 @@ static inline size_t xfrm_sa_len(struct xfrm_state *x)
l += nla_total_size(sizeof(x->props.extra_flags));
if (x->xso.dev)
l += nla_total_size(sizeof(x->xso));
+ if (x->props.output_mark)
+ l += nla_total_size(sizeof(x->props.output_mark));
/* Must count x->lastused as it may become non-zero behind our back. */
l += nla_total_size_64bit(sizeof(u64));
--
2.14.0.434.g98096fd7a8-goog
^ permalink raw reply related
* Re: [RFC net-next] net: xfrm: support setting an output mark.
From: Lorenzo Colitti @ 2017-08-09 17:13 UTC (permalink / raw)
To: Steffen Klassert
Cc: netdev@vger.kernel.org, David Miller, Jamal Hadi Salim,
Herbert Xu, Nathan Harold, Jonathan Basseri
In-Reply-To: <20170808075143.GR2631@secunet.com>
On Tue, Aug 8, 2017 at 4:51 PM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> I thought you can just split the 32 bit mark into two 16 bit marks
> by setting an appropriate mask at the xfrm and the routing mark.
> But this has the drawback that the socket needs to know how possibly
> tunneled packets should be routed.
Right. And if those bits are already used for something else (e.g.,
Android uses something like 20 bits for marks) then that's not
possible.
Also - the other approach of using the SA mark for routing the
tunneled packet, that has backwards compatibility issues. If someone
is using mark-based routing, and has configured an SA with a mark,
then making the mark influence the routing lookup would change how
those tunnels are routed and possibly break them.
> So we transform the packet and may 'transform' the mark on the packet
> too. This could make sense, but we have to point out the differences
> between the xfrm_mark and the output_mark on the SA very explicit.
Ack. Where should this be pointed out? I've sent out a non-RFC version
to netdev, mostly unchanged but including a fair bit more rationale in
the commit message:
https://patchwork.ozlabs.org/patch/799891/
Or did you mean it should be be documented this in the ip-xfrm man page, or...?
^ permalink raw reply
* Re: [PATCH V4 net 0/2] ipv6: fix flowlabel issue for reset packet
From: David Miller @ 2017-08-09 17:55 UTC (permalink / raw)
To: shli; +Cc: netdev, shli
In-Reply-To: <20170809145953.vhkajzzy7kanggum@kernel.org>
From: Shaohua Li <shli@kernel.org>
Date: Wed, 9 Aug 2017 07:59:53 -0700
> Could you please look at the patches?
If you actually looked in patchwork, the state of your patches is
"changes requested".
This means that you were given feedback asking to change something
about your patches, which means that a new submission is expected.
I think Cong Wang's function movement request is legitimate, so
please do it.
^ permalink raw reply
* Re: [PATCH v9 1/4] PCI: Add new PCIe Fabric End Node flag, PCI_DEV_FLAGS_NO_RELAXED_ORDERING
From: Raj, Ashok @ 2017-08-09 18:00 UTC (permalink / raw)
To: Casey Leedom
Cc: Bjorn Helgaas, Ding Tianhong, bhelgaas@google.com, Michael Werner,
Ganesh GR, asit.k.mallick@intel.com, patrick.j.cramer@intel.com,
Suravee.Suthikulpanit@amd.com, Bob.Shaw@amd.com,
l.stach@pengutronix.de, amira@mellanox.com,
gabriele.paoloni@huawei.com, David.Laight@aculab.com,
jeffrey.t.kirsher@intel.com, catalin.marinas@arm.com,
will.deacon@arm.com
In-Reply-To: <MWHPR12MB1600E9D94E74E1DECC192097C88B0@MWHPR12MB1600.namprd12.prod.outlook.com>
On Wed, Aug 09, 2017 at 04:46:07PM +0000, Casey Leedom wrote:
> | From: Raj, Ashok <ashok.raj@intel.com>
> | Sent: Wednesday, August 9, 2017 8:58 AM
> | ...
> | As Casey pointed out in an earlier thread, we choose the heavy hammer
> | approach because there are some that can lead to data-corruption as opposed
> | to perf degradation.
>
> Careful. As far as I'm aware, there is no Data Corruption problem
> whatsoever with Intel Root Ports and processing of Transaction Layer Packets
> with and without the Relaxed Ordering Attribute set.
That's right.. no data-corruption on Intel parts :-).. It was with
other vendor. Only performance issue with intel root-ports in the parts
identified by the optimization guide.
Cheers,
AShok
^ permalink raw reply
* Re: [PATCH net-next 1/9] bpf: add BPF_J{LT,LE,SLT,SLE} instructions
From: David Miller @ 2017-08-09 18:01 UTC (permalink / raw)
To: daniel; +Cc: ast, holzheu, naveen.n.rao, jakub.kicinski, netdev
In-Reply-To: <598B3FCA.7060904@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 09 Aug 2017 19:00:58 +0200
> On 08/09/2017 06:55 PM, David Miller wrote:
>> From: Daniel Borkmann <daniel@iogearbox.net>
>> Date: Wed, 9 Aug 2017 12:23:53 +0200
>>
>>> [1] https://github.com/borkmann/llvm/tree/bpf-insns
>>
>> How is this "backwards compatible"?
>>
>> If someone takes a new LLVM and tries to load those programs
>> into an older kernel they will be rejected.
>>
>> There appears to be no effort to make things work cleanly in
>> that situation at all.
>
> No, that was just the patch I used for LLVM to enable the
> insns, so not the final one that will be submitted there
> officially where we have a switch to enable/disable this
> functionality.
So how does this switch work and how are people expected to use this
switch? What is the default value and is it ever expected to change
in the future?
Yeah this seems hella awesome to get cilium programs smaller and
faster in a restricted environment where you control the running
kernel and everything.
But from a review perspective one should be completely uninterested in
that.
So, generally speaking how does this all pan out? I seriously doubt
the wisdom of this. You'll have to convince me of the long term
viability and whether we ever get to the point where everyone can
benefit from this. :)
^ permalink raw reply
* Re: [PATCH net-next 1/1] driver: pptp: Remove unnecessary statements in pptp_sock_destruct
From: Cong Wang @ 2017-08-09 18:08 UTC (permalink / raw)
To: Gao Feng; +Cc: xeb, David Miller, Linux Kernel Network Developers
In-Reply-To: <1502294276-20051-1-git-send-email-gfree.wind@vip.163.com>
On Wed, Aug 9, 2017 at 8:57 AM, <gfree.wind@vip.163.com> wrote:
> From: Gao Feng <gfree.wind@vip.163.com>
>
> In the commit ddab82821fa6 ("ppp: Fix a scheduling-while-atomic bug in
> del_chan"), I moved the synchronize_rcu() from del_chan() to pptp_release
> after del_chan() to avoid one scheduling-while-atomic bug.
>
> Actually the del_chan() and pppox_unbind_sock are unneccessary in the
> pptp_sock_destruct. Because the pptp sock refcnt wouldn't reach zero until
> sk_state is set as PPPOX_DEAD in pptp_release. By that time, the del_chan()
> and pppox_unbind_sock() have been invoked already and the condition check
> "!(sk->sk_state & PPPOX_DEAD)" of this sock must be false in pptp_sock_destruct.
I am not sure. The check for sock->sk in the beginning of pptp_release()
indicates there could be a case we could skip del_chan() in pptp_release(),
although I can't figure out how.
Also there is a suspicious sock_put() in pptp_release().
^ permalink raw reply
* Re: [ovs-dev] [PATCH net-next] openvswitch: add NSH support
From: Ben Pfaff @ 2017-08-09 18:09 UTC (permalink / raw)
To: Yang, Yi Y
Cc: Jan Scheurich, dev@openvswitch.org, netdev@vger.kernel.org,
Jiri Benc, davem@davemloft.net, Zoltán Balogh
In-Reply-To: <79BBBFE6CB6C9B488C1A45ACD284F51961C3ADBE@SHSMSX103.ccr.corp.intel.com>
On Wed, Aug 09, 2017 at 09:41:51AM +0000, Yang, Yi Y wrote:
> Hi, Jan
>
> I have worked out a patch, will send it quickly for Ben. In addition, I also will send out a patch to change encap_nsh &decap_nsh to push_nsh and pop_nsh. Per comments from all the people, we all agreed to do so :-)
>
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index bc6c94b..4936c12 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -793,7 +793,7 @@ struct ovs_action_push_eth {
> struct ovs_key_ethernet addresses;
> };
>
> -#define OVS_ENCAP_NSH_MAX_MD_LEN 16
> +#define OVS_ENCAP_NSH_MAX_MD_LEN 248
> /*
> * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH
> * @flags: NSH header flags.
> @@ -809,7 +809,7 @@ struct ovs_action_encap_nsh {
> uint8_t mdlen;
> uint8_t np;
> __be32 path_hdr;
> - uint8_t metadata[OVS_ENCAP_NSH_MAX_MD_LEN];
> + uint8_t metadata[];
> };
This brings the overall format of ovs_action_encap_nsh to:
struct ovs_action_encap_nsh {
uint8_t flags;
uint8_t mdtype;
uint8_t mdlen;
uint8_t np;
__be32 path_hdr;
uint8_t metadata[];
};
This is an unusual format for a Netlink attribute. More commonly, one
would put variable-length data into an attribute of its own, which
allows that data to be handled using the regular Netlink means. Then
the mdlen and metadata members could be removed, since they would be
part of the additional attribute, and one might expect the mdtype member
to be removed as well since each type of metadata would be in a
different attribute type.
So, a format closer to what I expect to see in Netlink is something like
this:
/**
* enum ovs_nsh_attr - Metadata attributes for %OVS_ACTION_ENCAP_NSH action.
*
* @OVS_NSH_ATTR_MD1: Contains 16-byte NSH type-1 metadata.
* @OVS_NSH_ATTR_MD2: Contains 0- to 255-byte variable-length NSH type-2
* metadata. */
enum ovs_nsh_attr {
OVS_NSH_ATTR_MD1,
OVS_NSH_ATTR_MD2
};
/*
* struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH
*
* @path_hdr: NSH service path id and service index.
* @flags: NSH header flags.
* @np: NSH next_protocol: Inner packet type.
*
* Followed by either %OVS_NSH_ATTR_MD1 or %OVS_NSH_ATTR_MD2 attribute.
*/
struct ovs_action_encap_nsh {
__be32 path_hdr;
uint8_t flags;
uint8_t np;
};
^ permalink raw reply
* Re: Re: Re:Re: Re: [PATCH net] ppp: Fix a scheduling-while-atomic bug in del_chan
From: Cong Wang @ 2017-08-09 18:18 UTC (permalink / raw)
To: Gao Feng; +Cc: xeb, David Miller, Linux Kernel Network Developers
In-Reply-To: <73e6ac77.45ea.15dc569d56a.Coremail.gfree.wind@vip.163.com>
On Tue, Aug 8, 2017 at 10:13 PM, Gao Feng <gfree.wind@vip.163.com> wrote:
> Maybe I didn't show my explanation clearly.
> I think it won't happen as I mentioned in the last email.
> Because the pptp_release invokes the synchronize_rcu to make sure it, and actually there is no one which would invoke del_chan except pptp_release.
> It is guaranteed by that the pptp_release doesn't put the sock refcnt until complete all cleanup include marking sk_state as PPPOX_DEAD.
>
> In other words, even though the pptp_release is not the last user of this sock, the other one wouldn't invoke del_chan in pptp_sock_destruct.
> Because the condition "!(sk->sk_state & PPPOX_DEAD)" must be false.
Only if sock->sk is always non-NULL for pptp_release(), which
is what I am not sure. If you look at other ->release(), similar checks
are there too, so not just for pptp.
>
> As summary, the del_chan and pppox_unbind_sock in pptp_sock_destruct are unnecessary.
> And it even brings confusing.
Sorry, I can't draw any conclusion for this.
^ permalink raw reply
* Re: [PATCH net-next] net: skb_needs_check() removes CHECKSUM_NONE check for tx.
From: Willem de Bruijn @ 2017-08-09 18:30 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: Network Development, Eric Dumazet, Willem de Bruijn
In-Reply-To: <1502280278-9970-1-git-send-email-xiangxia.m.yue@gmail.com>
On Wed, Aug 9, 2017 at 5:04 AM, Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> This patch reverts the commit 6e7bc478c9a0
> ("net: skb_needs_check() accepts CHECKSUM_NONE for tx"),
> because we removed the UFO support.
>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
I would wait until net is merged into net-next. This will cause a conflict.
Also, while logically equivalent, it is not a real revert (as in `git
revert $SHA1`) of that patch.
Aside from those concerns, I agree that the original patch is no
longer needed now that UFO is reverted.
^ permalink raw reply
* [PATCH v2 net-next 0/7] rtnetlink: allow selected handlers to run without rtnl
From: Florian Westphal @ 2017-08-09 18:41 UTC (permalink / raw)
To: netdev
Changes since v1:
In patch 6, don't make ipv6 route handlers lockless, they all have
assumptions on rtnl being held. Other patches are unchanged.
The RTNL mutex is used to serialize both rtnetlink calls and
dump requests.
Its also used to protect other things such as the list of current
net namespaces.
Unfortunately RTNL mutex is a performance issue, e.g. a cpu adding an
ip address prevents other cpus from seemingly unrelated tasks such as
dumping tc classifiers or doing rtnetlink route lookups.
This patch set adds basic infrastructure to start pushing the rtnl lock
down to those places that need it, or even elide it entirely in some cases.
Subsystems can now indicate that their doit() callback can run without
RTNL mutex, such callbacks can then run in parallel.
This will obviously need a lot of followup work; all current
users need to be audited/changed to benefit from this.
Initial no-rtnl spot is netns new/getid.
We have various 'get' handlers that are also a tempting target,
however, several of these depend on rtnl mutex to prevent information
from changing while objects are being read by rtnl handlers; however,
it doesn't appear impossible to change this.
Dumps are another problem entirely, see
commit 2907c35ff64708065 ("net: hold rtnl again in dump callbacks"),
this patchset doesn't touch dump requests.
^ permalink raw reply
* [PATCH v2 net-next 1/7] rtnetlink: call rtnl_calcit directly
From: Florian Westphal @ 2017-08-09 18:41 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
In-Reply-To: <20170809184153.16700-1-fw@strlen.de>
There is only a single place in the kernel that regisers the "calcit"
callback (to determine min allocation for dumps).
This is in rtnetlink.c for PF_UNSPEC RTM_GETLINK.
The function that checks for calcit presence at run time will first check
the requested family (which will always fail for !PF_UNSPEC as no callsite
registers this), then falls back to checking PF_UNSPEC.
Therefore we can just check if type is RTM_GETLINK and then do a direct
call. Because of fallback to PF_UNSPEC all RTM_GETLINK types used this
regardless of family.
This has the advantage that we don't need to allocate space for
the function pointer for all the other families.
A followup patch will drop the calcit function pointer from the
rtnl_link callback structure.
Signed-off-by: Florian Westphal <fw@strlen.de>
Reviewed-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
No changes since v1.
net/core/rtnetlink.c | 29 ++++-------------------------
1 file changed, 4 insertions(+), 25 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 9201e3621351..8c9d34deea7d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -62,7 +62,6 @@
struct rtnl_link {
rtnl_doit_func doit;
rtnl_dumpit_func dumpit;
- rtnl_calcit_func calcit;
};
static DEFINE_MUTEX(rtnl_mutex);
@@ -173,21 +172,6 @@ static rtnl_dumpit_func rtnl_get_dumpit(int protocol, int msgindex)
return tab[msgindex].dumpit;
}
-static rtnl_calcit_func rtnl_get_calcit(int protocol, int msgindex)
-{
- struct rtnl_link *tab;
-
- if (protocol <= RTNL_FAMILY_MAX)
- tab = rtnl_msg_handlers[protocol];
- else
- tab = NULL;
-
- if (tab == NULL || tab[msgindex].calcit == NULL)
- tab = rtnl_msg_handlers[PF_UNSPEC];
-
- return tab[msgindex].calcit;
-}
-
/**
* __rtnl_register - Register a rtnetlink message type
* @protocol: Protocol family or PF_UNSPEC
@@ -231,9 +215,6 @@ int __rtnl_register(int protocol, int msgtype,
if (dumpit)
tab[msgindex].dumpit = dumpit;
- if (calcit)
- tab[msgindex].calcit = calcit;
-
return 0;
}
EXPORT_SYMBOL_GPL(__rtnl_register);
@@ -277,7 +258,6 @@ int rtnl_unregister(int protocol, int msgtype)
rtnl_msg_handlers[protocol][msgindex].doit = NULL;
rtnl_msg_handlers[protocol][msgindex].dumpit = NULL;
- rtnl_msg_handlers[protocol][msgindex].calcit = NULL;
return 0;
}
@@ -4187,15 +4167,14 @@ static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
if (kind == 2 && nlh->nlmsg_flags&NLM_F_DUMP) {
struct sock *rtnl;
rtnl_dumpit_func dumpit;
- rtnl_calcit_func calcit;
u16 min_dump_alloc = 0;
dumpit = rtnl_get_dumpit(family, type);
if (dumpit == NULL)
return -EOPNOTSUPP;
- calcit = rtnl_get_calcit(family, type);
- if (calcit)
- min_dump_alloc = calcit(skb, nlh);
+
+ if (type == RTM_GETLINK)
+ min_dump_alloc = rtnl_calcit(skb, nlh);
__rtnl_unlock();
rtnl = net->rtnl;
@@ -4300,7 +4279,7 @@ void __init rtnetlink_init(void)
register_netdevice_notifier(&rtnetlink_dev_notifier);
rtnl_register(PF_UNSPEC, RTM_GETLINK, rtnl_getlink,
- rtnl_dump_ifinfo, rtnl_calcit);
+ rtnl_dump_ifinfo, NULL);
rtnl_register(PF_UNSPEC, RTM_SETLINK, rtnl_setlink, NULL, NULL);
rtnl_register(PF_UNSPEC, RTM_NEWLINK, rtnl_newlink, NULL, NULL);
rtnl_register(PF_UNSPEC, RTM_DELLINK, rtnl_dellink, NULL, NULL);
--
2.13.0
^ 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