* [net-next 7/7] igbvf: Use net_device_stats from struct net_device
From: Jeff Kirsher @ 2017-04-20 23:33 UTC (permalink / raw)
To: davem; +Cc: Tobias Klauser, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170420233335.34900-1-jeffrey.t.kirsher@intel.com>
From: Tobias Klauser <tklauser@distanz.ch>
Instead of using a private copy of struct net_device_stats in
struct igbvf_adapter, use stats from struct net_device. Also remove the
now unnecessary .ndo_get_stats function.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/igbvf/igbvf.h | 1 -
drivers/net/ethernet/intel/igbvf/netdev.c | 26 +++++---------------------
2 files changed, 5 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/intel/igbvf/igbvf.h b/drivers/net/ethernet/intel/igbvf/igbvf.h
index 2a53ed84d9fc..bf69f01f8467 100644
--- a/drivers/net/ethernet/intel/igbvf/igbvf.h
+++ b/drivers/net/ethernet/intel/igbvf/igbvf.h
@@ -243,7 +243,6 @@ struct igbvf_adapter {
/* OS defined structs */
struct net_device *netdev;
struct pci_dev *pdev;
- struct net_device_stats net_stats;
spinlock_t stats_lock; /* prevent concurrent stats updates */
/* structs defined in e1000_hw.h */
diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index 6029854b6a70..1b9cbbe88f6f 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -400,8 +400,8 @@ static bool igbvf_clean_rx_irq(struct igbvf_adapter *adapter,
adapter->total_rx_packets += total_packets;
adapter->total_rx_bytes += total_bytes;
- adapter->net_stats.rx_bytes += total_bytes;
- adapter->net_stats.rx_packets += total_packets;
+ netdev->stats.rx_bytes += total_bytes;
+ netdev->stats.rx_packets += total_packets;
return cleaned;
}
@@ -864,8 +864,8 @@ static bool igbvf_clean_tx_irq(struct igbvf_ring *tx_ring)
}
}
- adapter->net_stats.tx_bytes += total_bytes;
- adapter->net_stats.tx_packets += total_packets;
+ netdev->stats.tx_bytes += total_bytes;
+ netdev->stats.tx_packets += total_packets;
return count < tx_ring->count;
}
@@ -1838,7 +1838,7 @@ void igbvf_update_stats(struct igbvf_adapter *adapter)
UPDATE_VF_COUNTER(VFGPRLBC, gprlbc);
/* Fill out the OS statistics structure */
- adapter->net_stats.multicast = adapter->stats.mprc;
+ adapter->netdev->stats.multicast = adapter->stats.mprc;
}
static void igbvf_print_link_info(struct igbvf_adapter *adapter)
@@ -2374,21 +2374,6 @@ static void igbvf_reset_task(struct work_struct *work)
}
/**
- * igbvf_get_stats - Get System Network Statistics
- * @netdev: network interface device structure
- *
- * Returns the address of the device statistics structure.
- * The statistics are actually updated from the timer callback.
- **/
-static struct net_device_stats *igbvf_get_stats(struct net_device *netdev)
-{
- struct igbvf_adapter *adapter = netdev_priv(netdev);
-
- /* only return the current stats */
- return &adapter->net_stats;
-}
-
-/**
* igbvf_change_mtu - Change the Maximum Transfer Unit
* @netdev: network interface device structure
* @new_mtu: new value for maximum frame size
@@ -2675,7 +2660,6 @@ static const struct net_device_ops igbvf_netdev_ops = {
.ndo_open = igbvf_open,
.ndo_stop = igbvf_close,
.ndo_start_xmit = igbvf_xmit_frame,
- .ndo_get_stats = igbvf_get_stats,
.ndo_set_rx_mode = igbvf_set_rx_mode,
.ndo_set_mac_address = igbvf_set_mac,
.ndo_change_mtu = igbvf_change_mtu,
--
2.12.2
^ permalink raw reply related
* [net-next 6/7] igb: Enable reading of wake up packet
From: Jeff Kirsher @ 2017-04-20 23:33 UTC (permalink / raw)
To: davem; +Cc: Kim Tatt Chuah, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170420233335.34900-1-jeffrey.t.kirsher@intel.com>
From: Kim Tatt Chuah <kim.tatt.chuah@intel.com>
Currently, in igb_resume(), igb driver ignores the Wake Up Status (WUS)
and Wake Up Packet Memory (WUPM) registers. This patch enables the igb
driver to read the WUPM if the controller was woken by a wake up packet
that is not more than 128 bytes long (maximum WUPM size), then pass it
up the kernel network stack.
Signed-off-by: Kim Tatt Chuah <kim.tatt.chuah@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/igb/e1000_defines.h | 21 +++++++++++++++
drivers/net/ethernet/intel/igb/igb_main.c | 36 +++++++++++++++++++++++++-
2 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index 8aee314332a8..d8517779439b 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -39,6 +39,27 @@
#define E1000_WUFC_MC 0x00000008 /* Directed Multicast Wakeup Enable */
#define E1000_WUFC_BC 0x00000010 /* Broadcast Wakeup Enable */
+/* Wake Up Status */
+#define E1000_WUS_EX 0x00000004 /* Directed Exact */
+#define E1000_WUS_ARPD 0x00000020 /* Directed ARP Request */
+#define E1000_WUS_IPV4 0x00000040 /* Directed IPv4 */
+#define E1000_WUS_IPV6 0x00000080 /* Directed IPv6 */
+#define E1000_WUS_NSD 0x00000400 /* Directed IPv6 Neighbor Solicitation */
+
+/* Packet types that are enabled for wake packet delivery */
+#define WAKE_PKT_WUS ( \
+ E1000_WUS_EX | \
+ E1000_WUS_ARPD | \
+ E1000_WUS_IPV4 | \
+ E1000_WUS_IPV6 | \
+ E1000_WUS_NSD)
+
+/* Wake Up Packet Length */
+#define E1000_WUPL_MASK 0x00000FFF
+
+/* Wake Up Packet Memory stores the first 128 bytes of the wake up packet */
+#define E1000_WUPM_BYTES 128
+
/* Extended Device Control */
#define E1000_CTRL_EXT_SDP2_DATA 0x00000040 /* Value of SW Defineable Pin 2 */
#define E1000_CTRL_EXT_SDP3_DATA 0x00000080 /* Value of SW Defineable Pin 3 */
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 53e66c87abaf..1cf74aa4ebd9 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -7985,6 +7985,36 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
return 0;
}
+static void igb_deliver_wake_packet(struct net_device *netdev)
+{
+ struct igb_adapter *adapter = netdev_priv(netdev);
+ struct e1000_hw *hw = &adapter->hw;
+ struct sk_buff *skb;
+ u32 wupl;
+
+ wupl = rd32(E1000_WUPL) & E1000_WUPL_MASK;
+
+ /* WUPM stores only the first 128 bytes of the wake packet.
+ * Read the packet only if we have the whole thing.
+ */
+ if ((wupl == 0) || (wupl > E1000_WUPM_BYTES))
+ return;
+
+ skb = netdev_alloc_skb_ip_align(netdev, E1000_WUPM_BYTES);
+ if (!skb)
+ return;
+
+ skb_put(skb, wupl);
+
+ /* Ensure reads are 32-bit aligned */
+ wupl = roundup(wupl, 4);
+
+ memcpy_fromio(skb->data, hw->hw_addr + E1000_WUPM_REG(0), wupl);
+
+ skb->protocol = eth_type_trans(skb, netdev);
+ netif_rx(skb);
+}
+
#ifdef CONFIG_PM
#ifdef CONFIG_PM_SLEEP
static int igb_suspend(struct device *dev)
@@ -8014,7 +8044,7 @@ static int igb_resume(struct device *dev)
struct net_device *netdev = pci_get_drvdata(pdev);
struct igb_adapter *adapter = netdev_priv(netdev);
struct e1000_hw *hw = &adapter->hw;
- u32 err;
+ u32 err, val;
pci_set_power_state(pdev, PCI_D0);
pci_restore_state(pdev);
@@ -8045,6 +8075,10 @@ static int igb_resume(struct device *dev)
*/
igb_get_hw_control(adapter);
+ val = rd32(E1000_WUS);
+ if (val & WAKE_PKT_WUS)
+ igb_deliver_wake_packet(netdev);
+
wr32(E1000_WUS, ~0);
rtnl_lock();
--
2.12.2
^ permalink raw reply related
* Re: [PATCH net] net: ipv6: RTF_PCPU should not be settable from userspace
From: Cong Wang @ 2017-04-20 23:37 UTC (permalink / raw)
To: David Ahern
Cc: Linux Kernel Network Developers, Martin KaFai Lau,
Andrey Konovalov
In-Reply-To: <c6f4f382-26ac-6679-adba-605e8f29cf6a@cumulusnetworks.com>
On Thu, Apr 20, 2017 at 3:43 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
>
> I scanned the others. It is not clear that others should fail with
> EINVAL. Certainly a mask of unused flags can be added, but to me that is
> on top of this bug fix.
>
If we want to preserve those unused bits, we should reject them too.
RTF_PCPU is special here, it is used but only internally, so it makes
sense to silently clear it since we don't care whether people set it to
1 or 0. We should clear it for dumping too since it is internal only.
^ permalink raw reply
* Re: [PATCH net] net: ipv6: RTF_PCPU should not be settable from userspace
From: Martin KaFai Lau @ 2017-04-20 23:38 UTC (permalink / raw)
To: David Ahern; +Cc: Cong Wang, Linux Kernel Network Developers, Andrey Konovalov
In-Reply-To: <c6f4f382-26ac-6679-adba-605e8f29cf6a@cumulusnetworks.com>
On Thu, Apr 20, 2017 at 04:43:03PM -0600, David Ahern wrote:
> On 4/20/17 4:39 PM, Cong Wang wrote:
> > On Wed, Apr 19, 2017 at 2:19 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> >>
> >> Fix by checking for the flag and failing with EINVAL.
> >>
> >
> > I am still not sure about this. There are a few unused bits in
> > this flag, we simply ignore the rest, right? Why should we
> > reject this one instead of all of those we don't use?
> >
>
> RTF_PCPU most definitely should not be set by userspace.
+1
^ permalink raw reply
* Re: [PATCH v3 net-next 5/8] net/ncsi: Dump NCSI packet statistics
From: Gavin Shan @ 2017-04-20 23:38 UTC (permalink / raw)
To: David Miller; +Cc: gwshan, netdev, joe, kubakici
In-Reply-To: <20170420.132103.454438573956782815.davem@davemloft.net>
On Thu, Apr 20, 2017 at 01:21:03PM -0400, David Miller wrote:
>From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>Date: Tue, 18 Apr 2017 16:51:32 +1000
>
>> This creates /sys/kernel/debug/ncsi/<eth0>/stats to dump the NCSI
>> packets sent and received over all packages and channels. It's useful
>> to diagnose NCSI problems, especially when NCSI packages and channels
>> aren't probed properly. The statistics can be gained from debugfs file
>> as below:
>>
>> # cat /sys/kernel/debug/ncsi/eth0/stats
>
>There is no reason you cannot use ethtool statistics to provide this
>information to the user.
>
It can be dumped by ethtool, but it's more reasonable to dump them
through debugfs for couple of reasons: (1) ethtool usually dumps
statistics collected by hardware, but this debugfs file dumps the
statistics of packets seen (collected) by software. They are different
things. Note that NCSI channel collects statistics in hardware and it's
not exposed or dumped by this patchset. They are candidates for ethtool.
(2) To expose this through ethtool relies on the availability of the tool.
It's nicer not to depend on it. (3) This interface can be used to check
the debug packet has been sent successfully through /sys/kernel/debug/ncsi/eth0/pkt,
dumping the statistics through debugfs make this (debugging) mechanism
consistent.
Thanks,
Gavin
^ permalink raw reply
* [PATCH v2] net: core: Prevent from dereferencing null pointer when
From: Myungho Jung @ 2017-04-20 23:59 UTC (permalink / raw)
To: edumazet; +Cc: netdev
Added NULL check to make __dev_kfree_skb_irq consistent with kfree
family of functions.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=195289
Signed-off-by: Myungho Jung <mhjungk@gmail.com>
---
Changes in v2:
- Correct category in subject
net/core/dev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/core/dev.c b/net/core/dev.c
index 7869ae3..22be2a6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2450,6 +2450,9 @@ void __dev_kfree_skb_irq(struct sk_buff *skb, enum skb_free_reason reason)
{
unsigned long flags;
+ if (unlikely(!skb))
+ return;
+
if (likely(atomic_read(&skb->users) == 1)) {
smp_rmb();
atomic_set(&skb->users, 0);
--
2.7.4
^ permalink raw reply related
* RE: netlink: question about netlink testsuite
From: Yasuhara, Hiroyuki @ 2017-04-21 0:12 UTC (permalink / raw)
To: 'Cong Wang'; +Cc: netdev@vger.kernel.org
In-Reply-To: <CAM_iQpVZ8f2Dc6xmLW3uzHe1AY+E3k-6cUXjnY678OCPmqAySg@mail.gmail.com>
Hi,
> -----Original Message-----
> From: Cong Wang [mailto:xiyou.wangcong@gmail.com]
> Sent: Wednesday, April 19, 2017 3:06 PM
> Hello,
>
> On Tue, Apr 18, 2017 at 10:00 PM, Yasuhara, Hiroyuki
> <h.yasuhara@jp.fujitsu.com> wrote:
> > Hi,
> >
> > I'm looking for a testset about netlink function,
> > for example, making a lot of netlink sockets and
> > communicating with them in the testset.
> >
> > If anyone know such a testset, please give me the information.
> >
>
> libnl3 contains some test cases, which may be what you
> are looking for:
>
> https://github.com/thom311/libnl/tree/master/tests
Thank you for the information.
I'll try it.
Regards,
Yasuhara
^ permalink raw reply
* Re: [PATCH net] net: ipv6: RTF_PCPU should not be settable from userspace
From: Martin KaFai Lau @ 2017-04-21 0:16 UTC (permalink / raw)
To: Cong Wang; +Cc: David Ahern, Linux Kernel Network Developers, Andrey Konovalov
In-Reply-To: <CAM_iQpVOGq_ePfuTdXWharY8taHrPtW4rEeqomhZ1R8g75nXTQ@mail.gmail.com>
On Thu, Apr 20, 2017 at 04:37:18PM -0700, Cong Wang wrote:
> On Thu, Apr 20, 2017 at 3:43 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> >
> > I scanned the others. It is not clear that others should fail with
> > EINVAL. Certainly a mask of unused flags can be added, but to me that is
> > on top of this bug fix.
> >
>
> If we want to preserve those unused bits, we should reject them too.
>
> RTF_PCPU is special here, it is used but only internally, so it makes
> sense to silently clear it since we don't care whether people set it to
> 1 or 0. We should clear it for dumping too since it is internal only.
I agree with DavidA. The existing bits (including RTF_PCPU) during dumping
is part of the uapi already. We cannot stop displaying them now.
Silently accepting something instead of telling the userspace program
has a bug seems to be a dis-service to the end-user.
If there are other bits should be rejected too, they can be
done in the follow up patches.
^ permalink raw reply
* [PATCH] qed: fix kzalloc-simple.cocci warnings
From: kbuild test robot @ 2017-04-21 0:20 UTC (permalink / raw)
To: sudarsana.kalluru@cavium.com
Cc: kbuild-all, netdev, Sudarsana Reddy Kalluru, Yuval Mintz,
Ariel Elior, everest-linux-l2, linux-kernel
In-Reply-To: <201704210853.tUPLU4Mh%fengguang.wu@intel.com>
drivers/net/ethernet/qlogic/qed/qed_dcbx.c:1267:13-20: WARNING: kzalloc should be used for dcbx_info, instead of kmalloc/memset
Use kzalloc rather than kmalloc followed by memset with 0
This considers some simple cases that are common and easy to validate
Note in particular that there are no ...s in the rule, so all of the
matched code has to be contiguous
Generated by: scripts/coccinelle/api/alloc/kzalloc-simple.cocci
CC: sudarsana.kalluru@cavium.com <sudarsana.kalluru@cavium.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---
qed_dcbx.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
--- a/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dcbx.c
@@ -1264,11 +1264,10 @@ static struct qed_dcbx_get *qed_dcbnl_ge
{
struct qed_dcbx_get *dcbx_info;
- dcbx_info = kmalloc(sizeof(*dcbx_info), GFP_ATOMIC);
+ dcbx_info = kzalloc(sizeof(*dcbx_info), GFP_ATOMIC);
if (!dcbx_info)
return NULL;
- memset(dcbx_info, 0, sizeof(*dcbx_info));
if (qed_dcbx_query_params(hwfn, dcbx_info, type)) {
kfree(dcbx_info);
return NULL;
^ permalink raw reply
* [net:master 15/18] drivers/net/ethernet/qlogic/qed/qed_dcbx.c:1267:13-20: WARNING: kzalloc should be used for dcbx_info, instead of kmalloc/memset
From: kbuild test robot @ 2017-04-21 0:20 UTC (permalink / raw)
To: sudarsana.kalluru@cavium.com
Cc: kbuild-all, netdev, Sudarsana Reddy Kalluru, Yuval Mintz
tree: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git master
head: e0535ce58b92d7baf0b33284a6c4f8f0338f943e
commit: 62289ba27558553871fd047baadaaeda886c6a63 [15/18] qed: Fix possible system hang in the dcbnl-getdcbx() path.
coccinelle warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/qlogic/qed/qed_dcbx.c:1267:13-20: WARNING: kzalloc should be used for dcbx_info, instead of kmalloc/memset
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply
* Re: [PATCH v3 net-next 5/8] net/ncsi: Dump NCSI packet statistics
From: Florian Fainelli @ 2017-04-21 0:26 UTC (permalink / raw)
To: Gavin Shan, David Miller; +Cc: netdev, joe, kubakici
In-Reply-To: <20170420233812.GA7223@gwshan>
On 04/20/2017 04:38 PM, Gavin Shan wrote:
> On Thu, Apr 20, 2017 at 01:21:03PM -0400, David Miller wrote:
>> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>> Date: Tue, 18 Apr 2017 16:51:32 +1000
>>
>>> This creates /sys/kernel/debug/ncsi/<eth0>/stats to dump the NCSI
>>> packets sent and received over all packages and channels. It's useful
>>> to diagnose NCSI problems, especially when NCSI packages and channels
>>> aren't probed properly. The statistics can be gained from debugfs file
>>> as below:
>>>
>>> # cat /sys/kernel/debug/ncsi/eth0/stats
>>
>> There is no reason you cannot use ethtool statistics to provide this
>> information to the user.
>>
>
> It can be dumped by ethtool, but it's more reasonable to dump them
> through debugfs for couple of reasons: (1) ethtool usually dumps
> statistics collected by hardware, but this debugfs file dumps the
> statistics of packets seen (collected) by software. They are different
> things. Note that NCSI channel collects statistics in hardware and it's
> not exposed or dumped by this patchset. They are candidates for ethtool.
> (2) To expose this through ethtool relies on the availability of the tool.
> It's nicer not to depend on it. (3) This interface can be used to check
> the debug packet has been sent successfully through /sys/kernel/debug/ncsi/eth0/pkt,
> dumping the statistics through debugfs make this (debugging) mechanism
> consistent.
Can't you create a ncsi folder under /sys/class/net/eth0/nsci/ and then
put your stats in there? That would at least look slightly consistent
with what is already existing for the non-NC-SI networking stack.
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 1/3] net/tcp_fastopen: Disable active side TFO in certain scenarios
From: Yuchung Cheng @ 2017-04-21 0:29 UTC (permalink / raw)
To: Wei Wang; +Cc: netdev, David Miller, Eric Dumazet
In-Reply-To: <20170420214548.23666-2-tracywwnj@gmail.com>
On Thu, Apr 20, 2017 at 2:45 PM, Wei Wang <weiwan@google.com> wrote:
> From: Wei Wang <weiwan@google.com>
>
> Middlebox firewall issues can potentially cause server's data being
> blackholed after a successful 3WHS using TFO. Following are the related
> reports from Apple:
> https://www.nanog.org/sites/default/files/Paasch_Network_Support.pdf
> Slide 31 identifies an issue where the client ACK to the server's data
> sent during a TFO'd handshake is dropped.
> C ---> syn-data ---> S
> C <--- syn/ack ----- S
> C (accept & write)
> C <---- data ------- S
> C ----- ACK -> X S
> [retry and timeout]
>
> https://www.ietf.org/proceedings/94/slides/slides-94-tcpm-13.pdf
> Slide 5 shows a similar situation that the server's data gets dropped
> after 3WHS.
> C ---- syn-data ---> S
> C <--- syn/ack ----- S
> C ---- ack --------> S
> S (accept & write)
> C? X <- data ------ S
> [retry and timeout]
>
> This is the worst failure b/c the client can not detect such behavior to
> mitigate the situation (such as disabling TFO). Failing to proceed, the
> application (e.g., SSL library) may simply timeout and retry with TFO
> again, and the process repeats indefinitely.
>
> The proposed solution is to disable active TFO globally under the
> following circumstances:
> 1. client side TFO socket detects out of order FIN
> 2. client side TFO socket receives out of order RST
>
> We disable active side TFO globally for 1hr at first. Then if it
> happens again, we disable it for 2h, then 4h, 8h, ...
> And we reset the timeout to 1hr if a client side TFO sockets not opened
> on loopback has successfully received data segs from server.
> And we examine this condition during close().
>
> The rational behind it is that when such firewall issue happens,
> application running on the client should eventually close the socket as
> it is not able to get the data it is expecting. Or application running
> on the server should close the socket as it is not able to receive any
> response from client.
> In both cases, out of order FIN or RST will get received on the client
> given that the firewall will not block them as no data are in those
> frames.
> And we want to disable active TFO globally as it helps if the middle box
> is very close to the client and most of the connections are likely to
> fail.
>
> Also, add a debug sysctl:
> tcp_fastopen_blackhole_detect_timeout_sec:
> the initial timeout to use when firewall blackhole issue happens.
> This can be set and read.
> When setting it to 0, it means to disable the active disable logic.
>
> Signed-off-by: Wei Wang <weiwan@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
> ---
> Documentation/networking/ip-sysctl.txt | 8 +++
> include/linux/tcp.h | 1 +
> include/net/tcp.h | 6 ++
> net/ipv4/sysctl_net_ipv4.c | 21 +++++++
> net/ipv4/tcp.c | 1 +
> net/ipv4/tcp_fastopen.c | 101 +++++++++++++++++++++++++++++++++
> net/ipv4/tcp_input.c | 23 ++++++--
> net/ipv4/tcp_ipv4.c | 3 +
> 8 files changed, 160 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
> index b1c6500e7a8d..974ab47ae53a 100644
> --- a/Documentation/networking/ip-sysctl.txt
> +++ b/Documentation/networking/ip-sysctl.txt
> @@ -602,6 +602,14 @@ tcp_fastopen - INTEGER
> Note that that additional client or server features are only
> effective if the basic support (0x1 and 0x2) are enabled respectively.
>
> +tcp_fastopen_blackhole_timeout_sec - INTEGER
> + Initial time period in second to disable Fastopen on active TCP sockets
> + when a TFO firewall blackhole issue happens.
> + This time period will grow exponentially when more blackhole issues
> + get detected right after Fastopen is re-enabled and will reset to
> + initial value when the blackhole issue goes away.
> + By default, it is set to 1hr.
> +
> tcp_syn_retries - INTEGER
> Number of times initial SYNs for an active TCP connection attempt
> will be retransmitted. Should not be higher than 127. Default value
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index cfc2d9506ce8..cbe5b602a2d3 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -233,6 +233,7 @@ struct tcp_sock {
> u8 syn_data:1, /* SYN includes data */
> syn_fastopen:1, /* SYN includes Fast Open option */
> syn_fastopen_exp:1,/* SYN includes Fast Open exp. option */
> + syn_fastopen_ch:1, /* Active TFO re-enabling probe */
> syn_data_acked:1,/* data in SYN is acked by SYN-ACK */
> save_syn:1, /* Save headers of SYN packet */
> is_cwnd_limited:1;/* forward progress limited by snd_cwnd? */
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index cc6ae0a95201..c1abc2abbdcb 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1506,6 +1506,12 @@ struct tcp_fastopen_context {
> struct rcu_head rcu;
> };
>
> +extern unsigned int sysctl_tcp_fastopen_blackhole_timeout;
> +void tcp_fastopen_active_disable(void);
> +bool tcp_fastopen_active_should_disable(struct sock *sk);
> +void tcp_fastopen_active_disable_ofo_check(struct sock *sk);
> +void tcp_fastopen_active_timeout_reset(void);
> +
> /* Latencies incurred by various limits for a sender. They are
> * chronograph-like stats that are mutually exclusive.
> */
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index 6fb25693c00b..52fa0a6b78ff 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -347,6 +347,19 @@ static int proc_udp_early_demux(struct ctl_table *table, int write,
> return ret;
> }
>
> +static int proc_tfo_blackhole_detect_timeout(struct ctl_table *table,
> + int write,
> + void __user *buffer,
> + size_t *lenp, loff_t *ppos)
> +{
> + int ret;
> +
> + ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> + if (write && ret == 0)
> + tcp_fastopen_active_timeout_reset();
> + return ret;
> +}
> +
> static struct ctl_table ipv4_table[] = {
> {
> .procname = "tcp_timestamps",
> @@ -397,6 +410,14 @@ static struct ctl_table ipv4_table[] = {
> .proc_handler = proc_tcp_fastopen_key,
> },
> {
> + .procname = "tcp_fastopen_blackhole_timeout_sec",
> + .data = &sysctl_tcp_fastopen_blackhole_timeout,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_tfo_blackhole_detect_timeout,
> + .extra1 = &zero,
> + },
> + {
> .procname = "tcp_abort_on_overflow",
> .data = &sysctl_tcp_abort_on_overflow,
> .maxlen = sizeof(int),
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 04843ae77b9e..efc976ae66ae 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2296,6 +2296,7 @@ int tcp_disconnect(struct sock *sk, int flags)
> tcp_clear_xmit_timers(sk);
> __skb_queue_purge(&sk->sk_receive_queue);
> tcp_write_queue_purge(sk);
> + tcp_fastopen_active_disable_ofo_check(sk);
> skb_rbtree_purge(&tp->out_of_order_queue);
>
> inet->inet_dport = 0;
> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index 8ea4e9787f82..ff2d30ffc6f3 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -341,6 +341,13 @@ bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
> cookie->len = -1;
> return false;
> }
> +
> + /* Firewall blackhole issue check */
> + if (tcp_fastopen_active_should_disable(sk)) {
> + cookie->len = -1;
> + return false;
> + }
> +
> if (sysctl_tcp_fastopen & TFO_CLIENT_NO_COOKIE) {
> cookie->len = -1;
> return true;
> @@ -380,3 +387,97 @@ bool tcp_fastopen_defer_connect(struct sock *sk, int *err)
> return false;
> }
> EXPORT_SYMBOL(tcp_fastopen_defer_connect);
> +
> +/*
> + * The following code block is to deal with middle box issues with TFO:
> + * Middlebox firewall issues can potentially cause server's data being
> + * blackholed after a successful 3WHS using TFO.
> + * The proposed solution is to disable active TFO globally under the
> + * following circumstances:
> + * 1. client side TFO socket receives out of order FIN
> + * 2. client side TFO socket receives out of order RST
> + * We disable active side TFO globally for 1hr at first. Then if it
> + * happens again, we disable it for 2h, then 4h, 8h, ...
> + * And we reset the timeout back to 1hr when we see a successful active
> + * TFO connection with data exchanges.
> + */
> +
> +/* Default to 1hr */
> +unsigned int sysctl_tcp_fastopen_blackhole_timeout __read_mostly = 60 * 60;
> +static atomic_t tfo_active_disable_times __read_mostly = ATOMIC_INIT(0);
> +static unsigned long tfo_active_disable_stamp __read_mostly;
> +
> +/* Disable active TFO and record current jiffies and
> + * tfo_active_disable_times
> + */
> +void tcp_fastopen_active_disable(void)
> +{
> + atomic_inc(&tfo_active_disable_times);
> + tfo_active_disable_stamp = jiffies;
> +}
> +
> +/* Reset tfo_active_disable_times to 0 */
> +void tcp_fastopen_active_timeout_reset(void)
> +{
> + atomic_set(&tfo_active_disable_times, 0);
> +}
> +
> +/* Calculate timeout for tfo active disable
> + * Return true if we are still in the active TFO disable period
> + * Return false if timeout already expired and we should use active TFO
> + */
> +bool tcp_fastopen_active_should_disable(struct sock *sk)
> +{
> + int tfo_da_times = atomic_read(&tfo_active_disable_times);
> + int multiplier;
> + unsigned long timeout;
> +
> + if (!tfo_da_times)
> + return false;
> +
> + /* Limit timout to max: 2^6 * initial timeout */
> + multiplier = 1 << min(tfo_da_times - 1, 6);
> + timeout = multiplier * sysctl_tcp_fastopen_blackhole_timeout * HZ;
> + if (time_before(jiffies, tfo_active_disable_stamp + timeout))
> + return true;
> +
> + /* Mark check bit so we can check for successful active TFO
> + * condition and reset tfo_active_disable_times
> + */
> + tcp_sk(sk)->syn_fastopen_ch = 1;
> + return false;
> +}
> +
> +/* Disable active TFO if FIN is the only packet in the ofo queue
> + * and no data is received.
> + * Also check if we can reset tfo_active_disable_times if data is
> + * received successfully on a marked active TFO sockets opened on
> + * a non-loopback interface
> + */
> +void tcp_fastopen_active_disable_ofo_check(struct sock *sk)
> +{
> + struct tcp_sock *tp = tcp_sk(sk);
> + struct rb_node *p;
> + struct sk_buff *skb;
> + struct dst_entry *dst;
> +
> + if (!tp->syn_fastopen)
> + return;
> +
> + if (!tp->data_segs_in) {
> + p = rb_first(&tp->out_of_order_queue);
> + if (p && !rb_next(p)) {
> + skb = rb_entry(p, struct sk_buff, rbnode);
> + if (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN) {
> + tcp_fastopen_active_disable();
> + return;
> + }
> + }
> + } else if (tp->syn_fastopen_ch &&
> + atomic_read(&tfo_active_disable_times)) {
> + dst = sk_dst_get(sk);
> + if (!(dst && dst->dev && (dst->dev->flags & IFF_LOOPBACK)))
> + tcp_fastopen_active_timeout_reset();
> + dst_release(dst);
> + }
> +}
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 341f021f02a2..9f342a67dc74 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5300,8 +5300,16 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>
> if (rst_seq_match)
> tcp_reset(sk);
> - else
> + else {
> + /* Disable TFO if RST is out-of-order
> + * and no data has been received
> + * for current active TFO socket
> + */
> + if (tp->syn_fastopen && !tp->data_segs_in &&
> + sk->sk_state == TCP_ESTABLISHED)
> + tcp_fastopen_active_disable();
> tcp_send_challenge_ack(sk, skb);
> + }
> goto discard;
> }
>
> @@ -6044,9 +6052,16 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
> break;
> }
>
> - if (tp->linger2 < 0 ||
> - (TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(skb)->seq &&
> - after(TCP_SKB_CB(skb)->end_seq - th->fin, tp->rcv_nxt))) {
> + if (tp->linger2 < 0) {
> + tcp_done(sk);
> + NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONDATA);
> + return 1;
> + }
> + if (TCP_SKB_CB(skb)->end_seq != TCP_SKB_CB(skb)->seq &&
> + after(TCP_SKB_CB(skb)->end_seq - th->fin, tp->rcv_nxt)) {
> + /* Receive out of order FIN after close() */
> + if (tp->syn_fastopen && th->fin)
> + tcp_fastopen_active_disable();
> tcp_done(sk);
> NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONDATA);
> return 1;
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 20cbd2f07f28..cbbafe546c0f 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1855,6 +1855,9 @@ void tcp_v4_destroy_sock(struct sock *sk)
> /* Cleanup up the write buffer. */
> tcp_write_queue_purge(sk);
>
> + /* Check if we want to disable active TFO */
> + tcp_fastopen_active_disable_ofo_check(sk);
> +
> /* Cleans up our, hopefully empty, out_of_order_queue. */
> skb_rbtree_purge(&tp->out_of_order_queue);
>
> --
> 2.12.2.816.g2cccc81164-goog
>
^ permalink raw reply
* Re: [PATCH net-next 3/3] net/tcp_fastopen: Remove mss check in tcp_write_timeout()
From: Yuchung Cheng @ 2017-04-21 0:29 UTC (permalink / raw)
To: Wei Wang; +Cc: netdev, David Miller, Eric Dumazet
In-Reply-To: <20170420214548.23666-4-tracywwnj@gmail.com>
On Thu, Apr 20, 2017 at 2:45 PM, Wei Wang <weiwan@google.com> wrote:
> From: Wei Wang <weiwan@google.com>
>
> Christoph Paasch from Apple found another firewall issue for TFO:
> After successful 3WHS using TFO, server and client starts to exchange
> data. Afterwards, a 10s idle time occurs on this connection. After that,
> firewall starts to drop every packet on this connection.
>
> The fix for this issue is to extend existing firewall blackhole detection
> logic in tcp_write_timeout() by removing the mss check.
>
> Signed-off-by: Wei Wang <weiwan@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
> ---
> net/ipv4/tcp_timer.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
> index b2ab411c6d37..14672543cf0b 100644
> --- a/net/ipv4/tcp_timer.c
> +++ b/net/ipv4/tcp_timer.c
> @@ -201,11 +201,10 @@ static int tcp_write_timeout(struct sock *sk)
> if (retransmits_timed_out(sk, net->ipv4.sysctl_tcp_retries1, 0, 0)) {
> /* Some middle-boxes may black-hole Fast Open _after_
> * the handshake. Therefore we conservatively disable
> - * Fast Open on this path on recurring timeouts with
> - * few or zero bytes acked after Fast Open.
> + * Fast Open on this path on recurring timeouts after
> + * successful Fast Open.
> */
> - if (tp->syn_data_acked &&
> - tp->bytes_acked <= tp->rx_opt.mss_clamp) {
> + if (tp->syn_data_acked) {
> tcp_fastopen_cache_set(sk, 0, NULL, true, 0);
> if (icsk->icsk_retransmits == net->ipv4.sysctl_tcp_retries1)
> NET_INC_STATS(sock_net(sk),
> --
> 2.12.2.816.g2cccc81164-goog
>
^ permalink raw reply
* Re: [PATCH net-next 2/3] net/tcp_fastopen: Add snmp counter for blackhole detection
From: Yuchung Cheng @ 2017-04-21 0:30 UTC (permalink / raw)
To: Wei Wang; +Cc: netdev, David Miller, Eric Dumazet
In-Reply-To: <20170420214548.23666-3-tracywwnj@gmail.com>
On Thu, Apr 20, 2017 at 2:45 PM, Wei Wang <weiwan@google.com> wrote:
> From: Wei Wang <weiwan@google.com>
>
> This counter records the number of times the firewall blackhole issue is
> detected and active TFO is disabled.
>
> Signed-off-by: Wei Wang <weiwan@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
> ---
> include/net/tcp.h | 2 +-
> include/uapi/linux/snmp.h | 1 +
> net/ipv4/proc.c | 1 +
> net/ipv4/tcp_fastopen.c | 5 +++--
> net/ipv4/tcp_input.c | 4 ++--
> 5 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index c1abc2abbdcb..da28bef1d82b 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1507,7 +1507,7 @@ struct tcp_fastopen_context {
> };
>
> extern unsigned int sysctl_tcp_fastopen_blackhole_timeout;
> -void tcp_fastopen_active_disable(void);
> +void tcp_fastopen_active_disable(struct sock *sk);
> bool tcp_fastopen_active_should_disable(struct sock *sk);
> void tcp_fastopen_active_disable_ofo_check(struct sock *sk);
> void tcp_fastopen_active_timeout_reset(void);
> diff --git a/include/uapi/linux/snmp.h b/include/uapi/linux/snmp.h
> index cec0e171d20c..95cffcb21dfd 100644
> --- a/include/uapi/linux/snmp.h
> +++ b/include/uapi/linux/snmp.h
> @@ -259,6 +259,7 @@ enum
> LINUX_MIB_TCPFASTOPENPASSIVEFAIL, /* TCPFastOpenPassiveFail */
> LINUX_MIB_TCPFASTOPENLISTENOVERFLOW, /* TCPFastOpenListenOverflow */
> LINUX_MIB_TCPFASTOPENCOOKIEREQD, /* TCPFastOpenCookieReqd */
> + LINUX_MIB_TCPFASTOPENBLACKHOLE, /* TCPFastOpenBlackholeDetect */
/* TCPFastOpenBlackhole */
> LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES, /* TCPSpuriousRtxHostQueues */
> LINUX_MIB_BUSYPOLLRXPACKETS, /* BusyPollRxPackets */
> LINUX_MIB_TCPAUTOCORKING, /* TCPAutoCorking */
> diff --git a/net/ipv4/proc.c b/net/ipv4/proc.c
> index 4ccbf464d1ac..fa44e752a9a3 100644
> --- a/net/ipv4/proc.c
> +++ b/net/ipv4/proc.c
> @@ -281,6 +281,7 @@ static const struct snmp_mib snmp4_net_list[] = {
> SNMP_MIB_ITEM("TCPFastOpenPassiveFail", LINUX_MIB_TCPFASTOPENPASSIVEFAIL),
> SNMP_MIB_ITEM("TCPFastOpenListenOverflow", LINUX_MIB_TCPFASTOPENLISTENOVERFLOW),
> SNMP_MIB_ITEM("TCPFastOpenCookieReqd", LINUX_MIB_TCPFASTOPENCOOKIEREQD),
> + SNMP_MIB_ITEM("TCPFastOpenBlackhole", LINUX_MIB_TCPFASTOPENBLACKHOLE),
> SNMP_MIB_ITEM("TCPSpuriousRtxHostQueues", LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES),
> SNMP_MIB_ITEM("BusyPollRxPackets", LINUX_MIB_BUSYPOLLRXPACKETS),
> SNMP_MIB_ITEM("TCPAutoCorking", LINUX_MIB_TCPAUTOCORKING),
> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index ff2d30ffc6f3..4af82b914dd4 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -410,10 +410,11 @@ static unsigned long tfo_active_disable_stamp __read_mostly;
> /* Disable active TFO and record current jiffies and
> * tfo_active_disable_times
> */
> -void tcp_fastopen_active_disable(void)
> +void tcp_fastopen_active_disable(struct sock *sk)
> {
> atomic_inc(&tfo_active_disable_times);
> tfo_active_disable_stamp = jiffies;
> + NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPFASTOPENBLACKHOLE);
> }
>
> /* Reset tfo_active_disable_times to 0 */
> @@ -469,7 +470,7 @@ void tcp_fastopen_active_disable_ofo_check(struct sock *sk)
> if (p && !rb_next(p)) {
> skb = rb_entry(p, struct sk_buff, rbnode);
> if (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN) {
> - tcp_fastopen_active_disable();
> + tcp_fastopen_active_disable(sk);
> return;
> }
> }
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 9f342a67dc74..5af2f04f8859 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5307,7 +5307,7 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
> */
> if (tp->syn_fastopen && !tp->data_segs_in &&
> sk->sk_state == TCP_ESTABLISHED)
> - tcp_fastopen_active_disable();
> + tcp_fastopen_active_disable(sk);
> tcp_send_challenge_ack(sk, skb);
> }
> goto discard;
> @@ -6061,7 +6061,7 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
> after(TCP_SKB_CB(skb)->end_seq - th->fin, tp->rcv_nxt)) {
> /* Receive out of order FIN after close() */
> if (tp->syn_fastopen && th->fin)
> - tcp_fastopen_active_disable();
> + tcp_fastopen_active_disable(sk);
> tcp_done(sk);
> NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPABORTONDATA);
> return 1;
> --
> 2.12.2.816.g2cccc81164-goog
>
^ permalink raw reply
* Re: [PATCH v3 net-next 5/8] net/ncsi: Dump NCSI packet statistics
From: Gavin Shan @ 2017-04-21 0:44 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Gavin Shan, David Miller, netdev, joe, kubakici
In-Reply-To: <0289f0b2-36a8-f4a0-ef15-808aa66528fa@gmail.com>
On Thu, Apr 20, 2017 at 05:26:14PM -0700, Florian Fainelli wrote:
>On 04/20/2017 04:38 PM, Gavin Shan wrote:
>> On Thu, Apr 20, 2017 at 01:21:03PM -0400, David Miller wrote:
>>> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>> Date: Tue, 18 Apr 2017 16:51:32 +1000
>>>
>>>> This creates /sys/kernel/debug/ncsi/<eth0>/stats to dump the NCSI
>>>> packets sent and received over all packages and channels. It's useful
>>>> to diagnose NCSI problems, especially when NCSI packages and channels
>>>> aren't probed properly. The statistics can be gained from debugfs file
>>>> as below:
>>>>
>>>> # cat /sys/kernel/debug/ncsi/eth0/stats
>>>
>>> There is no reason you cannot use ethtool statistics to provide this
>>> information to the user.
>>>
>>
>> It can be dumped by ethtool, but it's more reasonable to dump them
>> through debugfs for couple of reasons: (1) ethtool usually dumps
>> statistics collected by hardware, but this debugfs file dumps the
>> statistics of packets seen (collected) by software. They are different
>> things. Note that NCSI channel collects statistics in hardware and it's
>> not exposed or dumped by this patchset. They are candidates for ethtool.
>> (2) To expose this through ethtool relies on the availability of the tool.
>> It's nicer not to depend on it. (3) This interface can be used to check
>> the debug packet has been sent successfully through /sys/kernel/debug/ncsi/eth0/pkt,
>> dumping the statistics through debugfs make this (debugging) mechanism
>> consistent.
>
>Can't you create a ncsi folder under /sys/class/net/eth0/nsci/ and then
>put your stats in there? That would at least look slightly consistent
>with what is already existing for the non-NC-SI networking stack.
Do you think it's good place to have /sys/class/net/eth0/ncsi/pkt?
Note this file accepts commands to send NCSI comands and dumps the
corresponding response on read. Also, it's a debugging interface
and disabled (invisible) for the most time. I think all directories
and files in /sys/class/net/eth0/ should be visible and the structure
is stable all the time.
Actually, I used procfs initially and replaced by debugfs according
to Joe's suggestion. I think debugfs is good enough for this case.
Cheers,
Gavin
^ permalink raw reply
* Re: [PATCH v3 net-next 5/8] net/ncsi: Dump NCSI packet statistics
From: David Miller @ 2017-04-21 0:58 UTC (permalink / raw)
To: gwshan; +Cc: netdev, joe, kubakici
In-Reply-To: <20170420233812.GA7223@gwshan>
From: Gavin Shan <gwshan@linux.vnet.ibm.com>
Date: Fri, 21 Apr 2017 09:38:12 +1000
> (1) ethtool usually dumps statistics collected by hardware, but this
> debugfs file dumps the statistics of packets seen (collected) by
> software.
ethtool is not strictly for hardware statistics, it's often used for
software maintained values
^ permalink raw reply
* Re: [PATCH v3 net-next 5/8] net/ncsi: Dump NCSI packet statistics
From: Florian Fainelli @ 2017-04-21 0:59 UTC (permalink / raw)
To: Gavin Shan; +Cc: David Miller, netdev, joe, kubakici
In-Reply-To: <20170421004435.GA18727@gwshan>
On 04/20/2017 05:44 PM, Gavin Shan wrote:
> On Thu, Apr 20, 2017 at 05:26:14PM -0700, Florian Fainelli wrote:
>> On 04/20/2017 04:38 PM, Gavin Shan wrote:
>>> On Thu, Apr 20, 2017 at 01:21:03PM -0400, David Miller wrote:
>>>> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>> Date: Tue, 18 Apr 2017 16:51:32 +1000
>>>>
>>>>> This creates /sys/kernel/debug/ncsi/<eth0>/stats to dump the NCSI
>>>>> packets sent and received over all packages and channels. It's useful
>>>>> to diagnose NCSI problems, especially when NCSI packages and channels
>>>>> aren't probed properly. The statistics can be gained from debugfs file
>>>>> as below:
>>>>>
>>>>> # cat /sys/kernel/debug/ncsi/eth0/stats
>>>>
>>>> There is no reason you cannot use ethtool statistics to provide this
>>>> information to the user.
>>>>
>>>
>>> It can be dumped by ethtool, but it's more reasonable to dump them
>>> through debugfs for couple of reasons: (1) ethtool usually dumps
>>> statistics collected by hardware, but this debugfs file dumps the
>>> statistics of packets seen (collected) by software. They are different
>>> things. Note that NCSI channel collects statistics in hardware and it's
>>> not exposed or dumped by this patchset. They are candidates for ethtool.
>>> (2) To expose this through ethtool relies on the availability of the tool.
>>> It's nicer not to depend on it. (3) This interface can be used to check
>>> the debug packet has been sent successfully through /sys/kernel/debug/ncsi/eth0/pkt,
>>> dumping the statistics through debugfs make this (debugging) mechanism
>>> consistent.
>>
>> Can't you create a ncsi folder under /sys/class/net/eth0/nsci/ and then
>> put your stats in there? That would at least look slightly consistent
>> with what is already existing for the non-NC-SI networking stack.
>
> Do you think it's good place to have /sys/class/net/eth0/ncsi/pkt?
> Note this file accepts commands to send NCSI comands and dumps the
> corresponding response on read. Also, it's a debugging interface
> and disabled (invisible) for the most time. I think all directories
> and files in /sys/class/net/eth0/ should be visible and the structure
> is stable all the time.
Statistics should not be debugging features having them all the time is
invaluable, so in that regard they could probably be part of a sysfs
interface of some kind. If this "pkt" file is special, then yes, maybe
debugfs is appropriate here.
It sounds like you may also want to consider doing a NC-SI generic
netlink family at some point, because chances are that are you going to
export more and more information over time, just like there could be
additional commands/actions to be performed as well as asynchronous
events. Netlink is great for that, downside is that you have to write a
custom user-space tool (or extend iproute2).
--
Florian
^ permalink raw reply
* Re: [PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.
From: Andrew Lunn @ 2017-04-21 1:12 UTC (permalink / raw)
To: Roger Quadros
Cc: davem, Florian Fainelli, tony, nsekhar, jsarha, netdev,
linux-omap, linux-kernel
In-Reply-To: <f372db02-a919-55b4-9200-20726cd13482@ti.com>
On Thu, Apr 20, 2017 at 05:11:53PM +0300, Roger Quadros wrote:
> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
>
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
>
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.
>
> [1] - am572x-idk, am571x-idk, a437x-idk
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
Hi Roger
Thanks for doing a generic solutions and the MDIO DT documentation.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: [PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.
From: Florian Fainelli @ 2017-04-21 1:23 UTC (permalink / raw)
To: Roger Quadros, davem, Andrew Lunn
Cc: tony, nsekhar, jsarha, netdev, linux-omap, linux-kernel
In-Reply-To: <f372db02-a919-55b4-9200-20726cd13482@ti.com>
Hi Roger,
On 04/20/2017 07:11 AM, Roger Quadros wrote:
> Some boards [1] leave the PHYs at an invalid state
> during system power-up or reset thus causing unreliability
> issues with the PHY which manifests as PHY not being detected
> or link not functional. To fix this, these PHYs need to be RESET
> via a GPIO connected to the PHY's RESET pin.
>
> Some boards have a single GPIO controlling the PHY RESET pin of all
> PHYs on the bus whereas some others have separate GPIOs controlling
> individual PHY RESETs.
>
> In both cases, the RESET de-assertion cannot be done in the PHY driver
> as the PHY will not probe till its reset is de-asserted.
> So do the RESET de-assertion in the MDIO bus driver.
>
> [1] - am572x-idk, am571x-idk, a437x-idk
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
A few comments on the binding and the code, sorry for this late review.
> +Example :
> +This example shows these optional properties, plus other properties
> +required for the TI Davinci MDIO driver.
> +
> + davinci_mdio: ethernet@0x5c030000 {
> + compatible = "ti,davinci_mdio";
> + reg = <0x5c030000 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> + reset-delay-us = <2>; /* PHY datasheet states 1uS min */
us is micro seconds, uS is micro siemens.
> +
> + ethphy0: ethernet-phy@1 {
> + reg = <1>;
> + };
> +
> + ethphy1: ethernet-phy@3 {
> + reg = <3>;
> + };
> + };
>
> + /* de-assert bus level PHY GPIO resets */
> + for (i = 0; i < bus->num_reset_gpios; i++) {
> + gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
> + GPIOD_OUT_LOW);
> + if (IS_ERR(gpiod)) {
> + err = PTR_ERR(gpiod);
> + if (err != -ENOENT) {
> + pr_err("mii_bus %s couldn't get reset GPIO\n",
> + bus->id);
Could we use dev_err(&bus->dev) here to better identify which MDIO bus
is returning the problem?
> + return err;
Should we somehow "unwind" the reset lines we were able to successfully
take out of reset and therefore put back into reset state? How about
mdiobus_unregister()? Should we have similar code there, if not for
correctness to be more power efficient?
> + }
> + } else {
> + gpiod_set_value_cansleep(gpiod, 1);
> + udelay(bus->reset_delay_us);
> + gpiod_set_value_cansleep(gpiod, 0);
Does that work even if the polarity of the reset line is active low?
Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH v3 net-next 5/8] net/ncsi: Dump NCSI packet statistics
From: Gavin Shan @ 2017-04-21 1:35 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Gavin Shan, David Miller, netdev, joe, kubakici
In-Reply-To: <6b310c50-bab4-b5a7-0317-5a430c55917a@gmail.com>
On Thu, Apr 20, 2017 at 05:59:27PM -0700, Florian Fainelli wrote:
>On 04/20/2017 05:44 PM, Gavin Shan wrote:
>> On Thu, Apr 20, 2017 at 05:26:14PM -0700, Florian Fainelli wrote:
>>> On 04/20/2017 04:38 PM, Gavin Shan wrote:
>>>> On Thu, Apr 20, 2017 at 01:21:03PM -0400, David Miller wrote:
>>>>> From: Gavin Shan <gwshan@linux.vnet.ibm.com>
>>>>> Date: Tue, 18 Apr 2017 16:51:32 +1000
>>>>>
>>>>>> This creates /sys/kernel/debug/ncsi/<eth0>/stats to dump the NCSI
>>>>>> packets sent and received over all packages and channels. It's useful
>>>>>> to diagnose NCSI problems, especially when NCSI packages and channels
>>>>>> aren't probed properly. The statistics can be gained from debugfs file
>>>>>> as below:
>>>>>>
>>>>>> # cat /sys/kernel/debug/ncsi/eth0/stats
>>>>>
>>>>> There is no reason you cannot use ethtool statistics to provide this
>>>>> information to the user.
>>>>>
>>>>
>>>> It can be dumped by ethtool, but it's more reasonable to dump them
>>>> through debugfs for couple of reasons: (1) ethtool usually dumps
>>>> statistics collected by hardware, but this debugfs file dumps the
>>>> statistics of packets seen (collected) by software. They are different
>>>> things. Note that NCSI channel collects statistics in hardware and it's
>>>> not exposed or dumped by this patchset. They are candidates for ethtool.
>>>> (2) To expose this through ethtool relies on the availability of the tool.
>>>> It's nicer not to depend on it. (3) This interface can be used to check
>>>> the debug packet has been sent successfully through /sys/kernel/debug/ncsi/eth0/pkt,
>>>> dumping the statistics through debugfs make this (debugging) mechanism
>>>> consistent.
>>>
>>> Can't you create a ncsi folder under /sys/class/net/eth0/nsci/ and then
>>> put your stats in there? That would at least look slightly consistent
>>> with what is already existing for the non-NC-SI networking stack.
>>
>> Do you think it's good place to have /sys/class/net/eth0/ncsi/pkt?
>> Note this file accepts commands to send NCSI comands and dumps the
>> corresponding response on read. Also, it's a debugging interface
>> and disabled (invisible) for the most time. I think all directories
>> and files in /sys/class/net/eth0/ should be visible and the structure
>> is stable all the time.
>
>Statistics should not be debugging features having them all the time is
>invaluable, so in that regard they could probably be part of a sysfs
>interface of some kind. If this "pkt" file is special, then yes, maybe
>debugfs is appropriate here.
>
>It sounds like you may also want to consider doing a NC-SI generic
>netlink family at some point, because chances are that are you going to
>export more and more information over time, just like there could be
>additional commands/actions to be performed as well as asynchronous
>events. Netlink is great for that, downside is that you have to write a
>custom user-space tool (or extend iproute2).
Yes, it's definitely good idea to cover everything using one interface.
It's for sure that the statistics collected by hardware should be dumped
by ethtool in future. I will move functionalities introduced by this
patchset to ethtool as well, as Dave suggested in another thread. Hope
Joe has no objection on this.
Cheers,
Gavin
^ permalink raw reply
* Re: [PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.
From: Andrew Lunn @ 2017-04-21 1:38 UTC (permalink / raw)
To: Florian Fainelli
Cc: Roger Quadros, davem, tony, nsekhar, jsarha, netdev, linux-omap,
linux-kernel
In-Reply-To: <fa24ae98-3d4f-4b11-bbb4-485b9ed41ed7@gmail.com>
> > + gpiod_set_value_cansleep(gpiod, 1);
> > + udelay(bus->reset_delay_us);
> > + gpiod_set_value_cansleep(gpiod, 0);
>
> Does that work even if the polarity of the reset line is active low?
Hi Florian
Yes, it does. The gpiod_ API takes care of that, if you set the flag
GPIO_ACTIVE_LOW in the device tree blob. This is one of the
improvements over the gpio_ API.
Andrew
^ permalink raw reply
* Re: [PATCH v3 net-next 5/8] net/ncsi: Dump NCSI packet statistics
From: Joe Perches @ 2017-04-21 1:43 UTC (permalink / raw)
To: Gavin Shan, Florian Fainelli; +Cc: David Miller, netdev, kubakici
In-Reply-To: <20170421013527.GA26300@gwshan>
On Fri, 2017-04-21 at 11:35 +1000, Gavin Shan wrote:
> Yes, it's definitely good idea to cover everything using one interface.
> It's for sure that the statistics collected by hardware should be dumped
> by ethtool in future. I will move functionalities introduced by this
> patchset to ethtool as well, as Dave suggested in another thread. Hope
> Joe has no objection on this.
Of course not.
^ permalink raw reply
* Re: [PATCH 2/2] sparc64: Add eBPF JIT.
From: David Miller @ 2017-04-21 1:49 UTC (permalink / raw)
To: daniel; +Cc: sparclinux, netdev, ast
In-Reply-To: <58F72F6B.9060808@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 19 Apr 2017 11:35:39 +0200
> On 04/18/2017 08:58 PM, David Miller wrote:
>> + if (!prog || !prog->len)
>> + return orig_prog;
>
> This condition can be removed, see also 93a73d442d37
> ("bpf, x86/arm64: remove useless checks on prog"), since
> there's no way we could land here under such circumstance.
Ok.
>> + bpf_flush_icache(ctx.image, ctx.image + image_size);
>
> Since remaining parts were filled through jit_fill_hole(),
> it would be better / more correct to flush the whole buffer,
> see also the recent ppc64 commit 10528b9c45cf ("powerpc/bpf:
> Flush the entire JIT buffer") that fixed it for their jit.
Ok, I've made that change too, thanks!
^ permalink raw reply
* [net-next 00/11][pull request] 10GbE Intel Wired LAN Driver Updates 2017-04-20
From: Jeff Kirsher @ 2017-04-21 1:50 UTC (permalink / raw)
To: davem; +Cc: Jeff Kirsher, netdev, nhorman, sassmann, jogreene
This series contains updates to ixgbe and ixgbevf only, most notable is
the addition of XDP support to our 10GbE drivers.
Paul fixes ixgbe to acquire the PHY semaphore before accessing PHY
registers when issuing a device reset.
John adds XDP support (yeah!) for ixgbe.
Emil fixes an issue by flushing the MACVLAN filters on VF reset to avoid
conflicts with other VFs that may end up using the same MAC address. Also
fixed a bug where ethtool -S displayed some empty fields for ixgbevf
because it was using ixgbe_stats instead ixgbevf_stats for
IXGBEVF_QUEUE_STATS_LEN.
Tony adds the ability to specify a zero MAC address in order to clear the
VF's MAC address from the RAR table. Also adds support for a new
1000Base-T device based on x550EM_X MAC type. Fixed an issue where the
RSS key specified by the user would be over-written with a pre-existing
value, so change the rss_key to a pointer so we can check to see if the
key has a value set before attempting to set it. Fixed the logic for
mailbox support for getting RETA and RSS values, which are only supported
by 82599 and x540 devices.
The following are changes since commit 6905e5a5c8d552ace1d65cacb43499a0eb1e0b89:
net/mlx5e: IPoIB, Fix error handling in mlx5_rdma_netdev_alloc()
and are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue 10GbE
Emil Tantilov (2):
ixgbe: clean macvlan MAC filter table on VF reset
ixgbevf: fix size of queue stats length
John Fastabend (3):
ixgbe: add XDP support for pass and drop actions
ixgbe: add support for XDP_TX action
ixgbe: delay tail write to every 'n' packets
Paul Greenwalt (2):
ixgbe: Acquire PHY semaphore before device reset
ixgbe: Add 1000Base-T device based on X550EM_X MAC
Tony Nguyen (4):
ixgbe: Allow setting zero MAC address for VF
ixgbe: Check for RSS key before setting value
ixgbevf: Fix errors in retrieving RETA and RSS from PF
ixgbevf: Check for RSS key before setting value
drivers/net/ethernet/intel/ixgbe/ixgbe.h | 27 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 33 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c | 78 +++-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 472 +++++++++++++++++++---
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 135 ++++---
drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 1 +
drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 8 +
drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 53 ++-
drivers/net/ethernet/intel/ixgbevf/ethtool.c | 5 +-
drivers/net/ethernet/intel/ixgbevf/ixgbevf.h | 2 +-
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 33 +-
drivers/net/ethernet/intel/ixgbevf/vf.c | 6 +-
12 files changed, 701 insertions(+), 152 deletions(-)
--
2.12.2
^ permalink raw reply
* [net-next 01/11] ixgbe: Acquire PHY semaphore before device reset
From: Jeff Kirsher @ 2017-04-21 1:50 UTC (permalink / raw)
To: davem; +Cc: Paul Greenwalt, netdev, nhorman, sassmann, jogreene, Jeff Kirsher
In-Reply-To: <20170421015029.18994-1-jeffrey.t.kirsher@intel.com>
From: Paul Greenwalt <paul.greenwalt@intel.com>
A recent firmware change fixed an issue to acquire the PHY semaphore before
accessing PHY registers. This led to a case where SW can issue a device
reset clearing the MDIO registers. This patch makes SW acquire the PHY
semaphore before issuing a device reset.
Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c | 8 ++++++++
drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c | 8 ++++++++
2 files changed, 16 insertions(+)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
index 84a467a8ed3d..6ea0d6a5fb90 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
@@ -95,6 +95,7 @@ s32 ixgbe_reset_hw_X540(struct ixgbe_hw *hw)
{
s32 status;
u32 ctrl, i;
+ u32 swfw_mask = hw->phy.phy_semaphore_mask;
/* Call adapter stop to disable tx/rx and clear interrupts */
status = hw->mac.ops.stop_adapter(hw);
@@ -105,10 +106,17 @@ s32 ixgbe_reset_hw_X540(struct ixgbe_hw *hw)
ixgbe_clear_tx_pending(hw);
mac_reset_top:
+ status = hw->mac.ops.acquire_swfw_sync(hw, swfw_mask);
+ if (status) {
+ hw_dbg(hw, "semaphore failed with %d", status);
+ return IXGBE_ERR_SWFW_SYNC;
+ }
+
ctrl = IXGBE_CTRL_RST;
ctrl |= IXGBE_READ_REG(hw, IXGBE_CTRL);
IXGBE_WRITE_REG(hw, IXGBE_CTRL, ctrl);
IXGBE_WRITE_FLUSH(hw);
+ hw->mac.ops.release_swfw_sync(hw, swfw_mask);
usleep_range(1000, 1200);
/* Poll for reset bit to self-clear indicating reset is complete */
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
index 2658394599e4..58d3bcaca2b9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
@@ -3318,6 +3318,7 @@ static s32 ixgbe_reset_hw_X550em(struct ixgbe_hw *hw)
u32 ctrl = 0;
u32 i;
bool link_up = false;
+ u32 swfw_mask = hw->phy.phy_semaphore_mask;
/* Call adapter stop to disable Tx/Rx and clear interrupts */
status = hw->mac.ops.stop_adapter(hw);
@@ -3363,9 +3364,16 @@ static s32 ixgbe_reset_hw_X550em(struct ixgbe_hw *hw)
ctrl = IXGBE_CTRL_RST;
}
+ status = hw->mac.ops.acquire_swfw_sync(hw, swfw_mask);
+ if (status) {
+ hw_dbg(hw, "semaphore failed with %d", status);
+ return IXGBE_ERR_SWFW_SYNC;
+ }
+
ctrl |= IXGBE_READ_REG(hw, IXGBE_CTRL);
IXGBE_WRITE_REG(hw, IXGBE_CTRL, ctrl);
IXGBE_WRITE_FLUSH(hw);
+ hw->mac.ops.release_swfw_sync(hw, swfw_mask);
usleep_range(1000, 1200);
/* Poll for reset bit to self-clear meaning reset is complete */
--
2.12.2
^ 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