* Re: [bpf PATCH 1/2] bpf: sockmap, error path can not release psock in multi-map case
From: Daniel Borkmann @ 2018-07-03 14:40 UTC (permalink / raw)
To: John Fastabend, ast; +Cc: netdev, kafai
In-Reply-To: <20180630135131.6395.86522.stgit@john-Precision-Tower-5810>
On 06/30/2018 03:51 PM, John Fastabend wrote:
> The current code, in the error path of sock_hash_ctx_update_elem,
> checks if the sock has a psock in the user data and if so decrements
> the reference count of the psock. However, if the error happens early
> in the error path we may have never incremented the psock reference
> count and if the psock exists because the sock is in another map then
> we may inadvertently decrement the reference count.
>
> Fix this by making the error path only call smap_release_sock if the
> error happens after the increment.
>
> Reported-by: syzbot+d464d2c20c717ef5a6a8@syzkaller.appspotmail.com
> Fixes: 81110384441a ("bpf: sockmap, add hash map support")
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> ---
> 0 files changed
>
> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
> index 4fc2cb1..63fb047 100644
> --- a/kernel/bpf/sockmap.c
> +++ b/kernel/bpf/sockmap.c
> @@ -1896,7 +1896,7 @@ static int __sock_map_ctx_update_elem(struct bpf_map *map,
> e = kzalloc(sizeof(*e), GFP_ATOMIC | __GFP_NOWARN);
> if (!e) {
> err = -ENOMEM;
> - goto out_progs;
> + goto out_free;
> }
> }
>
> @@ -2324,7 +2324,12 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
> if (err)
> goto err;
>
> - /* bpf_map_update_elem() can be called in_irq() */
> + psock = smap_psock_sk(sock);
> + if (unlikely(!psock)) {
> + err = -EINVAL;
> + goto err;
> + }
Is an error even possible at this point? If __sock_map_ctx_update_elem() succeeds,
we either allocated and linked a new psock to the sock or we inc'ed the existing
one's refcount. From my reading it seems we should always succeed the subsequent
smap_psock_sk(). If we would have failed here in between it would mean we'd have
a refcount imbalance somewhere?
> +
> raw_spin_lock_bh(&b->lock);
> l_old = lookup_elem_raw(head, hash, key, key_size);
> if (l_old && map_flags == BPF_NOEXIST) {
> @@ -2342,12 +2347,6 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
> goto bucket_err;
> }
>
> - psock = smap_psock_sk(sock);
> - if (unlikely(!psock)) {
> - err = -EINVAL;
> - goto bucket_err;
> - }
> -
> rcu_assign_pointer(e->hash_link, l_new);
> rcu_assign_pointer(e->htab,
> container_of(map, struct bpf_htab, map));
> @@ -2370,12 +2369,10 @@ static int sock_hash_ctx_update_elem(struct bpf_sock_ops_kern *skops,
> raw_spin_unlock_bh(&b->lock);
> return 0;
> bucket_err:
> + smap_release_sock(psock, sock);
> raw_spin_unlock_bh(&b->lock);
> err:
> kfree(e);
> - psock = smap_psock_sk(sock);
> - if (psock)
> - smap_release_sock(psock, sock);
> return err;
> }
>
>
Thanks,
Daniel
^ permalink raw reply
* Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301
From: Mark Brown @ 2018-07-03 14:50 UTC (permalink / raw)
To: Andreas Färber
Cc: netdev, linux-arm-kernel, linux-kernel, Jian-Hong Pan, Jiri Pirko,
Marcel Holtmann, David S . Miller, Matthias Brugger, Janus Piwek,
Michael Röder, Dollar Chen, Ken Yu, Ben Whitten,
Steve deRosier, linux-spi, LoRa_Community_Support
In-Reply-To: <4e06cc72-2092-70f3-c801-bf6e4c3cbec2@suse.de>
[-- Attachment #1: Type: text/plain, Size: 3135 bytes --]
On Mon, Jul 02, 2018 at 07:34:21PM +0200, Andreas Färber wrote:
> Hi Mark,
>
> This driver is still evolving, there's newer code on my lora-next branch
> already: https://github.com/afaerber/linux/commits/lora-next
Please don't top post, reply in line with needed context. This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.
> 2) This SPI device is in turn exposing the two SPI masters that you
> already found below, and I didn't see a sane way to split that code out
> into drivers/spi/, so it's in drivers/net/lora/ here - has there been
> any precedence either way?
A MFD?
> Am 02.07.2018 um 18:12 schrieb Mark Brown:
> > On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote:
> >> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable)
> >> +{
> >> + int ret;
> >> +
> >> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0");
> >> +
> >> + if (enable)
> >> + return;
> > So we never disable chip select?
> Not here, I instead did that in transfer_one below.
That's obviously at best going to be fragile, you're implementing half
the operation here and half somewhere else which is most likely going to
break at some point when the framework changes.
> >> + if (ret) {
> >> + dev_err(&spi->dev, "SPI radio address write failed\n");
> >> + return ret;
> >> + }
> >> +
> >> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
> >> + if (ret) {
> >> + dev_err(&spi->dev, "SPI radio data write failed\n");
> >> + return ret;
> >> + }
> > This looks awfully like you're coming in at the wrong abstraction layer
> > and the hardware actually implements a register abstraction rather than
> > a SPI one so you should be using regmap as the abstraction.
> I don't understand. Ben has suggested using regmap for the SPI _device_
> that we're talking to, which may be a good idea. But this SX1301 device
> in turn has two SPI _masters_ talking to an SX125x slave each. I don't
> see how using regmap instead of my wrappers avoids this spi_controller?
It seems obvious from the code that this isn't actually interacting with
a SPI controller, you're writing an address and a value to a register
map rather than dealing with a byte stream. There may be a SPI bus
somewhere behind some other hardware but you don't seem to be
interacting with it as such.
> The whole point of this spi_controller is to abstract and separate the
> SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver,
> instead of mixing it into the SX1301 driver - to me that looks cleaner
> and more extensible. It also has the side-effect that we could configure
> the two radios via DT (frequencies, clk output, etc.).
A register map would work just as well here, we already have plenty of
devices that abstract at this level (most obviously the I2C/SPI devices
that use it to offer both interfaces with a single core driver).
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* CHQ on 16-June-18
From: claudia.andrade @ 2018-07-03 11:18 UTC (permalink / raw)
[-- Attachment #1: Type: text/plain, Size: 104 bytes --]
Dear sir,
Please kindly approve as the bellow list and the attached file for
reference.
Regards
[-- Attachment #2: CHQ on 16-June-18.zip --]
[-- Type: application/zip, Size: 232461 bytes --]
^ permalink raw reply
* [PATCH v2] net: usb: asix: allow optionally getting mac address from device tree
From: Marcel Ziswiler @ 2018-07-03 15:06 UTC (permalink / raw)
To: netdev
Cc: Oliver Neukum, Marcel Ziswiler, linux-usb, David S. Miller,
Dean Jenkins, linux-kernel, Andrey Konovalov
From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
For Embedded use where e.g. AX88772B chips may be used without external
EEPROMs the boot loader may choose to pass the MAC address to be used
via device tree. Therefore, allow for optionally getting the MAC
address from device tree data e.g. as follows (excerpt from a T30 based
board, local-mac-address to be filled in by boot loader):
/* EHCI instance 1: USB2_DP/N -> AX88772B */
usb@7d004000 {
status = "okay";
#address-cells = <1>;
#size-cells = <0>;
asix@1 {
reg = <1>;
local-mac-address = [00 00 00 00 00 00];
};
};
Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---
Changes in v2:
- Use eth_platform_get_mac_address() as suggested by Andrew.
drivers/net/usb/asix_devices.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/drivers/net/usb/asix_devices.c b/drivers/net/usb/asix_devices.c
index 3d4f7959dabb..8f41c6bda8e5 100644
--- a/drivers/net/usb/asix_devices.c
+++ b/drivers/net/usb/asix_devices.c
@@ -691,24 +691,32 @@ static int ax88772_bind(struct usbnet *dev, struct usb_interface *intf)
u32 phyid;
struct asix_common_private *priv;
- usbnet_get_endpoints(dev,intf);
+ usbnet_get_endpoints(dev, intf);
- /* Get the MAC address */
- if (dev->driver_info->data & FLAG_EEPROM_MAC) {
- for (i = 0; i < (ETH_ALEN >> 1); i++) {
- ret = asix_read_cmd(dev, AX_CMD_READ_EEPROM, 0x04 + i,
- 0, 2, buf + i * 2, 0);
- if (ret < 0)
- break;
- }
+ /* Maybe the boot loader passed the MAC address via device tree */
+ if (!eth_platform_get_mac_address(&dev->udev->dev, buf)) {
+ netif_dbg(dev, ifup, dev->net,
+ "MAC address read from device tree");
} else {
- ret = asix_read_cmd(dev, AX_CMD_READ_NODE_ID,
- 0, 0, ETH_ALEN, buf, 0);
- }
+ /* Try getting the MAC address from EEPROM */
+ if (dev->driver_info->data & FLAG_EEPROM_MAC) {
+ for (i = 0; i < (ETH_ALEN >> 1); i++) {
+ ret = asix_read_cmd(dev, AX_CMD_READ_EEPROM,
+ 0x04 + i, 0, 2, buf + i * 2,
+ 0);
+ if (ret < 0)
+ break;
+ }
+ } else {
+ ret = asix_read_cmd(dev, AX_CMD_READ_NODE_ID,
+ 0, 0, ETH_ALEN, buf, 0);
+ }
- if (ret < 0) {
- netdev_dbg(dev->net, "Failed to read MAC address: %d\n", ret);
- return ret;
+ if (ret < 0) {
+ netdev_dbg(dev->net, "Failed to read MAC address: %d\n",
+ ret);
+ return ret;
+ }
}
asix_set_netdev_dev_addr(dev, buf);
--
2.14.4
^ permalink raw reply related
* Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301
From: Andreas Färber @ 2018-07-03 15:09 UTC (permalink / raw)
To: Mark Brown
Cc: Steve deRosier, Matthias Brugger, Jiri Pirko, Ben Whitten, netdev,
Marcel Holtmann, Dollar Chen, linux-kernel, Michael Röder,
Janus Piwek, linux-spi, LoRa_Community_Support, Jian-Hong Pan,
Ken Yu, David S . Miller, linux-arm-kernel
In-Reply-To: <20180703145022.GA13744@sirena.org.uk>
Am 03.07.2018 um 16:50 schrieb Mark Brown:
> On Mon, Jul 02, 2018 at 07:34:21PM +0200, Andreas Färber wrote:
>> Hi Mark,
>>
>> This driver is still evolving, there's newer code on my lora-next branch
>> already: https://github.com/afaerber/linux/commits/lora-next
>
> Please don't top post, reply in line with needed context. This allows
> readers to readily follow the flow of conversation and understand what
> you are talking about and also helps ensure that everything in the
> discussion is being addressed.
I did reply inline, if you cared to read on. ;)
>> 2) This SPI device is in turn exposing the two SPI masters that you
>> already found below, and I didn't see a sane way to split that code out
>> into drivers/spi/, so it's in drivers/net/lora/ here - has there been
>> any precedence either way?
>
> A MFD?
I know of mfd, but how would the the the net vs. spi pieces interact
then? Some functions would need to be exported then or is there an
easier way without needing to set a cross-module API in stone?
>> Am 02.07.2018 um 18:12 schrieb Mark Brown:
>>> On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote:
>>>> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0");
>>>> +
>>>> + if (enable)
>>>> + return;
>
>>> So we never disable chip select?
>
>> Not here, I instead did that in transfer_one below.
>
> That's obviously at best going to be fragile, you're implementing half
> the operation here and half somewhere else which is most likely going to
> break at some point when the framework changes.
>
>>>> + if (ret) {
>>>> + dev_err(&spi->dev, "SPI radio address write failed\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0);
>>>> + if (ret) {
>>>> + dev_err(&spi->dev, "SPI radio data write failed\n");
>>>> + return ret;
>>>> + }
>
>>> This looks awfully like you're coming in at the wrong abstraction layer
>>> and the hardware actually implements a register abstraction rather than
>>> a SPI one so you should be using regmap as the abstraction.
>
>> I don't understand. Ben has suggested using regmap for the SPI _device_
>> that we're talking to, which may be a good idea. But this SX1301 device
>> in turn has two SPI _masters_ talking to an SX125x slave each. I don't
>> see how using regmap instead of my wrappers avoids this spi_controller?
>
> It seems obvious from the code that this isn't actually interacting with
> a SPI controller, you're writing an address and a value to a register
> map rather than dealing with a byte stream. There may be a SPI bus
> somewhere behind some other hardware but you don't seem to be
> interacting with it as such.
>
>> The whole point of this spi_controller is to abstract and separate the
>> SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver,
>> instead of mixing it into the SX1301 driver - to me that looks cleaner
>> and more extensible. It also has the side-effect that we could configure
>> the two radios via DT (frequencies, clk output, etc.).
>
> A register map would work just as well here, we already have plenty of
> devices that abstract at this level (most obviously the I2C/SPI devices
> that use it to offer both interfaces with a single core driver).
The address and data registers together form a two-byte SPI message!
It is transmitted by writing to the CS register.
The received data is afterwards available in another register.
HTE,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] tcp: fix high tail latencies in DCTCP
From: Lawrence Brakmo @ 2018-07-03 15:10 UTC (permalink / raw)
To: Neal Cardwell
Cc: Netdev, Kernel Team, Blake Matheny, Alexei Starovoitov,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <CADVnQy=0QXUnPAEr26xpCsEgP5bbT1tvjEqQovaCYtxB_9+QyQ@mail.gmail.com>
On 7/2/18, 5:52 PM, "netdev-owner@vger.kernel.org on behalf of Neal Cardwell" <netdev-owner@vger.kernel.org on behalf of ncardwell@google.com> wrote:
On Mon, Jul 2, 2018 at 5:39 PM Lawrence Brakmo <brakmo@fb.com> wrote:
>
> When have observed high tail latencies when using DCTCP for RPCs as
> compared to using Cubic. For example, in one setup there are 2 hosts
> sending to a 3rd one, with each sender having 3 flows (1 stream,
> 1 1MB back-to-back RPCs and 1 10KB back-to-back RPCs). The following
> table shows the 99% and 99.9% latencies for both Cubic and dctcp:
>
> Cubic 99% Cubic 99.9% dctcp 99% dctcp 99.9%
> 1MB RPCs 2.6ms 5.5ms 43ms 208ms
> 10KB RPCs 1.1ms 1.3ms 53ms 212ms
>
> Looking at tcpdump traces showed that there are two causes for the
> latency.
>
> 1) RTOs caused by the receiver sending a dup ACK and not ACKing
> the last (and only) packet sent.
> 2) Delaying ACKs when the sender has a cwnd of 1, so everything
> pauses for the duration of the delayed ACK.
>
> The first patch fixes the cause of the dup ACKs, not updating DCTCP
> state when an ACK that was initially delayed has been sent with a
> data packet.
>
> The second patch insures that an ACK is sent immediately when a
> CWR marked packet arrives.
>
> With the patches the latencies for DCTCP now look like:
>
> dctcp 99% dctcp 99.9%
> 1MB RPCs 5.8ms 6.9ms
> 10KB RPCs 146us 203us
>
> Note that while the 1MB RPCs tail latencies are higher than Cubic's,
> the 10KB latencies are much smaller than Cubic's. These patches fix
> issues on the receiver, but tcpdump traces indicate there is an
> opportunity to also fix an issue at the sender that adds about 3ms
> to the tail latencies.
>
> The following trace shows the issue that tiggers an RTO (fixed by these patches):
>
> Host A sends the last packets of the request
> Host B receives them, and the last packet is marked with congestion (CE)
> Host B sends ACKs for packets not marked with congestion
> Host B sends data packet with reply and ACK for packet marked with
> congestion (TCP flag ECE)
> Host A receives ACKs with no ECE flag
> Host A receives data packet with ACK for the last packet of request
> and which has TCP ECE bit set
> Host A sends 1st data packet of the next request with TCP flag CWR
> Host B receives the packet (as seen in tcpdump at B), no CE flag
> Host B sends a dup ACK that also has the TCP ECE flag
> Host A RTO timer fires!
> Host A to send the next packet
> Host A receives an ACK for everything it has sent (i.e. Host B
> did receive 1st packet of request)
> Host A send more packets…
>
> [PATCH net-next v2 1/2] tcp: notify when a delayed ack is sent
> [PATCH net-next v2 2/2] tcp: ack immediately when a cwr packet
>
> net/ipv4/tcp_input.c | 16 +++++++++++-----
> net/ipv4/tcp_output.c | 4 ++--
> 2 files changed, 13 insertions(+), 7 deletions(-)
Thanks, Larry. Just for context, can you please let us know whether
your tests included zero, one, or both of Eric's recent commits
(listed below) that tuned the number of ACKs after ECN events? (Or
maybe the tests were literally using a net-next kernel?) Just wanted
to get a better handle on any possible interactions there.
Thanks!
neal
Yes, my test kernel includes both patches listed below. BTW, I will send a new patch where I move the call to tcp_incr_quickack from tcp_ecn_check_ce to tcp_ecn_accept_cwr.
---
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=522040ea5fdd1c33bbf75e1d7c7c0422b96a94ef
commit 522040ea5fdd1c33bbf75e1d7c7c0422b96a94ef
Author: Eric Dumazet <edumazet@google.com>
Date: Mon May 21 15:08:57 2018 -0700
tcp: do not aggressively quick ack after ECN events
ECN signals currently forces TCP to enter quickack mode for
up to 16 (TCP_MAX_QUICKACKS) following incoming packets.
We believe this is not needed, and only sending one immediate ack
for the current packet should be enough.
This should reduce the extra load noticed in DCTCP environments,
after congestion events.
This is part 2 of our effort to reduce pure ACK packets.
Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=15ecbe94a45ef88491ca459b26efdd02f91edb6d
commit 15ecbe94a45ef88491ca459b26efdd02f91edb6d
Author: Eric Dumazet <edumazet@google.com>
Date: Wed Jun 27 08:47:21 2018 -0700
tcp: add one more quick ack after after ECN events
Larry Brakmo proposal ( https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_935233_&d=DwIFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=pq_Mqvzfy-C8ltkgyx1u_g&m=-aaZOqM6jDpeeyDiKlVoiaQRFc55aJgk4jHQILSj3D4&s=tFz97077b4KhTvYd69-n-YhnWhX6PWZQceWJiylvL5Q&e=
tcp: force cwnd at least 2 in tcp_cwnd_reduction) made us rethink
about our recent patch removing ~16 quick acks after ECN events.
tcp_enter_quickack_mode(sk, 1) makes sure one immediate ack is sent,
but in the case the sender cwnd was lowered to 1, we do not want
to have a delayed ack for the next packet we will receive.
Fixes: 522040ea5fdd ("tcp: do not aggressively quick ack after ECN events")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Neal Cardwell <ncardwell@google.com>
Cc: Lawrence Brakmo <brakmo@fb.com>
Acked-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [RFC net-next 00/15] net: A socket API for LoRa
From: Jian-Hong Pan @ 2018-07-03 15:11 UTC (permalink / raw)
To: Andreas Färber
Cc: Michal Kubeček, Konstantin Böhm, contact, Ken Yu,
Michael Röder, Rob Herring, lora, Alexander Graf,
Jan Jongboom, Janus Piwek, Jon Ortego, devicetree, Jiri Pirko,
Marcel Holtmann, Mark Brown, Dollar Chen, Brian Ray,
linux-arm-kernel, lora, Matthias Brugger, Ben Whitten, netdev,
linux-kernel, linux-spi, Steve deRosier
In-Reply-To: <20180701110804.32415-1-afaerber@suse.de>
Hi Andreas,
First, thanks for your great jobs. LoRaWAN module needs the
compatible devices drivers.
I will reply the message under the LoRaWAN specification.
2018-07-01 19:07 GMT+08:00 Andreas Färber <afaerber@suse.de>:
> Hello,
>
> LoRa is a long-range, low-power wireless technology by Semtech.
> Unlike other LPWAN technologies, users don't need to rely on infrastructure
> providers and SIM cards and expensive subscription plans, they can set up
> their own gateways. Modules, adapters and evaluation boards are available
> from a large number of vendors.
>
> Many vendors also make available Open Source software examples on GitHub.
> But when taking a closer look, many of them combine licenses in ways that are
> not redistributable. My reports have remained without response or solution.
>
> https://github.com/ernstdevreede/lmic_pi/issues/2
> https://github.com/Snootlab/lmic_chisterapi/issues/2
> https://github.com/Snootlab/lora_chisterapi/issues/2
>
> Another issue was that most such projects around the Raspberry Pi make use of
> spidev to communicate with the Semtech chipsets from userspace. The Linux spi
> maintainers have chosen to greet any such users of spidev with a friendly
> WARN_ON(), preferring in-kernel spi drivers and white-listing individual
> devices only.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spidev.c?h=v4.17#n722
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/spi/spidev.c?h=v4.17#n667
>
> Also I don't quite see the point in having userspace probe what SPI devices
> are connected to a generic spidev driver when we have an easy Device Tree
> hardware description on arm/arm64 that could give us that info.
>
> I raised the topic during Q&A of a FOSDEM 2017 talk (cut off at the end
> of the video) but unfortunately found no one to collaborate on this.
>
> https://archive.fosdem.org/2017/schedule/event/lorawan/
>
> Instead of porting from wiringPi to a differently licensed GPIO library
> and dealing with seemingly unmaintained LoRaWAN code dumps, I started a
> spi kernel driver for SX1276 back in 2016. But obviously a kernel driver
> isn't too helpful without a userspace API to send and receive packets.
>
> This patchset, updated from 2017 and extended, is implementing kernel drivers
> for various LoRa chipsets and modules. As API I'm proposing a PF_LORA socket
> implementation. Why? LoRa uses data packets with headers and checksums
> and differing MTUs and multiple protocols layered on top of it. Apart from
> simple headers for addressing used by RadioHead library and IMST's LoRa P2P
> protocol, the main use case (not implemented in this patchset) is expected
> to be LoRaWAN. And LoRaWAN has competing proprietary protocols, such as
> Link Labs' Symphony Link or GlobalSat M.O.S.T. or RadioShuttle, that might
> at some point want to adopt a standard API for their implementations, too.
>
> Ready-made LoRa hardware modules come in three flavors,
> a) with SPI access to the underlying Semtech chipsets, needing a software
> implementation of e.g. LoRaWAN protocol stack (i.e., a soft MAC),
> b) with a custom, often UART based interface and a pre-certified LoRaWAN
> protocol stack already integrated (i.e., hard/full MAC), and
> c) with a microcontroller that serves not only for the protocol stack but
> also as application processor, not offering a ready-made interface.
>
> This patchset focuses on option a). An SX1276 based LoRaWAN stack appeared
> to be the project of Jian-Hong Pan and is not included here.
Thanks, LoRaWAN module needs the compatible device drivers.
> This patchset also includes drivers for b), from text based AT commands to
> a binary SLIP based HCI protocol.
> Hardware examples for c) are Murata CMWX1ZZABZ-078 and RAK813.
>
> This patchset is clearly not ready for merging, but is being submitted for
> discussion, as requested by Jiri, in particular of the design choices:
>
> 1) PF_LORA/AF_LORA and associated identifiers are proposed to represent
> this technology. While for an SX1276 - case a) above - it might work to
> layer LoRaWAN as a protocol option for PF_LORA and add LoRaWAN address
> fields to the union in my sockaddr_lora, how would that work for devices
> that only support LoRaWAN but not pure LoRa? Do we need both AF_LORA and
> AF_LORAWAN, or just a separate ETH_P_LORAWAN or ARPHRD_LORAWAN?
The LoRaWAN module also register the LoRa device as a network device.
Will use the macros AF_LORAWAN, PF_LORAWAN, ARPHRD_LORAWAN and ETH_P_LORAWAN.
> 2) PF_LORA is used with SOCK_DGRAM here. The assumption is that RAW mode
> would be DGRAM plus preamble plus optional checksum.
Is the preamble added by the hardware itself here or software here?
> 3) Only the transmit path is partially implemented already. The assumption
> is that the devices should go into receive mode by default and only
> interrupt that when asked to transmit.
If it goes with LoRaWAN spec., end devices should be in idle (or
called standby mode) by default. Unless the device is asked to be in
transmitting or receiving timing slot.
> 4) Some hardware settings need to be supplied externally, such as the radio
> frequency for some modules, but many others can be runtime-configured,
> such as Spreading Factor, Bandwidth, Sync Word, or which antenna to use.
> What settings should be implemented as socket option vs. netlink layer
> vs. ioctl vs. sysfs? What are the criteria to apply?
- We can have a pre-defined table according to LoRaWAN Regional Parameters.
- Device driver declares the hardware's capability, for example
frequency, TX power. And then registers as a LoRaWAN compatible
device.
- LoRaWAN module handle the requests from upper layer or MAC commands
from a gateway (netlink layer), than uses the pre-defined interface
functions to set the parameters.
LoRaWAN module will export the operation functions interface.
> 5) Many of the modules support multiple modes, such as LoRa, LoRaWAN and FSK.
> Lacking a LoRaWAN implementation, I am currently switching them into LoRa
> mode at probe time wherever possible. How do we deal with that properly?
- There are data rate tables defined in LoRaWAN Regional Parameters.
Those contain which data rate uses LoRa mode or FSK mode.
- LoRaWAN should handle the data rate changing requests and then calls
the corresponding operation functions. Of course, the hardware's
capability registered before should also be under consideration at the
same time.
> a) Is there any precedence from the Wifi world for dynamically selecting
> between our own trusted Open Source implementation vs. hardware/firmware
> accelerated and/or certified implementations?
>
> b) Would a proof of concept for FSK (non-LoRa) modes be required for
> merging any LoRa driver for chipsets that support both? Or is there any
> facility or design guidelines that would allow us to focus on LoRa and
> LoRaWAN and leave non-LoRa radio modes to later contributors?
>
> As evident by the many questions, this is my first deep dive into the Linux
> net subsystem. It's also my first experiments with the new serdev subsystem,
> so in particular the receive paths will need some review and optimizations.
>
> This patchset was developed and tested mainly as KMP, originally at
> https://github.com/afaerber/lora-modules. It was recently transformed into a
> linux-next based tree, still mostly tested on our openSUSE Tumbleweed kernel
> with a differing AF_LORA value below current AF_MAX limit.
>
> Some corresponding Device Tree Overlays have been collected here:
> https://github.com/afaerber/dt-overlays
>
> Only European models for 868 MHz and 433 MHz could be tested when available.
I can test 922-928 MHz in Taiwan.
But I need a workable LoRaWAN gateway first! XD
Cheers,
Jian-Hong Pan
> Thanks to all companies and people that have supported this project so far.
>
> Have a lot of fun!
>
> Cheers,
> Andreas
>
> Cc: Jian-Hong Pan <starnight@g.ncu.edu.tw>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Matthias Brugger <mbrugger@suse.com>
> Cc: Konstantin Böhm <konstantin.boehm@ancud.de>
> Cc: Jan Jongboom <jan.jongboom@arm.com>
> Cc: Janus Piwek <jpiwek@arroweurope.com>
> Cc: Michael Röder <michael.roeder@avnet.eu>
> Cc: Dollar Chen (陳義元) <dollar.chen@wtmec.com>
> Cc: Ken Yu (禹凯) <ken.yu@rakwireless.com>
> Cc: Jon Ortego <Jon.Ortego@imst.de>
> Cc: contact@snootlab.com
> Cc: Ben Whitten <ben.whitten@lairdtech.com>
> Cc: Brian Ray <brian.ray@link-labs.com>
> Cc: lora@globalsat.com.tw
> Cc: lora@radioshuttle.de
> Cc: Alexander Graf <agraf@suse.de>
> Cc: Michal Kubeček <mkubecek@suse.cz>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Steve deRosier <derosier@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-spi@vger.kernel.org
>
> Andreas Färber (15):
> net: Reserve protocol numbers for LoRa
> net: lora: Define sockaddr_lora
> net: lora: Add protocol numbers
> net: Add lora subsystem
> HACK: net: lora: Deal with .poll_mask in 4.18-rc2
> net: lora: Prepare for device drivers
> net: lora: Add Semtech SX1276
> net: lora: sx1276: Add debugfs
> net: lora: Prepare EUI helpers
> net: lora: Add Microchip RN2483
> net: lora: Add IMST WiMOD
> net: lora: Add USI WM-SG-SM-42
> net: lora: Prepare RAK RAK811
> net: lora: Prepare Semtech SX1257
> net: lora: Add Semtech SX1301
>
> drivers/net/Makefile | 1 +
> drivers/net/lora/Kconfig | 72 ++++
> drivers/net/lora/Makefile | 32 ++
> drivers/net/lora/dev.c | 125 ++++++
> drivers/net/lora/rak811.c | 219 +++++++++++
> drivers/net/lora/rn2483.c | 344 +++++++++++++++++
> drivers/net/lora/rn2483.h | 40 ++
> drivers/net/lora/rn2483_cmd.c | 130 +++++++
> drivers/net/lora/sx1257.c | 96 +++++
> drivers/net/lora/sx1276.c | 740 ++++++++++++++++++++++++++++++++++++
> drivers/net/lora/sx1301.c | 446 ++++++++++++++++++++++
> drivers/net/lora/usi.c | 411 ++++++++++++++++++++
> drivers/net/lora/wimod.c | 597 +++++++++++++++++++++++++++++
> include/linux/lora/dev.h | 44 +++
> include/linux/lora/skb.h | 29 ++
> include/linux/socket.h | 4 +-
> include/uapi/linux/if_arp.h | 1 +
> include/uapi/linux/if_ether.h | 1 +
> include/uapi/linux/lora.h | 24 ++
> net/Kconfig | 1 +
> net/Makefile | 1 +
> net/lora/Kconfig | 15 +
> net/lora/Makefile | 8 +
> net/lora/af_lora.c | 152 ++++++++
> net/lora/af_lora.h | 13 +
> net/lora/dgram.c | 297 +++++++++++++++
> security/selinux/hooks.c | 4 +-
> security/selinux/include/classmap.h | 4 +-
> 28 files changed, 3848 insertions(+), 3 deletions(-)
> create mode 100644 drivers/net/lora/Kconfig
> create mode 100644 drivers/net/lora/Makefile
> create mode 100644 drivers/net/lora/dev.c
> create mode 100644 drivers/net/lora/rak811.c
> create mode 100644 drivers/net/lora/rn2483.c
> create mode 100644 drivers/net/lora/rn2483.h
> create mode 100644 drivers/net/lora/rn2483_cmd.c
> create mode 100644 drivers/net/lora/sx1257.c
> create mode 100644 drivers/net/lora/sx1276.c
> create mode 100644 drivers/net/lora/sx1301.c
> create mode 100644 drivers/net/lora/usi.c
> create mode 100644 drivers/net/lora/wimod.c
> create mode 100644 include/linux/lora/dev.h
> create mode 100644 include/linux/lora/skb.h
> create mode 100644 include/uapi/linux/lora.h
> create mode 100644 net/lora/Kconfig
> create mode 100644 net/lora/Makefile
> create mode 100644 net/lora/af_lora.c
> create mode 100644 net/lora/af_lora.h
> create mode 100644 net/lora/dgram.c
>
> --
> 2.16.4
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] tcp: notify when a delayed ack is sent
From: Lawrence Brakmo @ 2018-07-03 15:15 UTC (permalink / raw)
To: Yuchung Cheng
Cc: netdev, Kernel Team, Blake Matheny, Alexei Starovoitov,
Neal Cardwell, Steve Ibanez, Eric Dumazet
In-Reply-To: <CAK6E8=f2n9PKwne5COvT0sFyFymfJDDeE9Vxk6PeTpOEwg7z8w@mail.gmail.com>
On 7/2/18, 4:50 PM, "Yuchung Cheng" <ycheng@google.com> wrote:
On Mon, Jul 2, 2018 at 2:39 PM, Lawrence Brakmo <brakmo@fb.com> wrote:
>
> DCTCP depends on the CA_EVENT_NON_DELAYED_ACK and CA_EVENT_DELAYED_ACK
> notifications to keep track if it needs to send an ACK for packets that
> were received with a particular ECN state but whose ACK was delayed.
>
> Under some circumstances, for example when a delayed ACK is sent with a
> data packet, DCTCP state was not being updated due to a lack of
> notification that the previously delayed ACK was sent. As a result, it
> would sometimes send a duplicate ACK when a new data packet arrived.
>
> This patch insures that DCTCP's state is correctly updated so it will
> not send the duplicate ACK.
Sorry to chime-in late here (lame excuse: IETF deadline)
IIRC this issue would exist prior to 4.11 kernel. While it'd be good
to fix that, it's not clear which patch introduced the regression
between 4.11 and 4.16? I assume you tested Eric's most recent quickack
fix.
In terms of the fix itself, it seems odd the tcp_send_ack() call in
DCTCP generates NON_DELAYED_ACK event to toggle DCTCP's
delayed_ack_reserved bit. Shouldn't the fix to have DCTCP send the
"prior" ACK w/o cancelling delayed-ACK and mis-recording that it's
cancelled, because that prior-ACK really is a supplementary old ACK.
But it's still unclear how this bug introduces the regression 4.11 - 4.16
Feedback is always appreciated! This issue is also present in 4.11 (that is where I discovered). I think the bug was introduces much earlier.
Yes, I tested with Eric's quickack fix, it did not fix either of the two issues that are fixed with this patch set.
As I mentioned earlier, the bug was introduced before 4.11. I am not sure I understand your comments. Yes, at some level it would make sense to change the delayed_ack_reserved bit directly, but we would still need to do it whenever we send the ACK, so I do not think it can be helped. Please clarify if I misunderstood your comment.
>
> Improved based on comments from Neal Cardwell <ncardwell@google.com>.
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> ---
> net/ipv4/tcp_output.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index f8f6129160dd..acefb64e8280 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -172,6 +172,8 @@ static inline void tcp_event_ack_sent(struct sock *sk, unsigned int pkts)
> __sock_put(sk);
> }
> tcp_dec_quickack_mode(sk, pkts);
> + if (inet_csk_ack_scheduled(sk))
> + tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
> inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
> }
>
> @@ -3567,8 +3569,6 @@ void tcp_send_ack(struct sock *sk)
> if (sk->sk_state == TCP_CLOSE)
> return;
>
> - tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
> -
> /* We are not putting this on the write queue, so
> * tcp_transmit_skb() will set the ownership to this
> * sock.
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] tcp: fix high tail latencies in DCTCP
From: Neal Cardwell @ 2018-07-03 15:25 UTC (permalink / raw)
To: Lawrence Brakmo
Cc: Netdev, Kernel Team, bmatheny, ast, Yuchung Cheng, Steve Ibanez,
Eric Dumazet
In-Reply-To: <DAC7EDC0-D829-4107-AE04-4850118F571D@fb.com>
On Tue, Jul 3, 2018 at 11:10 AM Lawrence Brakmo <brakmo@fb.com> wrote:
>
> On 7/2/18, 5:52 PM, "netdev-owner@vger.kernel.org on behalf of Neal Cardwell" <netdev-owner@vger.kernel.org on behalf of ncardwell@google.com> wrote:
>
> On Mon, Jul 2, 2018 at 5:39 PM Lawrence Brakmo <brakmo@fb.com> wrote:
> >
> > When have observed high tail latencies when using DCTCP for RPCs as
> > compared to using Cubic. For example, in one setup there are 2 hosts
> > sending to a 3rd one, with each sender having 3 flows (1 stream,
> > 1 1MB back-to-back RPCs and 1 10KB back-to-back RPCs). The following
> > table shows the 99% and 99.9% latencies for both Cubic and dctcp:
> >
> > Cubic 99% Cubic 99.9% dctcp 99% dctcp 99.9%
> > 1MB RPCs 2.6ms 5.5ms 43ms 208ms
> > 10KB RPCs 1.1ms 1.3ms 53ms 212ms
> >
> > Looking at tcpdump traces showed that there are two causes for the
> > latency.
> >
> > 1) RTOs caused by the receiver sending a dup ACK and not ACKing
> > the last (and only) packet sent.
> > 2) Delaying ACKs when the sender has a cwnd of 1, so everything
> > pauses for the duration of the delayed ACK.
> >
> > The first patch fixes the cause of the dup ACKs, not updating DCTCP
> > state when an ACK that was initially delayed has been sent with a
> > data packet.
> >
> > The second patch insures that an ACK is sent immediately when a
> > CWR marked packet arrives.
> >
> > With the patches the latencies for DCTCP now look like:
> >
> > dctcp 99% dctcp 99.9%
> > 1MB RPCs 5.8ms 6.9ms
> > 10KB RPCs 146us 203us
> >
> > Note that while the 1MB RPCs tail latencies are higher than Cubic's,
> > the 10KB latencies are much smaller than Cubic's. These patches fix
> > issues on the receiver, but tcpdump traces indicate there is an
> > opportunity to also fix an issue at the sender that adds about 3ms
> > to the tail latencies.
> >
> > The following trace shows the issue that tiggers an RTO (fixed by these patches):
> >
> > Host A sends the last packets of the request
> > Host B receives them, and the last packet is marked with congestion (CE)
> > Host B sends ACKs for packets not marked with congestion
> > Host B sends data packet with reply and ACK for packet marked with
> > congestion (TCP flag ECE)
> > Host A receives ACKs with no ECE flag
> > Host A receives data packet with ACK for the last packet of request
> > and which has TCP ECE bit set
> > Host A sends 1st data packet of the next request with TCP flag CWR
> > Host B receives the packet (as seen in tcpdump at B), no CE flag
> > Host B sends a dup ACK that also has the TCP ECE flag
> > Host A RTO timer fires!
> > Host A to send the next packet
> > Host A receives an ACK for everything it has sent (i.e. Host B
> > did receive 1st packet of request)
> > Host A send more packets…
> >
> > [PATCH net-next v2 1/2] tcp: notify when a delayed ack is sent
> > [PATCH net-next v2 2/2] tcp: ack immediately when a cwr packet
> >
> > net/ipv4/tcp_input.c | 16 +++++++++++-----
> > net/ipv4/tcp_output.c | 4 ++--
> > 2 files changed, 13 insertions(+), 7 deletions(-)
>
> Thanks, Larry. Just for context, can you please let us know whether
> your tests included zero, one, or both of Eric's recent commits
> (listed below) that tuned the number of ACKs after ECN events? (Or
> maybe the tests were literally using a net-next kernel?) Just wanted
> to get a better handle on any possible interactions there.
>
> Thanks!
>
> neal
>
> Yes, my test kernel includes both patches listed below.
OK, great.
> BTW, I will send a new
> patch where I move the call to tcp_incr_quickack from tcp_ecn_check_ce to tcp_ecn_accept_cwr.
OK, sounds good to me.
thanks,
neal
^ permalink raw reply
* Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301
From: Mark Brown @ 2018-07-03 15:31 UTC (permalink / raw)
To: Andreas Färber
Cc: Steve deRosier, Matthias Brugger, Jiri Pirko, Ben Whitten, netdev,
Marcel Holtmann, Dollar Chen, linux-kernel, Michael Röder,
Janus Piwek, linux-spi, LoRa_Community_Support, Jian-Hong Pan,
Ken Yu, David S . Miller, linux-arm-kernel
In-Reply-To: <809fc546-6012-37f3-9485-cfd8eaf99488@suse.de>
[-- Attachment #1.1: Type: text/plain, Size: 2006 bytes --]
On Tue, Jul 03, 2018 at 05:09:38PM +0200, Andreas Färber wrote:
> Am 03.07.2018 um 16:50 schrieb Mark Brown:
> >> 2) This SPI device is in turn exposing the two SPI masters that you
> >> already found below, and I didn't see a sane way to split that code out
> >> into drivers/spi/, so it's in drivers/net/lora/ here - has there been
> >> any precedence either way?
> > A MFD?
> I know of mfd, but how would the the the net vs. spi pieces interact
> then? Some functions would need to be exported then or is there an
> easier way without needing to set a cross-module API in stone?
It's an in-kernel ABI it's not exactly set in stone but yeah, you'll
need some interface. A lot of devices work by having the children know
that they're part of a MFD and fish things out of the parent device,
either the pdata or (in the common case where the MFD bit mostly just
instantiates subdevices and holds a regmap) with dev_get_regmap().
> > A register map would work just as well here, we already have plenty of
> > devices that abstract at this level (most obviously the I2C/SPI devices
> > that use it to offer both interfaces with a single core driver).
> The address and data registers together form a two-byte SPI message!
> It is transmitted by writing to the CS register.
> The received data is afterwards available in another register.
Right, but it seems from the code that the hardware understands that
it's formatting register I/O and not just shifting in and out a byte
stream which is what a SPI controller does. I'd not be surprised to
learn that the register you're calling a chip select register is a
strobe that initiates the transfer (and that this may be some of the
difficulty you're having with handling it in the way the framework
expects), the pattern with writing 1 followed immediately by 0 is a bit
of a flag here.
I've seen such before hardware where I know it was intentionally
designed that way so it wouldn't be totally surprising.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: recvmsg bug not clear
From: Randy Dunlap @ 2018-07-03 16:18 UTC (permalink / raw)
To: 積丹尼 Dan Jacobson, linux-kernel,
netdev@vger.kernel.org, Eric Dumazet
In-Reply-To: <87601whg6r.fsf@jidanni.org>
[adding netdev]
On 07/03/18 01:33, 積丹尼 Dan Jacobson wrote:
> Most users won't even know what string from this to google on to find
> out what this is all about:
>
>
> Jul 03 13:57:20 jidanni7 kernel: ------------[ cut here ]------------
> Jul 03 13:57:20 jidanni7 kernel: recvmsg bug 2: copied 73BCB6CD seq 70F17CBE rcvnxt 73BCB9AA fl 0
> Jul 03 13:57:20 jidanni7 kernel: WARNING: CPU: 2 PID: 1501 at /build/linux-uwVqDp/linux-4.16.16/net/ipv4/tcp.c:1881 tcp_recvmsg+0x649/0xb90
> Jul 03 13:57:20 jidanni7 kernel: Modules linked in: nft_r...
>
>
> So please make clearer messages.
That's a message from net/ipv4/tcp.c
--
~Randy
^ permalink raw reply
* [PATCH net-next v3 2/2] tcp: ack immediately when a cwr packet arrives
From: Lawrence Brakmo @ 2018-07-03 16:26 UTC (permalink / raw)
To: netdev
Cc: Kernel Team, Blake Matheny, Alexei Starovoitov, Neal Cardwell,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <20180703162615.314231-1-brakmo@fb.com>
We observed high 99 and 99.9% latencies when doing RPCs with DCTCP. The
problem is triggered when the last packet of a request arrives CE
marked. The reply will carry the ECE mark causing TCP to shrink its cwnd
to 1 (because there are no packets in flight). When the 1st packet of
the next request arrives, the ACK was sometimes delayed even though it
is CWR marked, adding up to 40ms to the RPC latency.
This patch insures that CWR makred data packets arriving will be acked
immediately.
Modified based on comments by Neal Cardwell <ncardwell@google.com>
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
net/ipv4/tcp_input.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2c5d70bc294e..23c2a43de8a4 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -246,8 +246,15 @@ static void tcp_ecn_queue_cwr(struct tcp_sock *tp)
static void tcp_ecn_accept_cwr(struct tcp_sock *tp, const struct sk_buff *skb)
{
- if (tcp_hdr(skb)->cwr)
+ if (tcp_hdr(skb)->cwr) {
tp->ecn_flags &= ~TCP_ECN_DEMAND_CWR;
+
+ /* If the sender is telling us it has entered CWR, then its
+ * cwnd may be very low (even just 1 packet), so we should ACK
+ * immediately.
+ */
+ tcp_enter_quickack_mode((struct sock *)tp, 2);
+ }
}
static void tcp_ecn_withdraw_cwr(struct tcp_sock *tp)
--
2.17.1
^ permalink raw reply related
* [PATCH net-next v3 0/2] tcp: fix high tail latencies in DCTCP
From: Lawrence Brakmo @ 2018-07-03 16:26 UTC (permalink / raw)
To: netdev
Cc: Kernel Team, Blake Matheny, Alexei Starovoitov, Neal Cardwell,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
When have observed high tail latencies when using DCTCP for RPCs as
compared to using Cubic. For example, in one setup there are 2 hosts
sending to a 3rd one, with each sender having 3 flows (1 stream,
1 1MB back-to-back RPCs and 1 10KB back-to-back RPCs). The following
table shows the 99% and 99.9% latencies for both Cubic and dctcp:
Cubic 99% Cubic 99.9% dctcp 99% dctcp 99.9%
1MB RPCs 2.6ms 5.5ms 43ms 208ms
10KB RPCs 1.1ms 1.3ms 53ms 212ms
Looking at tcpdump traces showed that there are two causes for the
latency.
1) RTOs caused by the receiver sending a dup ACK and not ACKing
the last (and only) packet sent.
2) Delaying ACKs when the sender has a cwnd of 1, so everything
pauses for the duration of the delayed ACK.
The first patch fixes the cause of the dup ACKs, not updating DCTCP
state when an ACK that was initially delayed has been sent with a
data packet.
The second patch insures that an ACK is sent immediately when a
CWR marked packet arrives.
With the patches the latencies for DCTCP now look like:
dctcp 99% dctcp 99.9%
1MB RPCs 5.8ms 6.9ms
10KB RPCs 146us 203us
Note that while the 1MB RPCs tail latencies are higher than Cubic's,
the 10KB latencies are much smaller than Cubic's. These patches fix
issues on the receiver, but tcpdump traces indicate there is an
opportunity to also fix an issue at the sender that adds about 3ms
to the tail latencies.
The following trace shows the issue that tiggers an RTO (fixed by these patches):
Host A sends the last packets of the request
Host B receives them, and the last packet is marked with congestion (CE)
Host B sends ACKs for packets not marked with congestion
Host B sends data packet with reply and ACK for packet marked with
congestion (TCP flag ECE)
Host A receives ACKs with no ECE flag
Host A receives data packet with ACK for the last packet of request
and which has TCP ECE bit set
Host A sends 1st data packet of the next request with TCP flag CWR
Host B receives the packet (as seen in tcpdump at B), no CE flag
Host B sends a dup ACK that also has the TCP ECE flag
Host A RTO timer fires!
Host A to send the next packet
Host A receives an ACK for everything it has sent (i.e. Host B
did receive 1st packet of request)
Host A send more packets…
v2: Removed call to tcp_ca_event from tcp_send_ack since I added one in
tcp_event_ack_sent. Based on Neal Cardwell <ncardwell@google.com>
feedback.
Modified tcp_ecn_check_ce (and renamed it tcp_ecn_check) instead of modifying
tcp_ack_send_check to insure an ACK when cwr is received.
v3: Handling cwr in tcp_ecn_accept_cwr instead of in tcp_ecn_check.
[PATCH net-next v3 1/2] tcp: notify when a delayed ack is sent
[PATCH net-next v3 2/2] tcp: ack immediately when a cwr packet
net/ipv4/tcp_input.c | 9 ++++++++-
net/ipv4/tcp_output.c | 4 ++--
2 files changed, 10 insertions(+), 3 deletions(-)
^ permalink raw reply
* [PATCH net-next v3 1/2] tcp: notify when a delayed ack is sent
From: Lawrence Brakmo @ 2018-07-03 16:26 UTC (permalink / raw)
To: netdev
Cc: Kernel Team, Blake Matheny, Alexei Starovoitov, Neal Cardwell,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <20180703162615.314231-1-brakmo@fb.com>
DCTCP depends on the CA_EVENT_NON_DELAYED_ACK and CA_EVENT_DELAYED_ACK
notifications to keep track if it needs to send an ACK for packets that
were received with a particular ECN state but whose ACK was delayed.
Under some circumstances, for example when a delayed ACK is sent with a
data packet, DCTCP state was not being updated due to a lack of
notification that the previously delayed ACK was sent. As a result, it
would sometimes send a duplicate ACK when a new data packet arrived.
This patch insures that DCTCP's state is correctly updated so it will
not send the duplicate ACK.
Improved based on comments from Neal Cardwell <ncardwell@google.com>.
Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
---
net/ipv4/tcp_output.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f8f6129160dd..acefb64e8280 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -172,6 +172,8 @@ static inline void tcp_event_ack_sent(struct sock *sk, unsigned int pkts)
__sock_put(sk);
}
tcp_dec_quickack_mode(sk, pkts);
+ if (inet_csk_ack_scheduled(sk))
+ tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
inet_csk_clear_xmit_timer(sk, ICSK_TIME_DACK);
}
@@ -3567,8 +3569,6 @@ void tcp_send_ack(struct sock *sk)
if (sk->sk_state == TCP_CLOSE)
return;
- tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
-
/* We are not putting this on the write queue, so
* tcp_transmit_skb() will set the ownership to this
* sock.
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v4 08/18] net: davinci_emac: potentially get the MAC address from MTD
From: Florian Fainelli @ 2018-07-03 16:39 UTC (permalink / raw)
To: Bartosz Golaszewski, Sekhar Nori, Kevin Hilman, Russell King,
Grygorii Strashko, David S . Miller, Srinivas Kandagatla,
Lukas Wunner, Rob Herring, Dan Carpenter, Ivan Khoronzhuk,
David Lechner, Greg Kroah-Hartman, Andrew Lunn, Jonathan Corbet
Cc: linux-arm-kernel, linux-kernel, linux-omap, netdev,
Bartosz Golaszewski
In-Reply-To: <20180629094039.7543-9-brgl@bgdev.pl>
On 06/29/2018 02:40 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> On da850-evm board we can read the MAC address from MTD. It's currently
> done in the relevant board file, but we want to get rid of all the MAC
> reading callbacks from the board file (SPI and NAND). Move the reading
> of the MAC address from SPI to the emac driver's probe function.
This should be made something generic to all drivers, not just something
the davinci_emac driver does, something like this actually:
https://lkml.org/lkml/2018/3/24/312
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/net/ethernet/ti/davinci_emac.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index a1a6445b5a7e..48e6a7755811 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -67,7 +67,7 @@
> #include <linux/of_irq.h>
> #include <linux/of_net.h>
> #include <linux/mfd/syscon.h>
> -
> +#include <linux/mtd/mtd.h>
> #include <asm/irq.h>
> #include <asm/page.h>
>
> @@ -1783,7 +1783,10 @@ static int davinci_emac_probe(struct platform_device *pdev)
> struct cpdma_params dma_params;
> struct clk *emac_clk;
> unsigned long emac_bus_frequency;
> -
> +#ifdef CONFIG_MTD
> + size_t mac_addr_len;
> + struct mtd_info *mtd;
> +#endif /* CONFIG_MTD */
>
> /* obtain emac clock from kernel */
> emac_clk = devm_clk_get(&pdev->dev, NULL);
> @@ -1815,6 +1818,19 @@ static int davinci_emac_probe(struct platform_device *pdev)
> goto err_free_netdev;
> }
>
> +#ifdef CONFIG_MTD
> + mtd = get_mtd_device_nm("MAC-Address");
> + if (!IS_ERR(mtd)) {
> + rc = mtd_read(mtd, 0, ETH_ALEN,
> + &mac_addr_len, priv->mac_addr);
> + if (rc == 0)
> + dev_info(&pdev->dev,
> + "Read MAC addr from SPI Flash: %pM\n",
> + priv->mac_addr);
> + put_mtd_device(mtd);
> + }
> +#endif /* CONFIG_MTD */
> +
> /* MAC addr and PHY mask , RMII enable info from platform_data */
> memcpy(priv->mac_addr, pdata->mac_addr, ETH_ALEN);
> priv->phy_id = pdata->phy_id;
>
--
Florian
^ permalink raw reply
* Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301
From: Andreas Färber @ 2018-07-03 16:40 UTC (permalink / raw)
To: Mark Brown
Cc: netdev, linux-arm-kernel, linux-kernel, Jian-Hong Pan, Jiri Pirko,
Marcel Holtmann, David S . Miller, Matthias Brugger, Janus Piwek,
Michael Röder, Dollar Chen, Ken Yu, Ben Whitten,
Steve deRosier, linux-spi, LoRa_Community_Support, Hasnain Virk
In-Reply-To: <20180703153137.GB13744@sirena.org.uk>
Am 03.07.2018 um 17:31 schrieb Mark Brown:
> On Tue, Jul 03, 2018 at 05:09:38PM +0200, Andreas Färber wrote:
>> Am 03.07.2018 um 16:50 schrieb Mark Brown:
>>> A register map would work just as well here, we already have plenty of
>>> devices that abstract at this level (most obviously the I2C/SPI devices
>>> that use it to offer both interfaces with a single core driver).
>
>> The address and data registers together form a two-byte SPI message!
>
>> It is transmitted by writing to the CS register.
>
>> The received data is afterwards available in another register.
>
> Right, but it seems from the code that the hardware understands that
> it's formatting register I/O and not just shifting in and out a byte
> stream which is what a SPI controller does. I'd not be surprised to
> learn that the register you're calling a chip select register is a
> strobe that initiates the transfer (and that this may be some of the
> difficulty you're having with handling it in the way the framework
> expects), the pattern with writing 1 followed immediately by 0 is a bit
> of a flag here.
Yeah, the current implementation assumes exactly that. :)
> I've seen such before hardware where I know it was intentionally
> designed that way so it wouldn't be totally surprising.
If we don't implement a spi_controller here, then IIUC we can't have
multiple spi_device implementations for the devices on the receiving
end, as they rely on a spi_controller for their APIs.
Do you have an alternative solution for abstraction? A regmap would seem
to require putting everything into a monolithic SX1301 driver despite
those connected chipsets actually being regular, external SPI chips that
could also be attached to non-SX1301 SPI masters.
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply
* Re: [PATCH net-next 01/10] r8169: add basic phylib support
From: Florian Fainelli @ 2018-07-03 16:42 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn
Cc: David Miller, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <238e0c62-7e95-df62-3837-5e8d1b7248f0@gmail.com>
On 07/02/2018 02:15 PM, Heiner Kallweit wrote:
> On 02.07.2018 23:02, Andrew Lunn wrote:
>>> +static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
>>> +{
>>> + struct rtl8169_private *tp = mii_bus->priv;
>>> +
>>> + return rtl_readphy(tp, phyreg);
>>
>> So there is no support for phyaddr?
>>
> Right, the chip can access only the one internal PHY, therefore it
> doesn't support phyaddr.
Then you might also want to set mii_bus->phy_mask accordingly such that
only the internal PHY address bit is cleared there?
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
From: Florian Fainelli @ 2018-07-03 16:44 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn
Cc: David Miller, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <8ef120d3-ac4a-7ffd-2104-76d6f1268105@gmail.com>
On 07/02/2018 02:24 PM, Heiner Kallweit wrote:
> On 02.07.2018 23:06, Andrew Lunn wrote:
>>> static void r8168_pll_power_down(struct rtl8169_private *tp)
>>> {
>>> if (r8168_check_dash(tp))
>>> @@ -4510,7 +4469,8 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
>>> if (rtl_wol_pll_power_down(tp))
>>> return;
>>>
>>> - r8168_phy_power_down(tp);
>>> + /* cover the case that PHY isn't connected */
>>> + phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
>>
>> This could do some more explanation. Why would it not be connected?
>>
> The PHY gets connected when the net_device is opened. If a network
> port isn't used then it will be runtime-suspended a few seconds after
> boot. In this case we call r8168_pll_power_down() with the PHY not
> being connected.
Would not the !netif_running() check in rtl8169_net_suspend() take care
of that though?
--
Florian
^ permalink raw reply
* Re: [PATCH v4 08/18] net: davinci_emac: potentially get the MAC address from MTD
From: David Lechner @ 2018-07-03 16:47 UTC (permalink / raw)
To: Florian Fainelli, Bartosz Golaszewski, Sekhar Nori, Kevin Hilman,
Russell King, Grygorii Strashko, David S . Miller,
Srinivas Kandagatla, Lukas Wunner, Rob Herring, Dan Carpenter,
Ivan Khoronzhuk, Greg Kroah-Hartman, Andrew Lunn, Jonathan Corbet
Cc: linux-arm-kernel, linux-kernel, linux-omap, netdev,
Bartosz Golaszewski
In-Reply-To: <03b77e24-9ab9-fa01-2387-9de0408a9942@gmail.com>
On 07/03/2018 11:39 AM, Florian Fainelli wrote:
>
>
> On 06/29/2018 02:40 AM, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>
>> On da850-evm board we can read the MAC address from MTD. It's currently
>> done in the relevant board file, but we want to get rid of all the MAC
>> reading callbacks from the board file (SPI and NAND). Move the reading
>> of the MAC address from SPI to the emac driver's probe function.
>
> This should be made something generic to all drivers, not just something
> the davinci_emac driver does, something like this actually:
>
> https://lkml.org/lkml/2018/3/24/312
>
I was thinking about suggesting adding a nvmem provider for MTD devices
as well. It would fix the kernel config dependency problems I was running
into since nvmem lookups can do -EPROBE_DEFER.
^ permalink raw reply
* Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
From: Florian Fainelli @ 2018-07-03 16:48 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn
Cc: David Miller, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <8d516fa8-30a5-df7e-737c-119d6e458d80@gmail.com>
On 07/02/2018 02:31 PM, Heiner Kallweit wrote:
> On 02.07.2018 23:20, Andrew Lunn wrote:
>> On Mon, Jul 02, 2018 at 09:37:08PM +0200, Heiner Kallweit wrote:
>>> Change rtl_speed_down() to use phylib.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
>>> 1 file changed, 15 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>>> index 311321ee..807fbc75 100644
>>> --- a/drivers/net/ethernet/realtek/r8169.c
>>> +++ b/drivers/net/ethernet/realtek/r8169.c
>>> @@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>>> rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
>>> }
>>>
>>> + /* We may have called rtl_speed_down before */
>>> + dev->phydev->advertising = dev->phydev->supported;
>>> + genphy_config_aneg(dev->phydev);
>>> +
>>> genphy_soft_reset(dev->phydev);
>>>
>>> rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>> @@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
>>> }
>>> }
>>>
>>> +#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
>>> +#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
>>> +#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
>>> +
>>> static void rtl_speed_down(struct rtl8169_private *tp)
>>> {
>>> - u32 adv;
>>> - int lpa;
>>> + struct phy_device *phydev = tp->dev->phydev;
>>> + u32 adv = phydev->lp_advertising & phydev->supported;
>>>
>>> - rtl_writephy(tp, 0x1f, 0x0000);
>>> - lpa = rtl_readphy(tp, MII_LPA);
>>> + if (adv & BASET10)
>>> + phydev->advertising &= ~(BASET100 | BASET1000);
>>> + else if (adv & BASET100)
>>> + phydev->advertising &= ~BASET1000;
>>>
>>> - if (lpa & (LPA_10HALF | LPA_10FULL))
>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
>>> - else if (lpa & (LPA_100HALF | LPA_100FULL))
>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
>>> - else
>>> - adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
>>> - ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
>>> - (tp->mii.supports_gmii ?
>>> - ADVERTISED_1000baseT_Half |
>>> - ADVERTISED_1000baseT_Full : 0);
>>> -
>>> - rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
>>> - adv);
>>> + genphy_config_aneg(phydev);
>>> }
>>
>> It probably it is me being too tired, but i don't get what this is
>> doing? Changing the local advertisement based on what the remote is
>> advertising. Why?
>>
> It also took me some time to understand what this speed_down is doing.
> If we suspend and wait for a WoL packet, then we don't have to burn all
> the energy for a GBit connection. Therefore we switch to the lowest
> speed supported by chip and link partner. This is done by removing
> higher speeds from the advertised modes and restarting an autonego.
This is something that the tg3 driver also does, we should probably
consider doing this as part of a generic PHY library helpers since I was
told by several HW engineers that usually 10Mbits for WoL is much more
energy efficient.
One thing that bothers me a bit is that this should ideally be offered
as both blocking and non-blocking options, because we might want to make
sure that at the time we suspend, and we already had a link established,
we successfully re-negotiate the link with the partner. I agree that
there could be any sort of link disruption happening at any point though..
--
Florian
^ permalink raw reply
* Re: [GIT] Networking
From: Linus Torvalds @ 2018-07-03 17:03 UTC (permalink / raw)
To: Ursula Braun
Cc: David Miller, Andrew Morton, Network Development,
Linux Kernel Mailing List
In-Reply-To: <5b5509f1-9713-f1bc-cb33-18ebdb1cb8c5@linux.ibm.com>
On Tue, Jul 3, 2018 at 6:52 AM Ursula Braun <ubraun@linux.ibm.com> wrote:
>
> Thanks Linus for your revert. I run a test, and found it is almost fine.
> Just these 2 lines are missing:
I should have realized that. I actually looked at it and decided your
other changes made that unnecessary, but that was a thinko on my part.
Fixed up. Thanks,
Linus
^ permalink raw reply
* Re: [RFC PATCH 00/11] OVS eBPF datapath.
From: Alexei Starovoitov @ 2018-07-03 17:56 UTC (permalink / raw)
To: William Tu
Cc: <dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>,
Tom Herbert via iovisor-dev, Linux Kernel Network Developers
In-Reply-To: <CALDO+SYzDDpTmJttghfjUYKbo3AHDaT4L154Acwn5BGqkytkHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Thu, Jun 28, 2018 at 07:19:35AM -0700, William Tu wrote:
> Hi Alexei,
>
> Thanks a lot for the feedback!
>
> On Wed, Jun 27, 2018 at 8:00 PM, Alexei Starovoitov
> <alexei.starovoitov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Sat, Jun 23, 2018 at 05:16:32AM -0700, William Tu wrote:
> >>
> >> Discussion
> >> ==========
> >> We are still actively working on finishing the feature, currently
> >> the basic forwarding and tunnel feature work, but still under
> >> heavy debugging and development. The purpose of this RFC is to
> >> get some early feedbacks and direction for finishing the complete
> >> features in existing kernel's OVS datapath (the net/openvswitch/*).
> >
> > Thank you for sharing the patches.
> >
> >> Three major issues we are worried:
> >> a. Megaflow support in BPF.
> >> b. Connection Tracking support in BPF.
> >
> > my opinion on the above two didn't change.
> > To recap:
> > A. Non scalable megaflow map is no go. I'd like to see packet classification
> > algorithm like hicuts or efficuts to be implemented instead, since it can be
> > shared by generic bpf, bpftiler, ovs and likely others.
>
> We did try the decision tree approach using dpdk's acl lib. The lookup
> speed is 6 times faster than the magaflow using tuple space.
> However, the update/insertion requires rebuilding/re-balancing the decision
> tree so it's way too slow. I think hicuts or efficuts suffers the same issue.
> So decision tree algos are scalable only for lookup operation due to its
> optimization over tree depth, but not scalable under
> update/insert/delete operations.
>
> On customer's system we see megaflow update/insert rate around 10 rules/sec,
> this makes decision tree unusable, unless we invent something to optimize the
> update/insert time or incremental update of these decision tree algo.
is this a typo? you probably meant 10K rule updates a second ?
Last time I've dealt with these algorithms we had 100K acl updates a second.
It was an important metric that we were optimizing for.
I'm pretty sure '*cuts' algos do many thousands per second non optimized.
> >> c. Verifier limitation.
> >
> > Not sure what limitations you're concerned about.
> >
>
> Mostly related to stack. The flow key OVS uses (struct sw_flow_key)
> is 464 byte. We trim a lot, now around 300 byte, but still huge, considering
> the BPF's stack limit is 512 byte.
have you tried using per-cpu array of one element with large value
instead of stack?
In the latest verifier most of the operations that can be done with the stack
pointer can be done with pointer to map value too.
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#1358): https://lists.iovisor.org/g/iovisor-dev/message/1358
Mute This Topic: https://lists.iovisor.org/mt/22656941/1132507
Group Owner: iovisor-dev+owner-9jONkmmOlFHEE9lA1F8Ukti2O/JbrIOy@public.gmane.org
Unsubscribe: https://lists.iovisor.org/g/iovisor-dev/unsub [glki-iovisor-dev@m.gmane.org]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] tcp: notify when a delayed ack is sent
From: Lawrence Brakmo @ 2018-07-03 18:38 UTC (permalink / raw)
To: Neal Cardwell, Yuchung Cheng
Cc: Netdev, Kernel Team, Blake Matheny, Alexei Starovoitov,
Steve Ibanez, Eric Dumazet, Daniel Borkmann
In-Reply-To: <CADVnQykD8i68w8zySXv1tnVnrYYiGC9oVRMk70xeC9mE9PtnfQ@mail.gmail.com>
On 7/3/18, 6:15 AM, "Neal Cardwell" <ncardwell@google.com> wrote:
On Mon, Jul 2, 2018 at 7:49 PM Yuchung Cheng <ycheng@google.com> wrote:
>
> On Mon, Jul 2, 2018 at 2:39 PM, Lawrence Brakmo <brakmo@fb.com> wrote:
> >
> > DCTCP depends on the CA_EVENT_NON_DELAYED_ACK and CA_EVENT_DELAYED_ACK
> > notifications to keep track if it needs to send an ACK for packets that
> > were received with a particular ECN state but whose ACK was delayed.
> >
> > Under some circumstances, for example when a delayed ACK is sent with a
> > data packet, DCTCP state was not being updated due to a lack of
> > notification that the previously delayed ACK was sent. As a result, it
> > would sometimes send a duplicate ACK when a new data packet arrived.
> >
> > This patch insures that DCTCP's state is correctly updated so it will
> > not send the duplicate ACK.
> Sorry to chime-in late here (lame excuse: IETF deadline)
>
> IIRC this issue would exist prior to 4.11 kernel. While it'd be good
> to fix that, it's not clear which patch introduced the regression
> between 4.11 and 4.16? I assume you tested Eric's most recent quickack
> fix.
I don't think Larry is saying there's a regression between 4.11 and
4.16. His recent "tcp: force cwnd at least 2 in tcp_cwnd_reduction"
patch here:
https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_935233_&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=pq_Mqvzfy-C8ltkgyx1u_g&m=7ko5C_ln2b7gZvz2A_UrZzz0AlcnhrW7-9KRahj_PEA&s=HcpZvo1TulN4-Y7Jhba5KM1MIaPwnBC95T8pLZfJESI&e=
said that 4.11 was bad (high tail latency and lots of RTOs) and 4.16
was still bad but with different netstat counters (no RTOs but still
high tail latency):
"""
On 4.11, pcap traces indicate that in some instances the 1st packet of
the RPC is received but no ACK is sent before the packet is
retransmitted. On 4.11 netstat shows TCP timeouts, with some of them
spurious.
On 4.16, we don't see retransmits in netstat but the high tail latencies
are still there.
"""
I suspect the RTOs disappeared but latencies remained too high because
between 4.11 and 4.16 we introduced:
tcp: allow TLP in ECN CWR
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=b4f70c3d4ec32a2ff4c62e1e2da0da5f55fe12bd
So the RTOs probably disappeared because this commit turned them into
TLPs. But the latencies remained high because the fundamental bug
remained throughout 4.11 and 4.16 and today: the DCTCP use of
tcp_send_ack() with an old rcv_nxt caused delayed ACKs to be cancelled
when they should not have been.
> In terms of the fix itself, it seems odd the tcp_send_ack() call in
> DCTCP generates NON_DELAYED_ACK event to toggle DCTCP's
> delayed_ack_reserved bit. Shouldn't the fix to have DCTCP send the
> "prior" ACK w/o cancelling delayed-ACK and mis-recording that it's
> cancelled, because that prior-ACK really is a supplementary old ACK.
This patch is fixing an issue that's orthogonal to the one you are
talking about. Using the taxonomy from our team's internal discussion
yesterday, the issue you mention where the DCTCP "prior" ACK is
cancelling delayed ACKs is issue (4); the issue that this particular
"tcp: notify when a delayed ack is sent" patch from Larry fixes is
issue (3). It's a bit tricky because both issues appeared in Larry's
trace summary and packetdrill script to reproduce the issue.
neal
I was able to track the patch that introduced the problem:
commit 3759824da87b30ce7a35b4873b62b0ba38905ef5
Author: Yuchung Cheng <ycheng@google.com>
Date: Wed Jul 1 14:11:15 2015 -0700
tcp: PRR uses CRB mode by default and SS mode conditionally
I tested a kernel which reverted the relevant change (see diff below) and the high tail latencies of more than 40ms disappeared. However, the 10KB high percentile latencies are 4ms vs. less than 200us with my patches. It looks like the patch above ended up reducing the cwnd to 1 in the scenarios that were triggering the high tail latencies. That is, it increased the likelihood of triggering actual bugs in the network stack code that my patch set fixes.
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2c5d70bc294e..50fabb07d739 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -2468,13 +2468,14 @@ void tcp_cwnd_reduction(struct sock *sk, int newly_acked_sacked, int flag)
u64 dividend = (u64)tp->snd_ssthresh * tp->prr_delivered +
tp->prior_cwnd - 1;
sndcnt = div_u64(dividend, tp->prior_cwnd) - tp->prr_out;
- } else if ((flag & FLAG_RETRANS_DATA_ACKED) &&
- !(flag & FLAG_LOST_RETRANS)) {
+ } else {
+// } else if ((flag & FLAG_RETRANS_DATA_ACKED) &&
+// !(flag & FLAG_LOST_RETRANS)) {
sndcnt = min_t(int, delta,
max_t(int, tp->prr_delivered - tp->prr_out,
newly_acked_sacked) + 1);
- } else {
- sndcnt = min(delta, newly_acked_sacked);
+// } else {
+// sndcnt = min(delta, newly_acked_sacked);
}
/* Force a fast retransmit upon entering fast recovery */
sndcnt = max(sndcnt, (tp->prr_out ? 0 : 1));
^ permalink raw reply related
* Re: [Intel-wired-lan] [jkirsher/next-queue PATCH v2 0/7] Add support for L2 Fwd Offload w/o ndo_select_queue
From: Jeff Kirsher @ 2018-07-03 18:42 UTC (permalink / raw)
To: Alexander Duyck; +Cc: intel-wired-lan, netdev
In-Reply-To: <20180612151322.86792.97587.stgit@ahduyck-green-test.jf.intel.com>
On Tue, Jun 12, 2018 at 8:18 AM, Alexander Duyck
<alexander.h.duyck@intel.com> wrote:
> This patch series is meant to allow support for the L2 forward offload, aka
> MACVLAN offload without the need for using ndo_select_queue.
>
> The existing solution currently requires that we use ndo_select_queue in
> the transmit path if we want to associate specific Tx queues with a given
> MACVLAN interface. In order to get away from this we need to repurpose the
> tc_to_txq array and XPS pointer for the MACVLAN interface and use those as
> a means of accessing the queues on the lower device. As a result we cannot
> offload a device that is configured as multiqueue, however it doesn't
> really make sense to configure a macvlan interfaced as being multiqueue
> anyway since it doesn't really have a qdisc of its own in the first place.
>
> I am submitting this as an RFC for the netdev mailing list, and officially
> submitting it for testing to Jeff Kirsher's next-queue in order to validate
> the ixgbe specific bits.
>
> The big changes in this set are:
> Allow lower device to update tc_to_txq and XPS map of offloaded MACVLAN
> Disable XPS for single queue devices
> Replace accel_priv with sb_dev in ndo_select_queue
> Add sb_dev parameter to fallback function for ndo_select_queue
> Consolidated ndo_select_queue functions that appeared to be duplicates
>
> v2: Implement generic "select_queue" functions instead of "fallback" functions.
> Tweak last two patches to account for changes in dev_pick_tx_xxx functions.
>
> ---
>
> Alexander Duyck (7):
> net-sysfs: Drop support for XPS and traffic_class on single queue device
> net: Add support for subordinate device traffic classes
> ixgbe: Add code to populate and use macvlan tc to Tx queue map
> net: Add support for subordinate traffic classes to netdev_pick_tx
> net: Add generic ndo_select_queue functions
> net: allow ndo_select_queue to pass netdev
> net: allow fallback function to pass netdev
>
Alex, there were recent changes to Dave's net-next which caused a
conflict with one or more of your patches. So I have removed this
series from my tree for now, and will work with you on updating the
series to work with the latest net-next tree.
^ permalink raw reply
* Re: [PATCHv2 net-next 2/2] selftests: add a selftest for directed broadcast forwarding
From: David Ahern @ 2018-07-03 19:23 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, davem, Davide Caratti, Ido Schimmel
In-Reply-To: <CADvbK_ciwsQuJ3Ep0_oFzmFSAPGMB9vjB=KXZ=feG5X1M=oZnw@mail.gmail.com>
On 7/3/18 5:36 AM, Xin Long wrote:
> On Mon, Jul 2, 2018 at 11:12 PM, David Ahern <dsahern@gmail.com> wrote:
>> On 7/2/18 12:30 AM, Xin Long wrote:
>>> +ping_ipv4()
>>> +{
>>> + sysctl_set net.ipv4.icmp_echo_ignore_broadcasts 0
>>> + bc_forwarding_disable
>>> + ping_test $h1 198.51.100.255
>>> +
>>> + iptables -A INPUT -i vrf-r1 -p icmp -j DROP
>>> + bc_forwarding_restore
>>> + bc_forwarding_enable
>>> + ping_test $h1 198.51.100.255
>>> +
>>> + bc_forwarding_restore
>>> + iptables -D INPUT -i vrf-r1 -p icmp -j DROP
>>> + sysctl_restore net.ipv4.icmp_echo_ignore_broadcasts
>>> +}
>>
>> Both tests fail for me:
>> TEST: ping [FAIL]
>> TEST: ping [FAIL]
> I think 'ip vrf exec ...' is not working in your env, while
> the testing is using "ip vrf exec vrf-h1 ping ..."
>
> You can test it by:
> # ip link add dev vrf-test type vrf table 1111
> # ip vrf exec vrf-test ls
well, that's embarrassing. yes, I updated ip and forgot to apply the bpf
workaround to define the syscall number (not defined in jessie).
>
>>
>> Why the need for the iptables rule?
> This iptables rule is to block the echo_request packet going to
> route's local_in.
> When bc_forwarding is NOT doing forwarding well but the packet
> goes to the route's local_in, it will fail.
>
> Without this rule, the 2nd ping will always succeed, we can't tell the
> echo_reply is from route or h2.
>
> Or you have a better way to test this?
your commands are not a proper test. The test should succeed and fail
based on the routing lookup, not iptables rules.
>
>>
>> And, PAUSE_ON_FAIL is not working to take a look at why tests are
>> failing. e.g.,
>>
>> PAUSE_ON_FAIL=yes ./router_broadcast.sh
>>
>> just continues on. Might be something with the infrastructure scripts.
> Yes, in ./router_broadcast.sh, it loads lib.sh where it loads forwarding.config
> where it has "PAUSE_ON_FAIL=no", which would override your
> "PAUSE_ON_FAIL=yes".
>
ack. bit by that as well.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox