* [PATCH v2 net-next] openvswitch: kernel datapath clone action
From: Yifeng Sun @ 2018-07-02 15:18 UTC (permalink / raw)
To: pshelar, azhou, netdev; +Cc: Yifeng Sun
Add 'clone' action to kernel datapath by using existing functions.
When actions within clone don't modify the current flow, the flow
key is not cloned before executing clone actions.
This is a follow up patch for this incomplete work:
https://patchwork.ozlabs.org/patch/722096/
v1 -> v2:
Refactor as advised by reviewer.
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Andy Zhou <azhou@ovn.org>
---
include/linux/openvswitch.h | 5 +++
include/uapi/linux/openvswitch.h | 3 ++
net/openvswitch/actions.c | 33 ++++++++++++++++++
net/openvswitch/flow_netlink.c | 73 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 114 insertions(+)
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index e6b240b6..379affc 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -21,4 +21,9 @@
#include <uapi/linux/openvswitch.h>
+#define OVS_CLONE_ATTR_EXEC 0 /* Specify an u32 value. When nonzero,
+ * actions in clone will not change flow
+ * keys. False otherwise.
+ */
+
#endif /* _LINUX_OPENVSWITCH_H */
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 863aaba..dbe0cbe 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -840,6 +840,8 @@ struct ovs_action_push_eth {
* @OVS_ACTION_ATTR_POP_NSH: pop the outermost NSH header off the packet.
* @OVS_ACTION_ATTR_METER: Run packet through a meter, which may drop the
* packet, or modify the packet (e.g., change the DSCP field).
+ * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
+ * actions without affecting the original packet and key.
*
* Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all
* fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -873,6 +875,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_PUSH_NSH, /* Nested OVS_NSH_KEY_ATTR_*. */
OVS_ACTION_ATTR_POP_NSH, /* No argument. */
OVS_ACTION_ATTR_METER, /* u32 meter ID. */
+ OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */
__OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
* from userspace. */
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 30a5df2..85ae53d 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1057,6 +1057,28 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
clone_flow_key);
}
+/* When 'last' is true, clone() should always consume the 'skb'.
+ * Otherwise, clone() should keep 'skb' intact regardless what
+ * actions are executed within clone().
+ */
+static int clone(struct datapath *dp, struct sk_buff *skb,
+ struct sw_flow_key *key, const struct nlattr *attr,
+ bool last)
+{
+ struct nlattr *actions;
+ struct nlattr *clone_arg;
+ int rem = nla_len(attr);
+ bool dont_clone_flow_key;
+
+ /* The first action is always 'OVS_CLONE_ATTR_ARG'. */
+ clone_arg = nla_data(attr);
+ dont_clone_flow_key = nla_get_u32(clone_arg);
+ actions = nla_next(clone_arg, &rem);
+
+ return clone_execute(dp, skb, key, 0, actions, rem, last,
+ !dont_clone_flow_key);
+}
+
static void execute_hash(struct sk_buff *skb, struct sw_flow_key *key,
const struct nlattr *attr)
{
@@ -1336,6 +1358,17 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
consume_skb(skb);
return 0;
}
+ break;
+
+ case OVS_ACTION_ATTR_CLONE: {
+ bool last = nla_is_last(a, rem);
+
+ err = clone(dp, skb, key, a, last);
+ if (last)
+ return err;
+
+ break;
+ }
}
if (unlikely(err)) {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 391c407..a70097e 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2460,6 +2460,40 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
return 0;
}
+static int validate_and_copy_clone(struct net *net,
+ const struct nlattr *attr,
+ const struct sw_flow_key *key,
+ struct sw_flow_actions **sfa,
+ __be16 eth_type, __be16 vlan_tci,
+ bool log, bool last)
+{
+ int start, err;
+ u32 exec;
+
+ if (nla_len(attr) && nla_len(attr) < NLA_HDRLEN)
+ return -EINVAL;
+
+ start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CLONE, log);
+ if (start < 0)
+ return start;
+
+ exec = last || !actions_may_change_flow(attr);
+
+ err = ovs_nla_add_action(sfa, OVS_CLONE_ATTR_EXEC, &exec,
+ sizeof(exec), log);
+ if (err)
+ return err;
+
+ err = __ovs_nla_copy_actions(net, attr, key, sfa,
+ eth_type, vlan_tci, log);
+ if (err)
+ return err;
+
+ add_nested_action_end(*sfa, start);
+
+ return 0;
+}
+
void ovs_match_init(struct sw_flow_match *match,
struct sw_flow_key *key,
bool reset_key,
@@ -2849,6 +2883,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
[OVS_ACTION_ATTR_PUSH_NSH] = (u32)-1,
[OVS_ACTION_ATTR_POP_NSH] = 0,
[OVS_ACTION_ATTR_METER] = sizeof(u32),
+ [OVS_ACTION_ATTR_CLONE] = (u32)-1,
};
const struct ovs_action_push_vlan *vlan;
int type = nla_type(a);
@@ -3038,6 +3073,18 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
/* Non-existent meters are simply ignored. */
break;
+ case OVS_ACTION_ATTR_CLONE: {
+ bool last = nla_is_last(a, rem);
+
+ err = validate_and_copy_clone(net, a, key, sfa,
+ eth_type, vlan_tci,
+ log, last);
+ if (err)
+ return err;
+ skip_copy = true;
+ break;
+ }
+
default:
OVS_NLERR(log, "Unknown Action type %d", type);
return -EINVAL;
@@ -3116,6 +3163,26 @@ static int sample_action_to_attr(const struct nlattr *attr,
return err;
}
+static int clone_action_to_attr(const struct nlattr *attr,
+ struct sk_buff *skb)
+{
+ struct nlattr *start;
+ int err = 0, rem = nla_len(attr);
+
+ start = nla_nest_start(skb, OVS_ACTION_ATTR_CLONE);
+ if (!start)
+ return -EMSGSIZE;
+
+ err = ovs_nla_put_actions(nla_data(attr), rem, skb);
+
+ if (err)
+ nla_nest_cancel(skb, start);
+ else
+ nla_nest_end(skb, start);
+
+ return err;
+}
+
static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
{
const struct nlattr *ovs_key = nla_data(a);
@@ -3204,6 +3271,12 @@ int ovs_nla_put_actions(const struct nlattr *attr, int len, struct sk_buff *skb)
return err;
break;
+ case OVS_ACTION_ATTR_CLONE:
+ err = clone_action_to_attr(a, skb);
+ if (err)
+ return err;
+ break;
+
default:
if (nla_put(skb, type, nla_len(a), nla_data(a)))
return -EMSGSIZE;
--
2.7.4
^ permalink raw reply related
* [PATCH wpan 1/2] net: 6lowpan: fix reserved space for single frames
From: Alexander Aring @ 2018-07-02 20:32 UTC (permalink / raw)
To: stefan; +Cc: linux-wpan, netdev, kernel, Alexander Aring
This patch fixes patch add handling to take care tail and headroom for
single 6lowpan frames. We need to be sure we have a skb with the right
head and tailroom for single frames. This patch do it by using
skb_copy_expand() if head and tailroom is not enough allocated by upper
layer.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=195059
Reported-by: David Palma <david.palma@ntnu.no>
Reported-by: Rabi Narayan Sahoo <rabinarayans0828@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
net/ieee802154/6lowpan/tx.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/net/ieee802154/6lowpan/tx.c b/net/ieee802154/6lowpan/tx.c
index e6ff5128e61a..d0c4d220de08 100644
--- a/net/ieee802154/6lowpan/tx.c
+++ b/net/ieee802154/6lowpan/tx.c
@@ -265,9 +265,24 @@ netdev_tx_t lowpan_xmit(struct sk_buff *skb, struct net_device *ldev)
/* We must take a copy of the skb before we modify/replace the ipv6
* header as the header could be used elsewhere
*/
- skb = skb_unshare(skb, GFP_ATOMIC);
- if (!skb)
- return NET_XMIT_DROP;
+ if (unlikely(skb_headroom(skb) < ldev->needed_headroom ||
+ skb_tailroom(skb) < ldev->needed_tailroom)) {
+ struct sk_buff *nskb;
+
+ nskb = skb_copy_expand(skb, ldev->needed_headroom,
+ ldev->needed_tailroom, GFP_ATOMIC);
+ if (likely(skb)) {
+ consume_skb(skb);
+ skb = nskb;
+ } else {
+ kfree_skb(skb);
+ return NET_XMIT_DROP;
+ }
+ } else {
+ skb = skb_unshare(skb, GFP_ATOMIC);
+ if (!skb)
+ return NET_XMIT_DROP;
+ }
ret = lowpan_header(skb, ldev, &dgram_size, &dgram_offset);
if (ret < 0) {
--
2.11.0
^ permalink raw reply related
* [PATCH wpan 2/2] net: mac802154: tx: expand tailroom if necessary
From: Alexander Aring @ 2018-07-02 20:32 UTC (permalink / raw)
To: stefan; +Cc: linux-wpan, netdev, kernel, Alexander Aring
In-Reply-To: <20180702203203.21316-1-aring@mojatatu.com>
This patch is necessary if case of AF_PACKET or other socket interface
which I am aware of it and didn't allocated the necessary room.
Reported-by: David Palma <david.palma@ntnu.no>
Reported-by: Rabi Narayan Sahoo <rabinarayans0828@gmail.com>
Signed-off-by: Alexander Aring <aring@mojatatu.com>
---
net/mac802154/tx.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/net/mac802154/tx.c b/net/mac802154/tx.c
index 7e253455f9dd..bcd1a5e6ebf4 100644
--- a/net/mac802154/tx.c
+++ b/net/mac802154/tx.c
@@ -63,8 +63,21 @@ ieee802154_tx(struct ieee802154_local *local, struct sk_buff *skb)
int ret;
if (!(local->hw.flags & IEEE802154_HW_TX_OMIT_CKSUM)) {
- u16 crc = crc_ccitt(0, skb->data, skb->len);
+ struct sk_buff *nskb;
+ u16 crc;
+
+ if (unlikely(skb_tailroom(skb) < IEEE802154_FCS_LEN)) {
+ nskb = skb_copy_expand(skb, 0, IEEE802154_FCS_LEN,
+ GFP_ATOMIC);
+ if (likely(nskb)) {
+ consume_skb(skb);
+ skb = nskb;
+ } else {
+ goto err_tx;
+ }
+ }
+ crc = crc_ccitt(0, skb->data, skb->len);
put_unaligned_le16(crc, skb_put(skb, 2));
}
--
2.11.0
^ permalink raw reply related
* Re: Regression introduced by "r8169: simplify rtl_set_mac_address"
From: Heiner Kallweit @ 2018-07-02 20:35 UTC (permalink / raw)
To: Corinna Vinschen; +Cc: netdev@vger.kernel.org
In-Reply-To: <20180702194812.GG3111@calimero.vinschen.de>
On 02.07.2018 21:48, Corinna Vinschen wrote:
> Hi,
>
> the patch 1f7aa2bc268e, "r8169: simplify rtl_set_mac_address",
> introduced a regression found by trying to team a r8169 NIC.
>
Thanks for reporting!
> Try the following (assuming the r8169 NIC is eth0):
>
> $ nmcli con add type team con-name team0 ifname nm-team config \
> '{"runner": {"name": "lacp"}, "link_watch": {"name": "ethtool"}}' \
> ipv4.method disable ipv6.method ignore
> $ nmcli con add type ethernet ifname eth0 con-name team0.0 master nm-team
> $ nmcli con up team0.0
> $ teamdctl nm-team port present eth0
> command call failed (No such device)
>
> Bisecting turned up commit 1f7aa2bc268e, "r8169: simplify
> rtl_set_mac_address" as the culprit. Reverting this patch fixes
> the issue and the teamdctl call succeeds.
>
> The reason is apparently the usage of eth_mac_addr here. eth_mac_addr
> calls eth_prepare_mac_addr_change which checks for IFF_LIVE_ADDR_CHANGE.
> Debugging shows this flag not being set on r8169, thus
> eth_prepare_mac_addr_change returns -EBUSY (no idea why userspace claims
> "No such device", rather than "Device or resource busy", but that's not
> the point here).
>
> Note that other devices like igb, don't call eth_mac_addr either, but
> rather call memcpy by themselves to copy the new MAC, just as the
> original r8169 code did, too. Consequentially this problem is not
> present on igb.
>
Doing the memcpy directly in the driver (like it was before) would be
a solution, however I'd consider this more a workaround because purpose
of the core functions is to encapsulate such details.
> I suggest to revert this change in the first place, but I wonder if
> we're not just missing to set IFF_LIVE_ADDR_CHANGE in a lot of drivers.
>
I'd prefer to keep the code and set flag IFF_LIVE_ADDR_CHANGE in the
driver. Setting this flag via ethtool --set-priv-flags in theory would
also be an option, however the r8169 driver doesn't implement the
ethtool_ops set_priv_flags callback.
>
> Thanks,
> Corinna
>
Rgds, Heiner
^ permalink raw reply
* Re: [PATCH] 6lowpan: iphc: reset mac_header after decompress to fix panic
From: Alexander Aring @ 2018-07-02 20:43 UTC (permalink / raw)
To: Michael Scott
Cc: Jukka Rissanen, David S. Miller, linux-bluetooth, linux-wpan,
netdev, linux-kernel
In-Reply-To: <516fbc65-cc4b-8016-de5a-e2240b779d15@opensourcefoundries.com>
Hi,
On Mon, Jul 02, 2018 at 12:45:41PM -0700, Michael Scott wrote:
> Hello Alexander,
>
...
> > Question is for me: which upper layer wants access MAC header here on
> > receive path?
> > It cannot parsed anyhow because so far I know no upper layer can parse
> > at the moment 802.15.4 frames (which is a complex format). Maybe over
> > some header_ops callback?
>
> I was testing a C program which performs NAT64 handling on packets
> destined to a certain IPv6 subnet (64:ff9b::). To do this, the application
> opens a RAW socket like this: sniff_sock = socket(PF_PACKET, SOCK_RAW,
> htons(ETH_P_ALL)); It then sets promiscuous mode and enters a looping call
> of:
> length = recv(sniff_sock, buffer, PACKET_BUFFER, MSG_TRUNC); My host PC
> kernel would then promptly crash on me. (I'm going to purposely avoid the
> obvious point of: this probably isn't the best way to parse packets for
> NAT64 translation as you will get every single packet incoming or outgoing
> on the host.) Turns out, testing the program on an 802.15.4 6lowpan
> interface exposed some of the issues which this mailing list (but not
> myself) is well aware of (no L2 data in the RAW packets) and also led me to
> debugging this patch to stop the kernel crash. TL;DR: To summarize, any
> PF_PACKET SOCK_RAW socket which recv()'s IPv6 data from a 6lowpan node will
> cause this kernel crash eventually (checked on kernel 4.15, 4.16, 4.17 and
> 4.18-rc1). - Mike
> >
"any PF_PACKET SOCK_RAW" can't be otherwise I would also see it with my
sniffer programs e.g. wireshark or tcpdump which use libpcap.
There need to be some different in the handling. This is what I have
currently in my mind.
I currently not sure how to set skb->mac_header if interface is RAW_IP.
It seems there is an indicator that mac header is not set. Example:
diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
index 6b1042e21656..e6ec2df3afe0 100644
--- a/net/6lowpan/iphc.c
+++ b/net/6lowpan/iphc.c
@@ -770,6 +770,7 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
hdr.hop_limit, &hdr.daddr);
skb_push(skb, sizeof(hdr));
+ skb->mac_header = (typeof(skb->mac_header))~0U;
skb_reset_network_header(skb);
skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
Maybe we should lookup what skb->mac_header points to on tun interfaces
then do the same.
- Alex
^ permalink raw reply related
* RE: [RFC net-next 15/15] net: lora: Add Semtech SX1301
From: Ben Whitten @ 2018-07-02 20:43 UTC (permalink / raw)
To: Andreas Färber, Mark Brown
Cc: netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Jian-Hong Pan, Jiri Pirko,
Marcel Holtmann, David S . Miller, Matthias Brugger, Janus Piwek,
Michael Röder, Dollar Chen, Ken Yu, Steve deRosier,
linux-spi@vger.kernel.org, LoRa_Community_Support@semtech.com
In-Reply-To: <4e06cc72-2092-70f3-c801-bf6e4c3cbec2@suse.de>
> Subject: Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301
>
> 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
>
> The reason you're in CC on this RFC is two-fold:
>
> 1) You applied Ben's patch to associate "semtech,sx1301" with spidev,
> whereas I am now preparing a new driver for the same compatible.
>
> 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?
In my work in progress driver I just register one controller for the sx1301 with two chip selects and use the chip select information to choose the correct radio to send to, this is based on the DT reg information. No need to register two separate masters.
> More inline ...
>
> 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;
> >> +
> >> + ret = sx1301_radio_set_cs(spi->controller, enable);
> >> + if (ret)
> >> + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret);
> >> +}
> >
> > So we never disable chip select?
>
> Not here, I instead did that in transfer_one below.
>
> Unfortunately there seems to be no documentation, only reference code:
>
> https://github.com/Lora-
> net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L121
> https://github.com/Lora-
> net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L165
>
> It sets CS to 0 before writing to address and data registers, then
> immediately sets CS to 1 and back to 0 before reading or ending the
> write transaction. I've tried to force the same behavior in this driver.
> My guess was that CS is high-active during the short 1-0 cycle, because
> if it's low-active during the register writes then why the heck is it
> set to 0 again in the end instead of keeping at 1... confusing.
>
> Maybe the Semtech folks CC'ed can comment how these registers work?
>
> >> + if (tx_buf) {
> >> + ret = sx1301_write(ssx->parent, ssx->regs +
> REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0);
> >
> > This looks confused. We're in an if (tx_buf) block but there's a use of
> > the ternery operator that appears to be checking if we have a tx_buf?
>
> Yeah, as mentioned this RFC is not ready for merging - checkpatch.pl
> will complain about lines too long, and TODOs are sprinkled all over or
> not even mentioned. It's a Proof of Concept that a net_device could work
> for a wide range of spi and serdev based drivers, and on top this device
> has more than one channel, which may influence network-level design
> discussions.
>
> That said, I'll happily drop the second check. Thanks for spotting!
>
> >> + 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?
> 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.).
You want an SPI controller in the SX1301 as the down stream radios are SPI and could be attached directly to a host SPI bus, makes sense to have one radio driver and talk through the SX1301.
But you should use the regmap to access the SX1301 master controller registers.
Example I use with one SPI master and some clock info:
eg:
sx1301: sx1301@0 {
compatible = "semtech,sx1301";
reg = <0>;
#address-cells = <1>;
#size-cells = <0>;
spi-max-frequency = <8000000>;
gpios-reset = <&pioA 26 GPIO_ACTIVE_HIGH>;
clocks = <&radio1 0>, <&clkhs 0>;
clock-names = "clk32m", "clkhs";
radio0: sx1257@0 {
compatible = "semtech,sx125x";
reg = <0>;
spi-max-frequency = <8000000>;
tx;
clocks = <&tcxo 0>;
clock-names = "tcxo";
};
radio1: sx1257@1 {
compatible = "semtech,sx125x";
reg = <1>;
spi-max-frequency = <8000000>;
#clock-cells = <0>;
clocks = <&tcxo 0>;
clock-names = "tcxo";
clock-output-names = "clk32m";
};
};
> You will find a datasheet with some diagrams mentioning "SPI" at:
> https://www.semtech.com/products/wireless-rf/lora-gateways/SX1301
>
> >> + if (rx_buf) {
> >> + ret = sx1301_read(ssx->parent, ssx->regs +
> REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]);
> >> + if (ret) {
> >> + dev_err(&spi->dev, "SPI radio data read failed\n");
> >> + return ret;
> >> + }
> >> + }
> >
> > For a read we never set an address?
>
> To read, you first write the address via tx_buf, then either in the same
> transfer in the third byte or in a subsequent one-byte transfer as first
> byte you get the data.
>
> If you have better ideas how to structure this, do let me know.
>
> >> +static void sx1301_radio_setup(struct spi_controller *ctrl)
> >> +{
> >> + ctrl->mode_bits = SPI_CS_HIGH | SPI_NO_CS;
> >
> > This controller has no chip select but we provided a set_cs operation?
>
> Oops, I played around with those two options and was hoping SPI_NO_CS
> would avoid the undesired set_cs invocations, but it didn't work as
> expected and so I added the "if (enabled)" check above.
>
> Thanks for your review,
>
> 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
* [PATCH net] r8169: fix mac address change
From: Heiner Kallweit @ 2018-07-02 20:49 UTC (permalink / raw)
To: David Miller, Corinna Vinschen, netdev@vger.kernel.org
Network core refuses to change mac address because flag
IFF_LIVE_ADDR_CHANGE isn't set. Set this missing flag.
Fixes: 1f7aa2bc268e ("r8169: simplify rtl_set_mac_address")
Reported-by: Corinna Vinschen <vinschen@redhat.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f80ac894..a390db27 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -7607,6 +7607,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
NETIF_F_HW_VLAN_CTAG_RX;
dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO |
NETIF_F_HIGHDMA;
+ dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
tp->cp_cmd |= RxChkSum | RxVlan;
--
2.18.0
^ permalink raw reply related
* [PATCH] gen_stats: Fix netlink stats dumping in the presence of padding
From: Toke Høiland-Jørgensen @ 2018-07-02 20:52 UTC (permalink / raw)
To: netdev; +Cc: cake
The gen_stats facility will add a header for the toplevel nlattr of type
TCA_STATS2 that contains all stats added by qdisc callbacks. A reference
to this header is stored in the gnet_dump struct, and when all the
per-qdisc callbacks have finished adding their stats, the length of the
containing header will be adjusted to the right value.
However, on architectures that need padding (i.e., that don't set
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS), the padding nlattr is added
before the stats, which means that the stored pointer will point to the
padding, and so when the header is fixed up, the result is just a very
big padding nlattr. Because most qdiscs also supply the legacy TCA_STATS
struct, this problem has been mostly invisible, but we exposed it with
the netlink attribute-based statistics in CAKE.
Fix the issue by fixing up the stored pointer if it points to a padding
nlattr.
Tested-by: Pete Heist <pete@heistp.net>
Tested-by: Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>
Signed-off-by: Toke Høiland-Jørgensen <toke@toke.dk>
---
net/core/gen_stats.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index b2b2323bdc84..188d693cb251 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -77,8 +77,20 @@ gnet_stats_start_copy_compat(struct sk_buff *skb, int type, int tc_stats_type,
d->lock = lock;
spin_lock_bh(lock);
}
- if (d->tail)
- return gnet_stats_copy(d, type, NULL, 0, padattr);
+ if (d->tail) {
+ int ret = gnet_stats_copy(d, type, NULL, 0, padattr);
+
+ /* The initial attribute added in gnet_stats_copy() may be
+ * preceded by a padding attribute, in which case d->tail will
+ * end up pointing at the padding instead of the real attribute.
+ * Fix this so gnet_stats_finish_copy() adjusts the length of
+ * the right attribute.
+ */
+ if (ret == 0 && d->tail->nla_type == padattr)
+ d->tail = (struct nlattr *)((char *)d->tail +
+ NLA_ALIGN(d->tail->nla_len));
+ return ret;
+ }
return 0;
}
^ permalink raw reply related
* Re: [PATCH net-next 01/10] r8169: add basic phylib support
From: Andrew Lunn @ 2018-07-02 21:02 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <60049e7e-b86d-1968-cdfd-7e0f91a25b88@gmail.com>
> +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?
It would be better to trap the phyaddr which are not supported and
return 0xffff.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
From: Andrew Lunn @ 2018-07-02 21:06 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <318ae669-d89c-636d-94ac-d4c9f662400e@gmail.com>
> 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?
Andrew
^ permalink raw reply
* Re: [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset
From: Andrew Lunn @ 2018-07-02 21:08 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <ac6d5745-2c4c-d229-12f3-b6b45b95b8ed@gmail.com>
On Mon, Jul 02, 2018 at 09:36:58PM +0200, Heiner Kallweit wrote:
> Use genphy_soft_reset() instead of open-coding a PHY soft reset. We have
> to do an explicit PHY soft reset because some chips use the genphy driver
> which uses a no-op as soft_reset callback.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset
From: Andrew Lunn @ 2018-07-02 21:11 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <968b2379-9201-d4e5-502c-566dfef1a1ea@gmail.com>
On Mon, Jul 02, 2018 at 09:37:03PM +0200, Heiner Kallweit wrote:
> Switch to using phy_ethtool_nway_reset().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH bpf 3/3] bpf: undo prog rejection on read-only lock failure
From: Daniel Borkmann @ 2018-07-02 21:12 UTC (permalink / raw)
To: Kees Cook; +Cc: Alexei Starovoitov, Network Development, Laura Abbott
In-Reply-To: <CAGXu5jLCFhxw2MVg4Gfz=yrYzxONr0ESHtePPeS=xgM3mQUhsQ@mail.gmail.com>
On 07/02/2018 08:48 PM, Kees Cook wrote:
> On Fri, Jun 29, 2018 at 4:47 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 06/29/2018 08:42 PM, Kees Cook wrote:
>>> On Thu, Jun 28, 2018 at 2:34 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>> Kees suggested that if set_memory_*() can fail, we should annotate it with
>>>> __must_check, and all callers need to deal with it gracefully given those
>>>> set_memory_*() markings aren't "advisory", but they're expected to actually
>>>> do what they say. This might be an option worth to move forward in future
>>>> but would at the same time require that set_memory_*() calls from supporting
>>>> archs are guaranteed to be "atomic" in that they provide rollback if part
>>>> of the range fails, once that happened, the transition from RW -> RO could
>>>> be made more robust that way, while subsequent RO -> RW transition /must/
>>>> continue guaranteeing to always succeed the undo part.
>>>
>>> Does this mean we can have BPF filters that aren't read-only then?
>>> What's the situation where set_memory_ro() fails? (Can it be induced
>>> by the user?)
>>
>> My understanding is that the cpa_process_alias() would attempt to also change
>> attributes of physmap ranges, and it found that a large page had to be split
>> for this but failed in doing so thus attributes couldn't be updated there due
>> to page alloc error. Attempting to change the primary mapping which would be
>> directly the addr passed to set_memory_ro() was however set to read-only
>> despite error. While for reproduction I had a toggle on the alloc_pages() in
>> split_large_page() to have it fail, I only could trigger it occasionally; I
>> used the selftest suite in a loop to stress test and it hit about or twice
>> over hours.
>
> Okay, so it's pretty rare; that's good! :P
>
> It really seems like this should be a situation that never fails, but
> if we ARE going to allow failures, then I think we need to propagate
> them up to callers. That means modules could fail to load in these
> cases, etc, etc. Since this is a fundamental protection, we need to
> either never fail to set things RO or we need to disallow operation
> continuing in the face of something NOT being RO.
Yeah, fully agree with you, and set_memory_*() would need to be reworked for the
archs implementing them to recover from failure and rollback any attribute changes
already done from the call. Today it's not the case, so this would be first step,
and second step to add the checks for error and bail out from various call-sites.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net-next 06/10] r8169: use phy_mii_ioctl
From: Andrew Lunn @ 2018-07-02 21:13 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <9da55dc3-6e96-14e2-ac1c-1d621da533fe@gmail.com>
On Mon, Jul 02, 2018 at 09:37:05PM +0200, Heiner Kallweit wrote:
> Switch to using phy_mii_ioctl().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
From: Andrew Lunn @ 2018-07-02 21:20 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <5be3d523-bb26-c2d6-71f4-165e9c6e45a9@gmail.com>
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?
Andrew
^ permalink raw reply
* Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
From: Andrew Lunn @ 2018-07-02 21:21 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <fb72a450-8f15-2f34-38ae-26c7c3a5d159@gmail.com>
> - auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
This bit you probably want to keep. The PHY never says it support
Pause. The MAC needs to enable pause if the MAC supports pause.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 0/7] net/ipv6: Fix route append and replace use cases
From: David Ahern @ 2018-07-02 21:23 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Thomas.Winter, idosch, sharpd, roopa
In-Reply-To: <6d61c529-5b51-dc3b-86ee-e912f1a8e0a8@gmail.com>
On 5/22/18 2:44 PM, David Ahern wrote:
> On 5/22/18 12:46 PM, David Miller wrote:
>>
>> Ok, I'll apply this series.
>>
>> But if this breaks things for anyone in a practical way, I am unfortunately
>> going to have to revert no matter how silly the current behavior may be.
>>
>
> Understood. I have to try the best option first. I'll look at
> regressions if they happen.
>
Debugging the problem Sowmini reported and I discovered NetworkManager
likes to manage link-local prefix routes (fe80::/64), and it likes to do
so with NLM_F_APPEND so the routes end up like this:
fe80::/64 dev eth0 proto kernel metric 100 pref medium
fe80::/64 dev eth1 proto kernel metric 101 pref medium
fe80::/64 proto kernel metric 256
nexthop dev eth1 weight 1
nexthop dev eth0 weight 1 pref medium
NM deletes the kernel installed routes, and then adds new ones -- twice:
once with the custom metric and then re-installs the route it deleted
but with the append flag.
Given the short amount of time left for 4.18 (and summer PTO), I think a
revert is the only option; we can try again in a future release.
^ permalink raw reply
* Re: [PATCH net-next 1/2] tcp: notify when a delayed ack is sent
From: Lawrence Brakmo @ 2018-07-02 21:24 UTC (permalink / raw)
To: Neal Cardwell
Cc: Netdev, Kernel Team, Blake Matheny, Alexei Starovoitov,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <CADVnQykHXktzKY_jXa9GRwPciTf-1CsAegGRPyUc+F1mdMMmBw@mail.gmail.com>
On 7/2/18, 8:18 AM, "netdev-owner@vger.kernel.org on behalf of Neal Cardwell" <netdev-owner@vger.kernel.org on behalf of ncardwell@google.com> wrote:
On Fri, Jun 29, 2018 at 9:48 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.
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> ---
> net/ipv4/tcp_output.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index f8f6129160dd..41f6ad7a21e4 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);
> }
Thanks for this fix! Seems like this would work, but if I am reading
this correctly then it seems like this would cause a duplicate call to
tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK) when we are sending a pure
ACK (delayed or non-delayed):
(1) once from tcp_send_ack() before we send the ACK:
tcp_send_ack(struct sock *sk)
-> tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
(2) then again from tcp_event_ack_sent() after we have sent the ACK:
tcp_event_ack_sent()
-> if (inet_csk_ack_scheduled(sk))
tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK);
What if we remove the original CA_EVENT_NON_DELAYED_ACK call and just
replace it with your new one? (not compiled, not tested):
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 3889dcd4868d4..bddb49617d9be 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -184,6 +184,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);
}
@@ -3836,8 +3838,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.
Aside from lower CPU overhead, one nice benefit of that is that we
then only call tcp_ca_event(sk, CA_EVENT_NON_DELAYED_ACK) in one
place, which might be a little easier to reason about.
Does that work?
neal
Thanks Neal, good catch! I will resubmit the patch.
^ permalink raw reply
* Re: [PATCH net-next 2/2] tcp: ack immediately when a cwr packet arrives
From: Lawrence Brakmo @ 2018-07-02 21:24 UTC (permalink / raw)
To: Neal Cardwell
Cc: Netdev, Kernel Team, Blake Matheny, Alexei Starovoitov,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <CADVnQy=3s4AAX-SyHaRURPgA=mN=m4DBP=Jz3HGt5pBPbiRaaA@mail.gmail.com>
On 7/2/18, 7:57 AM, "Neal Cardwell" <ncardwell@google.com> wrote:
On Sat, Jun 30, 2018 at 9:47 PM Lawrence Brakmo <brakmo@fb.com> wrote:
> I see two issues, one is that entering quickack mode as you
> mentioned does not insure that it will still be on when the CWR
> arrives. The second issue is that the problem occurs right after the
> receiver sends a small reply which results in entering pingpong mode
> right before the sender starts the new request by sending just one
> packet (i.e. forces delayed ack).
>
> I compiled and tested your patch. Both 99 and 99.9 percentile
> latencies are around 40ms. Looking at the packet traces shows that
> some CWR marked packets are not being ack immediately (delayed by
> 40ms).
Thanks, Larry! So your tests provide nice, specific evidence that it
is good to force an immediate ACK when a receiver receives a packet
with CWR marked. Given that, I am wondering what the simplest way is
to achieve that goal.
What if, rather than plumbing a new specific signal into
__tcp_ack_snd_check(), we use the existing general quick-ack
mechanism, where various parts of the TCP stack (like
__tcp_ecn_check_ce()) are already using the quick-ack mechanism to
"remotely" signal to __tcp_ack_snd_check() that they want an immediate
ACK.
For example, would it work to do something like:
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c53ae5fc834a5..8168d1938b376 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -262,6 +262,12 @@ static void __tcp_ecn_check_ce(struct sock *sk,
const struct sk_buff *skb)
{
struct tcp_sock *tp = tcp_sk(sk);
+ /* 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.
+ */
+ if (tcp_hdr(skb)->cwr)
+ tcp_enter_quickack_mode(sk, 2);
+
switch (TCP_SKB_CB(skb)->ip_dsfield & INET_ECN_MASK) {
case INET_ECN_NOT_ECT:
/* Insure that GCN will not continue to mark packets. */
And then since that broadens the mission of this function beyond
checking just the ECT/CE bits, I supposed we could rename the
__tcp_ecn_check_ce() and tcp_ecn_check_ce() functions to
__tcp_ecn_check() and tcp_ecn_check(), or something like that.
Would that work for this particular issue?
Neal
Thanks Neal, it does work and is cleaner than what I was doing. I will submit a revised patch set.
^ permalink raw reply
* Re: [PATCH net-next 01/10] r8169: add basic phylib support
From: Heiner Kallweit @ 2018-07-02 21:15 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <20180702210239.GA12564@lunn.ch>
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.
> It would be better to trap the phyaddr which are not supported and
> return 0xffff.
>
OK
> Andrew
>
^ permalink raw reply
* Re: [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
From: Heiner Kallweit @ 2018-07-02 21:24 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <20180702210646.GB12564@lunn.ch>
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.
> Andrew
>
^ permalink raw reply
* Re: [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
From: Heiner Kallweit @ 2018-07-02 21:31 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, Florian Fainelli, Realtek linux nic maintainers,
netdev@vger.kernel.org
In-Reply-To: <20180702212005.GF12564@lunn.ch>
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.
> Andrew
>
^ permalink raw reply
* Re: [PATCH] 6lowpan: iphc: reset mac_header after decompress to fix panic
From: Alexander Aring @ 2018-07-02 21:31 UTC (permalink / raw)
To: Michael Scott
Cc: Jukka Rissanen, David S. Miller, linux-bluetooth, linux-wpan,
netdev, linux-kernel
In-Reply-To: <20180702204346.d7bynetvzw3ayn5m@x220t>
Hi,
On Mon, Jul 02, 2018 at 04:43:46PM -0400, Alexander Aring wrote:
> Hi,
>
> On Mon, Jul 02, 2018 at 12:45:41PM -0700, Michael Scott wrote:
> > Hello Alexander,
> >
> ...
> > > Question is for me: which upper layer wants access MAC header here on
> > > receive path?
> > > It cannot parsed anyhow because so far I know no upper layer can parse
> > > at the moment 802.15.4 frames (which is a complex format). Maybe over
> > > some header_ops callback?
> >
> > I was testing a C program which performs NAT64 handling on packets
> > destined to a certain IPv6 subnet (64:ff9b::). To do this, the application
> > opens a RAW socket like this: sniff_sock = socket(PF_PACKET, SOCK_RAW,
> > htons(ETH_P_ALL)); It then sets promiscuous mode and enters a looping call
> > of:
> > length = recv(sniff_sock, buffer, PACKET_BUFFER, MSG_TRUNC); My host PC
> > kernel would then promptly crash on me. (I'm going to purposely avoid the
> > obvious point of: this probably isn't the best way to parse packets for
> > NAT64 translation as you will get every single packet incoming or outgoing
> > on the host.) Turns out, testing the program on an 802.15.4 6lowpan
> > interface exposed some of the issues which this mailing list (but not
> > myself) is well aware of (no L2 data in the RAW packets) and also led me to
> > debugging this patch to stop the kernel crash. TL;DR: To summarize, any
> > PF_PACKET SOCK_RAW socket which recv()'s IPv6 data from a 6lowpan node will
> > cause this kernel crash eventually (checked on kernel 4.15, 4.16, 4.17 and
> > 4.18-rc1). - Mike
> > >
>
> "any PF_PACKET SOCK_RAW" can't be otherwise I would also see it with my
> sniffer programs e.g. wireshark or tcpdump which use libpcap.
>
> There need to be some different in the handling. This is what I have
> currently in my mind.
>
> I currently not sure how to set skb->mac_header if interface is RAW_IP.
> It seems there is an indicator that mac header is not set. Example:
>
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 6b1042e21656..e6ec2df3afe0 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -770,6 +770,7 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
> hdr.hop_limit, &hdr.daddr);
>
> skb_push(skb, sizeof(hdr));
> + skb->mac_header = (typeof(skb->mac_header))~0U;
> skb_reset_network_header(skb);
> skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
>
>
> Maybe we should lookup what skb->mac_header points to on tun interfaces
> then do the same.
So far I see [0] tun does the same as your patch approach does. network
header and mac header points to the same.
I think then we should go with your patch.
Then we just need to solve the issue to get mac header information on
top of lowpan socket layer... out of scope issue but indeed we need to
solve that. As we talked there exists even UDP protocols which needs mac
header information. It's in a pretty early state, I have no idea how
this fits sometimes with fragmentation handling together. At least for
L2 address handling and fragmentation it should be fine (one of the
fragment indentifier).
- Alex
[0] https://elixir.bootlin.com/linux/v4.18-rc3/source/drivers/net/tun.c#L1912
^ permalink raw reply
* [PATCH net-next v2 0/2] tcp: fix high tail latencies in DCTCP
From: Lawrence Brakmo @ 2018-07-02 21:39 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…
[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(-)
^ permalink raw reply
* [PATCH net-next v2 1/2] tcp: notify when a delayed ack is sent
From: Lawrence Brakmo @ 2018-07-02 21:39 UTC (permalink / raw)
To: netdev
Cc: Kernel Team, Blake Matheny, Alexei Starovoitov, Neal Cardwell,
Yuchung Cheng, Steve Ibanez, Eric Dumazet
In-Reply-To: <20180702213908.1246455-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
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