* [PATCH 1/2] net: emac: implement 802.1Q VLAN TX tagging support
From: Christian Lamparter @ 2018-10-17 19:53 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller
As per' APM82181 Embedded Processor User Manual 26.1 EMAC Features:
VLAN:
- Support for VLAN tag ID in compliance with IEEE 802.3ac.
- VLAN tag insertion or replacement for transmit packets
This patch completes the missing code for the VLAN tx tagging
support, as the the EMAC_MR1_VLE was already enabled.
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
drivers/net/ethernet/ibm/emac/core.c | 32 ++++++++++++++++++++++++----
drivers/net/ethernet/ibm/emac/core.h | 6 +++++-
2 files changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index 760b2ad8e295..be560f9031f4 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -37,6 +37,7 @@
#include <linux/ethtool.h>
#include <linux/mii.h>
#include <linux/bitops.h>
+#include <linux/if_vlan.h>
#include <linux/workqueue.h>
#include <linux/of.h>
#include <linux/of_address.h>
@@ -674,7 +675,7 @@ static int emac_configure(struct emac_instance *dev)
ndev->dev_addr[5]);
/* VLAN Tag Protocol ID */
- out_be32(&p->vtpid, 0x8100);
+ out_be32(&p->vtpid, ETH_P_8021Q);
/* Receive mode register */
r = emac_iff2rmr(ndev);
@@ -1435,6 +1436,22 @@ static inline netdev_tx_t emac_xmit_finish(struct emac_instance *dev, int len)
return NETDEV_TX_OK;
}
+static inline u16 emac_tx_vlan(struct emac_instance *dev, struct sk_buff *skb)
+{
+ /* Handle VLAN TPID and TCI insert if this is a VLAN skb */
+ if (emac_has_feature(dev, EMAC_FTR_HAS_VLAN_CTAG_TX) &&
+ skb_vlan_tag_present(skb)) {
+ struct emac_regs __iomem *p = dev->emacp;
+
+ /* update the VLAN TCI */
+ out_be32(&p->vtci, (u32)skb_vlan_tag_get(skb));
+
+ /* Insert VLAN tag */
+ return EMAC_TX_CTRL_IVT;
+ }
+ return 0;
+}
+
/* Tx lock BH */
static netdev_tx_t emac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
{
@@ -1443,7 +1460,7 @@ static netdev_tx_t emac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
int slot;
u16 ctrl = EMAC_TX_CTRL_GFCS | EMAC_TX_CTRL_GP | MAL_TX_CTRL_READY |
- MAL_TX_CTRL_LAST | emac_tx_csum(dev, skb);
+ MAL_TX_CTRL_LAST | emac_tx_csum(dev, skb) | emac_tx_vlan(dev, skb);
slot = dev->tx_slot++;
if (dev->tx_slot == NUM_TX_BUFF) {
@@ -1518,7 +1535,7 @@ emac_start_xmit_sg(struct sk_buff *skb, struct net_device *ndev)
goto stop_queue;
ctrl = EMAC_TX_CTRL_GFCS | EMAC_TX_CTRL_GP | MAL_TX_CTRL_READY |
- emac_tx_csum(dev, skb);
+ emac_tx_csum(dev, skb) | emac_tx_vlan(dev, skb);
slot = dev->tx_slot;
/* skb data */
@@ -2891,7 +2908,8 @@ static int emac_init_config(struct emac_instance *dev)
if (of_device_is_compatible(np, "ibm,emac-apm821xx")) {
dev->features |= (EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE |
EMAC_FTR_APM821XX_NO_HALF_DUPLEX |
- EMAC_FTR_460EX_PHY_CLK_FIX);
+ EMAC_FTR_460EX_PHY_CLK_FIX |
+ EMAC_FTR_HAS_VLAN_CTAG_TX);
}
} else if (of_device_is_compatible(np, "ibm,emac4")) {
dev->features |= EMAC_FTR_EMAC4;
@@ -3148,6 +3166,12 @@ static int emac_probe(struct platform_device *ofdev)
if (dev->tah_dev) {
ndev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG;
+
+ if (emac_has_feature(dev, EMAC_FTR_HAS_VLAN_CTAG_TX)) {
+ ndev->vlan_features |= ndev->hw_features;
+ ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_TX;
+ }
+
ndev->features |= ndev->hw_features | NETIF_F_RXCSUM;
}
ndev->watchdog_timeo = 5 * HZ;
diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
index 84caa4a3fc52..8d84d439168c 100644
--- a/drivers/net/ethernet/ibm/emac/core.h
+++ b/drivers/net/ethernet/ibm/emac/core.h
@@ -334,6 +334,8 @@ struct emac_instance {
* APM821xx does not support Half Duplex mode
*/
#define EMAC_FTR_APM821XX_NO_HALF_DUPLEX 0x00001000
+/* EMAC can insert 802.1Q tag */
+#define EMAC_FTR_HAS_VLAN_CTAG_TX 0x00002000
/* Right now, we don't quite handle the always/possible masks on the
* most optimal way as we don't have a way to say something like
@@ -363,7 +365,9 @@ enum {
EMAC_FTR_460EX_PHY_CLK_FIX |
EMAC_FTR_440EP_PHY_CLK_FIX |
EMAC_APM821XX_REQ_JUMBO_FRAME_SIZE |
- EMAC_FTR_APM821XX_NO_HALF_DUPLEX,
+ EMAC_FTR_APM821XX_NO_HALF_DUPLEX |
+ EMAC_FTR_HAS_VLAN_CTAG_TX |
+ 0,
};
static inline int emac_has_feature(struct emac_instance *dev,
--
2.19.1
^ permalink raw reply related
* [PATCH 2/2] net: emac: implement TCP TSO
From: Christian Lamparter @ 2018-10-17 19:53 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller
In-Reply-To: <ee18e29386c00415991a790e88200ca6f5fd8fad.1539804852.git.chunkeey@gmail.com>
This patch enables TSO(v4) hw feature for emac driver.
As atleast the APM82181's TCP/IP acceleration hardware
controller (TAH) provides TCP segmentation support in
the transmit path.
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
drivers/net/ethernet/ibm/emac/core.c | 101 ++++++++++++++++++++++++++-
drivers/net/ethernet/ibm/emac/core.h | 4 ++
drivers/net/ethernet/ibm/emac/emac.h | 7 ++
drivers/net/ethernet/ibm/emac/tah.c | 20 ++++++
drivers/net/ethernet/ibm/emac/tah.h | 2 +
5 files changed, 133 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ibm/emac/core.c b/drivers/net/ethernet/ibm/emac/core.c
index be560f9031f4..49ffbd6e1707 100644
--- a/drivers/net/ethernet/ibm/emac/core.c
+++ b/drivers/net/ethernet/ibm/emac/core.c
@@ -38,6 +38,9 @@
#include <linux/mii.h>
#include <linux/bitops.h>
#include <linux/if_vlan.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/tcp.h>
#include <linux/workqueue.h>
#include <linux/of.h>
#include <linux/of_address.h>
@@ -1410,6 +1413,52 @@ static inline u16 emac_tx_csum(struct emac_instance *dev,
return 0;
}
+const u32 tah_ss[TAH_NO_SSR] = { 9000, 4500, 1500, 1300, 576, 176 };
+
+static int emac_tx_tso(struct emac_instance *dev, struct sk_buff *skb,
+ u16 *ctrl)
+{
+ if (emac_has_feature(dev, EMAC_FTR_TAH_HAS_TSO) &&
+ skb_is_gso(skb) && !!(skb_shinfo(skb)->gso_type &
+ (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
+ u32 seg_size = 0, i;
+
+ /* Get the MTU */
+ seg_size = skb_shinfo(skb)->gso_size + tcp_hdrlen(skb)
+ + skb_network_header_len(skb);
+
+ /* Restriction applied for the segmentation size
+ * to use HW segmentation offload feature: the size
+ * of the segment must not be less than 168 bytes for
+ * DIX formatted segments, or 176 bytes for
+ * IEEE formatted segments.
+ *
+ * I use value 176 to check for the segment size here
+ * as it can cover both 2 conditions above.
+ */
+ if (seg_size < 176)
+ return -ENODEV;
+
+ /* Get the best suitable MTU */
+ for (i = 0; i < ARRAY_SIZE(tah_ss); i++) {
+ u32 curr_seg = tah_ss[i];
+
+ if (curr_seg > dev->ndev->mtu ||
+ curr_seg > seg_size)
+ continue;
+
+ *ctrl &= ~EMAC_TX_CTRL_TAH_CSUM;
+ *ctrl |= EMAC_TX_CTRL_TAH_SSR(i);
+ return 0;
+ }
+
+ /* none found fall back to software */
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static inline netdev_tx_t emac_xmit_finish(struct emac_instance *dev, int len)
{
struct emac_regs __iomem *p = dev->emacp;
@@ -1452,8 +1501,46 @@ static inline u16 emac_tx_vlan(struct emac_instance *dev, struct sk_buff *skb)
return 0;
}
+static netdev_tx_t
+emac_start_xmit(struct sk_buff *skb, struct net_device *ndev);
+
+static netdev_tx_t emac_sw_tso(struct sk_buff *skb, struct net_device *ndev)
+{
+ struct emac_instance *dev = netdev_priv(ndev);
+ struct sk_buff *segs, *curr;
+
+ segs = skb_gso_segment(skb, ndev->features &
+ ~(NETIF_F_TSO | NETIF_F_TSO6));
+ if (IS_ERR_OR_NULL(segs)) {
+ goto drop;
+ } else {
+ while (segs) {
+ /* check for overflow */
+ if (dev->tx_cnt >= NUM_TX_BUFF) {
+ dev_kfree_skb_any(segs);
+ goto drop;
+ }
+
+ curr = segs;
+ segs = curr->next;
+ curr->next = NULL;
+
+ emac_start_xmit(curr, ndev);
+ }
+ dev_consume_skb_any(skb);
+ }
+
+ return NETDEV_TX_OK;
+
+drop:
+ ++dev->estats.tx_dropped;
+ dev_kfree_skb_any(skb);
+ return NETDEV_TX_OK;
+}
+
/* Tx lock BH */
-static netdev_tx_t emac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+static netdev_tx_t
+emac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
{
struct emac_instance *dev = netdev_priv(ndev);
unsigned int len = skb->len;
@@ -1462,6 +1549,9 @@ static netdev_tx_t emac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
u16 ctrl = EMAC_TX_CTRL_GFCS | EMAC_TX_CTRL_GP | MAL_TX_CTRL_READY |
MAL_TX_CTRL_LAST | emac_tx_csum(dev, skb) | emac_tx_vlan(dev, skb);
+ if (emac_tx_tso(dev, skb, &ctrl))
+ return emac_sw_tso(skb, ndev);
+
slot = dev->tx_slot++;
if (dev->tx_slot == NUM_TX_BUFF) {
dev->tx_slot = 0;
@@ -1536,6 +1626,9 @@ emac_start_xmit_sg(struct sk_buff *skb, struct net_device *ndev)
ctrl = EMAC_TX_CTRL_GFCS | EMAC_TX_CTRL_GP | MAL_TX_CTRL_READY |
emac_tx_csum(dev, skb) | emac_tx_vlan(dev, skb);
+ if (emac_tx_tso(dev, skb, &ctrl))
+ return emac_sw_tso(skb, ndev);
+
slot = dev->tx_slot;
/* skb data */
@@ -2946,6 +3039,9 @@ static int emac_init_config(struct emac_instance *dev)
if (dev->tah_ph != 0) {
#ifdef CONFIG_IBM_EMAC_TAH
dev->features |= EMAC_FTR_HAS_TAH;
+
+ if (of_device_is_compatible(np, "ibm,emac-apm821xx"))
+ dev->features |= EMAC_FTR_TAH_HAS_TSO;
#else
printk(KERN_ERR "%pOF: TAH support not enabled !\n", np);
return -ENXIO;
@@ -3167,6 +3263,9 @@ static int emac_probe(struct platform_device *ofdev)
if (dev->tah_dev) {
ndev->hw_features = NETIF_F_IP_CSUM | NETIF_F_SG;
+ if (emac_has_feature(dev, EMAC_FTR_TAH_HAS_TSO))
+ ndev->hw_features |= NETIF_F_TSO;
+
if (emac_has_feature(dev, EMAC_FTR_HAS_VLAN_CTAG_TX)) {
ndev->vlan_features |= ndev->hw_features;
ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_TX;
diff --git a/drivers/net/ethernet/ibm/emac/core.h b/drivers/net/ethernet/ibm/emac/core.h
index 8d84d439168c..e40ccee58775 100644
--- a/drivers/net/ethernet/ibm/emac/core.h
+++ b/drivers/net/ethernet/ibm/emac/core.h
@@ -336,6 +336,8 @@ struct emac_instance {
#define EMAC_FTR_APM821XX_NO_HALF_DUPLEX 0x00001000
/* EMAC can insert 802.1Q tag */
#define EMAC_FTR_HAS_VLAN_CTAG_TX 0x00002000
+/* TAH can do TCP segmentation offload */
+#define EMAC_FTR_TAH_HAS_TSO 0x00004000
/* Right now, we don't quite handle the always/possible masks on the
* most optimal way as we don't have a way to say something like
@@ -352,6 +354,8 @@ enum {
#endif
#ifdef CONFIG_IBM_EMAC_TAH
EMAC_FTR_HAS_TAH |
+ EMAC_FTR_TAH_HAS_TSO |
+
#endif
#ifdef CONFIG_IBM_EMAC_ZMII
EMAC_FTR_HAS_ZMII |
diff --git a/drivers/net/ethernet/ibm/emac/emac.h b/drivers/net/ethernet/ibm/emac/emac.h
index e2f80cca9bed..833967aceb2f 100644
--- a/drivers/net/ethernet/ibm/emac/emac.h
+++ b/drivers/net/ethernet/ibm/emac/emac.h
@@ -266,6 +266,13 @@ struct emac_regs {
#define EMAC_TX_CTRL_IVT 0x0020
#define EMAC_TX_CTRL_RVT 0x0010
#define EMAC_TX_CTRL_TAH_CSUM 0x000e
+#define EMAC_TX_CTRL_TAH_SSR(idx) (((idx) + 1) << 1)
+#define EMAC_TX_CTRL_TAH_SSR5 0x000c
+#define EMAC_TX_CTRL_TAH_SSR4 0x000a
+#define EMAC_TX_CTRL_TAH_SSR3 0x0008
+#define EMAC_TX_CTRL_TAH_SSR2 0x0006
+#define EMAC_TX_CTRL_TAH_SSR1 0x0004
+#define EMAC_TX_CTRL_TAH_SSR0 0x0002
/* EMAC specific TX descriptor status fields (read access) */
#define EMAC_TX_ST_BFCS 0x0200
diff --git a/drivers/net/ethernet/ibm/emac/tah.c b/drivers/net/ethernet/ibm/emac/tah.c
index 9912456dca48..a0236eb305ae 100644
--- a/drivers/net/ethernet/ibm/emac/tah.c
+++ b/drivers/net/ethernet/ibm/emac/tah.c
@@ -45,6 +45,24 @@ void tah_detach(struct platform_device *ofdev, int channel)
mutex_unlock(&dev->lock);
}
+static void tah_set_ssr(struct platform_device *ofdev)
+{
+ struct tah_instance *dev = dev_get_drvdata(&ofdev->dev);
+ struct tah_regs __iomem *p = dev->base;
+ int i;
+
+ mutex_lock(&dev->lock);
+
+ for (i = 0; i < ARRAY_SIZE(tah_ss); i++) {
+ /* Segment size can be up to 16K, but needs
+ * to be a multiple of 2 bytes
+ */
+ out_be32(&p->ssr0 + i, (tah_ss[i] & 0x3ffe) << 16);
+ }
+
+ mutex_unlock(&dev->lock);
+}
+
void tah_reset(struct platform_device *ofdev)
{
struct tah_instance *dev = platform_get_drvdata(ofdev);
@@ -64,6 +82,8 @@ void tah_reset(struct platform_device *ofdev)
out_be32(&p->mr,
TAH_MR_CVR | TAH_MR_ST_768 | TAH_MR_TFS_10KB | TAH_MR_DTFP |
TAH_MR_DIG);
+
+ tah_set_ssr(ofdev);
}
int tah_get_regs_len(struct platform_device *ofdev)
diff --git a/drivers/net/ethernet/ibm/emac/tah.h b/drivers/net/ethernet/ibm/emac/tah.h
index 4d5f336f07b3..2cb0629f30e2 100644
--- a/drivers/net/ethernet/ibm/emac/tah.h
+++ b/drivers/net/ethernet/ibm/emac/tah.h
@@ -36,6 +36,8 @@ struct tah_regs {
u32 tsr;
};
+#define TAH_NO_SSR 6
+extern const u32 tah_ss[TAH_NO_SSR];
/* TAH device */
struct tah_instance {
--
2.19.1
^ permalink raw reply related
* [PATCH v2] isdn: hfc_{pci,sx}: Avoid empty body if statements
From: Nathan Chancellor @ 2018-10-18 3:49 UTC (permalink / raw)
To: Karsten Keil, David S. Miller
Cc: netdev, linux-kernel, Masahiro Yamada, Nathan Chancellor
In-Reply-To: <20181017180657.9410-1-natechancellor@gmail.com>
Clang warns:
drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
[-Werror,-Wempty-body]
if (Read_hfc(cs, HFCPCI_INT_S1));
^
drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
separate line to silence this warning
In my attempt to hide the warnings because I thought they didn't serve
any purpose[1], Masahiro Yamada pointed out that {Read,Write}_hfc in
hci_pci.c should be using a standard register access method; otherwise,
the compiler will just remove the if statements.
For hfc_pci, use the versions of {Read,Write}_hfc found in
drivers/isdn/hardware/mISDN/hfc_pCI.h then remove the empty if statements.
For hfc_sx, {Read,Write}_hfc are already use a proper register accessor
(inb, outb) so just remove the unnecessary if statements.
[1]: https://lore.kernel.org/lkml/20181016021454.11953-1-natechancellor@gmail.com/
Link: https://github.com/ClangBuiltLinux/linux/issues/66
Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
v1 -> v2:
* Remove unnecessary cast to void, just remove if statements (thanks to
review from Masahiro).
* Clean up commit message.
drivers/isdn/hisax/hfc_pci.c | 6 +++---
drivers/isdn/hisax/hfc_pci.h | 4 ++--
drivers/isdn/hisax/hfc_sx.c | 6 +++---
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
index 8e5b03161b2f..7bcd104e9dfe 100644
--- a/drivers/isdn/hisax/hfc_pci.c
+++ b/drivers/isdn/hisax/hfc_pci.c
@@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs)
Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
/* Clear already pending ints */
- if (Read_hfc(cs, HFCPCI_INT_S1));
+ Read_hfc(cs, HFCPCI_INT_S1);
Write_hfc(cs, HFCPCI_STATES, HFCPCI_LOAD_STATE | 2); /* HFC ST 2 */
udelay(10);
@@ -158,7 +158,7 @@ reset_hfcpci(struct IsdnCardState *cs)
/* Finally enable IRQ output */
cs->hw.hfcpci.int_m2 = HFCPCI_IRQ_ENABLE;
Write_hfc(cs, HFCPCI_INT_M2, cs->hw.hfcpci.int_m2);
- if (Read_hfc(cs, HFCPCI_INT_S1));
+ Read_hfc(cs, HFCPCI_INT_S1);
}
/***************************************************/
@@ -1537,7 +1537,7 @@ hfcpci_bh(struct work_struct *work)
cs->hw.hfcpci.int_m1 &= ~HFCPCI_INTS_TIMER;
Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
/* Clear already pending ints */
- if (Read_hfc(cs, HFCPCI_INT_S1));
+ Read_hfc(cs, HFCPCI_INT_S1);
Write_hfc(cs, HFCPCI_STATES, 4 | HFCPCI_LOAD_STATE);
udelay(10);
Write_hfc(cs, HFCPCI_STATES, 4);
diff --git a/drivers/isdn/hisax/hfc_pci.h b/drivers/isdn/hisax/hfc_pci.h
index 4e58700a3e61..4c3b3ba35726 100644
--- a/drivers/isdn/hisax/hfc_pci.h
+++ b/drivers/isdn/hisax/hfc_pci.h
@@ -228,8 +228,8 @@ typedef union {
} fifo_area;
-#define Write_hfc(a, b, c) (*(((u_char *)a->hw.hfcpci.pci_io) + b) = c)
-#define Read_hfc(a, b) (*(((u_char *)a->hw.hfcpci.pci_io) + b))
+#define Write_hfc(a, b, c) (writeb(c, (a->hw.hfcpci.pci_io) + b))
+#define Read_hfc(a, b) (readb((a->hw.hfcpci.pci_io) + b))
extern void main_irq_hcpci(struct BCState *bcs);
extern void releasehfcpci(struct IsdnCardState *cs);
diff --git a/drivers/isdn/hisax/hfc_sx.c b/drivers/isdn/hisax/hfc_sx.c
index 4d3b4b2f2612..12af628d9b2c 100644
--- a/drivers/isdn/hisax/hfc_sx.c
+++ b/drivers/isdn/hisax/hfc_sx.c
@@ -381,7 +381,7 @@ reset_hfcsx(struct IsdnCardState *cs)
Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1);
/* Clear already pending ints */
- if (Read_hfc(cs, HFCSX_INT_S1));
+ Read_hfc(cs, HFCSX_INT_S1);
Write_hfc(cs, HFCSX_STATES, HFCSX_LOAD_STATE | 2); /* HFC ST 2 */
udelay(10);
@@ -411,7 +411,7 @@ reset_hfcsx(struct IsdnCardState *cs)
/* Finally enable IRQ output */
cs->hw.hfcsx.int_m2 = HFCSX_IRQ_ENABLE;
Write_hfc(cs, HFCSX_INT_M2, cs->hw.hfcsx.int_m2);
- if (Read_hfc(cs, HFCSX_INT_S2));
+ Read_hfc(cs, HFCSX_INT_S2);
}
/***************************************************/
@@ -1288,7 +1288,7 @@ hfcsx_bh(struct work_struct *work)
cs->hw.hfcsx.int_m1 &= ~HFCSX_INTS_TIMER;
Write_hfc(cs, HFCSX_INT_M1, cs->hw.hfcsx.int_m1);
/* Clear already pending ints */
- if (Read_hfc(cs, HFCSX_INT_S1));
+ Read_hfc(cs, HFCSX_INT_S1);
Write_hfc(cs, HFCSX_STATES, 4 | HFCSX_LOAD_STATE);
udelay(10);
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] isdn: hfc_{pci,sx}: Avoid empty body if statements and use proper register accessors
From: Nathan Chancellor @ 2018-10-18 3:34 UTC (permalink / raw)
To: Masahiro Yamada; +Cc: isdn, Networking, Linux Kernel Mailing List
In-Reply-To: <CAK7LNARjwZr2NPNE9aGnzQv-uUWjf1=A+kX3pjALqYfsWORv5g@mail.gmail.com>
On Thu, Oct 18, 2018 at 12:23:37PM +0900, Masahiro Yamada wrote:
> Hi Nathan,
>
> On Thu, Oct 18, 2018 at 3:09 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang warns:
> >
> > drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
> > [-Werror,-Wempty-body]
> > if (Read_hfc(cs, HFCPCI_INT_S1));
> > ^
> > drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
> > separate line to silence this warning
> >
> > Use the format found in drivers/isdn/hardware/mISDN/hfcpci.c of casting
> > the return of Read_hfc to void, instead of using an empty if statement.
> >
> > While we're at it, Masahiro Yamada pointed out that {Read,Write}_hfc
> > should be using a standard access method in hfc_pci.h. Use the one found
> > in drivers/isdn/hardware/mISDN/hfc_pci.h.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/66
> > Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> > drivers/isdn/hisax/hfc_pci.c | 6 +++---
> > drivers/isdn/hisax/hfc_pci.h | 4 ++--
> > drivers/isdn/hisax/hfc_sx.c | 6 +++---
> > 3 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
> > index 8e5b03161b2f..a63b9155b697 100644
> > --- a/drivers/isdn/hisax/hfc_pci.c
> > +++ b/drivers/isdn/hisax/hfc_pci.c
> > @@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs)
> > Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
> >
> > /* Clear already pending ints */
> > - if (Read_hfc(cs, HFCPCI_INT_S1));
> > + (void) Read_hfc(cs, HFCPCI_INT_S1);
>
>
>
> Why is this '(void)' necessary?
>
It's not. I never tested without it since last time I tried to remove
the if statement, GCC complained at me but that was before the readb
change like you suggested.
I will go ahead and send a v2 with that cleaned up.
> I see no warning without it.
>
>
>
> --
> Best Regards
> Masahiro Yamada
Thanks for the review!
Nathan
^ permalink raw reply
* Re: [PATCH net] net: ipmr: fix unresolved entry dumps
From: Nikolay Aleksandrov @ 2018-10-17 19:35 UTC (permalink / raw)
To: netdev; +Cc: davem, Colin Ian King
In-Reply-To: <20181017193434.11383-1-nikolay@cumulusnetworks.com>
On 17/10/2018 22:34, Nikolay Aleksandrov wrote:
> If the skb space ends in an unresolved entry while dumping we'll miss
> some unresolved entries. The reason is due to zeroing the entry counter
> between dumping resolved and unresolved mfc entries. We should just
> keep counting until the whole table is dumped and zero when we move to
> the next as we have a separate table counter.
>
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Fixes: 8fb472c09b9d ("ipmr: improve hash scalability")
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
> Dropped Yuval's mail because it bounces.
>
> net/ipv4/ipmr_base.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/net/ipv4/ipmr_base.c b/net/ipv4/ipmr_base.c
> index 1ad9aa62a97b..eab8cd5ec2f5 100644
> --- a/net/ipv4/ipmr_base.c
> +++ b/net/ipv4/ipmr_base.c
> @@ -296,8 +296,6 @@ int mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb,
> next_entry:
> e++;
> }
> - e = 0;
> - s_e = 0;
>
> spin_lock_bh(lock);
> list_for_each_entry(mfc, &mrt->mfc_unres_queue, list) {
>
+CC Colin
Sorry about that, my script somehow missed the reported-by email.
^ permalink raw reply
* Re: [PATCH linux-firmware] linux-firmware: liquidio: fix GPL compliance issue
From: Manlunas, Felix @ 2018-10-17 19:34 UTC (permalink / raw)
To: linux-firmware@kernel.org
Cc: netdev@vger.kernel.org, fweimer@redhat.com, Awasthi, Manish,
Panicker, Manojkumar, Masood, Faisal, Vatsavayi, Raghu,
Chickles, Derek, Burla, Satananda
In-Reply-To: <20180928235051.GA3269@felix-thinkpad.cavium.com>
On Fri, Sep 28, 2018 at 04:50:51PM -0700, Felix Manlunas wrote:
> Part of the code inside the lio_vsw_23xx.bin firmware image is under GPL,
> but the LICENCE.cavium file neglects to indicate that. However,
> LICENCE.cavium does correctly specify the license that covers the other
> Cavium firmware images that do not contain any GPL code.
>
> Fix the GPL compliance issue by adding a new file, LICENCE.cavium_liquidio,
> which correctly shows the GPL boilerplate. This new file specifies the
> licenses for all liquidio firmware, including the ones that do not have
> GPL code.
>
> Change the liquidio section of WHENCE to point to LICENCE.cavium_liquidio.
>
> Reported-by: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Manish Awasthi <manish.awasthi@cavium.com>
> Signed-off-by: Manoj Panicker <manojkumar.panicker@cavium.com>
> Signed-off-by: Faisal Masood <faisal.masood@cavium.com>
> Signed-off-by: Felix Manlunas <felix.manlunas@cavium.com>
> ---
> LICENCE.cavium_liquidio | 429 ++++++++++++++++++++++++++++++++++++++++++++++++
> WHENCE | 2 +-
> 2 files changed, 430 insertions(+), 1 deletion(-)
> create mode 100644 LICENCE.cavium_liquidio
Hello Maintainers of linux-firmware.git,
Any feedback about this patch?
Thanks,
Felix
^ permalink raw reply
* [PATCH net] net: ipmr: fix unresolved entry dumps
From: Nikolay Aleksandrov @ 2018-10-17 19:34 UTC (permalink / raw)
To: netdev; +Cc: davem, Nikolay Aleksandrov
In-Reply-To: <7e1a20a0-9ab2-5680-bde9-185c72242680@cumulusnetworks.com>
If the skb space ends in an unresolved entry while dumping we'll miss
some unresolved entries. The reason is due to zeroing the entry counter
between dumping resolved and unresolved mfc entries. We should just
keep counting until the whole table is dumped and zero when we move to
the next as we have a separate table counter.
Reported-by: Colin Ian King <colin.king@canonical.com>
Fixes: 8fb472c09b9d ("ipmr: improve hash scalability")
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
Dropped Yuval's mail because it bounces.
net/ipv4/ipmr_base.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/ipv4/ipmr_base.c b/net/ipv4/ipmr_base.c
index 1ad9aa62a97b..eab8cd5ec2f5 100644
--- a/net/ipv4/ipmr_base.c
+++ b/net/ipv4/ipmr_base.c
@@ -296,8 +296,6 @@ int mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb,
next_entry:
e++;
}
- e = 0;
- s_e = 0;
spin_lock_bh(lock);
list_for_each_entry(mfc, &mrt->mfc_unres_queue, list) {
--
2.17.2
^ permalink raw reply related
* Re: [PATCH v3] virtio_net: avoid using netif_tx_disable() for serializing tx routine
From: ake @ 2018-10-18 3:25 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, David S. Miller, virtualization, netdev, linux-kernel
In-Reply-To: <20181017110741-mutt-send-email-mst@kernel.org>
On 2018/10/18 0:09, Michael S. Tsirkin wrote:
> On Wed, Oct 17, 2018 at 07:44:12PM +0900, Ake Koomsin wrote:
>> Commit 713a98d90c5e ("virtio-net: serialize tx routine during reset")
>> introduces netif_tx_disable() after netif_device_detach() in order to
>> avoid use-after-free of tx queues. However, there are two issues.
>>
>> 1) Its operation is redundant with netif_device_detach() in case the
>> interface is running.
>> 2) In case of the interface is not running before suspending and
>> resuming, the tx does not get resumed by netif_device_attach().
>> This results in losing network connectivity.
>>
>> It is better to use netif_tx_lock_bh()/netif_tx_unlock_bh() instead for
>> serializing tx routine during reset. This also preserves the symmetry
>> of netif_device_detach() and netif_device_attach().
>>
>> Fixes commit 713a98d90c5e ("virtio-net: serialize tx routine during reset")
>> Signed-off-by: Ake Koomsin <ake@igel.co.jp>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> Thanks a lot for debugging!
> Seems like stable material to me, right?
Yes. With this patch, we can avoid network connectivity lost
because of tx not get re-enabled under some situation. Plus, it avoids
redundant operation between netif_device_detach() and
netif_tx_disable().
I tested the patch on Linux net-next and QEMU master by suspending/
resuming the virtual machine repeatedly. The network looks no problem
and has no connectivity lost so far. I tested with both user-mode
networking and tap interface.
Best Regards
>> ---
>> drivers/net/virtio_net.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 3f5aa59c37b7..3e2c041d76ac 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -2267,8 +2267,9 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
>> /* Make sure no work handler is accessing the device */
>> flush_work(&vi->config_work);
>>
>> + netif_tx_lock_bh(vi->dev);
>> netif_device_detach(vi->dev);
>> - netif_tx_disable(vi->dev);
>> + netif_tx_unlock_bh(vi->dev);
>> cancel_delayed_work_sync(&vi->refill);
>>
>> if (netif_running(vi->dev)) {
>> @@ -2304,7 +2305,9 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>> }
>> }
>>
>> + netif_tx_lock_bh(vi->dev);
>> netif_device_attach(vi->dev);
>> + netif_tx_unlock_bh(vi->dev);
>> return err;
>> }
>>
>> --
>> 2.19.1
^ permalink raw reply
* Re: [PATCH net] r8169: fix NAPI handling under high load
From: Heiner Kallweit @ 2018-10-17 19:27 UTC (permalink / raw)
To: Holger Hoffstätte, David Miller,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <e7607b0f-86ed-6784-638c-37d68d910fb7@applied-asynchrony.com>
On 17.10.2018 21:11, Holger Hoffstätte wrote:
> On 10/17/18 20:12, Heiner Kallweit wrote:
>> On 16.10.2018 23:17, Holger Hoffstätte wrote:
>>> On 10/16/18 22:37, Heiner Kallweit wrote:
>>>> rtl_rx() and rtl_tx() are called only if the respective bits are set
>>>> in the interrupt status register. Under high load NAPI may not be
>>>> able to process all data (work_done == budget) and it will schedule
>>>> subsequent calls to the poll callback.
>>>> rtl_ack_events() however resets the bits in the interrupt status
>>>> register, therefore subsequent calls to rtl8169_poll() won't call
>>>> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
>>>
>>> Very interesting! Could this be the reason for the mysterious
>>> hangs & resets we experienced when enabling BQL for r8169?
>>> They happened more often with TSO/GSO enabled and several people
>>> attempted to fix those hangs unsuccessfully; it was later reverted
>>> and has been since then (#87cda7cb43).
>>> If this bug has been there "forever" it might be tempting to
>>> re-apply BQL and see what happens. Any chance you could give that
>>> a try? I'll gladly test patches, just like I'll run this one.
>>>
>> After reading through the old mail threads regarding BQL on r8169
>> I don't think the fix here is related.
>> It seems that BQL on r8169 worked fine for most people, just one
>> had problems on one of his systems. I assume the issue was specific
>
> I continued to use the BQL patch in my private tree after it was reverted
> and also had occasional timeouts, but *only* after I started playing
> with ethtool to change offload settings. Without offloads or the BQL patch
> everything has been rock-solid since then.
> The other weird problem was that timeouts would occur on an otherwise
> *completely idle* system. Since that occasionally borked my NFS server
> over night I ultimately removed BQL as well. Rock-solid since then.
>
>> I will apply the old BQL patch and see how it's on my system
>> (with GRO and SG enabled).
>
> I don't think it still applies cleanly, but if you cook up an updated
> version I'll gladly test it.
>
> Thanks! :)
> Holger
>
Good to know. What's your kernel version and RTL8168 chip version?
Regarding the chip version the dmesg line with the XID would be relevant.
Below is the slightly modified original BQL patch, I just moved the call
to netdev_reset_queue(). This patch applies at least to latest linux-next.
My test system:
- RTL8168evl
- latest linux-next
- BQL patch applied
- SG/GRO enabled:
rx-checksumming: on
tx-checksumming: on
tx-checksum-ipv4: on
tx-checksum-ip-generic: off [fixed]
tx-checksum-ipv6: on
tx-checksum-fcoe-crc: off [fixed]
tx-checksum-sctp: off [fixed]
scatter-gather: on
tx-scatter-gather: on
tx-scatter-gather-fraglist: off [fixed]
tcp-segmentation-offload: on
tx-tcp-segmentation: on
tx-tcp-ecn-segmentation: off [fixed]
tx-tcp-mangleid-segmentation: on
tx-tcp6-segmentation: on
udp-fragmentation-offload: off
generic-segmentation-offload: on
generic-receive-offload: on
large-receive-offload: off [fixed]
rx-vlan-offload: on
tx-vlan-offload: on
I briefly tested normal operation and did some tests with iperf3.
Everything looks good so far.
---
drivers/net/ethernet/realtek/r8169.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0d8070adc..e236b46b8 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5852,6 +5852,7 @@ static void rtl8169_tx_clear(struct rtl8169_private *tp)
{
rtl8169_tx_clear_range(tp, tp->dirty_tx, NUM_TX_DESC);
tp->cur_tx = tp->dirty_tx = 0;
+ netdev_reset_queue(tp->dev);
}
static void rtl_reset_work(struct rtl8169_private *tp)
@@ -6154,6 +6155,8 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
txd->opts2 = cpu_to_le32(opts[1]);
+ netdev_sent_queue(dev, skb->len);
+
skb_tx_timestamp(skb);
/* Force memory writes to complete before releasing descriptor */
@@ -6252,7 +6255,7 @@ static void rtl8169_pcierr_interrupt(struct net_device *dev)
static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
{
- unsigned int dirty_tx, tx_left;
+ unsigned int dirty_tx, tx_left, bytes_compl = 0, pkts_compl = 0;
dirty_tx = tp->dirty_tx;
smp_rmb();
@@ -6276,10 +6279,8 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
rtl8169_unmap_tx_skb(tp_to_dev(tp), tx_skb,
tp->TxDescArray + entry);
if (status & LastFrag) {
- u64_stats_update_begin(&tp->tx_stats.syncp);
- tp->tx_stats.packets++;
- tp->tx_stats.bytes += tx_skb->skb->len;
- u64_stats_update_end(&tp->tx_stats.syncp);
+ pkts_compl++;
+ bytes_compl += tx_skb->skb->len;
dev_consume_skb_any(tx_skb->skb);
tx_skb->skb = NULL;
}
@@ -6288,6 +6289,13 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
}
if (tp->dirty_tx != dirty_tx) {
+ netdev_completed_queue(dev, pkts_compl, bytes_compl);
+
+ u64_stats_update_begin(&tp->tx_stats.syncp);
+ tp->tx_stats.packets += pkts_compl;
+ tp->tx_stats.bytes += bytes_compl;
+ u64_stats_update_end(&tp->tx_stats.syncp);
+
tp->dirty_tx = dirty_tx;
/* Sync with rtl8169_start_xmit:
* - publish dirty_tx ring index (write barrier)
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] isdn: hfc_{pci,sx}: Avoid empty body if statements and use proper register accessors
From: Masahiro Yamada @ 2018-10-18 3:23 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: isdn, Networking, Linux Kernel Mailing List
In-Reply-To: <20181017180657.9410-1-natechancellor@gmail.com>
Hi Nathan,
On Thu, Oct 18, 2018 at 3:09 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns:
>
> drivers/isdn/hisax/hfc_pci.c:131:34: error: if statement has empty body
> [-Werror,-Wempty-body]
> if (Read_hfc(cs, HFCPCI_INT_S1));
> ^
> drivers/isdn/hisax/hfc_pci.c:131:34: note: put the semicolon on a
> separate line to silence this warning
>
> Use the format found in drivers/isdn/hardware/mISDN/hfcpci.c of casting
> the return of Read_hfc to void, instead of using an empty if statement.
>
> While we're at it, Masahiro Yamada pointed out that {Read,Write}_hfc
> should be using a standard access method in hfc_pci.h. Use the one found
> in drivers/isdn/hardware/mISDN/hfc_pci.h.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/66
> Suggested-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> drivers/isdn/hisax/hfc_pci.c | 6 +++---
> drivers/isdn/hisax/hfc_pci.h | 4 ++--
> drivers/isdn/hisax/hfc_sx.c | 6 +++---
> 3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/isdn/hisax/hfc_pci.c b/drivers/isdn/hisax/hfc_pci.c
> index 8e5b03161b2f..a63b9155b697 100644
> --- a/drivers/isdn/hisax/hfc_pci.c
> +++ b/drivers/isdn/hisax/hfc_pci.c
> @@ -128,7 +128,7 @@ reset_hfcpci(struct IsdnCardState *cs)
> Write_hfc(cs, HFCPCI_INT_M1, cs->hw.hfcpci.int_m1);
>
> /* Clear already pending ints */
> - if (Read_hfc(cs, HFCPCI_INT_S1));
> + (void) Read_hfc(cs, HFCPCI_INT_S1);
Why is this '(void)' necessary?
I see no warning without it.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH net] r8169: fix NAPI handling under high load
From: Holger Hoffstätte @ 2018-10-17 19:11 UTC (permalink / raw)
To: Heiner Kallweit, David Miller, Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <62974f0f-1938-3635-69d4-204ed8c587b3@gmail.com>
On 10/17/18 20:12, Heiner Kallweit wrote:
> On 16.10.2018 23:17, Holger Hoffstätte wrote:
>> On 10/16/18 22:37, Heiner Kallweit wrote:
>>> rtl_rx() and rtl_tx() are called only if the respective bits are set
>>> in the interrupt status register. Under high load NAPI may not be
>>> able to process all data (work_done == budget) and it will schedule
>>> subsequent calls to the poll callback.
>>> rtl_ack_events() however resets the bits in the interrupt status
>>> register, therefore subsequent calls to rtl8169_poll() won't call
>>> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
>>
>> Very interesting! Could this be the reason for the mysterious
>> hangs & resets we experienced when enabling BQL for r8169?
>> They happened more often with TSO/GSO enabled and several people
>> attempted to fix those hangs unsuccessfully; it was later reverted
>> and has been since then (#87cda7cb43).
>> If this bug has been there "forever" it might be tempting to
>> re-apply BQL and see what happens. Any chance you could give that
>> a try? I'll gladly test patches, just like I'll run this one.
>>
> After reading through the old mail threads regarding BQL on r8169
> I don't think the fix here is related.
> It seems that BQL on r8169 worked fine for most people, just one
> had problems on one of his systems. I assume the issue was specific
I continued to use the BQL patch in my private tree after it was reverted
and also had occasional timeouts, but *only* after I started playing
with ethtool to change offload settings. Without offloads or the BQL patch
everything has been rock-solid since then.
The other weird problem was that timeouts would occur on an otherwise
*completely idle* system. Since that occasionally borked my NFS server
over night I ultimately removed BQL as well. Rock-solid since then.
> I will apply the old BQL patch and see how it's on my system
> (with GRO and SG enabled).
I don't think it still applies cleanly, but if you cook up an updated
version I'll gladly test it.
Thanks! :)
Holger
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: emit RECORD_MMAP events for bpf prog load/unload
From: Alexei Starovoitov @ 2018-10-17 19:08 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: David Ahern, Song Liu, Alexei Starovoitov, Peter Zijlstra,
Alexei Starovoitov, David S . Miller, Daniel Borkmann, Networking,
Kernel Team
In-Reply-To: <20181017185347.GB4589@kernel.org>
On 10/17/18 11:53 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Oct 17, 2018 at 04:36:08PM +0000, Alexei Starovoitov escreveu:
>> On 10/17/18 8:09 AM, David Ahern wrote:
>>> On 10/16/18 11:43 PM, Song Liu wrote:
>>>> I agree that processing events while recording has significant overhead.
>>>> In this case, perf user space need to know details about the the jited BPF
>>>> program. It is impossible to pass all these details to user space through
>>>> the relatively stable ring_buffer API. Therefore, some processing of the
>>>> data is necessary (get bpf prog_id from ring buffer, and then fetch program
>>>> details via BPF_OBJ_GET_INFO_BY_FD.
>>>>
>>>> I have some idea on processing important data with relatively low overhead.
>>>> Let me try implement it.
>>>>
>>>
>>> As I understand it, you want this series:
>>>
>>> kernel: add event to perf buffer on bpf prog load
>>>
>>> userspace: perf reads the event and grabs information about the program
>>> from the fd
>>>
>>> Is that correct?
>>>
>>> Userpsace is not awakened immediately when an event is added the the
>>> ring. It is awakened once the number of events crosses a watermark. That
>>> means there is an unknown - and potentially long - time window where the
>>> program can be unloaded before perf reads the event.
>
>>> So no matter what you do expecting perf record to be able to process the
>>> event quickly is an unreasonable expectation.
>
>> yes... unless we go with threaded model as Arnaldo suggested and use
>> single event as a watermark to wakeup our perf thread.
>> In such case there is still a race window between user space waking up
>> and doing _single_ bpf_get_fd_from_id() call to hold that prog
>> and some other process trying to instantly unload the prog it
>> just loaded.
>> I think such race window is extremely tiny and if perf misses
>> those load/unload events it's a good thing, since there is no chance
>> that normal pmu event samples would be happening during prog execution.
>
>> The alternative approach with no race window at all is to burden kernel
>> RECORD_* events with _all_ information about bpf prog. Which is jited
>> addresses, jited image itself, info about all subprogs, info about line
>> info, all BTF data, etc. As I said earlier I'm strongly against such
>> RECORD_* bloating.
>> Instead we need to find a way to process new RECORD_BPF events with
>> single prog_id field in perf user space with minimal race
>> and threaded approach sounds like a win to me.
>
> There is another alternative, I think: put just a content based hash,
> like a git commit id into a PERF_RECORD_MMAP3 new record, and when the
> validator does the jit, etc, it stashes the content that
> BPF_OBJ_GET_INFO_BY_FD would get somewhere, some filesystem populated by
> the kernel right after getting stuff from sys_bpf() and preparing it for
> use, then we know that in (start, end) we have blob foo with content id,
> that we will use to retrieve information that augments what we know with
> just (start, end, id) and allows annotation, etc.
>
> That stash space for jitted stuff gets garbage collected from time to
> time or is even completely disabled if the user is not interested in
> such augmentation, just like one can do disabling perf's ~/.debug/
> directory hashed by build-id.
>
> I think with this we have no races, the PERF_RECORD_MMAP3 gets just what
> is in PERF_RECORD_MMAP2 plus some extra 20 bytes for such content based
> cookie and we solve the other race we already have with kernel modules,
> DSOs, etc.
>
> I have mentioned this before, there were objections, perhaps this time I
> formulated in a different way that makes it more interesting?
that 'content based hash' we already have. It's called program tag.
and we already taught iovisor/bcc to stash that stuff into
/var/tmp/bcc/bpf_prog_TAG/ directory.
Unfortunately that approach didn't work.
JITed image only exists in the kernel. It's there only when
program is loaded and it's the one that matter the most for performance
analysis, since sample IPs are pointing into it.
Also the line info mapping that user space knows is not helping much
either, since verifier optimizes the instructions and then JIT
does more. The debug_info <-> JIT relationship must be preserved
by the kernel and returned to user space.
The only other non-race way is to keep all that info in the kernel after
program is unloaded, but that is even worse than bloating RECORD*s
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: add cg_skb_is_valid_access for BPF_PROG_TYPE_CGROUP_SKB
From: Song Liu @ 2018-10-17 19:07 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, netdev@vger.kernel.org, ast@kernel.org,
daniel@iogearbox.net, Kernel Team
In-Reply-To: <7c6474c0-27a2-bd6c-cd0f-c835856224cb@fb.com>
> On Oct 17, 2018, at 12:02 PM, Alexei Starovoitov <ast@fb.com> wrote:
>
> On 10/17/18 10:26 AM, Alexei Starovoitov wrote:
>> On Tue, Oct 16, 2018 at 10:56:05PM -0700, Song Liu wrote:
>>> BPF programs of BPF_PROG_TYPE_CGROUP_SKB need to access headers in the
>>> skb. This patch enables direct access of skb for these programs.
>>
>> The lack of direct packet access in CGROUP_SKB progs was
>> an unpleasant surprise to me, so thank you for fixing it,
>> but there are few issues with the patch. See below.
>>
>>> In __cgroup_bpf_run_filter_skb(), bpf_compute_data_pointers() is called
>>> to compute proper data_end for the BPF program.
>>>
>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>> ---
>>> kernel/bpf/cgroup.c | 4 ++++
>>> net/core/filter.c | 26 +++++++++++++++++++++++++-
>>> 2 files changed, 29 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>>> index 00f6ed2e4f9a..340d496f35bd 100644
>>> --- a/kernel/bpf/cgroup.c
>>> +++ b/kernel/bpf/cgroup.c
>>> @@ -566,6 +566,10 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
>>> save_sk = skb->sk;
>>> skb->sk = sk;
>>> __skb_push(skb, offset);
>>> +
>>> + /* compute pointers for the bpf prog */
>>> + bpf_compute_data_pointers(skb);
>>> +
>>> ret = BPF_PROG_RUN_ARRAY(cgrp->bpf.effective[type], skb,
>>> bpf_prog_run_save_cb);
>>> __skb_pull(skb, offset);
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 1a3ac6c46873..8b5a502e241f 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -5346,6 +5346,30 @@ static bool sk_filter_is_valid_access(int off, int size,
>>> return bpf_skb_is_valid_access(off, size, type, prog, info);
>>> }
>>>
>>> +static bool cg_skb_is_valid_access(int off, int size,
>>> + enum bpf_access_type type,
>>> + const struct bpf_prog *prog,
>>> + struct bpf_insn_access_aux *info)
>>> +{
>>> + if (type == BPF_WRITE)
>>> + return false;
>>
>> this disables writes into cb[0..4] that were allowed for cgroup_inet_* before.
>> One can argue that this may break existing progs,
>> but looking at the place where BPF_CGROUP_RUN_PROG_INET_INGRESS is called
>> it seems it's actually not correct in all cases to access cb there.
>> Just few lines down we call bpf_prog_run_save_cb() which save/restores
>> these 24 bytes.
>> So we have two option either add save/restore for INET_INGRESS only
>> or disable read and write access to cb[0..4] for CGROUP_SKB progs.
>> I prefer the former.
>>
>>> +
>>> + switch (off) {
>>> + case bpf_ctx_range(struct __sk_buff, len):
>>> + break;
>>> + case bpf_ctx_range(struct __sk_buff, data):
>>> + info->reg_type = PTR_TO_PACKET;
>>> + break;
>>> + case bpf_ctx_range(struct __sk_buff, data_end):
>>> + info->reg_type = PTR_TO_PACKET_END;
>>> + break;
>>> + default:
>>> + return false;
>>> + }
>>
>> this also enables access to a range of fields family..local_port.
>> It's ok to do for egress, but not for ingress unless we
>> add code similar to the bottom of sk_filter_trim_cap() that
>> inits skb->sk.
>>
>> above change also allows access to data_meta and flow_keys
>> which is not correct.
>>
>> Considering all that I'm proposing to fix INET_INGRESS call site
>> similar to code below it in sk_filter_trim_cap().
>> In particular to do:
>> struct sock *save_sk = skb->sk;
>> skb->sk = sk;
>> save and clear cb
>> BPF_CGROUP_RUN_PROG_INET_INGRESS
>> restore cb
>> skb->sk = save_sk;
>>
>> all of above can probaby be inside BPF_CGROUP_RUN_PROG_INET_INGRESS macro.
>> Then in this cg_skb_is_valid_access() allow access to data/data_end
>> and family..local_port range as well.
>> while disallowing access to flow_keys and data_meta.
>>
>> In patch 2 we gotta have tests for all these fields.
>>
>> Thoughts?
>
> chatted with Song offline.
> I completely misread 'return false' in the above as 'break'.
> The patch actually disables access to pkt_type, mark, queue_mapping
> and so on. Which is not correct either.
> Since tests were not failing we really need to improve this aspect
> of test coverage in test_verifier.c
>
> Also I missed that __cgroup_bpf_run_filter_skb() already
> does save_sk = skb->sk; skb->sk = sk;
> and bpf_prog_run_save_cb()
> So no issue in the existing code. That was false alarm.
> Revising the proposal...
> I think cg_skb_is_valid_access() can be made similar to
> lwt_is_valid_access().
> Allowing writes into mark, priority, cb[0..4]
> and read of data/data_end.
> In addition it's also ok to allow family..local_port range
> (unlike lwt where sk may not be present).
> and no access to data_meta and flow_keys.
Thanks Alexei! I will send v2 shortly.
Song
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: add cg_skb_is_valid_access for BPF_PROG_TYPE_CGROUP_SKB
From: Alexei Starovoitov @ 2018-10-17 19:02 UTC (permalink / raw)
To: Alexei Starovoitov, Song Liu
Cc: netdev@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
Kernel Team
In-Reply-To: <20181017172636.57adi6yv7znuaqg5@ast-mbp.dhcp.thefacebook.com>
On 10/17/18 10:26 AM, Alexei Starovoitov wrote:
> On Tue, Oct 16, 2018 at 10:56:05PM -0700, Song Liu wrote:
>> BPF programs of BPF_PROG_TYPE_CGROUP_SKB need to access headers in the
>> skb. This patch enables direct access of skb for these programs.
>
> The lack of direct packet access in CGROUP_SKB progs was
> an unpleasant surprise to me, so thank you for fixing it,
> but there are few issues with the patch. See below.
>
>> In __cgroup_bpf_run_filter_skb(), bpf_compute_data_pointers() is called
>> to compute proper data_end for the BPF program.
>>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>> kernel/bpf/cgroup.c | 4 ++++
>> net/core/filter.c | 26 +++++++++++++++++++++++++-
>> 2 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>> index 00f6ed2e4f9a..340d496f35bd 100644
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -566,6 +566,10 @@ int __cgroup_bpf_run_filter_skb(struct sock *sk,
>> save_sk = skb->sk;
>> skb->sk = sk;
>> __skb_push(skb, offset);
>> +
>> + /* compute pointers for the bpf prog */
>> + bpf_compute_data_pointers(skb);
>> +
>> ret = BPF_PROG_RUN_ARRAY(cgrp->bpf.effective[type], skb,
>> bpf_prog_run_save_cb);
>> __skb_pull(skb, offset);
>> diff --git a/net/core/filter.c b/net/core/filter.c
>> index 1a3ac6c46873..8b5a502e241f 100644
>> --- a/net/core/filter.c
>> +++ b/net/core/filter.c
>> @@ -5346,6 +5346,30 @@ static bool sk_filter_is_valid_access(int off, int size,
>> return bpf_skb_is_valid_access(off, size, type, prog, info);
>> }
>>
>> +static bool cg_skb_is_valid_access(int off, int size,
>> + enum bpf_access_type type,
>> + const struct bpf_prog *prog,
>> + struct bpf_insn_access_aux *info)
>> +{
>> + if (type == BPF_WRITE)
>> + return false;
>
> this disables writes into cb[0..4] that were allowed for cgroup_inet_* before.
> One can argue that this may break existing progs,
> but looking at the place where BPF_CGROUP_RUN_PROG_INET_INGRESS is called
> it seems it's actually not correct in all cases to access cb there.
> Just few lines down we call bpf_prog_run_save_cb() which save/restores
> these 24 bytes.
> So we have two option either add save/restore for INET_INGRESS only
> or disable read and write access to cb[0..4] for CGROUP_SKB progs.
> I prefer the former.
>
>> +
>> + switch (off) {
>> + case bpf_ctx_range(struct __sk_buff, len):
>> + break;
>> + case bpf_ctx_range(struct __sk_buff, data):
>> + info->reg_type = PTR_TO_PACKET;
>> + break;
>> + case bpf_ctx_range(struct __sk_buff, data_end):
>> + info->reg_type = PTR_TO_PACKET_END;
>> + break;
>> + default:
>> + return false;
>> + }
>
> this also enables access to a range of fields family..local_port.
> It's ok to do for egress, but not for ingress unless we
> add code similar to the bottom of sk_filter_trim_cap() that
> inits skb->sk.
>
> above change also allows access to data_meta and flow_keys
> which is not correct.
>
> Considering all that I'm proposing to fix INET_INGRESS call site
> similar to code below it in sk_filter_trim_cap().
> In particular to do:
> struct sock *save_sk = skb->sk;
> skb->sk = sk;
> save and clear cb
> BPF_CGROUP_RUN_PROG_INET_INGRESS
> restore cb
> skb->sk = save_sk;
>
> all of above can probaby be inside BPF_CGROUP_RUN_PROG_INET_INGRESS macro.
> Then in this cg_skb_is_valid_access() allow access to data/data_end
> and family..local_port range as well.
> while disallowing access to flow_keys and data_meta.
>
> In patch 2 we gotta have tests for all these fields.
>
> Thoughts?
chatted with Song offline.
I completely misread 'return false' in the above as 'break'.
The patch actually disables access to pkt_type, mark, queue_mapping
and so on. Which is not correct either.
Since tests were not failing we really need to improve this aspect
of test coverage in test_verifier.c
Also I missed that __cgroup_bpf_run_filter_skb() already
does save_sk = skb->sk; skb->sk = sk;
and bpf_prog_run_save_cb()
So no issue in the existing code. That was false alarm.
Revising the proposal...
I think cg_skb_is_valid_access() can be made similar to
lwt_is_valid_access().
Allowing writes into mark, priority, cb[0..4]
and read of data/data_end.
In addition it's also ok to allow family..local_port range
(unlike lwt where sk may not be present).
and no access to data_meta and flow_keys.
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: emit RECORD_MMAP events for bpf prog load/unload
From: Arnaldo Carvalho de Melo @ 2018-10-17 18:53 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David Ahern, Song Liu, Alexei Starovoitov, Peter Zijlstra,
Alexei Starovoitov, David S . Miller, Daniel Borkmann, Networking,
Kernel Team
In-Reply-To: <97707bf2-ace3-18a5-3621-f69122dd93df@fb.com>
Em Wed, Oct 17, 2018 at 04:36:08PM +0000, Alexei Starovoitov escreveu:
> On 10/17/18 8:09 AM, David Ahern wrote:
> > On 10/16/18 11:43 PM, Song Liu wrote:
> >> I agree that processing events while recording has significant overhead.
> >> In this case, perf user space need to know details about the the jited BPF
> >> program. It is impossible to pass all these details to user space through
> >> the relatively stable ring_buffer API. Therefore, some processing of the
> >> data is necessary (get bpf prog_id from ring buffer, and then fetch program
> >> details via BPF_OBJ_GET_INFO_BY_FD.
> >>
> >> I have some idea on processing important data with relatively low overhead.
> >> Let me try implement it.
> >>
> >
> > As I understand it, you want this series:
> >
> > kernel: add event to perf buffer on bpf prog load
> >
> > userspace: perf reads the event and grabs information about the program
> > from the fd
> >
> > Is that correct?
> >
> > Userpsace is not awakened immediately when an event is added the the
> > ring. It is awakened once the number of events crosses a watermark. That
> > means there is an unknown - and potentially long - time window where the
> > program can be unloaded before perf reads the event.
> > So no matter what you do expecting perf record to be able to process the
> > event quickly is an unreasonable expectation.
> yes... unless we go with threaded model as Arnaldo suggested and use
> single event as a watermark to wakeup our perf thread.
> In such case there is still a race window between user space waking up
> and doing _single_ bpf_get_fd_from_id() call to hold that prog
> and some other process trying to instantly unload the prog it
> just loaded.
> I think such race window is extremely tiny and if perf misses
> those load/unload events it's a good thing, since there is no chance
> that normal pmu event samples would be happening during prog execution.
> The alternative approach with no race window at all is to burden kernel
> RECORD_* events with _all_ information about bpf prog. Which is jited
> addresses, jited image itself, info about all subprogs, info about line
> info, all BTF data, etc. As I said earlier I'm strongly against such
> RECORD_* bloating.
> Instead we need to find a way to process new RECORD_BPF events with
> single prog_id field in perf user space with minimal race
> and threaded approach sounds like a win to me.
There is another alternative, I think: put just a content based hash,
like a git commit id into a PERF_RECORD_MMAP3 new record, and when the
validator does the jit, etc, it stashes the content that
BPF_OBJ_GET_INFO_BY_FD would get somewhere, some filesystem populated by
the kernel right after getting stuff from sys_bpf() and preparing it for
use, then we know that in (start, end) we have blob foo with content id,
that we will use to retrieve information that augments what we know with
just (start, end, id) and allows annotation, etc.
That stash space for jitted stuff gets garbage collected from time to
time or is even completely disabled if the user is not interested in
such augmentation, just like one can do disabling perf's ~/.debug/
directory hashed by build-id.
I think with this we have no races, the PERF_RECORD_MMAP3 gets just what
is in PERF_RECORD_MMAP2 plus some extra 20 bytes for such content based
cookie and we solve the other race we already have with kernel modules,
DSOs, etc.
I have mentioned this before, there were objections, perhaps this time I
formulated in a different way that makes it more interesting?
- Arnaldo
^ permalink raw reply
* Re: Form sk_buff from DMA page
From: David Miller @ 2018-10-17 18:49 UTC (permalink / raw)
To: keyurp; +Cc: netdev
In-Reply-To: <BN6PR02MB2641F9CF498E60FB3AD98DE6B7FF0@BN6PR02MB2641.namprd02.prod.outlook.com>
From: Keyur Amrutbhai Patel <keyurp@xilinx.com>
Date: Wed, 17 Oct 2018 17:32:33 +0000
> Can anyone help me on how to form sk_buff from DMA page? Basically I
> get complete packet from DMA as single page.
Read any driver that calls build_skb().
^ permalink raw reply
* Re: ipmr, ip6mr: Unite dumproute flows
From: Nikolay Aleksandrov @ 2018-10-17 18:49 UTC (permalink / raw)
To: Colin Ian King, Yuval Mintz
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
netdev@vger.kernel.org
In-Reply-To: <c3d958d0-9916-48d9-ad8d-076cda75c93f@canonical.com>
On 17/10/2018 21:22, Colin Ian King wrote:
> Hi,
>
> Static analysis on linux-next has picked up a potential bug with commit:
>
> commit 7b0db85737db3f4d76b2a412e4f19eae59b8b494
> Author: Yuval Mintz <yuvalm@mellanox.com>
> Date: Wed Feb 28 23:29:39 2018 +0200
>
> ipmr, ip6mr: Unite dumproute flows
>
> in function mr_table_dump(), s_e is and never seems to change, so the
> check if (e < s_e) is never true: See code below, as annotated by
> CoverityScan:
>
> 317 }
> assignment: Assigning: e = 0U.
>
> 318 e = 0;
> assignment: Assigning: s_e = 0U.
>
> 319 s_e = 0;
> 320
> 321 spin_lock_bh(lock);
> 322 list_for_each_entry(mfc, &mrt->mfc_unres_queue, list) {
> at_least: At condition e < s_e, the value of e must be at least 0.
> const: At condition e < s_e, the value of s_e must be equal to 0.
> dead_error_condition: The condition e < s_e cannot be true.
>
> 323 if (e < s_e)
>
> CID 1474511 (#1 of 1): Logically dead code (DEADCODE)
> dead_error_line: Execution cannot reach this statement: goto next_entry2;.
>
> 324 goto next_entry2;
> 325 if (filter->dev &&
> 326 !mr_mfc_uses_dev(mrt, mfc, filter->dev))
> 327 goto next_entry2;
> 328
> 329 err = fill(mrt, skb, NETLINK_CB(cb->skb).portid,
> 330 cb->nlh->nlmsg_seq, mfc, RTM_NEWROUTE, flags);
> 331 if (err < 0) {
> 332 spin_unlock_bh(lock);
> 333 goto out;
> 334 }
> 335next_entry2:
>
> incr: Incrementing e. The value of e is now 1.
> incr: Incrementing e. The value of e is now 2.
>
> 336 e++;
>
>
> Note that the zero'ing of e and s_e for ipmr and ip6mr was added in by
> earlier commits:
>
> commit 8fb472c09b9df478a062eacc7841448e40fc3c17
> Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> Date: Thu Jan 12 15:53:33 2017 +0100
>
> ipmr: improve hash scalability
>
> commit 87c418bf1323d57140f4b448715f64de3fbb7e91
> Author: Yuval Mintz <yuvalm@mellanox.com>
> Date: Wed Feb 28 23:29:31 2018 +0200
>
> ip6mr: Align hash implementation to ipmr
>
> Anyhow, this looks incorrect, but I'm not familiar with this code to
> suggest the correct fix.
>
> Colin
>
Indeed, there's a case where we might miss an unresolved entry due to the zeroing.
I'll send a fix shortly after testing.
Thanks,
Nik
^ permalink raw reply
* Fwd: Re: [PATCH net] r8169: fix NAPI handling under high load
From: Heiner Kallweit @ 2018-10-17 18:48 UTC (permalink / raw)
To: Tomas Szepe; +Cc: netdev@vger.kernel.org, Holger Hoffstätte
In-Reply-To: <62974f0f-1938-3635-69d4-204ed8c587b3@gmail.com>
Tomas,
more than three years back you reported network problems after BQL
was added to the r8169 driver. Due to this the change was reverted.
Now the discussion to add BQL popped up again.
You mentioned that the issue exists on one of your systems only.
Therefore it could be an issue specific to a particular chip version.
I'd be interested in the chip version of the affected system.
You linked to another similar report, there the chip version was:
r8169 0000:10:00.0 eth0: RTL8168c/8111c at 0xf8130000, 00:e0:4c:68:48:d2, XID 1c4000c0 IRQ 29
In case you still have the affected system or at least the old dmesg
logs, I'd appreciate if you could let me know the quoted line from
dmesg output.
Thanks a lot,
Heiner
-------- Forwarded Message --------
Subject: Re: [PATCH net] r8169: fix NAPI handling under high load
Date: Wed, 17 Oct 2018 20:12:48 +0200
From: Heiner Kallweit <hkallweit1@gmail.com>
To: Holger Hoffstätte <holger@applied-asynchrony.com>, David Miller <davem@davemloft.net>, Realtek linux nic maintainers <nic_swsd@realtek.com>
CC: netdev@vger.kernel.org <netdev@vger.kernel.org>
On 16.10.2018 23:17, Holger Hoffstätte wrote:
> On 10/16/18 22:37, Heiner Kallweit wrote:
>> rtl_rx() and rtl_tx() are called only if the respective bits are set
>> in the interrupt status register. Under high load NAPI may not be
>> able to process all data (work_done == budget) and it will schedule
>> subsequent calls to the poll callback.
>> rtl_ack_events() however resets the bits in the interrupt status
>> register, therefore subsequent calls to rtl8169_poll() won't call
>> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
>
> Very interesting! Could this be the reason for the mysterious
> hangs & resets we experienced when enabling BQL for r8169?
> They happened more often with TSO/GSO enabled and several people
> attempted to fix those hangs unsuccessfully; it was later reverted
> and has been since then (#87cda7cb43).
> If this bug has been there "forever" it might be tempting to
> re-apply BQL and see what happens. Any chance you could give that
> a try? I'll gladly test patches, just like I'll run this one.
>
After reading through the old mail threads regarding BQL on r8169
I don't think the fix here is related.
It seems that BQL on r8169 worked fine for most people, just one
had problems on one of his systems. I assume the issue was specific
to one chip version. From the ~ 50 chip versions supported by
r8169 more or less each one requires its own quirks.
If we're lucky the chip-version-specific issue has been fixed in
the meantime and we can simply apply the old BQL patch again.
If it turns out that certain chip versions can't be used with BQL,
then we can disable the feature for these chip versions instead
of removing the feature completely.
I will apply the old BQL patch and see how it's on my system
(with GRO and SG enabled).
> cheers
> Holger
>
Heiner
^ permalink raw reply
* Re: [PATCH net-next V2 6/8] vhost: packed ring support
From: Jason Wang @ 2018-10-18 2:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, linux-kernel, virtualization, maxime.coquelin, wexu
In-Reply-To: <20181015062241-mutt-send-email-mst@kernel.org>
On 2018/10/15 下午6:25, Michael S. Tsirkin wrote:
> On Mon, Oct 15, 2018 at 10:51:06AM +0800, Jason Wang wrote:
>> On 2018年10月15日 10:43, Michael S. Tsirkin wrote:
>>> On Mon, Oct 15, 2018 at 10:22:33AM +0800, Jason Wang wrote:
>>>> On 2018年10月13日 01:23, Michael S. Tsirkin wrote:
>>>>> On Fri, Oct 12, 2018 at 10:32:44PM +0800, Tiwei Bie wrote:
>>>>>> On Mon, Jul 16, 2018 at 11:28:09AM +0800, Jason Wang wrote:
>>>>>> [...]
>>>>>>> @@ -1367,10 +1397,48 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *arg
>>>>>>> vq->last_avail_idx = s.num;
>>>>>>> /* Forget the cached index value. */
>>>>>>> vq->avail_idx = vq->last_avail_idx;
>>>>>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) {
>>>>>>> + vq->last_avail_wrap_counter = wrap_counter;
>>>>>>> + vq->avail_wrap_counter = vq->last_avail_wrap_counter;
>>>>>>> + }
>>>>>>> break;
>>>>>>> case VHOST_GET_VRING_BASE:
>>>>>>> s.index = idx;
>>>>>>> s.num = vq->last_avail_idx;
>>>>>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED))
>>>>>>> + s.num |= vq->last_avail_wrap_counter << 31;
>>>>>>> + if (copy_to_user(argp, &s, sizeof(s)))
>>>>>>> + r = -EFAULT;
>>>>>>> + break;
>>>>>>> + case VHOST_SET_VRING_USED_BASE:
>>>>>>> + /* Moving base with an active backend?
>>>>>>> + * You don't want to do that.
>>>>>>> + */
>>>>>>> + if (vq->private_data) {
>>>>>>> + r = -EBUSY;
>>>>>>> + break;
>>>>>>> + }
>>>>>>> + if (copy_from_user(&s, argp, sizeof(s))) {
>>>>>>> + r = -EFAULT;
>>>>>>> + break;
>>>>>>> + }
>>>>>>> + if (vhost_has_feature(vq, VIRTIO_F_RING_PACKED)) {
>>>>>>> + wrap_counter = s.num >> 31;
>>>>>>> + s.num &= ~(1 << 31);
>>>>>>> + }
>>>>>>> + if (s.num > 0xffff) {
>>>>>>> + r = -EINVAL;
>>>>>>> + break;
>>>>>>> + }
>>>>>> Do we want to put wrap_counter at bit 15?
>>>>> I think I second that - seems to be consistent with
>>>>> e.g. event suppression structure and the proposed
>>>>> extension to driver notifications.
>>>> Ok, I assumes packed virtqueue support 64K but looks not. I can change it to
>>>> bit 15 and GET_VRING_BASE need to be changed as well.
>>>>
>>>>>> If put wrap_counter at bit 31, the check (s.num > 0xffff)
>>>>>> won't be able to catch the illegal index 0x8000~0xffff for
>>>>>> packed ring.
>>>>>>
>>>> Do we need to clarify this in the spec?
>>> Isn't this all internal vhost stuff?
>> I meant the illegal index 0x8000-0xffff.
> It does say packed virtqueues support up to 2 15 entries each.
>
> But yes we can add a requirement that devices do not expose
> larger rings. Split does not support 2**16 either, right?
> With 2**16 enties avail index becomes 0 and ring looks empty.
>
Yes, so it's better to clarify this in the spec.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* RE: Form sk_buff from DMA page
From: Keyur Amrutbhai Patel @ 2018-10-17 18:45 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev@vger.kernel.org
In-Reply-To: <20181017105050.695eab3b@shemminger-XPS-13-9360>
Hi Stephen,
Thanks you for answering my query.
I am looking into build_skb it seems addressing to my query.
Appreciated..
Any good article on sk_buff which explains everything about it?
Regards,
Keyur
-----Original Message-----
From: Stephen Hemminger <stephen@networkplumber.org>
Sent: Wednesday, October 17, 2018 11:21 PM
To: Keyur Amrutbhai Patel <keyurp@xilinx.com>
Cc: netdev@vger.kernel.org
Subject: Re: Form sk_buff from DMA page
EXTERNAL EMAIL
On Wed, 17 Oct 2018 17:32:33 +0000
Keyur Amrutbhai Patel <keyurp@xilinx.com> wrote:
> Hi,
>
> Can anyone help me on how to form sk_buff from DMA page? Basically I get complete packet from DMA as single page.
>
> Regards,
> Keyur
> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
Did you look at build_skb?
^ permalink raw reply
* Re: ipmr, ip6mr: Unite dumproute flows
From: Colin Ian King @ 2018-10-17 18:22 UTC (permalink / raw)
To: Yuval Mintz, Nikolay Aleksandrov
Cc: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
netdev@vger.kernel.org
Hi,
Static analysis on linux-next has picked up a potential bug with commit:
commit 7b0db85737db3f4d76b2a412e4f19eae59b8b494
Author: Yuval Mintz <yuvalm@mellanox.com>
Date: Wed Feb 28 23:29:39 2018 +0200
ipmr, ip6mr: Unite dumproute flows
in function mr_table_dump(), s_e is and never seems to change, so the
check if (e < s_e) is never true: See code below, as annotated by
CoverityScan:
317 }
assignment: Assigning: e = 0U.
318 e = 0;
assignment: Assigning: s_e = 0U.
319 s_e = 0;
320
321 spin_lock_bh(lock);
322 list_for_each_entry(mfc, &mrt->mfc_unres_queue, list) {
at_least: At condition e < s_e, the value of e must be at least 0.
const: At condition e < s_e, the value of s_e must be equal to 0.
dead_error_condition: The condition e < s_e cannot be true.
323 if (e < s_e)
CID 1474511 (#1 of 1): Logically dead code (DEADCODE)
dead_error_line: Execution cannot reach this statement: goto next_entry2;.
324 goto next_entry2;
325 if (filter->dev &&
326 !mr_mfc_uses_dev(mrt, mfc, filter->dev))
327 goto next_entry2;
328
329 err = fill(mrt, skb, NETLINK_CB(cb->skb).portid,
330 cb->nlh->nlmsg_seq, mfc, RTM_NEWROUTE, flags);
331 if (err < 0) {
332 spin_unlock_bh(lock);
333 goto out;
334 }
335next_entry2:
incr: Incrementing e. The value of e is now 1.
incr: Incrementing e. The value of e is now 2.
336 e++;
Note that the zero'ing of e and s_e for ipmr and ip6mr was added in by
earlier commits:
commit 8fb472c09b9df478a062eacc7841448e40fc3c17
Author: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Thu Jan 12 15:53:33 2017 +0100
ipmr: improve hash scalability
commit 87c418bf1323d57140f4b448715f64de3fbb7e91
Author: Yuval Mintz <yuvalm@mellanox.com>
Date: Wed Feb 28 23:29:31 2018 +0200
ip6mr: Align hash implementation to ipmr
Anyhow, this looks incorrect, but I'm not familiar with this code to
suggest the correct fix.
Colin
^ permalink raw reply
* Re: [PATCH net] r8169: fix NAPI handling under high load
From: Heiner Kallweit @ 2018-10-17 18:12 UTC (permalink / raw)
To: Holger Hoffstätte, David Miller,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <ada39488-8e9c-d6a8-461e-672642497db2@applied-asynchrony.com>
On 16.10.2018 23:17, Holger Hoffstätte wrote:
> On 10/16/18 22:37, Heiner Kallweit wrote:
>> rtl_rx() and rtl_tx() are called only if the respective bits are set
>> in the interrupt status register. Under high load NAPI may not be
>> able to process all data (work_done == budget) and it will schedule
>> subsequent calls to the poll callback.
>> rtl_ack_events() however resets the bits in the interrupt status
>> register, therefore subsequent calls to rtl8169_poll() won't call
>> rtl_rx() and rtl_tx() - chip interrupts are still disabled.
>
> Very interesting! Could this be the reason for the mysterious
> hangs & resets we experienced when enabling BQL for r8169?
> They happened more often with TSO/GSO enabled and several people
> attempted to fix those hangs unsuccessfully; it was later reverted
> and has been since then (#87cda7cb43).
> If this bug has been there "forever" it might be tempting to
> re-apply BQL and see what happens. Any chance you could give that
> a try? I'll gladly test patches, just like I'll run this one.
>
After reading through the old mail threads regarding BQL on r8169
I don't think the fix here is related.
It seems that BQL on r8169 worked fine for most people, just one
had problems on one of his systems. I assume the issue was specific
to one chip version. From the ~ 50 chip versions supported by
r8169 more or less each one requires its own quirks.
If we're lucky the chip-version-specific issue has been fixed in
the meantime and we can simply apply the old BQL patch again.
If it turns out that certain chip versions can't be used with BQL,
then we can disable the feature for these chip versions instead
of removing the feature completely.
I will apply the old BQL patch and see how it's on my system
(with GRO and SG enabled).
> cheers
> Holger
>
Heiner
^ permalink raw reply
* Re: [PATCH net] sctp: fix the data size calculation in sctp_data_size
From: Marcelo Ricardo Leitner @ 2018-10-17 18:00 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman
In-Reply-To: <c407b682a424f0441d9b9a29ef6296c8e9824b64.1539781887.git.lucien.xin@gmail.com>
On Wed, Oct 17, 2018 at 09:11:27PM +0800, Xin Long wrote:
> sctp data size should be calculated by subtracting data chunk header's
> length from chunk_hdr->length, not just data header.
>
> Fixes: 668c9beb9020 ("sctp: implement assign_number for sctp_stream_interleave")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> include/net/sctp/sm.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/net/sctp/sm.h b/include/net/sctp/sm.h
> index 5ef1bad..9e3d327 100644
> --- a/include/net/sctp/sm.h
> +++ b/include/net/sctp/sm.h
> @@ -347,7 +347,7 @@ static inline __u16 sctp_data_size(struct sctp_chunk *chunk)
> __u16 size;
>
> size = ntohs(chunk->chunk_hdr->length);
> - size -= sctp_datahdr_len(&chunk->asoc->stream);
> + size -= sctp_datachk_len(&chunk->asoc->stream);
>
> return size;
> }
> --
> 2.1.0
>
^ permalink raw reply
* Re: Form sk_buff from DMA page
From: Stephen Hemminger @ 2018-10-17 17:50 UTC (permalink / raw)
To: Keyur Amrutbhai Patel; +Cc: netdev@vger.kernel.org
In-Reply-To: <BN6PR02MB2641F9CF498E60FB3AD98DE6B7FF0@BN6PR02MB2641.namprd02.prod.outlook.com>
On Wed, 17 Oct 2018 17:32:33 +0000
Keyur Amrutbhai Patel <keyurp@xilinx.com> wrote:
> Hi,
>
> Can anyone help me on how to form sk_buff from DMA page? Basically I get complete packet from DMA as single page.
>
> Regards,
> Keyur
> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
Did you look at build_skb?
^ permalink raw reply
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
From: Martin Lau @ 2018-10-17 17:50 UTC (permalink / raw)
To: Yonghong Song
Cc: Edward Cree, Alexei Starovoitov, daniel@iogearbox.net,
netdev@vger.kernel.org, Kernel Team
In-Reply-To: <0d0534e8-d05a-5541-2380-58a4ea36551b@fb.com>
On Wed, Oct 17, 2018 at 10:25:21AM -0700, Yonghong Song wrote:
>
>
> On 10/17/18 9:13 AM, Edward Cree wrote:
> > On 17/10/18 08:23, Yonghong Song wrote:
> >> This patch adds BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
> >> support to the type section. BTF_KIND_FUNC_PROTO is used
> >> to specify the type of a function pointer. With this,
> >> BTF has a complete set of C types (except float).
> >>
> >> BTF_KIND_FUNC is used to specify the signature of a
> >> defined subprogram. BTF_KIND_FUNC_PROTO can be referenced
> >> by another type, e.g., a pointer type, and BTF_KIND_FUNC
> >> type cannot be referenced by another type.
> > Why are distinct kinds created for these? A function body is
> > a value of function type, and since there's no way (in C) to
> > declare a variable of function type (only pointer-to-
> > function), any declaration of function type must necessarily
> > be a BTF_KIND_FUNC, whereas any other reference to a function
> > type (e.g. a declaration of type pointer to function type)
> > must, as you state above, be a BTF_KIND_FUNC_PROTO.
> > In fact, you can tell the difference just from name_off, since
> > a (C-legal) BTF_KIND_FUNC_PROTO will always be anonymous (as
> > the pointee of a pointer type), while a BTF_KIND_FUNC will
> > have the name of the subprogram.
>
> What you stated is true, BTF_KIND_FUNC_PROTO corresponds to
> dwarf subroutine tag which has no name while BTF_KIND_FUNC
> must have a valid name. The original design is to have both
> since they are corresponding to different dwarf constructs.
>
> Martin, what do you think?
I prefer to have separate kinds. We need a way to distinguish them.
For example, the BTF verifier is checking it. Having two kinds is
cleaner instead of resorting to other hints from 'struct btf_type'.
We don't lack of bits for kind.
>
> >
> > -Ed
> >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox