* Re: kernel BUG at drivers/vhost/vhost.c:LINE! (2)
From: Stefan Hajnoczi @ 2018-04-09 13:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, netdev, syzkaller-bugs, syzbot, linux-kernel,
Linux Virtualization
In-Reply-To: <20180409032835.GB1648@stefanha-x1.localdomain>
On Mon, Apr 9, 2018 at 11:28 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Apr 09, 2018 at 05:44:36AM +0300, Michael S. Tsirkin wrote:
>> On Mon, Apr 09, 2018 at 10:37:45AM +0800, Stefan Hajnoczi wrote:
>> > On Sat, Apr 7, 2018 at 3:02 AM, syzbot
>> > <syzbot+65a84dde0214b0387ccd@syzkaller.appspotmail.com> wrote:
>> > > syzbot hit the following crash on upstream commit
>> > > 38c23685b273cfb4ccf31a199feccce3bdcb5d83 (Fri Apr 6 04:29:35 2018 +0000)
>> > > Merge tag 'armsoc-drivers' of
>> > > git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc
>> > > syzbot dashboard link:
>> > > https://syzkaller.appspot.com/bug?extid=65a84dde0214b0387ccd
>> >
>> > To prevent duplicated work: I am working on this one.
>> >
>> > Stefan
>>
>> Do you want to try this patchset:
>> https://lkml.org/lkml/2018/4/5/665
>>
>> ?
>
> Thanks, I'll give it a shot.
>
> I also noticed a regression in commit
> d65026c6c62e7d9616c8ceb5a53b68bcdc050525 ("vhost: validate log when
> IOTLB is enabled") and am currently testing a fix.
I have sent a fix:
https://lkml.org/lkml/2018/4/9/390
Stefan
^ permalink raw reply
* Re: WARNING in ip_rt_bug
From: Eric Dumazet @ 2018-04-09 13:11 UTC (permalink / raw)
To: Dmitry Vyukov, syzbot
Cc: David Miller, Alexey Kuznetsov, LKML, netdev, syzkaller-bugs,
Hideaki YOSHIFUJI, Eric Dumazet
In-Reply-To: <CACT4Y+YfM=SL3-cgeJ_7JCvAwX2KsyMWV=HHpGfLV=-RKo7TQQ@mail.gmail.com>
On 04/08/2018 11:06 PM, Dmitry Vyukov wrote:
> On Mon, Apr 9, 2018 at 7:59 AM, syzbot
> <syzbot+b09ac67a2af842b12eab@syzkaller.appspotmail.com> wrote:
>> Hello,
>>
>> syzbot hit the following crash on net-next commit
>> 8bde261e535257e81087d39ff808414e2f5aa39d (Sun Apr 1 02:31:43 2018 +0000)
>> Merge tag 'mlx5-updates-2018-03-30' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux
>> syzbot dashboard link:
>> https://syzkaller.appspot.com/bug?extid=b09ac67a2af842b12eab
>>
>> Unfortunately, I don't have any reproducer for this crash yet.
>> Raw console output:
>> https://syzkaller.appspot.com/x/log.txt?id=5991727739437056
>> Kernel config:
>> https://syzkaller.appspot.com/x/.config?id=3327544840960562528
>> compiler: gcc (GCC) 7.1.1 20170620
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+b09ac67a2af842b12eab@syzkaller.appspotmail.com
>> It will help syzbot understand when the bug is fixed. See footer for
>> details.
>> If you forward the report, please keep this part and the footer.
>
>
> +Eric said that perhaps we just need to revert:
>
> commit c378a9c019cf5e017d1ed24954b54fae7bebd2bc
> Date: Sat May 21 07:16:42 2011 +0000
> ipv4: Give backtrace in ip_rt_bug().
>
And David replied :
<quote>
Let's not do the revert, I wouldn't have seen the backtrace which
points where this bug is if we had.
icmp_route_lookup(), in one branch, does an input route lookup and
uses the result of that to send the icmp message.
That can't be right, input routes should never be used for
transmitting traffice and that's how we end up at ip_rt_bug().
</quote>
^ permalink raw reply
* [PATCH] net/mlx5: remove some extraneous spaces in indentations
From: Colin King @ 2018-04-09 12:43 UTC (permalink / raw)
To: Saeed Mahameed, Matan Barak, Leon Romanovsky, netdev, linux-rdma
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There are several lines where there is an extraneous space causing
indentation misalignment. Remove them.
Cleans up Cocconelle warnings:
./drivers/net/ethernet/mellanox/mlx5/core/qp.c:409:3-18: code aligned
with following code on line 410
./drivers/net/ethernet/mellanox/mlx5/core/qp.c:415:3-18: code aligned
with following code on line 416
./drivers/net/ethernet/mellanox/mlx5/core/qp.c:421:3-18: code aligned
with following code on line 422
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/ethernet/mellanox/mlx5/core/qp.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/qp.c b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
index 02d6c5b5d502..4ca07bfb6b14 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/qp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
@@ -407,21 +407,21 @@ static int modify_qp_mbox_alloc(struct mlx5_core_dev *dev, u16 opcode, int qpn,
case MLX5_CMD_OP_RST2INIT_QP:
if (MBOX_ALLOC(mbox, rst2init_qp))
return -ENOMEM;
- MOD_QP_IN_SET_QPC(rst2init_qp, mbox->in, opcode, qpn,
- opt_param_mask, qpc);
- break;
+ MOD_QP_IN_SET_QPC(rst2init_qp, mbox->in, opcode, qpn,
+ opt_param_mask, qpc);
+ break;
case MLX5_CMD_OP_INIT2RTR_QP:
if (MBOX_ALLOC(mbox, init2rtr_qp))
return -ENOMEM;
- MOD_QP_IN_SET_QPC(init2rtr_qp, mbox->in, opcode, qpn,
- opt_param_mask, qpc);
- break;
+ MOD_QP_IN_SET_QPC(init2rtr_qp, mbox->in, opcode, qpn,
+ opt_param_mask, qpc);
+ break;
case MLX5_CMD_OP_RTR2RTS_QP:
if (MBOX_ALLOC(mbox, rtr2rts_qp))
return -ENOMEM;
- MOD_QP_IN_SET_QPC(rtr2rts_qp, mbox->in, opcode, qpn,
- opt_param_mask, qpc);
- break;
+ MOD_QP_IN_SET_QPC(rtr2rts_qp, mbox->in, opcode, qpn,
+ opt_param_mask, qpc);
+ break;
case MLX5_CMD_OP_RTS2RTS_QP:
if (MBOX_ALLOC(mbox, rts2rts_qp))
return -ENOMEM;
--
2.15.1
^ permalink raw reply related
* Re: [PATCH 2/2] alx: add disable_wol paramenter
From: Andrew Lunn @ 2018-04-09 12:39 UTC (permalink / raw)
To: AceLan Kao
Cc: Jay Cliburn, Chris Snook, David S . Miller, Rakesh Pandit, netdev,
linux-kernel
In-Reply-To: <1523273714-17264-2-git-send-email-acelan.kao@canonical.com>
On Mon, Apr 09, 2018 at 07:35:14PM +0800, AceLan Kao wrote:
> The WoL feature was reported broken and will lead to
> the system resume immediately after suspending.
> This symptom is not happening on every system, so adding
> disable_wol option and disable WoL by default to prevent the issue from
> happening again.
> const char alx_drv_name[] = "alx";
>
> +/* disable WoL by default */
> +bool disable_wol = 1;
> +module_param(disable_wol, bool, 0);
> +MODULE_PARM_DESC(disable_wol, "Disable Wake on Lan feature");
> +
Hi AceLan
This seems like you are papering over the cracks. And module
parameters are not liked.
Please try to find the real problem.
Andrew
^ permalink raw reply
* Re: Passing uninitialised local variable
From: Petr Machata @ 2018-04-09 12:23 UTC (permalink / raw)
To: Arend van Spriel
Cc: Himanshu Jha, franky.lin, hante.meuleman, chi-hsien.lin,
wright.feng, kvalo, johannes.berg, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, netdev
In-Reply-To: <5ABD5735.1050608@broadcom.com>
Arend van Spriel <arend.vanspriel@broadcom.com> writes:
> On 3/28/2018 1:20 PM, Himanshu Jha wrote:
>> I recently found that a local variable in passed uninitialised to the
>> function at
>>
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:2950
>>
>> u32 var;
>> err = brcmf_fil_iovar_int_get(ifp, "dtim_assoc", &var);
>>
>> s32
>> brcmf_fil_iovar_int_get(struct brcmf_if *ifp, char *name, u32 *data)
>> {
>> __le32 data_le = cpu_to_le32(*data);
>> }
>>
>> We can cleary see that 'var' in used uninitialised in the very first line
>> which is an undefined behavior.
>
> Why undefined? We copy some stack data and we do transfer that to the device. However in this case
> the device does nothing with it and it is simply overwritten by the response.
"Undefined behavior" is a technical term for when there are no
guarantees as to what the result of executing a given code will be. None
at all--it might for example abort, and that would be perfectly valid as
well. (To be clear, this is not about the device, but about the CPU that
this code runs on.)
Uninitialized reads are one example of a code construct that invokes
undefined behavior.
Thanks,
Petr
^ permalink raw reply
* [PATCH 2/2] alx: add disable_wol paramenter
From: AceLan Kao @ 2018-04-09 11:35 UTC (permalink / raw)
To: Jay Cliburn, Chris Snook, David S . Miller, Rakesh Pandit, netdev,
linux-kernel
In-Reply-To: <1523273714-17264-1-git-send-email-acelan.kao@canonical.com>
The WoL feature was reported broken and will lead to
the system resume immediately after suspending.
This symptom is not happening on every system, so adding
disable_wol option and disable WoL by default to prevent the issue from
happening again.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=61651
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
drivers/net/ethernet/atheros/alx/ethtool.c | 7 ++++++-
drivers/net/ethernet/atheros/alx/main.c | 5 +++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 859e272..e50129d 100644
--- a/drivers/net/ethernet/atheros/alx/ethtool.c
+++ b/drivers/net/ethernet/atheros/alx/ethtool.c
@@ -46,6 +46,8 @@
#include "reg.h"
#include "hw.h"
+extern const bool disable_wol;
+
/* The order of these strings must match the order of the fields in
* struct alx_hw_stats
* See hw.h
@@ -315,6 +317,9 @@ static void alx_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
struct alx_priv *alx = netdev_priv(netdev);
struct alx_hw *hw = &alx->hw;
+ if (disable_wol)
+ return;
+
wol->supported = WAKE_MAGIC | WAKE_PHY;
wol->wolopts = 0;
@@ -329,7 +334,7 @@ static int alx_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
struct alx_priv *alx = netdev_priv(netdev);
struct alx_hw *hw = &alx->hw;
- if (wol->wolopts & ~(WAKE_MAGIC | WAKE_PHY))
+ if (disable_wol || (wol->wolopts & ~(WAKE_MAGIC | WAKE_PHY)))
return -EOPNOTSUPP;
hw->sleep_ctrl = 0;
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index c0e2bb2..c27fdf7 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -51,6 +51,11 @@
const char alx_drv_name[] = "alx";
+/* disable WoL by default */
+bool disable_wol = 1;
+module_param(disable_wol, bool, 0);
+MODULE_PARM_DESC(disable_wol, "Disable Wake on Lan feature");
+
static void alx_free_txbuf(struct alx_tx_queue *txq, int entry)
{
struct alx_buffer *txb = &txq->bufs[entry];
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] Revert "alx: remove WoL support"
From: AceLan Kao @ 2018-04-09 11:35 UTC (permalink / raw)
To: Jay Cliburn, Chris Snook, David S . Miller, Rakesh Pandit, netdev,
linux-kernel
This reverts commit bc2bebe8de8ed4ba6482c9cc370b0dd72ffe8cd2.
There are still many people need this feature, so try adding it back.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=61651
Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
---
drivers/net/ethernet/atheros/alx/ethtool.c | 36 +++++++
drivers/net/ethernet/atheros/alx/hw.c | 154 ++++++++++++++++++++++++++++-
drivers/net/ethernet/atheros/alx/hw.h | 5 +
drivers/net/ethernet/atheros/alx/main.c | 142 ++++++++++++++++++++++++--
4 files changed, 326 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/atheros/alx/ethtool.c b/drivers/net/ethernet/atheros/alx/ethtool.c
index 2f4eabf..859e272 100644
--- a/drivers/net/ethernet/atheros/alx/ethtool.c
+++ b/drivers/net/ethernet/atheros/alx/ethtool.c
@@ -310,11 +310,47 @@ static int alx_get_sset_count(struct net_device *netdev, int sset)
}
}
+static void alx_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
+{
+ struct alx_priv *alx = netdev_priv(netdev);
+ struct alx_hw *hw = &alx->hw;
+
+ wol->supported = WAKE_MAGIC | WAKE_PHY;
+ wol->wolopts = 0;
+
+ if (hw->sleep_ctrl & ALX_SLEEP_WOL_MAGIC)
+ wol->wolopts |= WAKE_MAGIC;
+ if (hw->sleep_ctrl & ALX_SLEEP_WOL_PHY)
+ wol->wolopts |= WAKE_PHY;
+}
+
+static int alx_set_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
+{
+ struct alx_priv *alx = netdev_priv(netdev);
+ struct alx_hw *hw = &alx->hw;
+
+ if (wol->wolopts & ~(WAKE_MAGIC | WAKE_PHY))
+ return -EOPNOTSUPP;
+
+ hw->sleep_ctrl = 0;
+
+ if (wol->wolopts & WAKE_MAGIC)
+ hw->sleep_ctrl |= ALX_SLEEP_WOL_MAGIC;
+ if (wol->wolopts & WAKE_PHY)
+ hw->sleep_ctrl |= ALX_SLEEP_WOL_PHY;
+
+ device_set_wakeup_enable(&alx->hw.pdev->dev, hw->sleep_ctrl);
+
+ return 0;
+}
+
const struct ethtool_ops alx_ethtool_ops = {
.get_pauseparam = alx_get_pauseparam,
.set_pauseparam = alx_set_pauseparam,
.get_msglevel = alx_get_msglevel,
.set_msglevel = alx_set_msglevel,
+ .get_wol = alx_get_wol,
+ .set_wol = alx_set_wol,
.get_link = ethtool_op_get_link,
.get_strings = alx_get_strings,
.get_sset_count = alx_get_sset_count,
diff --git a/drivers/net/ethernet/atheros/alx/hw.c b/drivers/net/ethernet/atheros/alx/hw.c
index 6ac40b0..f9bf612 100644
--- a/drivers/net/ethernet/atheros/alx/hw.c
+++ b/drivers/net/ethernet/atheros/alx/hw.c
@@ -332,6 +332,16 @@ void alx_set_macaddr(struct alx_hw *hw, const u8 *addr)
alx_write_mem32(hw, ALX_STAD1, val);
}
+static void alx_enable_osc(struct alx_hw *hw)
+{
+ u32 val;
+
+ /* rising edge */
+ val = alx_read_mem32(hw, ALX_MISC);
+ alx_write_mem32(hw, ALX_MISC, val & ~ALX_MISC_INTNLOSC_OPEN);
+ alx_write_mem32(hw, ALX_MISC, val | ALX_MISC_INTNLOSC_OPEN);
+}
+
static void alx_reset_osc(struct alx_hw *hw, u8 rev)
{
u32 val, val2;
@@ -774,7 +784,6 @@ int alx_setup_speed_duplex(struct alx_hw *hw, u32 ethadv, u8 flowctrl)
return err;
}
-
void alx_post_phy_link(struct alx_hw *hw)
{
u16 phy_val, len, agc;
@@ -848,6 +857,65 @@ void alx_post_phy_link(struct alx_hw *hw)
}
}
+/* NOTE:
+ * 1. phy link must be established before calling this function
+ * 2. wol option (pattern,magic,link,etc.) is configed before call it.
+ */
+int alx_pre_suspend(struct alx_hw *hw, int speed, u8 duplex)
+{
+ u32 master, mac, phy, val;
+ int err = 0;
+
+ master = alx_read_mem32(hw, ALX_MASTER);
+ master &= ~ALX_MASTER_PCLKSEL_SRDS;
+ mac = hw->rx_ctrl;
+ /* 10/100 half */
+ ALX_SET_FIELD(mac, ALX_MAC_CTRL_SPEED, ALX_MAC_CTRL_SPEED_10_100);
+ mac &= ~(ALX_MAC_CTRL_FULLD | ALX_MAC_CTRL_RX_EN | ALX_MAC_CTRL_TX_EN);
+
+ phy = alx_read_mem32(hw, ALX_PHY_CTRL);
+ phy &= ~(ALX_PHY_CTRL_DSPRST_OUT | ALX_PHY_CTRL_CLS);
+ phy |= ALX_PHY_CTRL_RST_ANALOG | ALX_PHY_CTRL_HIB_PULSE |
+ ALX_PHY_CTRL_HIB_EN;
+
+ /* without any activity */
+ if (!(hw->sleep_ctrl & ALX_SLEEP_ACTIVE)) {
+ err = alx_write_phy_reg(hw, ALX_MII_IER, 0);
+ if (err)
+ return err;
+ phy |= ALX_PHY_CTRL_IDDQ | ALX_PHY_CTRL_POWER_DOWN;
+ } else {
+ if (hw->sleep_ctrl & (ALX_SLEEP_WOL_MAGIC | ALX_SLEEP_CIFS))
+ mac |= ALX_MAC_CTRL_RX_EN | ALX_MAC_CTRL_BRD_EN;
+ if (hw->sleep_ctrl & ALX_SLEEP_CIFS)
+ mac |= ALX_MAC_CTRL_TX_EN;
+ if (duplex == DUPLEX_FULL)
+ mac |= ALX_MAC_CTRL_FULLD;
+ if (speed == SPEED_1000)
+ ALX_SET_FIELD(mac, ALX_MAC_CTRL_SPEED,
+ ALX_MAC_CTRL_SPEED_1000);
+ phy |= ALX_PHY_CTRL_DSPRST_OUT;
+ err = alx_write_phy_ext(hw, ALX_MIIEXT_ANEG,
+ ALX_MIIEXT_S3DIG10,
+ ALX_MIIEXT_S3DIG10_SL);
+ if (err)
+ return err;
+ }
+
+ alx_enable_osc(hw);
+ hw->rx_ctrl = mac;
+ alx_write_mem32(hw, ALX_MASTER, master);
+ alx_write_mem32(hw, ALX_MAC_CTRL, mac);
+ alx_write_mem32(hw, ALX_PHY_CTRL, phy);
+
+ /* set val of PDLL D3PLLOFF */
+ val = alx_read_mem32(hw, ALX_PDLL_TRNS1);
+ val |= ALX_PDLL_TRNS1_D3PLLOFF_EN;
+ alx_write_mem32(hw, ALX_PDLL_TRNS1, val);
+
+ return 0;
+}
+
bool alx_phy_configured(struct alx_hw *hw)
{
u32 cfg, hw_cfg;
@@ -920,6 +988,26 @@ int alx_clear_phy_intr(struct alx_hw *hw)
return alx_read_phy_reg(hw, ALX_MII_ISR, &isr);
}
+int alx_config_wol(struct alx_hw *hw)
+{
+ u32 wol = 0;
+ int err = 0;
+
+ /* turn on magic packet event */
+ if (hw->sleep_ctrl & ALX_SLEEP_WOL_MAGIC)
+ wol |= ALX_WOL0_MAGIC_EN | ALX_WOL0_PME_MAGIC_EN;
+
+ /* turn on link up event */
+ if (hw->sleep_ctrl & ALX_SLEEP_WOL_PHY) {
+ wol |= ALX_WOL0_LINK_EN | ALX_WOL0_PME_LINK;
+ /* only link up can wake up */
+ err = alx_write_phy_reg(hw, ALX_MII_IER, ALX_IER_LINK_UP);
+ }
+ alx_write_mem32(hw, ALX_WOL0, wol);
+
+ return err;
+}
+
void alx_disable_rss(struct alx_hw *hw)
{
u32 ctrl = alx_read_mem32(hw, ALX_RXQ0);
@@ -1044,6 +1132,70 @@ void alx_mask_msix(struct alx_hw *hw, int index, bool mask)
alx_post_write(hw);
}
+int alx_select_powersaving_speed(struct alx_hw *hw, int *speed, u8 *duplex)
+{
+ int i, err;
+ u16 lpa;
+
+ err = alx_read_phy_link(hw);
+ if (err)
+ return err;
+
+ if (hw->link_speed == SPEED_UNKNOWN) {
+ *speed = SPEED_UNKNOWN;
+ *duplex = DUPLEX_UNKNOWN;
+ return 0;
+ }
+
+ err = alx_read_phy_reg(hw, MII_LPA, &lpa);
+ if (err)
+ return err;
+
+ if (!(lpa & LPA_LPACK)) {
+ *speed = hw->link_speed;
+ return 0;
+ }
+
+ if (lpa & LPA_10FULL) {
+ *speed = SPEED_10;
+ *duplex = DUPLEX_FULL;
+ } else if (lpa & LPA_10HALF) {
+ *speed = SPEED_10;
+ *duplex = DUPLEX_HALF;
+ } else if (lpa & LPA_100FULL) {
+ *speed = SPEED_100;
+ *duplex = DUPLEX_FULL;
+ } else {
+ *speed = SPEED_100;
+ *duplex = DUPLEX_HALF;
+ }
+
+ if (*speed == hw->link_speed && *duplex == hw->duplex)
+ return 0;
+ err = alx_write_phy_reg(hw, ALX_MII_IER, 0);
+ if (err)
+ return err;
+ err = alx_setup_speed_duplex(hw, alx_speed_to_ethadv(*speed, *duplex) |
+ ADVERTISED_Autoneg, ALX_FC_ANEG |
+ ALX_FC_RX | ALX_FC_TX);
+ if (err)
+ return err;
+
+ /* wait for linkup */
+ for (i = 0; i < ALX_MAX_SETUP_LNK_CYCLE; i++) {
+ msleep(100);
+
+ err = alx_read_phy_link(hw);
+ if (err < 0)
+ return err;
+ if (hw->link_speed != SPEED_UNKNOWN)
+ break;
+ }
+ if (i == ALX_MAX_SETUP_LNK_CYCLE)
+ return -ETIMEDOUT;
+
+ return 0;
+}
bool alx_get_phy_info(struct alx_hw *hw)
{
diff --git a/drivers/net/ethernet/atheros/alx/hw.h b/drivers/net/ethernet/atheros/alx/hw.h
index e42d7e0..a7fb6c8 100644
--- a/drivers/net/ethernet/atheros/alx/hw.h
+++ b/drivers/net/ethernet/atheros/alx/hw.h
@@ -487,6 +487,8 @@ struct alx_hw {
u8 flowctrl;
u32 adv_cfg;
+ u32 sleep_ctrl;
+
spinlock_t mdio_lock;
struct mdio_if_info mdio;
u16 phy_id[2];
@@ -549,12 +551,14 @@ void alx_reset_pcie(struct alx_hw *hw);
void alx_enable_aspm(struct alx_hw *hw, bool l0s_en, bool l1_en);
int alx_setup_speed_duplex(struct alx_hw *hw, u32 ethadv, u8 flowctrl);
void alx_post_phy_link(struct alx_hw *hw);
+int alx_pre_suspend(struct alx_hw *hw, int speed, u8 duplex);
int alx_read_phy_reg(struct alx_hw *hw, u16 reg, u16 *phy_data);
int alx_write_phy_reg(struct alx_hw *hw, u16 reg, u16 phy_data);
int alx_read_phy_ext(struct alx_hw *hw, u8 dev, u16 reg, u16 *pdata);
int alx_write_phy_ext(struct alx_hw *hw, u8 dev, u16 reg, u16 data);
int alx_read_phy_link(struct alx_hw *hw);
int alx_clear_phy_intr(struct alx_hw *hw);
+int alx_config_wol(struct alx_hw *hw);
void alx_cfg_mac_flowcontrol(struct alx_hw *hw, u8 fc);
void alx_start_mac(struct alx_hw *hw);
int alx_reset_mac(struct alx_hw *hw);
@@ -563,6 +567,7 @@ bool alx_phy_configured(struct alx_hw *hw);
void alx_configure_basic(struct alx_hw *hw);
void alx_mask_msix(struct alx_hw *hw, int index, bool mask);
void alx_disable_rss(struct alx_hw *hw);
+int alx_select_powersaving_speed(struct alx_hw *hw, int *speed, u8 *duplex);
bool alx_get_phy_info(struct alx_hw *hw);
void alx_update_hw_stats(struct alx_hw *hw);
diff --git a/drivers/net/ethernet/atheros/alx/main.c b/drivers/net/ethernet/atheros/alx/main.c
index 567ee54..c0e2bb2 100644
--- a/drivers/net/ethernet/atheros/alx/main.c
+++ b/drivers/net/ethernet/atheros/alx/main.c
@@ -1070,6 +1070,7 @@ static int alx_init_sw(struct alx_priv *alx)
alx->dev->max_mtu = ALX_MAX_FRAME_LEN(ALX_MAX_FRAME_SIZE);
alx->tx_ringsz = 256;
alx->rx_ringsz = 512;
+ hw->sleep_ctrl = ALX_SLEEP_WOL_MAGIC | ALX_SLEEP_WOL_PHY;
hw->imt = 200;
alx->int_mask = ALX_ISR_MISC;
hw->dma_chnl = hw->max_dma_chnl;
@@ -1346,6 +1347,66 @@ static int alx_stop(struct net_device *netdev)
return 0;
}
+static int __alx_shutdown(struct pci_dev *pdev, bool *wol_en)
+{
+ struct alx_priv *alx = pci_get_drvdata(pdev);
+ struct net_device *netdev = alx->dev;
+ struct alx_hw *hw = &alx->hw;
+ int err, speed;
+ u8 duplex;
+
+ netif_device_detach(netdev);
+
+ if (netif_running(netdev))
+ __alx_stop(alx);
+
+#ifdef CONFIG_PM_SLEEP
+ err = pci_save_state(pdev);
+ if (err)
+ return err;
+#endif
+
+ err = alx_select_powersaving_speed(hw, &speed, &duplex);
+ if (err)
+ return err;
+ err = alx_clear_phy_intr(hw);
+ if (err)
+ return err;
+ err = alx_pre_suspend(hw, speed, duplex);
+ if (err)
+ return err;
+ err = alx_config_wol(hw);
+ if (err)
+ return err;
+
+ *wol_en = false;
+ if (hw->sleep_ctrl & ALX_SLEEP_ACTIVE) {
+ netif_info(alx, wol, netdev,
+ "wol: ctrl=%X, speed=%X\n",
+ hw->sleep_ctrl, speed);
+ device_set_wakeup_enable(&pdev->dev, true);
+ *wol_en = true;
+ }
+
+ pci_disable_device(pdev);
+
+ return 0;
+}
+
+static void alx_shutdown(struct pci_dev *pdev)
+{
+ int err;
+ bool wol_en;
+
+ err = __alx_shutdown(pdev, &wol_en);
+ if (!err) {
+ pci_wake_from_d3(pdev, wol_en);
+ pci_set_power_state(pdev, PCI_D3hot);
+ } else {
+ dev_err(&pdev->dev, "shutdown fail %d\n", err);
+ }
+}
+
static void alx_link_check(struct work_struct *work)
{
struct alx_priv *alx;
@@ -1841,6 +1902,8 @@ static int alx_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto out_unmap;
}
+ device_set_wakeup_enable(&pdev->dev, hw->sleep_ctrl);
+
netdev_info(netdev,
"Qualcomm Atheros AR816x/AR817x Ethernet [%pM]\n",
netdev->dev_addr);
@@ -1883,12 +1946,22 @@ static void alx_remove(struct pci_dev *pdev)
static int alx_suspend(struct device *dev)
{
struct pci_dev *pdev = to_pci_dev(dev);
- struct alx_priv *alx = pci_get_drvdata(pdev);
+ int err;
+ bool wol_en;
+
+ err = __alx_shutdown(pdev, &wol_en);
+ if (err) {
+ dev_err(&pdev->dev, "shutdown fail in suspend %d\n", err);
+ return err;
+ }
+
+ if (wol_en) {
+ pci_prepare_to_sleep(pdev);
+ } else {
+ pci_wake_from_d3(pdev, false);
+ pci_set_power_state(pdev, PCI_D3hot);
+ }
- if (!netif_running(alx->dev))
- return 0;
- netif_device_detach(alx->dev);
- __alx_stop(alx);
return 0;
}
@@ -1896,23 +1969,69 @@ static int alx_resume(struct device *dev)
{
struct pci_dev *pdev = to_pci_dev(dev);
struct alx_priv *alx = pci_get_drvdata(pdev);
+ struct net_device *netdev = alx->dev;
struct alx_hw *hw = &alx->hw;
+ int err;
+
+ pci_set_power_state(pdev, PCI_D0);
+ pci_restore_state(pdev);
+ pci_save_state(pdev);
+
+ pci_enable_wake(pdev, PCI_D3hot, 0);
+ pci_enable_wake(pdev, PCI_D3cold, 0);
+ hw->link_speed = SPEED_UNKNOWN;
+ alx->int_mask = ALX_ISR_MISC;
+
+ alx_reset_pcie(hw);
alx_reset_phy(hw);
- if (!netif_running(alx->dev))
- return 0;
- netif_device_attach(alx->dev);
- return __alx_open(alx, true);
+ pci_set_power_state(pdev, PCI_D0);
+ pci_restore_state(pdev);
+ pci_save_state(pdev);
+
+ pci_enable_wake(pdev, PCI_D3hot, 0);
+ pci_enable_wake(pdev, PCI_D3cold, 0);
+
+ hw->link_speed = SPEED_UNKNOWN;
+ alx->int_mask = ALX_ISR_MISC;
+
+ alx_reset_pcie(hw);
+ alx_reset_phy(hw);
+
+ err = alx_reset_mac(hw);
+ if (err) {
+ netif_err(alx, hw, alx->dev,
+ "resume:reset_mac fail %d\n", err);
+ return -EIO;
+ }
+
+ err = alx_setup_speed_duplex(hw, hw->adv_cfg, hw->flowctrl);
+ if (err) {
+ netif_err(alx, hw, alx->dev,
+ "resume:setup_speed_duplex fail %d\n", err);
+ return -EIO;
+ }
+
+ if (netif_running(netdev)) {
+ err = __alx_open(alx, true);
+ if (err)
+ return err;
+ }
+
+ netif_device_attach(netdev);
+
+ return err;
}
+#endif
+#ifdef CONFIG_PM_SLEEP
static SIMPLE_DEV_PM_OPS(alx_pm_ops, alx_suspend, alx_resume);
#define ALX_PM_OPS (&alx_pm_ops)
#else
#define ALX_PM_OPS NULL
#endif
-
static pci_ers_result_t alx_pci_error_detected(struct pci_dev *pdev,
pci_channel_state_t state)
{
@@ -1955,6 +2074,8 @@ static pci_ers_result_t alx_pci_error_slot_reset(struct pci_dev *pdev)
}
pci_set_master(pdev);
+ pci_enable_wake(pdev, PCI_D3hot, 0);
+ pci_enable_wake(pdev, PCI_D3cold, 0);
alx_reset_pcie(hw);
if (!alx_reset_mac(hw))
@@ -2011,6 +2132,7 @@ static struct pci_driver alx_driver = {
.id_table = alx_pci_tbl,
.probe = alx_probe,
.remove = alx_remove,
+ .shutdown = alx_shutdown,
.err_handler = &alx_err_handlers,
.driver.pm = ALX_PM_OPS,
};
--
2.7.4
^ permalink raw reply related
* Re: [iovisor-dev] Best userspace programming API for XDP features query to kernel?
From: Jesper Dangaard Brouer @ 2018-04-09 11:26 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Eric Leblond, Victor Julien, Peter Manev, oisf-devel,
Jakub Kicinski, Alexei Starovoitov, iovisor-dev@lists.iovisor.org,
Saeed Mahameed, Daniel Borkmann, netdev, davem, Jiri Benc, brouer
In-Reply-To: <4b850e02-0481-dd75-9341-3951aa76272e@iogearbox.net>
On Fri, 6 Apr 2018 12:36:18 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 04/05/2018 10:51 PM, Jesper Dangaard Brouer wrote:
> > On Thu, 5 Apr 2018 12:37:19 +0200
> > Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> >> On 04/04/2018 02:28 PM, Jesper Dangaard Brouer via iovisor-dev wrote:
> >>> Hi Suricata people,
> >>>
> >>> When Eric Leblond (and I helped) integrated XDP in Suricata, we ran
> >>> into the issue, that at Suricata load/start time, we cannot determine
> >>> if the chosen XDP config options, like xdp-cpu-redirect[1], is valid on
> >>> this HW (e.g require driver XDP_REDIRECT support and bpf cpumap).
> >>>
> >>> We would have liked a way to report that suricata.yaml config was
> >>> invalid for this hardware/setup. Now, it just loads, and packets gets
> >>> silently dropped by XDP (well a WARN_ONCE and catchable via tracepoints).
> >>>
> >>> My question to suricata developers: (Q1) Do you already have code that
> >>> query the kernel or drivers for features?
> >>>
> >>> At the IOvisor call (2 weeks ago), we discussed two options of exposing
> >>> XDP features avail in a given driver.
> >>>
> >>> Option#1: Extend existing ethtool -k/-K "offload and other features"
> >>> with some XDP features, that userspace can query. (Do you already query
> >>> offloads, regarding Q1)
> >>>
> >>> Option#2: Invent a new 'ip link set xdp' netlink msg with a query option.
> >>
> >> I don't really mind if you go via ethtool, as long as we handle this
> >> generically from there and e.g. call the dev's ndo_bpf handler such that
> >> we keep all the information in one place. This can be a new ndo_bpf command
> >> e.g. XDP_QUERY_FEATURES or such.
> >
> > Just to be clear: notice as Victor points out[2], they are programmable
> > going though the IOCTL (SIOCETHTOOL) and not using cmdline tools.
>
> Sure, that was perfectly clear. (But at the same time if you extend the
> ioctl, it's obvious to also add support to actual ethtool cmdline tool.)
>
> > [2] https://github.com/OISF/suricata/blob/master/src/util-ioctl.c#L326
> >
> > If you want everything to go through the drivers ndo_bpf call anyway
> > (which userspace API is netlink based) then at what point to you
>
> Not really, that's the front end. ndo_bpf itself is a plain netdev op
> and has no real tie to netlink.
>
> > want drivers to call their own ndo_bpf, when activated though their
> > ethtool_ops ? (Sorry, but I don't follow the flow you are proposing)
> >
> > Notice, I'm not directly against using the drivers ndo_bpf call. I can
> > see it does provide kernel more flexibility than the ethtool IOCTL.
>
> What I was saying is that even if you go via ethtool ioctl api, where
> you end up in dev_ethtool() and have some new ETHTOOL_* query command,
> then instead of adding a new ethtool_ops callback, we can and should
> reuse ndo_bpf from there.
Okay, you want to catch this in dev_ethtool(), that connected the dots
for me, thanks. BUT it does feel a little fake and confusing to do
this, as the driver developers seeing the info via ethtool, would
expect they need to implement an ethtool_ops callback.
My original idea behind going through the ethtool APIs, were that every
driver is already using this, and it's familiar to the driver
developers. And there are existing userspace tools that sysadm's
already know, that we can leverage.
Thinking about this over the weekend. I have changed my mind a bit,
based on your feedback. I do buy into your idea of forcing things
through the drivers ndo_bpf call. I'm no-longer convinced this should
go through ethtool, but as (you desc) we can, if we find that this is
the easiest ways to export and leverage an existing userspace tool.
> [...]
> > Here, I want to discuss how drivers expose/tell userspace that they
> > support a given feature: Specifically a bit for: XDP_REDIRECT action
> > support.
> >
> >> Same for meta data,
> >
> > Well, not really. It would be a "nice-to-have", but not strictly
> > needed as a feature bit. XDP meta-data is controlled via a helper.
> > And the BPF-prog can detect/see runtime, that the helper bpf_xdp_adjust_meta
> > returns -ENOTSUPP (and need to check the ret value anyhow). Thus,
> > there is that not much gained by exposing this to be detected setup
> > time, as all drivers should eventually support this, and we can detect
> > it runtime.
> >
> > The missing XDP_REDIRECT action features bit it different, as the
> > BPF-prog cannot detect runtime that this is an unsupported action.
> > Plus, setup time we cannot query the driver for supported XDP actions.
>
> Ok, so with the example of meta data, you're arguing that it's okay
> to load a native XDP program onto a driver, and run actual traffic on
> the NIC in order probe for the availability of the feature when you're
> saying that it "can detect/see [at] runtime". I totally agree with you
> that all drivers should eventually support this (same with XDP_REDIRECT),
> but today there are even differences in drivers on bpf_xdp_adjust_meta()/
> bpf_xdp_adjust_head() with regards to how much headroom they have available,
> etc (e.g. some of them have none), so right now you can either go and
> read the code or do a runtime test with running actual traffic through
> the NIC to check whether your BPF prog is supported or not. Theoretically,
> you can do the same runtime test with XDP_REDIRECT (taking the warn in
> bpf_warn_invalid_xdp_action() aside for a moment), but you do have the
> trace_xdp_exception() tracepoint to figure it out, yes, it's a painful
> hassle, but overall, it's not that different as you were trying to argue
> here. For /both/ cases it would be nice to know at setup time whether
> this would be supported or not. Hence, such query is not just limited to
> XDP_REDIRECT alone. Eventually once such interface is agreed upon,
> undoubtedly the list of feature bits will grow is what I'm trying to say;
> only arguing on the XDP_REDIRECT here would be short term.
>
> [...]
> The underlying problem is effectively the decoupling of program
> verification that doesn't have/know the context of where it is being
> attached to in this case.
Yes, exactly, that the underlying problem. (plus the first XDP prog
gets verified and driver attached, and subsequent added bpf tail calls,
cannot be "rejected" (at "driver attachment time") as its too late).
> Thinking out loud for a sec on couple of other options aside
> from feature bits, what about i) providing the target ifindex to the
> verifier for XDP programs, such that at verification time you have the
> full context similar to nfp offload case today,
I do like that we could provide the target ifindex earlier. But
userspace still need some way to express that it e.g. need the
XDP_REDIRECT features (as verifier cannot reliability detect the action
return codes used, as discussed before, due to bpf tail calls and maps
used as return values).
> or ii) populating some
> XDP specific auxillary data to the BPF program at verification time such
> that the driver can check at program attach time whether the requested
> features are possible and if not it will reject and respond with netlink
> extack message to the user (as we do in various XDP attach cases already
> through XDP_SETUP_PROG{,_HW}).
I like proposal ii) better. But how do I specify/input that I need
e.g. the XDP_REDIRECT feature, such that is it avail to "the BPF
program at verification time"?
My proposal iii), what about at XDP attachment time, create a new
netlink attribute that describe XDP action codes the program
needs/wants. If the info is provided, the ndo_bpf call check and
reject, and respond with netlink extack message.
If I want to query for avail action codes, then I can use the same
netlink attribute format, and kernel will return it "populated" with
the info.
It is very useful that the program gets rejected at attachment time (to
avoid loading an XDP prog that silently drops packets). BUT I also
want a query option/functionality (reuse netlink attr format).
Specifically for Suricata the same "bypass" features can be implemented
at different levels (XDP, XDP-generic or clsbpf), and the XDP program
contains multiple features. Thus, depending on what NIC driver
supports, we want to load different XDP and/or clsbpf/TC BPF-programs.
Thus, still support the same user requested feature/functionality, even
if XDP_REDIRECT is not avail, just slightly slower.
> This would, for example, avoid the need for feature bits, and do actual
> rejection of the program while retaining flexibility (and avoiding to
> expose bits that over time hopefully will deprecate anyway due to all
> XDP aware drivers implementing them). For both cases i) and ii), it
> would mean we make the verifier a bit smarter with regards to keeping
> track of driver related (XDP) requirements. Program return code checking
> is already present in the verifier's check_return_code() and we could
> extend it for XDP as well, for example. Seems cleaner and more extensible
> than feature bits, imho.
I thought the verifier's check_return_code() couldn't see/verify if the
XDP return action code is provided as a map lookup result(?). How is
that handled?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH] netfilter: fix CONFIG_NF_REJECT_IPV6=m link error
From: Arnd Bergmann @ 2018-04-09 10:53 UTC (permalink / raw)
To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal,
David S. Miller
Cc: Arnd Bergmann, netfilter-devel, coreteam, netdev, linux-kernel
We get a new link error with CONFIG_NFT_REJECT_INET=y and CONFIG_NF_REJECT_IPV6=m
after larger parts of the nftables modules are linked together:
net/netfilter/nft_reject_inet.o: In function `nft_reject_inet_eval':
nft_reject_inet.c:(.text+0x17c): undefined reference to `nf_send_unreach6'
nft_reject_inet.c:(.text+0x190): undefined reference to `nf_send_reset6'
The problem is that with NF_TABLES_INET set, we implicitly try to use
the ipv6 version as well for NFT_REJECT, but when CONFIG_IPV6 is set to
a loadable module, it's impossible to reach that.
The best workaround I found is to express the above as a Kconfig
dependency, forcing NFT_REJECT itself to be 'm' in that particular
configuration.
Fixes: 02c7b25e5f54 ("netfilter: nf_tables: build-in filter chain type")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
net/netfilter/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
index 704b3832dbad..44d8a55e9721 100644
--- a/net/netfilter/Kconfig
+++ b/net/netfilter/Kconfig
@@ -594,6 +594,7 @@ config NFT_QUOTA
config NFT_REJECT
default m if NETFILTER_ADVANCED=n
tristate "Netfilter nf_tables reject support"
+ depends on !NF_TABLES_INET || (IPV6!=m || m)
help
This option adds the "reject" expression that you can use to
explicitly deny and notify via TCP reset/ICMP informational errors
--
2.9.0
^ permalink raw reply related
* Re: [RFC bpf-next] bpf: document eBPF helpers and add a script to generate man page
From: Markus Heiser @ 2018-04-09 10:52 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jonathan Corbet, Quentin Monnet, ast, netdev, oss-drivers,
Linux Doc Mailing List, linux-man
In-Reply-To: <b2fbd09d-7ba2-9884-e18d-eac586af1665@iogearbox.net>
> Am 09.04.2018 um 12:08 schrieb Daniel Borkmann <daniel@iogearbox.net>:
[...]
>> May I completely misunderstood you, so correct my if I'am wrong:
>>
>> - ./scripts/bpf_helpers_doc.py : produces reST markup from C-comments
>> - ./scripts/kerne-doc : produces reST markup from C-comments
>>
>> IMO: both are doing the same job, so why not using kernel-doc?
>
> They are not really doing the same job, in bpf_helpers_doc.py case you don't
> want the whole header rendered, but just a fraction of it, that is, the
> single big comment which describes all BPF helper functions that a BPF
> program developer has available to use in user space - aka the entries in
> the __BPF_FUNC_MAPPER() macro;
> I also doubt the latter would actually qualify
> in kdoc context as some sort of a function description.
latter .. ah, OK .. thanks for clarifying.
-- Markus --
^ permalink raw reply
* Re: [PATCH v2 2/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent
From: Steffen Klassert @ 2018-04-09 10:34 UTC (permalink / raw)
To: Kevin Easton; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
In-Reply-To: <494dcebdc07530d6afbd7eda300da890a3e68e1c.1523115061.git.kevin@guarana.org>
On Sat, Apr 07, 2018 at 11:40:47AM -0400, Kevin Easton wrote:
> Several places use (x + 7) / 8 to convert from a number of bits to a number
> of bytes. Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
> with other parts of the same file.
>
> Signed-off-by: Kevin Easton <kevin@guarana.org>
Please resubmit this one to ipsec-next after the
merge window. Thanks!
^ permalink raw reply
* Re: [PATCH v2 1/2] af_key: Always verify length of provided sadb_key
From: Steffen Klassert @ 2018-04-09 10:33 UTC (permalink / raw)
To: Kevin Easton; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
In-Reply-To: <de803d3d01f8b70d306c0d52569cc5265ed20bc9.1523115061.git.kevin@guarana.org>
On Sat, Apr 07, 2018 at 11:40:33AM -0400, Kevin Easton wrote:
> Key extensions (struct sadb_key) include a user-specified number of key
> bits. The kernel uses that number to determine how much key data to copy
> out of the message in pfkey_msg2xfrm_state().
>
> The length of the sadb_key message must be verified to be long enough,
> even in the case of SADB_X_AALG_NULL. Furthermore, the sadb_key_len value
> must be long enough to include both the key data and the struct sadb_key
> itself.
>
> Introduce a helper function verify_key_len(), and call it from
> parse_exthdrs() where other exthdr types are similarly checked for
> correctness.
>
> Signed-off-by: Kevin Easton <kevin@guarana.org>
> Reported-by: syzbot+5022a34ca5a3d49b84223653fab632dfb7b4cf37@syzkaller.appspotmail.com
Applied to the ipsec tree, thanks Kevin!
^ permalink raw reply
* Re: [PATCH v2 0/2] af_key: Fix for sadb_key memcpy read overrun
From: Steffen Klassert @ 2018-04-09 10:32 UTC (permalink / raw)
To: Kevin Easton; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
In-Reply-To: <cover.1523115061.git.kevin@guarana.org>
On Sat, Apr 07, 2018 at 11:40:18AM -0400, Kevin Easton wrote:
> As found by syzbot, af_key does not properly validate the key length in
> sadb_key messages from userspace. This can result in copying from beyond
> the end of the sadb_key part of the message, or indeed beyond the end of
> the entire packet.
>
> Both these patches apply cleanly to ipsec-next. Based on Steffen's
> feedback I have re-ordered them so that the fix only is in patch 1, which
> I would suggest is also a stable tree candidate, whereas patch 2 is a
> cleanup only.
I think here is some explanation needed. Usually bugfixes and cleanups
don't go to the same tree. On IPsec bugfixes go to the'ipsec' tree
while cleanups and new features go to the 'ipsec-next' tree. So
you need to split up your patchsets into patches that are targeted
to 'ipsec' and 'ipsec-next'. Aside from that, we are in 'merge window'
currently. This means that most maintainers don't accept patches to
their -next trees. If you have patches for a -next tree, wait until
the merge window is over (when v4.17-rc1 is released) and send them
then.
^ permalink raw reply
* Re: [RFC bpf-next] bpf: document eBPF helpers and add a script to generate man page
From: Daniel Borkmann @ 2018-04-09 10:08 UTC (permalink / raw)
To: Markus Heiser
Cc: Jonathan Corbet, Quentin Monnet, ast, netdev, oss-drivers,
Linux Doc Mailing List, linux-man
In-Reply-To: <C6491E01-3FC2-4AC4-8190-7B17CFAC01AD@darmarit.de>
On 04/09/2018 11:35 AM, Markus Heiser wrote:
>
>> Am 09.04.2018 um 11:25 schrieb Daniel Borkmann <daniel@iogearbox.net>:
>>
>> On 04/09/2018 11:21 AM, Markus Heiser wrote:
>> [...]
>>> Do we really need another kernel-doc parser?
>>>
>>> ./scripts/kernel-doc include/uapi/linux/bpf.h
>>>
>>> should already do the job (producing .rst). For more infos, take a look at
>>
>> This has absolutely zero to do with kernel-doc, but rather producing
>> a description of BPF helper function that are later assembled into an
>> actual man-page that BPF program developers (user space) can use.
>
> May I completely misunderstood you, so correct my if I'am wrong:
>
> - ./scripts/bpf_helpers_doc.py : produces reST markup from C-comments
> - ./scripts/kerne-doc : produces reST markup from C-comments
>
> IMO: both are doing the same job, so why not using kernel-doc?
They are not really doing the same job, in bpf_helpers_doc.py case you don't
want the whole header rendered, but just a fraction of it, that is, the
single big comment which describes all BPF helper functions that a BPF
program developer has available to use in user space - aka the entries in
the __BPF_FUNC_MAPPER() macro; I also doubt the latter would actually qualify
in kdoc context as some sort of a function description.
^ permalink raw reply
* Re: [RFC PATCH bpf-next 2/6] bpf: add bpf_get_stack helper
From: Daniel Borkmann @ 2018-04-09 10:01 UTC (permalink / raw)
To: Alexei Starovoitov, Yonghong Song, netdev; +Cc: kernel-team
In-Reply-To: <49c15114-c518-b591-470a-b4073a675588@fb.com>
On 04/09/2018 07:02 AM, Alexei Starovoitov wrote:
> On 4/8/18 9:53 PM, Yonghong Song wrote:
>>>> @@ -1004,7 +1007,8 @@ static void __bpf_prog_put(struct bpf_prog
>>>> *prog, bool do_idr_lock)
>>>> bpf_prog_kallsyms_del(prog->aux->func[i]);
>>>> bpf_prog_kallsyms_del(prog);
>>>>
>>>> - call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu);
>>>> + synchronize_rcu();
>>>> + __bpf_prog_put_rcu(&prog->aux->rcu);
>>>
>>> there should have been lockdep splat.
>>> We cannot call synchronize_rcu here, since we cannot sleep
>>> in some cases.
>>
>> Let me double check this. The following is the reason
>> why I am using synchronize_rcu().
>>
>> With call_rcu(&prog->aux->rcu, __bpf_prog_put_rcu) and
>> _bpf_prog_put_rcu calls put_callchain_buffers() which
>> calls mutex_lock(), the runtime with CONFIG_DEBUG_ATOMIC_SLEEP=y
>> will complains since potential sleep inside the call_rcu is not
>> allowed.
>
> I see. Indeed. We cannot call put_callchain_buffers() from rcu callback,
> but doing synchronize_rcu() here is also not possible.
> How about moving put_callchain into bpf_prog_free_deferred() ?
+1, the assumption is that you can call bpf_prog_put() and also the
bpf_map_put() from any context. Sleeping here for a long time might
subtly break things badly.
^ permalink raw reply
* Re: [PATCH v4] net: thunderx: rework mac addresses list to u64 array
From: Vadim Lomovtsev @ 2018-04-09 9:42 UTC (permalink / raw)
To: David Miller
Cc: sgoutham, sunil.kovvuri, rric, linux-arm-kernel, netdev,
linux-kernel, dnelson, robin.murphy, hch, gustavo,
Vadim.Lomovtsev
In-Reply-To: <20180408.124200.2204489876311923873.davem@davemloft.net>
On Sun, Apr 08, 2018 at 12:42:00PM -0400, David Miller wrote:
> From: Vadim Lomovtsev <Vadim.Lomovtsev@caviumnetworks.com>
> Date: Fri, 6 Apr 2018 12:53:54 -0700
>
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct *work_arg)
> > work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > - struct xcast_addr *xaddr, *next;
> > + int idx = 0;
>
> No need to initialize idx.
>
> > + for (idx = 0; idx < vf_work->mc->count; idx++) {
>
> As it is always explicitly initialized at, and only used inside of,
> this loop.
Ok, will post next version shortly.
Thanks for your time.
Vadim
^ permalink raw reply
* Re: [RFC bpf-next] bpf: document eBPF helpers and add a script to generate man page
From: Markus Heiser @ 2018-04-09 9:35 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jonathan Corbet, Quentin Monnet, ast, netdev, oss-drivers,
Linux Doc Mailing List, linux-man
In-Reply-To: <81150b4e-79d5-f837-72bc-d988e87f842a@iogearbox.net>
> Am 09.04.2018 um 11:25 schrieb Daniel Borkmann <daniel@iogearbox.net>:
>
> On 04/09/2018 11:21 AM, Markus Heiser wrote:
> [...]
>> Do we really need another kernel-doc parser?
>>
>> ./scripts/kernel-doc include/uapi/linux/bpf.h
>>
>> should already do the job (producing .rst). For more infos, take a look at
>
> This has absolutely zero to do with kernel-doc, but rather producing
> a description of BPF helper function that are later assembled into an
> actual man-page that BPF program developers (user space) can use.
May I completely misunderstood you, so correct my if I'am wrong:
- ./scripts/bpf_helpers_doc.py : produces reST markup from C-comments
- ./scripts/kerne-doc : produces reST markup from C-comments
IMO: both are doing the same job, so why not using kernel-doc?
-- Markus --
^ permalink raw reply
* Re: [RFC bpf-next] bpf: document eBPF helpers and add a script to generate man page
From: Markus Heiser @ 2018-04-09 9:26 UTC (permalink / raw)
To: Daniel Borkmann, Jonathan Corbet
Cc: Quentin Monnet, ast, netdev, oss-drivers, Linux Doc Mailing List,
linux-man
In-Reply-To: <05d2d03a-0b39-9d0c-9ba0-3461afc45fac@iogearbox.net>
On 04/06/2018 01:11 PM, Quentin Monnet wrote:
>> eBPF helper functions can be called from within eBPF programs to perform
>> a variety of tasks that would be otherwise hard or impossible to do with
>> eBPF itself. There is a growing number of such helper functions in the
>> kernel, but documentation is scarce. The main user space header file
>> does contain a short commented description of most helpers, but it is
>> somewhat outdated and not complete. It is more a "cheat sheet" than a
>> real documentation accessible to new eBPF developers.
>>
>> This commit attempts to improve the situation by replacing the existing
>> overview for the helpers with a more developed description. Furthermore,
>> a Python script is added to generate a manual page for eBPF helpers. The
>> workflow is the following, and requires the rst2man utility:
>>
>> $ ./scripts/bpf_helpers_doc.py \
>> --filename include/uapi/linux/bpf.h > /tmp/bpf-helpers.rst
>> $ rst2man /tmp/bpf-helpers.rst > /tmp/bpf-helpers.7
>> $ man /tmp/bpf-helpers.7
>>
>> The objective is to keep all documentation related to the helpers in a
>> single place,
Do we really need another kernel-doc parser?
./scripts/kernel-doc include/uapi/linux/bpf.h
should already do the job (producing .rst). For more infos, take a look at
https://www.kernel.org/doc/html/latest/doc-guide/index.html
-- Markus --
PS: sorry for re-post, first post was HTML which is not accepted by ML :o
^ permalink raw reply
* Re: [RFC bpf-next] bpf: document eBPF helpers and add a script to generate man page
From: Daniel Borkmann @ 2018-04-09 9:25 UTC (permalink / raw)
To: Markus Heiser, Jonathan Corbet
Cc: Quentin Monnet, ast, netdev, oss-drivers, Linux Doc Mailing List,
linux-man
In-Reply-To: <AB89F6E2-DE8A-4259-B361-BE96AD32E84E@darmarit.de>
On 04/09/2018 11:21 AM, Markus Heiser wrote:
[...]
> Do we really need another kernel-doc parser?
>
> ./scripts/kernel-doc include/uapi/linux/bpf.h
>
> should already do the job (producing .rst). For more infos, take a look at
This has absolutely zero to do with kernel-doc, but rather producing
a description of BPF helper function that are later assembled into an
actual man-page that BPF program developers (user space) can use.
^ permalink raw reply
* Re: [RFC bpf-next] bpf: document eBPF helpers and add a script to generate man page
From: Daniel Borkmann @ 2018-04-09 9:01 UTC (permalink / raw)
To: Quentin Monnet, ast; +Cc: netdev, oss-drivers, linux-doc, linux-man
In-Reply-To: <20180406111122.11038-1-quentin.monnet@netronome.com>
On 04/06/2018 01:11 PM, Quentin Monnet wrote:
> eBPF helper functions can be called from within eBPF programs to perform
> a variety of tasks that would be otherwise hard or impossible to do with
> eBPF itself. There is a growing number of such helper functions in the
> kernel, but documentation is scarce. The main user space header file
> does contain a short commented description of most helpers, but it is
> somewhat outdated and not complete. It is more a "cheat sheet" than a
> real documentation accessible to new eBPF developers.
>
> This commit attempts to improve the situation by replacing the existing
> overview for the helpers with a more developed description. Furthermore,
> a Python script is added to generate a manual page for eBPF helpers. The
> workflow is the following, and requires the rst2man utility:
>
> $ ./scripts/bpf_helpers_doc.py \
> --filename include/uapi/linux/bpf.h > /tmp/bpf-helpers.rst
> $ rst2man /tmp/bpf-helpers.rst > /tmp/bpf-helpers.7
> $ man /tmp/bpf-helpers.7
>
> The objective is to keep all documentation related to the helpers in a
> single place, and to be able to generate from here a manual page that
> could be packaged in the man-pages repository and shipped with most
> distributions [1].
>
> Additionally, parsing the prototypes of the helper functions could
> hopefully be reused, with a different Printer object, to generate
> header files needed in some eBPF-related projects.
>
> Regarding the description of each helper, it comprises several items:
>
> - The function prototype.
> - A description of the function and of its arguments (except for a
> couple of cases, when there are no arguments and the return value
> makes the function usage really obvious).
> - A description of return values (if not void).
> - A listing of eBPF program types (if relevant, map types) compatible
> with the helper.
> - Information about the helper being restricted to GPL programs, or not.
> - The kernel version in which the helper was introduced.
> - The commit that introduced the helper (this is mostly to have it in
> the source of the man page, as it can be used to track changes and
> update the page).
>
> For several helpers, descriptions are inspired (at times, nearly copied)
> from the commit logs introducing them in the kernel--Many thanks to
> their respective authors! They were completed as much as possible, the
> objective being to have something easily accessible even for people just
> starting with eBPF. There is probably a bit more work to do in this
> direction for some helpers.
>
> Some RST formatting is used in the descriptions (not in function
> prototypes, to keep them readable, but the Python script provided in
> order to generate the RST for the manual page does add formatting to
> prototypes, to produce something pretty) to get "bold" and "italics" in
> manual pages. Hopefully, the descriptions in bpf.h file remains
> perfectly readable. Note that the few trailing white spaces are
> intentional, removing them would break paragraphs for rst2man.
>
> The descriptions should ideally be updated each time someone adds a new
> helper, or updates the behaviour (compatibility extended to new program
> types, new socket option supported...) or the interface (new flags
> available, ...) of existing ones.
>
> [1] I have not contacted people from the man-pages project prior to
> sending this RFC, so I can offer no guaranty at this time that they
> would accept to take the generated man page.
>
> Cc: linux-doc@vger.kernel.org
> Cc: linux-man@vger.kernel.org
> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Great work, thanks a lot for doing this!
[...]
> + * int bpf_probe_read(void *dst, u32 size, const void *src)
> + * Description
> + * For tracing programs, safely attempt to read *size* bytes from
> + * address *src* and store the data in *dst*.
> + * Return
> + * 0 on success, or a negative error in case of failure.
> + * For
> + * *Tracing programs*.
> + * GPL only
> + * Yes
> + * Since
> + * Linux 4.1
> + *
> + * .. commit 2541517c32be
> *
> * u64 bpf_ktime_get_ns(void)
> - * Return: current ktime
> - *
> - * int bpf_trace_printk(const char *fmt, int fmt_size, ...)
> - * Return: length of buffer written or negative error
> + * Description
> + * Return the time elapsed since system boot, in nanoseconds.
> + * Return
> + * Current *ktime*.
> + * For
> + * All program types, except
> + * **BPF_PROG_TYPE_CGROUP_DEVICE**.
I think we should probably always just list the actual program types instead,
since when new types get added to the kernel not supporting bpf_ktime_get_ns()
helper in this example, the above statement would be misleading (potentially
more misleading than the other way around when it's not yet mentioned to be
supported).
> + * GPL only
> + * Yes
> + * Since
> + * Linux 4.1
> + *
> + * .. commit d9847d310ab4
> + *
> + * int bpf_trace_printk(const char *fmt, u32 fmt_size, ...)
> + * Description
> + * This helper is a "printk()-like" facility for debugging. It
> + * prints a message defined by format *fmt* (of size *fmt_size*)
> + * to file *\/sys/kernel/debug/tracing/trace* from DebugFS, if
> + * available. It can take up to three additional **u64**
> + * arguments (as an eBPF helpers, the total number of arguments is
> + * limited to five). Each time the helper is called, it appends a
> + * line that looks like the following:
> + *
> + * ::
> + *
> + * telnet-470 [001] .N.. 419421.045894: 0x00000001: BPF command: 2
> + *
> + * In the above:
> + *
> + * * ``telnet`` is the name of the current task.
> + * * ``470`` is the PID of the current task.
> + * * ``001`` is the CPU number on which the task is
> + * running.
> + * * In ``.N..``, each character refers to a set of
> + * options (whether irqs are enabled, scheduling
> + * options, whether hard/softirqs are running, level of
> + * preempt_disabled respectively). **N** means that
> + * **TIF_NEED_RESCHED** and **PREEMPT_NEED_RESCHED**
> + * are set.
> + * * ``419421.045894`` is a timestamp.
> + * * ``0x00000001`` is a fake value used by BPF for the
> + * instruction pointer register.
> + * * ``BPF command: 2`` is the message formatted with
> + * *fmt*.
> + *
> + * The conversion specifiers supported by *fmt* are similar, but
> + * more limited than for printk(). They are **%d**, **%i**,
> + * **%u**, **%x**, **%ld**, **%li**, **%lu**, **%lx**, **%lld**,
> + * **%lli**, **%llu**, **%llx**, **%p**, **%s**. No modifier (size
> + * of field, padding with zeroes, etc.) is available, and the
> + * helper will silently fail if it encounters an unknown
> + * specifier.
> + *
> + * Also, note that **bpf_trace_printk**\ () is slow, and should
> + * only be used for debugging purposes. For passing values to user
> + * space, perf events should be preferred.
> + * Return
> + * The number of bytes written to the buffer, or a negative error
> + * in case of failure.
> + * For
> + * All types of programs.
Ditto, and various other places.
> + * GPL only
> + * Yes
> + * Since
> + * Linux 4.1
> + *
> + * .. commit 9c959c863f82
> *
> * u32 bpf_prandom_u32(void)
> - * Return: random value
> - *
> - * u32 bpf_raw_smp_processor_id(void)
> - * Return: SMP processor ID
> - *
[...]
> + * u32 bpf_get_smp_processor_id(void)
> + * Return
> + * The SMP (Symmetric multiprocessing) processor id.
> + * For
> + * *Networking programs*.
Ditto plus above is actually not correct. See tracing_func_proto():
[...]
case BPF_FUNC_get_smp_processor_id:
return &bpf_get_smp_processor_id_proto;
[...]
> + * GPL only
> + * No
> + * Since
> + * Linux 4.1
> + *
> + * .. commit c04167ce2ca0
> + *
> + * int bpf_skb_store_bytes(struct sk_buff *skb, u32 offset, const void *from, u32 len, u64 flags)
> + * Description
> + * Store *len* bytes from address *from* into the packet
> + * associated to *skb*, at *offset*. *flags* are a combination of
> + * **BPF_F_RECOMPUTE_CSUM** (automatically recompute the
> + * checksum for the packet after storing the bytes) and
> + * **BPF_F_INVALIDATE_HASH** (set *skb*\ **->hash**, *skb*\
> + * **->swhash** and *skb*\ **->l4hash** to 0).
> + *
> + * A call to this helper is susceptible to change data from the
> + * packet. Therefore, at load time, all checks on pointers
> + * previously done by the verifier are invalidated and must be
> + * performed again.
> + * Return
[...]
Agree with Alexei that 'Description' but also 'Return' is the most useful
(the latter we can still improve a bit wrt errors). 'For' gets indeed quickly
out of hand but I do think that for BPF program developers 'Since' has good
value to quickly check when a helper could be available, but then again
this is actually tied to an individual kconfig and/or program type, and could
potentially require to add or (worst case) remove the info in a second commit
e.g. after the merge window similar with the commit sha. Probably best indeed
to stick to the signature, 'Description' and 'Return' initially.
We should probably also have the bpf_helpers_doc.py workflow in a separate
comment above this one such that developers don't have to look up the original
commit message, but have the required steps to render the rst/man page available
right where they need it.
Thanks,
Daniel
^ permalink raw reply
* [PATCH] slip: Check if rstate is initialized before uncompressing
From: Tejaswi Tanikella @ 2018-04-09 8:53 UTC (permalink / raw)
To: netdev
On receiving a packet the state index points to the rstate which must be
used to fill up IP and TCP headers. But if the state index points to a
rstate which is unitialized, i.e. filled with zeros, it gets stuck in an
infinite loop inside ip_fast_csum trying to compute the ip checsum of a
header with zero length.
89.666953: <2> [<ffffff9dd3e94d38>] slhc_uncompress+0x464/0x468
89.666965: <2> [<ffffff9dd3e87d88>] ppp_receive_nonmp_frame+0x3b4/0x65c
89.666978: <2> [<ffffff9dd3e89dd4>] ppp_receive_frame+0x64/0x7e0
89.666991: <2> [<ffffff9dd3e8a708>] ppp_input+0x104/0x198
89.667005: <2> [<ffffff9dd3e93868>] pppopns_recv_core+0x238/0x370
89.667027: <2> [<ffffff9dd4428fc8>] __sk_receive_skb+0xdc/0x250
89.667040: <2> [<ffffff9dd3e939e4>] pppopns_recv+0x44/0x60
89.667053: <2> [<ffffff9dd4426848>] __sock_queue_rcv_skb+0x16c/0x24c
89.667065: <2> [<ffffff9dd4426954>] sock_queue_rcv_skb+0x2c/0x38
89.667085: <2> [<ffffff9dd44f7358>] raw_rcv+0x124/0x154
89.667098: <2> [<ffffff9dd44f7568>] raw_local_deliver+0x1e0/0x22c
89.667117: <2> [<ffffff9dd44c8ba0>] ip_local_deliver_finish+0x70/0x24c
89.667131: <2> [<ffffff9dd44c92f4>] ip_local_deliver+0x100/0x10c
./scripts/faddr2line vmlinux slhc_uncompress+0x464/0x468 output:
ip_fast_csum at arch/arm64/include/asm/checksum.h:40
(inlined by) slhc_uncompress at drivers/net/slip/slhc.c:615
Adding a variable to indicate if the current rstate is initialized. If
such a packet arrives, move to toss state.
Signed-off-by: Tejaswi Tanikella <tejaswit@codeaurora.org>
---
drivers/net/slip/slhc.c | 5 +++++
include/net/slhc_vj.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/net/slip/slhc.c b/drivers/net/slip/slhc.c
index 5782733..e710b0b 100644
--- a/drivers/net/slip/slhc.c
+++ b/drivers/net/slip/slhc.c
@@ -509,6 +509,10 @@ struct slcompress *
if(x < 0 || x > comp->rslot_limit)
goto bad;
+ /* Check if the cstate is initialized */
+ if(!comp->rstate[x].initialized)
+ goto bad;
+
comp->flags &=~ SLF_TOSS;
comp->recv_current = x;
} else {
@@ -673,6 +677,7 @@ struct slcompress *
if (cs->cs_tcp.doff > 5)
memcpy(cs->cs_tcpopt, icp + ihl*4 + sizeof(struct tcphdr), (cs->cs_tcp.doff - 5) * 4);
cs->cs_hsize = ihl*2 + cs->cs_tcp.doff*2;
+ cs->initialized = 1;
/* Put headers back on packet
* Neither header checksum is recalculated
*/
diff --git a/include/net/slhc_vj.h b/include/net/slhc_vj.h
index 8716d59..d673365 100644
--- a/include/net/slhc_vj.h
+++ b/include/net/slhc_vj.h
@@ -127,6 +127,7 @@
*/
struct cstate {
byte_t cs_this; /* connection id number (xmit) */
+ byte_t initialized; /* non-zero if initialized */
struct cstate *next; /* next in ring (xmit) */
struct iphdr cs_ip; /* ip/tcp hdr from most recent packet */
struct tcphdr cs_tcp;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next 6/6] netdevsim: Add simple FIB resource controller via devlink
From: Jiri Pirko @ 2018-04-09 8:18 UTC (permalink / raw)
To: David Ahern
Cc: netdev, davem, roopa, shm, jiri, idosch, jakub.kicinski,
andy.roulin
In-Reply-To: <c85f2b41-6100-fe12-b30a-8f12f63e8aab@cumulusnetworks.com>
Fri, Apr 06, 2018 at 11:22:29PM CEST, dsa@cumulusnetworks.com wrote:
>On 4/5/18 11:52 PM, Jiri Pirko wrote:
>> Thu, Apr 05, 2018 at 11:06:41PM CEST, dsa@cumulusnetworks.com wrote:
>>> On 4/5/18 2:10 PM, David Ahern wrote:
>>>>
>>>> The ASIC here is the kernel tables in a namespace. It does not make
>>>> sense to have 2 devlink instances for a single namespace.
>>>
>>> I put this example controller in netdevsim per a suggestion from Ido.
>>> The netdevsim seemed like a good idea given that modules intention --
>>> testing network facilities. Perhaps I should have done this as a
>>> completely standalone module ...
>>>
>>> The intention is to treat the kernel's tables *per namespace* as a
>>> standalone entity that can be managed very similar to ASIC resources.
>>
>> So you say you want to treat a namespace as an ASIC? That sounds very
>> odd to me :/
>
>Why? The kernel has forwarding tables, acl's, etc just like the ASIC,
>and each namespace is a separate set of tables.
I don't get it. What's the point? For HW, the reason is it has limited
resources and those resources are not mapped 1:1 with kernel object.
However, for kernel, that is meaningless.
>
>If you think about it, userspace "programs" the kernel just like mlxsw
>and userspace SDKs "program" an asic.
I don't give a **** about sdks. I have no clue why you mention that here.
>
>
>>> Given that I can add a resource controller module
>>> (drivers/net/kern_res_mgr.c?) that creates a 'struct device' per network
>>> namespace with a devlink instance. In this case the device would very
>>> much be tied to the namespace 1:1.
>>
>> That sounds more reasonable and accurate, yet still odd. You would not
>> have any netdevices there? Any ports?
>>
>
>Sure, what ever ports are assigned to or created in the namespace.
>
>Nothing about the devlink API says it has to be a real h/w device.
Sure, it could represent something made-up, like netdevsim. However I
see a big misfit when you want to represent a namespace.
>Nothing about the devlink API says it can only be used for real h/w that
>has ports represented by netdevices that the devlink instance some how
>has "control" over.
>
>As the netdevsim demo shows, I can build an L3 resource controller for
>the kernel tables using just the devlink API and the in-kernel notifiers.
^ permalink raw reply
* Re: [RFC PATCH net-next v5 3/4] virtio_net: Extend virtio to use VF datapath when available
From: Jiri Pirko @ 2018-04-09 8:07 UTC (permalink / raw)
To: Samudrala, Sridhar
Cc: mst, stephen, davem, netdev, virtualization, virtio-dev,
jesse.brandeburg, alexander.h.duyck, kubakici, jasowang,
loseweigh
In-Reply-To: <bb4f0821-246f-1052-de8b-d96be3388ed3@intel.com>
Sat, Apr 07, 2018 at 12:59:14AM CEST, sridhar.samudrala@intel.com wrote:
>On 4/6/2018 5:48 AM, Jiri Pirko wrote:
>> Thu, Apr 05, 2018 at 11:08:22PM CEST, sridhar.samudrala@intel.com wrote:
[...]
>> > +static int virtnet_bypass_join_child(struct net_device *bypass_netdev,
>> > + struct net_device *child_netdev)
>> > +{
>> > + struct virtnet_bypass_info *vbi;
>> > + bool backup;
>> > +
>> > + vbi = netdev_priv(bypass_netdev);
>> > + backup = (child_netdev->dev.parent == bypass_netdev->dev.parent);
>> > + if (backup ? rtnl_dereference(vbi->backup_netdev) :
>> > + rtnl_dereference(vbi->active_netdev)) {
>> > + netdev_info(bypass_netdev,
>> > + "%s attempting to join bypass dev when %s already present\n",
>> > + child_netdev->name, backup ? "backup" : "active");
>> Bypass module should check if there is already some other netdev
>> enslaved and refuse right there.
>
>This will work for virtio-net with 3 netdev model, but this check has to be done by netvsc
>as its bypass_netdev is same as the backup_netdev.
>Will add a flag while registering with the bypass module to indicate if the driver is doing
>a 2 netdev or 3 netdev model and based on that flag this check can be done in bypass module
>for 3 netdev scenario.
Just let me undestand it clearly. What I expect the difference would be
between 2netdev and3 netdev model is this:
2netdev:
bypass_master
/
/
VF_slave
3netdev:
bypass_master
/ \
/ \
VF_slave backup_slave
Is that correct? If not, how does it look like?
Thanks!
^ permalink raw reply
* [PATCH RESEND v2] vhost-net: set packet weight of tx polling to 2 * vq size
From: haibinzhang(张海斌) @ 2018-04-09 7:22 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, kvm@vger.kernel.org,
virtualization@lists.linux-foundation.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: lidongchen(陈立东),
yunfangtai(台运方)
handle_tx will delay rx for tens or even hundreds of milliseconds when tx busy
polling udp packets with small length(e.g. 1byte udp payload), because setting
VHOST_NET_WEIGHT takes into account only sent-bytes but no single packet length.
Ping-Latencies shown below were tested between two Virtual Machines using
netperf (UDP_STREAM, len=1), and then another machine pinged the client:
vq size=256
Packet-Weight Ping-Latencies(millisecond)
min avg max
Origin 3.319 18.489 57.303
64 1.643 2.021 2.552
128 1.825 2.600 3.224
256 1.997 2.710 4.295
512 1.860 3.171 4.631
1024 2.002 4.173 9.056
2048 2.257 5.650 9.688
4096 2.093 8.508 15.943
vq size=512
Packet-Weight Ping-Latencies(millisecond)
min avg max
Origin 6.537 29.177 66.245
64 2.798 3.614 4.403
128 2.861 3.820 4.775
256 3.008 4.018 4.807
512 3.254 4.523 5.824
1024 3.079 5.335 7.747
2048 3.944 8.201 12.762
4096 4.158 11.057 19.985
Seems pretty consistent, a small dip at 2 VQ sizes.
Ring size is a hint from device about a burst size it can tolerate. Based on
benchmarks, set the weight to 2 * vq size.
To evaluate this change, another tests were done using netperf(RR, TX) between
two machines with Intel(R) Xeon(R) Gold 6133 CPU @ 2.50GHz, and vq size was
tweaked through qemu. Results shown below does not show obvious changes.
vq size=256 TCP_RR vq size=512 TCP_RR
size/sessions/+thu%/+normalize% size/sessions/+thu%/+normalize%
1/ 1/ -7%/ -2% 1/ 1/ 0%/ -2%
1/ 4/ +1%/ 0% 1/ 4/ +1%/ 0%
1/ 8/ +1%/ -2% 1/ 8/ 0%/ +1%
64/ 1/ -6%/ 0% 64/ 1/ +7%/ +3%
64/ 4/ 0%/ +2% 64/ 4/ -1%/ +1%
64/ 8/ 0%/ 0% 64/ 8/ -1%/ -2%
256/ 1/ -3%/ -4% 256/ 1/ -4%/ -2%
256/ 4/ +3%/ +4% 256/ 4/ +1%/ +2%
256/ 8/ +2%/ 0% 256/ 8/ +1%/ -1%
vq size=256 UDP_RR vq size=512 UDP_RR
size/sessions/+thu%/+normalize% size/sessions/+thu%/+normalize%
1/ 1/ -5%/ +1% 1/ 1/ -3%/ -2%
1/ 4/ +4%/ +1% 1/ 4/ -2%/ +2%
1/ 8/ -1%/ -1% 1/ 8/ -1%/ 0%
64/ 1/ -2%/ -3% 64/ 1/ +1%/ +1%
64/ 4/ -5%/ -1% 64/ 4/ +2%/ 0%
64/ 8/ 0%/ -1% 64/ 8/ -2%/ +1%
256/ 1/ +7%/ +1% 256/ 1/ -7%/ 0%
256/ 4/ +1%/ +1% 256/ 4/ -3%/ -4%
256/ 8/ +2%/ +2% 256/ 8/ +1%/ +1%
vq size=256 TCP_STREAM vq size=512 TCP_STREAM
size/sessions/+thu%/+normalize% size/sessions/+thu%/+normalize%
64/ 1/ 0%/ -3% 64/ 1/ 0%/ 0%
64/ 4/ +3%/ -1% 64/ 4/ -2%/ +4%
64/ 8/ +9%/ -4% 64/ 8/ -1%/ +2%
256/ 1/ +1%/ -4% 256/ 1/ +1%/ +1%
256/ 4/ -1%/ -1% 256/ 4/ -3%/ 0%
256/ 8/ +7%/ +5% 256/ 8/ -3%/ 0%
512/ 1/ +1%/ 0% 512/ 1/ -1%/ -1%
512/ 4/ +1%/ -1% 512/ 4/ 0%/ 0%
512/ 8/ +7%/ -5% 512/ 8/ +6%/ -1%
1024/ 1/ 0%/ -1% 1024/ 1/ 0%/ +1%
1024/ 4/ +3%/ 0% 1024/ 4/ +1%/ 0%
1024/ 8/ +8%/ +5% 1024/ 8/ -1%/ 0%
2048/ 1/ +2%/ +2% 2048/ 1/ -1%/ 0%
2048/ 4/ +1%/ 0% 2048/ 4/ 0%/ -1%
2048/ 8/ -2%/ 0% 2048/ 8/ 5%/ -1%
4096/ 1/ -2%/ 0% 4096/ 1/ -2%/ 0%
4096/ 4/ +2%/ 0% 4096/ 4/ 0%/ 0%
4096/ 8/ +9%/ -2% 4096/ 8/ -5%/ -1%
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Haibin Zhang <haibinzhang@tencent.com>
Signed-off-by: Yunfang Tai <yunfangtai@tencent.com>
Signed-off-by: Lidong Chen <lidongchen@tencent.com>
---
drivers/vhost/net.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 8139bc70ad7d..3563a305cc0a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -44,6 +44,10 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
* Using this limit prevents one virtqueue from starving others. */
#define VHOST_NET_WEIGHT 0x80000
+/* Max number of packets transferred before requeueing the job.
+ * Using this limit prevents one virtqueue from starving rx. */
+#define VHOST_NET_PKT_WEIGHT(vq) ((vq)->num * 2)
+
/* MAX number of TX used buffers for outstanding zerocopy */
#define VHOST_MAX_PEND 128
#define VHOST_GOODCOPY_LEN 256
@@ -473,6 +477,7 @@ static void handle_tx(struct vhost_net *net)
struct socket *sock;
struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
bool zcopy, zcopy_used;
+ int sent_pkts = 0;
mutex_lock(&vq->mutex);
sock = vq->private_data;
@@ -580,7 +585,8 @@ static void handle_tx(struct vhost_net *net)
else
vhost_zerocopy_signal_used(net, vq);
vhost_net_tx_packet(net);
- if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
+ if (unlikely(total_len >= VHOST_NET_WEIGHT) ||
+ unlikely(++sent_pkts >= VHOST_NET_PKT_WEIGHT(vq))) {
vhost_poll_queue(&vq->poll);
break;
}
--
2.12.3
^ permalink raw reply related
* Re: [PATCH net-next v2] ipvs: fix multiplicative hashing in sh/dh/lblc/lblcr algorithms
From: Simon Horman @ 2018-04-09 7:16 UTC (permalink / raw)
To: Julian Anastasov
Cc: Vincent Bernat, Wensong Zhang, David S. Miller, netdev, lvs-devel
In-Reply-To: <alpine.LFD.2.20.1804011407500.9304@ja.home.ssi.bg>
On Sun, Apr 01, 2018 at 02:11:51PM +0300, Julian Anastasov wrote:
>
> Hello,
>
> On Sun, 1 Apr 2018, Vincent Bernat wrote:
>
> > The sh/dh/lblc/lblcr algorithms are using Knuth's multiplicative
> > hashing incorrectly. Replace its use by the hash_32() macro, which
> > correctly implements this algorithm. It doesn't use the same constant,
> > but it shouldn't matter.
> >
> > Signed-off-by: Vincent Bernat <vincent@bernat.im>
>
> Looks good to me, thanks! Simon, please apply, if possible
> with the extra space removed, see below...
>
> Acked-by: Julian Anastasov <ja@ssi.bg>
Thanks, I have applied this with the extra space removed.
I will submit it for inclusion in v4.18.
^ 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