* [PATCH net-next v2 06/10] r8169: use phy_mii_ioctl
From: Heiner Kallweit @ 2018-07-10 18:39 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <84ac1cb7-bafd-5f15-8cda-e8dfd7d0deb3@gmail.com>
Switch to using phy_mii_ioctl().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/ethernet/realtek/r8169.c | 25 +++----------------------
1 file changed, 3 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f0aa1b54..deed477e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4283,31 +4283,12 @@ static int rtl_set_mac_address(struct net_device *dev, void *p)
return 0;
}
-static int rtl_xmii_ioctl(struct rtl8169_private *tp,
- struct mii_ioctl_data *data, int cmd)
-{
- switch (cmd) {
- case SIOCGMIIPHY:
- data->phy_id = 32; /* Internal PHY */
- return 0;
-
- case SIOCGMIIREG:
- data->val_out = rtl_readphy(tp, data->reg_num & 0x1f);
- return 0;
-
- case SIOCSMIIREG:
- rtl_writephy(tp, data->reg_num & 0x1f, data->val_in);
- return 0;
- }
- return -EOPNOTSUPP;
-}
-
static int rtl8169_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
- struct rtl8169_private *tp = netdev_priv(dev);
- struct mii_ioctl_data *data = if_mii(ifr);
+ if (!netif_running(dev))
+ return -ENODEV;
- return netif_running(dev) ? rtl_xmii_ioctl(tp, data, cmd) : -ENODEV;
+ return phy_mii_ioctl(dev->phydev, ifr, cmd);
}
static void rtl_init_mdio_ops(struct rtl8169_private *tp)
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v2 05/10] r8169: use phy_ethtool_nway_reset
From: Heiner Kallweit @ 2018-07-10 18:39 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <84ac1cb7-bafd-5f15-8cda-e8dfd7d0deb3@gmail.com>
Switch to using phy_ethtool_nway_reset().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/ethernet/realtek/Kconfig | 1 -
drivers/net/ethernet/realtek/r8169.c | 9 +--------
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 7fb1af1f..e1cd934c 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -100,7 +100,6 @@ config R8169
select FW_LOADER
select CRC32
select PHYLIB
- select MII
---help---
Say Y here if you have a Realtek 8169 PCI Gigabit Ethernet adapter.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 5282e413..f0aa1b54 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1984,13 +1984,6 @@ static void rtl8169_get_strings(struct net_device *dev, u32 stringset, u8 *data)
}
}
-static int rtl8169_nway_reset(struct net_device *dev)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- return mii_nway_restart(&tp->mii);
-}
-
/*
* Interrupt coalescing
*
@@ -2221,7 +2214,7 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.get_sset_count = rtl8169_get_sset_count,
.get_ethtool_stats = rtl8169_get_ethtool_stats,
.get_ts_info = ethtool_op_get_ts_info,
- .nway_reset = rtl8169_nway_reset,
+ .nway_reset = phy_ethtool_nway_reset,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
};
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v2 04/10] r8169: use phy_ethtool_(g|s)et_link_ksettings
From: Heiner Kallweit @ 2018-07-10 18:39 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <84ac1cb7-bafd-5f15-8cda-e8dfd7d0deb3@gmail.com>
Use phy_ethtool_(g|s)et_link_ksettings() for the respective ethtool_ops
callbacks.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
drivers/net/ethernet/realtek/r8169.c | 35 +++-------------------------
1 file changed, 3 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index d5133fde..5282e413 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1809,35 +1809,6 @@ static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), swab16(opts2 & 0xffff));
}
-static int rtl8169_get_link_ksettings(struct net_device *dev,
- struct ethtool_link_ksettings *cmd)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- mii_ethtool_get_link_ksettings(&tp->mii, cmd);
-
- return 0;
-}
-
-static int rtl8169_set_link_ksettings(struct net_device *dev,
- const struct ethtool_link_ksettings *cmd)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
- int rc;
- u32 advertising;
-
- if (!ethtool_convert_link_mode_to_legacy_u32(&advertising,
- cmd->link_modes.advertising))
- return -EINVAL;
-
- rtl_lock_work(tp);
- rc = rtl8169_set_speed(dev, cmd->base.autoneg, cmd->base.speed,
- cmd->base.duplex, advertising);
- rtl_unlock_work(tp);
-
- return rc;
-}
-
static void rtl8169_get_regs(struct net_device *dev, struct ethtool_regs *regs,
void *p)
{
@@ -2092,7 +2063,7 @@ static const struct rtl_coalesce_info *rtl_coalesce_info(struct net_device *dev)
const struct rtl_coalesce_info *ci;
int rc;
- rc = rtl8169_get_link_ksettings(dev, &ecmd);
+ rc = phy_ethtool_get_link_ksettings(dev, &ecmd);
if (rc < 0)
return ERR_PTR(rc);
@@ -2251,8 +2222,8 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.get_ethtool_stats = rtl8169_get_ethtool_stats,
.get_ts_info = ethtool_op_get_ts_info,
.nway_reset = rtl8169_nway_reset,
- .get_link_ksettings = rtl8169_get_link_ksettings,
- .set_link_ksettings = rtl8169_set_link_ksettings,
+ .get_link_ksettings = phy_ethtool_get_link_ksettings,
+ .set_link_ksettings = phy_ethtool_set_link_ksettings,
};
static void rtl8169_get_mac_version(struct rtl8169_private *tp,
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v2 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset
From: Heiner Kallweit @ 2018-07-10 18:39 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <84ac1cb7-bafd-5f15-8cda-e8dfd7d0deb3@gmail.com>
Use genphy_soft_reset() instead of open-coding a PHY soft reset. We have
to do an explicit PHY soft reset because some chips use the genphy driver
which uses a no-op as soft_reset callback.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/ethernet/realtek/r8169.c | 27 +--------------------------
1 file changed, 1 insertion(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index e22d4e64..d5133fde 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1441,19 +1441,6 @@ static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
RTL_R8(tp, ChipCmd);
}
-static unsigned int rtl8169_xmii_reset_pending(struct rtl8169_private *tp)
-{
- return rtl_readphy(tp, MII_BMCR) & BMCR_RESET;
-}
-
-static void rtl8169_xmii_reset_enable(struct rtl8169_private *tp)
-{
- unsigned int val;
-
- val = rtl_readphy(tp, MII_BMCR) | BMCR_RESET;
- rtl_writephy(tp, MII_BMCR, val & 0xffff);
-}
-
static void rtl_link_chg_patch(struct rtl8169_private *tp)
{
struct net_device *dev = tp->dev;
@@ -4252,18 +4239,6 @@ static void rtl_schedule_task(struct rtl8169_private *tp, enum rtl_flag flag)
schedule_work(&tp->wk.work);
}
-DECLARE_RTL_COND(rtl_phy_reset_cond)
-{
- return rtl8169_xmii_reset_pending(tp);
-}
-
-static void rtl8169_phy_reset(struct net_device *dev,
- struct rtl8169_private *tp)
-{
- rtl8169_xmii_reset_enable(tp);
- rtl_msleep_loop_wait_low(tp, &rtl_phy_reset_cond, 1, 100);
-}
-
static bool rtl_tbi_enabled(struct rtl8169_private *tp)
{
return (tp->mac_version == RTL_GIGA_MAC_VER_01) &&
@@ -4294,7 +4269,7 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
}
- rtl8169_phy_reset(dev, tp);
+ genphy_soft_reset(dev->phydev);
rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v2 02/10] r8169: use phy_resume/phy_suspend
From: Heiner Kallweit @ 2018-07-10 18:39 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <84ac1cb7-bafd-5f15-8cda-e8dfd7d0deb3@gmail.com>
Use phy_resume() / phy_suspend() instead of open coding this functionality.
The chip version specific differences are handled by the respective PHY
drivers.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- no changes
---
drivers/net/ethernet/realtek/r8169.c | 48 +++-------------------------
1 file changed, 5 insertions(+), 43 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index d1951f32..e22d4e64 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4450,47 +4450,6 @@ static bool rtl_wol_pll_power_down(struct rtl8169_private *tp)
return true;
}
-static void r8168_phy_power_up(struct rtl8169_private *tp)
-{
- rtl_writephy(tp, 0x1f, 0x0000);
- switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_11:
- case RTL_GIGA_MAC_VER_12:
- case RTL_GIGA_MAC_VER_17 ... RTL_GIGA_MAC_VER_28:
- case RTL_GIGA_MAC_VER_31:
- rtl_writephy(tp, 0x0e, 0x0000);
- break;
- default:
- break;
- }
- rtl_writephy(tp, MII_BMCR, BMCR_ANENABLE);
-
- /* give MAC/PHY some time to resume */
- msleep(20);
-}
-
-static void r8168_phy_power_down(struct rtl8169_private *tp)
-{
- rtl_writephy(tp, 0x1f, 0x0000);
- switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_32:
- case RTL_GIGA_MAC_VER_33:
- case RTL_GIGA_MAC_VER_40:
- case RTL_GIGA_MAC_VER_41:
- rtl_writephy(tp, MII_BMCR, BMCR_ANENABLE | BMCR_PDOWN);
- break;
-
- case RTL_GIGA_MAC_VER_11:
- case RTL_GIGA_MAC_VER_12:
- case RTL_GIGA_MAC_VER_17 ... RTL_GIGA_MAC_VER_28:
- case RTL_GIGA_MAC_VER_31:
- rtl_writephy(tp, 0x0e, 0x0200);
- default:
- rtl_writephy(tp, MII_BMCR, BMCR_PDOWN);
- break;
- }
-}
-
static void r8168_pll_power_down(struct rtl8169_private *tp)
{
if (r8168_check_dash(tp))
@@ -4503,7 +4462,8 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
if (rtl_wol_pll_power_down(tp))
return;
- r8168_phy_power_down(tp);
+ /* cover the case that PHY isn't connected */
+ phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
switch (tp->mac_version) {
case RTL_GIGA_MAC_VER_25 ... RTL_GIGA_MAC_VER_33:
@@ -4556,7 +4516,9 @@ static void r8168_pll_power_up(struct rtl8169_private *tp)
break;
}
- r8168_phy_power_up(tp);
+ phy_resume(tp->dev->phydev);
+ /* give MAC/PHY some time to resume */
+ msleep(20);
}
static void rtl_pll_power_down(struct rtl8169_private *tp)
--
2.18.0
^ permalink raw reply related
* [PATCH net-next v2 01/10] r8169: add basic phylib support
From: Heiner Kallweit @ 2018-07-10 18:39 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <84ac1cb7-bafd-5f15-8cda-e8dfd7d0deb3@gmail.com>
Add basic phylib support to r8169. All now unneeded old PHY handling code
will be removed in subsequent patches.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- return error in mdio ops if phyaddr > 0
- advertise pause modes
---
drivers/net/ethernet/realtek/Kconfig | 1 +
drivers/net/ethernet/realtek/r8169.c | 158 +++++++++++++++++++++------
2 files changed, 127 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 7c69f4c8..7fb1af1f 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -99,6 +99,7 @@ config R8169
depends on PCI
select FW_LOADER
select CRC32
+ select PHYLIB
select MII
---help---
Say Y here if you have a Realtek 8169 PCI Gigabit Ethernet adapter.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index d598fdf0..d1951f32 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -16,6 +16,7 @@
#include <linux/delay.h>
#include <linux/ethtool.h>
#include <linux/mii.h>
+#include <linux/phy.h>
#include <linux/if_vlan.h>
#include <linux/crc32.h>
#include <linux/in.h>
@@ -754,6 +755,7 @@ struct rtl8169_private {
} wk;
struct mii_if_info mii;
+ struct mii_bus *mii_bus;
dma_addr_t counters_phys_addr;
struct rtl8169_counters *counters;
struct rtl8169_tc_offsets tc_offset;
@@ -1444,11 +1446,6 @@ static unsigned int rtl8169_xmii_reset_pending(struct rtl8169_private *tp)
return rtl_readphy(tp, MII_BMCR) & BMCR_RESET;
}
-static unsigned int rtl8169_xmii_link_ok(struct rtl8169_private *tp)
-{
- return RTL_R8(tp, PHYstatus) & LinkStatus;
-}
-
static void rtl8169_xmii_reset_enable(struct rtl8169_private *tp)
{
unsigned int val;
@@ -1513,25 +1510,6 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
}
}
-static void rtl8169_check_link_status(struct net_device *dev,
- struct rtl8169_private *tp)
-{
- struct device *d = tp_to_dev(tp);
-
- if (rtl8169_xmii_link_ok(tp)) {
- rtl_link_chg_patch(tp);
- /* This is to cancel a scheduled suspend if there's one. */
- pm_request_resume(d);
- netif_carrier_on(dev);
- if (net_ratelimit())
- netif_info(tp, ifup, dev, "link up\n");
- } else {
- netif_carrier_off(dev);
- netif_info(tp, ifdown, dev, "link down\n");
- pm_runtime_idle(d);
- }
-}
-
#define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST)
static u32 __rtl8169_get_wol(struct rtl8169_private *tp)
@@ -6221,7 +6199,6 @@ static void rtl_reset_work(struct rtl8169_private *tp)
napi_enable(&tp->napi);
rtl_hw_start(tp);
netif_wake_queue(dev);
- rtl8169_check_link_status(dev, tp);
}
static void rtl8169_tx_timeout(struct net_device *dev)
@@ -6838,7 +6815,7 @@ static void rtl_slow_event_work(struct rtl8169_private *tp)
rtl8169_pcierr_interrupt(dev);
if (status & LinkChg)
- rtl8169_check_link_status(dev, tp);
+ phy_mac_interrupt(dev->phydev);
rtl_irq_enable_all(tp);
}
@@ -6920,10 +6897,59 @@ static void rtl8169_rx_missed(struct net_device *dev)
RTL_W32(tp, RxMissed, 0);
}
+static void r8169_phylink_handler(struct net_device *ndev)
+{
+ struct rtl8169_private *tp = netdev_priv(ndev);
+
+ if (netif_carrier_ok(ndev)) {
+ rtl_link_chg_patch(tp);
+ pm_request_resume(&tp->pci_dev->dev);
+ } else {
+ pm_runtime_idle(&tp->pci_dev->dev);
+ }
+
+ if (net_ratelimit())
+ phy_print_status(ndev->phydev);
+}
+
+static int r8169_phy_connect(struct rtl8169_private *tp)
+{
+ struct phy_device *phydev;
+ phy_interface_t phy_mode;
+ int ret;
+
+ phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII :
+ PHY_INTERFACE_MODE_MII;
+
+ phydev = mdiobus_get_phy(tp->mii_bus, 0);
+ if (!phydev)
+ return -ENODEV;
+
+ ret = phy_connect_direct(tp->dev, phydev, r8169_phylink_handler,
+ phy_mode);
+ if (ret)
+ return ret;
+
+ if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
+ netif_info(tp, probe, tp->dev,
+ "Restrict PHY to 100Mbit because MAC doesn't support 1GBit\n");
+ phy_set_max_speed(phydev, SPEED_100);
+ }
+
+ /* Ensure to advertise everything, incl. pause */
+ phydev->advertising = phydev->supported;
+
+ phy_attached_info(phydev);
+
+ return 0;
+}
+
static void rtl8169_down(struct net_device *dev)
{
struct rtl8169_private *tp = netdev_priv(dev);
+ phy_stop(dev->phydev);
+
napi_disable(&tp->napi);
netif_stop_queue(dev);
@@ -6963,6 +6989,8 @@ static int rtl8169_close(struct net_device *dev)
cancel_work_sync(&tp->wk.work);
+ phy_disconnect(dev->phydev);
+
pci_free_irq(pdev, 0, tp);
dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
@@ -7023,6 +7051,10 @@ static int rtl_open(struct net_device *dev)
if (retval < 0)
goto err_release_fw_2;
+ retval = r8169_phy_connect(tp);
+ if (retval)
+ goto err_free_irq;
+
rtl_lock_work(tp);
set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -7038,16 +7070,17 @@ static int rtl_open(struct net_device *dev)
if (!rtl8169_init_counter_offsets(tp))
netif_warn(tp, hw, dev, "counter reset/update failed\n");
+ phy_start(dev->phydev);
netif_start_queue(dev);
rtl_unlock_work(tp);
pm_runtime_put_sync(&pdev->dev);
-
- rtl8169_check_link_status(dev, tp);
out:
return retval;
+err_free_irq:
+ pci_free_irq(pdev, 0, tp);
err_release_fw_2:
rtl_release_firmware(tp);
rtl8169_rx_clear(tp);
@@ -7126,6 +7159,7 @@ static void rtl8169_net_suspend(struct net_device *dev)
if (!netif_running(dev))
return;
+ phy_stop(dev->phydev);
netif_device_detach(dev);
netif_stop_queue(dev);
@@ -7158,6 +7192,8 @@ static void __rtl8169_resume(struct net_device *dev)
rtl_pll_power_up(tp);
rtl8169_init_phy(dev, tp);
+ phy_start(tp->dev->phydev);
+
rtl_lock_work(tp);
napi_enable(&tp->napi);
set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -7303,6 +7339,7 @@ static void rtl_remove_one(struct pci_dev *pdev)
netif_napi_del(&tp->napi);
unregister_netdev(dev);
+ mdiobus_unregister(tp->mii_bus);
rtl_release_firmware(tp);
@@ -7388,6 +7425,57 @@ DECLARE_RTL_COND(rtl_rxtx_empty_cond)
return (RTL_R8(tp, MCU) & RXTX_EMPTY) == RXTX_EMPTY;
}
+static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
+{
+ struct rtl8169_private *tp = mii_bus->priv;
+
+ if (phyaddr > 0)
+ return -EINVAL;
+
+ return rtl_readphy(tp, phyreg);
+}
+
+static int r8169_mdio_write_reg(struct mii_bus *mii_bus, int phyaddr,
+ int phyreg, u16 val)
+{
+ struct rtl8169_private *tp = mii_bus->priv;
+
+ if (phyaddr > 0)
+ return -EINVAL;
+
+ rtl_writephy(tp, phyreg, val);
+
+ return 0;
+}
+
+static int r8169_mdio_register(struct rtl8169_private *tp)
+{
+ struct pci_dev *pdev = tp->pci_dev;
+ struct mii_bus *new_bus;
+ int ret;
+
+ new_bus = devm_mdiobus_alloc(&pdev->dev);
+ if (!new_bus)
+ return -ENOMEM;
+
+ new_bus->name = "r8169";
+ new_bus->phy_mask = ~1;
+ new_bus->priv = tp;
+ new_bus->parent = &pdev->dev;
+ new_bus->irq[0] = PHY_IGNORE_INTERRUPT;
+ snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x",
+ PCI_DEVID(pdev->bus->number, pdev->devfn));
+
+ new_bus->read = r8169_mdio_read_reg;
+ new_bus->write = r8169_mdio_write_reg;
+
+ ret = mdiobus_register(new_bus);
+ if (!ret)
+ tp->mii_bus = new_bus;
+
+ return ret;
+}
+
static void rtl_hw_init_8168g(struct rtl8169_private *tp)
{
u32 data;
@@ -7644,10 +7732,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
pci_set_drvdata(pdev, dev);
- rc = register_netdev(dev);
- if (rc < 0)
+ rc = r8169_mdio_register(tp);
+ if (rc)
return rc;
+ rc = register_netdev(dev);
+ if (rc)
+ goto err_mdio_unregister;
+
netif_info(tp, probe, dev, "%s, %pM, XID %08x, IRQ %d\n",
rtl_chip_infos[chipset].name, dev->dev_addr,
(u32)(RTL_R32(tp, TxConfig) & 0xfcf0f8ff),
@@ -7662,12 +7754,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
if (r8168_check_dash(tp))
rtl8168_driver_start(tp);
- netif_carrier_off(dev);
-
if (pci_dev_run_wake(pdev))
pm_runtime_put_sync(&pdev->dev);
return 0;
+
+err_mdio_unregister:
+ mdiobus_unregister(tp->mii_bus);
+ return rc;
}
static struct pci_driver rtl8169_pci_driver = {
--
2.18.0
^ permalink raw reply related
* Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian
From: Daniel Borkmann @ 2018-07-10 18:13 UTC (permalink / raw)
To: Martin KaFai Lau, David Laight
Cc: Okash Khawaja, Alexei Starovoitov, Yonghong Song, Jakub Kicinski,
David S. Miller, netdev@vger.kernel.org, kernel-team@fb.com,
linux-kernel@vger.kernel.org
In-Reply-To: <20180710171836.xcegzammqym4sqri@kafai-mbp.dhcp.thefacebook.com>
On 07/10/2018 07:18 PM, Martin KaFai Lau wrote:
[...]
> I would prefer to keep it as u16 which is the max width that is allowed for
> this field in the wire format. Keeping the usage consistent can avoid
> accidentally incorrect offsetting or writing wrong data out in other
> cases.
+1
^ permalink raw reply
* Re: [RFC] Add BPF_SYNCHRONIZE bpf(2) command
From: Paul E. McKenney @ 2018-07-10 17:42 UTC (permalink / raw)
To: Joel Fernandes
Cc: Joel Fernandes, Mathieu Desnoyers, Alexei Starovoitov,
Daniel Colascione, Alexei Starovoitov, linux-kernel, Tim Murray,
Daniel Borkmann, netdev, fengc
In-Reply-To: <20180710172957.GA103636@joelaf.mtv.corp.google.com>
On Tue, Jul 10, 2018 at 10:29:57AM -0700, Joel Fernandes wrote:
> On Tue, Jul 10, 2018 at 10:12:29AM -0700, Paul E. McKenney wrote:
> [..]
> > > > > The other question I have is about the whole "nohz-full doesn't work" thing.
> > > > > I didn't fully understand why. RCU is already tracking the state of nohz-full
> > > > > CPUs because the rcu dynticks code in (kernel/rcu/tree.c) monitors
> > > > > transitions to and from usermode even if the timer tick is turned off. So why
> > > > > would it not work?
> > > >
> > > > In the nohz_full case, there is no need for sys_membarrier()'s call to
> > > > synchronize_sched() to interact directly with the nohz_full CPU. It
> > > > can instead look at the target CPU's dyntick-idle state, and that state
> > > > would potentially have been set in the dim distant past, thus having
> > > > no effect on the target CPU's current execution.
> > >
> > > In nohz-idle case though, there's nothing to promote the barrier() to
> > > smp_mb() if you were to purely look at the dynticks-idle state on the
> > > nohz-full CPU executing in user mode?
> > >
> > > So then it makes sense to me now that nohz-full needs something to IPI that
> > > CPU inorder to enforce the needed memory barrier and pure synchronize_sched()
> > > wouldn't work. So then makes me think the expedited versions of
> > > synchronize_sched should be able to do the job but I could off on a different
> > > track..
> >
> > The problem is that the expedited versions also check the dyntick-idle
> > state and don't touch idle (or nohz_full usermode) CPUs. This is by
> > design for the battery-powered embedded use case. ;-)
>
> Oh ok! ;)
>
> I guess there's also a MEMBARRIER_CMD_GLOBAL_EXPEDITED which seems to IPI
> CPUs (I'm guessing regardless of dynticks state) and execute smp_mb within
> the IPI so userspace can fallback to using that incase MEMBARRIER_CMD_GLOBAL
> returns -EINVAL.
Yes, and this avoids IPIing idle CPUs via the ->mm checks. But it will
IPI nohz_full CPUs in that same process, as it must for correctness.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse
From: Jakub Kicinski @ 2018-07-10 18:09 UTC (permalink / raw)
To: Andrey Ignatov; +Cc: alexei.starovoitov, daniel, oss-drivers, netdev
In-Reply-To: <20180710042319.GA77322@rdna-mbp.dhcp.thefacebook.com>
On Mon, 9 Jul 2018 21:23:20 -0700, Andrey Ignatov wrote:
> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 19:49 -0700]:
> > On Mon, 9 Jul 2018 13:22:54 -0700, Andrey Ignatov wrote:
> > > Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 11:01 -0700]:
> > > fd of every map is set to -1 in bpf_object__init_maps() that, in turn, is
> > > called from __bpf_object__open():
> > >
> > > for (i = 0; i < nr_maps; i++)
> > > obj->maps[i].fd = -1;
> > >
> > > Later it will either contain valid fd that is >= 0, or that same -1, what
> > > should be enough to identify fd presence.
> >
> > I thought it to be cleaner to indicate the fd has been pre-set, in case
> > things get more complicated in the future and fd >= 0 becomes ambiguous.
> >
> > But no strong preference, should I change?
>
> My preference (not strong either) is to avoid a new field whenever it's
> possible. Though if you have a use-case that can't be covered by
> (fd >= 0) keeping the field is fine as well.
Okay, I can change. I think it may be worthwhile keeping the
information that the map definition was replaced by something that did
not come from the ELF file, but I don't actually have a use for it
right now, so we can just add it back later :)
> > > > + map->fd = dup(fd);
> > >
> > > Unfortunately, new descriptor created by dup(2) will not have O_CLOEXEC set, in
> > > contrast to original fd returned by kernel on map creation.
> > >
> > > libbpf has other interface shortcomings where it comes up. E.g. struct
> > > bpf_object owns all descriptors it contains (progs, maps) and closes them in
> > > bpf_object__close(). if one wants to open/load ELF, then close it but
> > > keep, say, prog fd to attach it to cgroup some time later, then fd
> > > should be duplicated as well to get a new one not owned by bpf_object.
> > >
> > > Currently I use this workaround to avoid time when new fd doesn't have
> > > O_CLOEXEC:
> > >
> > > int new_prog_fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
> > > if (new_prog_fd < 0 ||
> > > dup3(bpf_program__fd(prog), new_prog_fd, O_CLOEXEC) == -1) {
> > > /* .. handle error .. */
> > > close(new_prog_fd);
> > > }
> > > /* .. use new_prog_fd with O_CLOEXEC set */
> > >
> > > Not sure how to simplify it. dup2() has same problem with regard to
> > > O_CLOEXEC.
> > >
> > > Use-case: standalone server application that uses libbpf and does
> > > fork()/execve() a lot.
> >
> > Good point! I have no better ideas. Although being slightly paranoid
> > I would perhaps use "/" instead of "/dev/null"? Shouldn't matter?
>
> No strong preferences, important thing is to create fd with O_CLOEXEC
> set somehow.
>
> Is it safer to use "/" than "/dev/null"? (trying to understand if I
> should change my code as well)
IDK :) Could there be a crazy scenario when someone runs chroot or a
very broken system without /dev/null? / should always be there?
Thanks for all the other reviews, I will update the code accordingly!
^ permalink raw reply
* Re: [PATCH RFC net-next] openvswitch: Queue upcalls to userspace in per-port round-robin order
From: Pravin Shelar @ 2018-07-10 18:31 UTC (permalink / raw)
To: Matteo Croce
Cc: ovs dev, Linux Kernel Network Developers, Jiri Benc,
Stefano Brivio
In-Reply-To: <20180704142342.21740-1-mcroce-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Wed, Jul 4, 2018 at 7:23 AM, Matteo Croce <mcroce-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> From: Stefano Brivio <sbrivio-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> Open vSwitch sends to userspace all received packets that have
> no associated flow (thus doing an "upcall"). Then the userspace
> program creates a new flow and determines the actions to apply
> based on its configuration.
>
> When a single port generates a high rate of upcalls, it can
> prevent other ports from dispatching their own upcalls. vswitchd
> overcomes this problem by creating many netlink sockets for each
> port, but it quickly exceeds any reasonable maximum number of
> open files when dealing with huge amounts of ports.
>
> This patch queues all the upcalls into a list, ordering them in
> a per-port round-robin fashion, and schedules a deferred work to
> queue them to userspace.
>
> The algorithm to queue upcalls in a round-robin fashion,
> provided by Stefano, is based on these two rules:
> - upcalls for a given port must be inserted after all the other
> occurrences of upcalls for the same port already in the queue,
> in order to avoid out-of-order upcalls for a given port
> - insertion happens once the highest upcall count for any given
> port (excluding the one currently at hand) is greater than the
> count for the port we're queuing to -- if this condition is
> never true, upcall is queued at the tail. This results in a
> per-port round-robin order.
>
> In order to implement a fair round-robin behaviour, a variable
> queueing delay is introduced. This will be zero if the upcalls
> rate is below a given threshold, and grows linearly with the
> queue utilisation (i.e. upcalls rate) otherwise.
>
> This ensures fairness among ports under load and with few
> netlink sockets.
>
Thanks for the patch.
This patch is adding following overhead for upcall handling:
1. kmalloc.
2. global spin-lock.
3. context switch to single worker thread.
I think this could become bottle neck on most of multi core systems.
You have mentioned issue with existing fairness mechanism, Can you
elaborate on those, I think we could improve that before implementing
heavy weight fairness in upcall handling.
^ permalink raw reply
* Re: [PATCH net-next 01/13] ARM: net: bpf: enumerate the JIT scratch stack layout
From: Daniel Borkmann @ 2018-07-10 18:30 UTC (permalink / raw)
To: Russell King; +Cc: netdev, linux-arm-kernel
In-Reply-To: <E1fcrsG-0005UK-Un@rmk-PC.armlinux.org.uk>
Hi Russell,
thanks a lot for your work on the arm32 JIT!
On 07/10/2018 02:36 PM, Russell King wrote:
> Enumerate the contents of the JIT scratch stack layout used for storing
> some of the JITs 64-bit registers, tail call counter and AX register.
>
> XXX: what about the skb_copy_bits buffer - this appears to overlap with
> the first word of the JITs accessible stack.
Could you elaborate on that case? Unless I'm missing something there should
be no use of the skb_copy_bits buffer anymore (aka former SKB_BUFFER at
STACK_VAR(SCRATCH_SIZE) offset), but aside from that it's not supposed to
overlap either.
Thanks,
Daniel
^ permalink raw reply
* [PATCH net-next v2 00/10] r8169: add phylib support
From: Heiner Kallweit @ 2018-07-10 18:29 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
Now that all the basic refactoring has been done we can add phylib
support. This patch series was successfully tested on:
RTL8168h
RTL8168evl
RTL8169sb
Changes in v2:
- return error in mdio ops if phyaddr > 0
- advertise pause modes
- added reviewed-by for several patches
Heiner Kallweit (10):
r8169: add basic phylib support
r8169: use phy_resume/phy_suspend
r8169: replace open-coded PHY soft reset with genphy_soft_reset
r8169: use phy_ethtool_(g|s)et_link_ksettings
r8169: use phy_ethtool_nway_reset
r8169: use phy_mii_ioctl
r8169: migrate speed_down function to phylib
r8169: remove rtl8169_set_speed_xmii
r8169: remove mii_if_info member from struct rtl8169_private
r8169: don't read chip phy status register
drivers/net/ethernet/realtek/Kconfig | 2 +-
drivers/net/ethernet/realtek/r8169.c | 466 ++++++++++-----------------
2 files changed, 164 insertions(+), 304 deletions(-)
--
2.18.0
^ permalink raw reply
* Re: [PATCH rdma-next 0/9] Support mlx5 flow steering with RAW data
From: Jason Gunthorpe @ 2018-07-10 18:28 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180708102445.25496-1-leon@kernel.org>
On Sun, Jul 08, 2018 at 01:24:36PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> >From Yishai:
>
> This series introduces vendor create and destroy flow methods on the
> uverbs flow object by using the KABI infra-structure.
>
> It's done in a way that enables the driver to get its specific device
> attributes in a raw data to match its underlay specification while still
> using the generic ib_flow object for cleanup and code sharing.
>
> In addition, a specific mlx5 matcher object and its create/destroy
> methods were introduced. This object matches the underlay flow steering
> mask specification and is used as part of mlx5 create flow input data.
>
> This series supports IB_QP/TIR as its flow steering destination as
> applicable today via the ib_create_flow API, however, it adds also an
> option to work with DEVX object which its destination can be both TIR
> and flow table.
>
> Few changes were done in the mlx5 core layer to support forward
> compatible for the device specification raw data and to support flow
> table when the DEVX destination is used.
>
> As part of this series the default IB destroy handler
> (i.e. uverbs_destroy_def_handler()) was exposed from IB core to be
> used by the drivers and existing code was refactored to use it.
>
> Thanks
> IB: Enable uverbs_destroy_def_handler to be used by drivers
I applied this one
> Yishai Hadas (9):
> net/mlx5: Add forward compatible support for the FTE match data
> net/mlx5: Add support for flow table destination number
> IB/mlx5: Introduce flow steering matcher object
> IB: Consider ib_flow creation by the KABI infrastructure
> IB/mlx5: Introduce vendor create and destroy flow methods
> IB/mlx5: Support adding flow steering rule by raw data
> IB/mlx5: Add support for a flow table destination
> IB/mlx5: Expose vendor flow trees
The rest will need to be resent after the comments are addressed.
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH bpf-next v2 11/12] tools: libbpf: allow map reuse
From: Daniel Borkmann @ 2018-07-10 18:16 UTC (permalink / raw)
To: Jakub Kicinski, Andrey Ignatov; +Cc: alexei.starovoitov, oss-drivers, netdev
In-Reply-To: <20180710110906.48df8981@cakuba.lan>
On 07/10/2018 08:09 PM, Jakub Kicinski wrote:
> On Mon, 9 Jul 2018 21:23:20 -0700, Andrey Ignatov wrote:
>> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 19:49 -0700]:
>>> On Mon, 9 Jul 2018 13:22:54 -0700, Andrey Ignatov wrote:
>>>> Jakub Kicinski <jakub.kicinski@netronome.com> [Mon, 2018-07-09 11:01 -0700]:
>>>> fd of every map is set to -1 in bpf_object__init_maps() that, in turn, is
>>>> called from __bpf_object__open():
>>>>
>>>> for (i = 0; i < nr_maps; i++)
>>>> obj->maps[i].fd = -1;
>>>>
>>>> Later it will either contain valid fd that is >= 0, or that same -1, what
>>>> should be enough to identify fd presence.
>>>
>>> I thought it to be cleaner to indicate the fd has been pre-set, in case
>>> things get more complicated in the future and fd >= 0 becomes ambiguous.
>>>
>>> But no strong preference, should I change?
>>
>> My preference (not strong either) is to avoid a new field whenever it's
>> possible. Though if you have a use-case that can't be covered by
>> (fd >= 0) keeping the field is fine as well.
>
> Okay, I can change. I think it may be worthwhile keeping the
> information that the map definition was replaced by something that did
> not come from the ELF file, but I don't actually have a use for it
> right now, so we can just add it back later :)
>
>>>>> + map->fd = dup(fd);
>>>>
>>>> Unfortunately, new descriptor created by dup(2) will not have O_CLOEXEC set, in
>>>> contrast to original fd returned by kernel on map creation.
>>>>
>>>> libbpf has other interface shortcomings where it comes up. E.g. struct
>>>> bpf_object owns all descriptors it contains (progs, maps) and closes them in
>>>> bpf_object__close(). if one wants to open/load ELF, then close it but
>>>> keep, say, prog fd to attach it to cgroup some time later, then fd
>>>> should be duplicated as well to get a new one not owned by bpf_object.
>>>>
>>>> Currently I use this workaround to avoid time when new fd doesn't have
>>>> O_CLOEXEC:
>>>>
>>>> int new_prog_fd = open("/dev/null", O_RDONLY | O_CLOEXEC);
>>>> if (new_prog_fd < 0 ||
>>>> dup3(bpf_program__fd(prog), new_prog_fd, O_CLOEXEC) == -1) {
>>>> /* .. handle error .. */
>>>> close(new_prog_fd);
>>>> }
>>>> /* .. use new_prog_fd with O_CLOEXEC set */
>>>>
>>>> Not sure how to simplify it. dup2() has same problem with regard to
>>>> O_CLOEXEC.
>>>>
>>>> Use-case: standalone server application that uses libbpf and does
>>>> fork()/execve() a lot.
>>>
>>> Good point! I have no better ideas. Although being slightly paranoid
>>> I would perhaps use "/" instead of "/dev/null"? Shouldn't matter?
>>
>> No strong preferences, important thing is to create fd with O_CLOEXEC
>> set somehow.
>>
>> Is it safer to use "/" than "/dev/null"? (trying to understand if I
>> should change my code as well)
>
> IDK :) Could there be a crazy scenario when someone runs chroot or a
> very broken system without /dev/null? / should always be there?
>
> Thanks for all the other reviews, I will update the code accordingly!
Ok, I've tossed the v2 series from patchwork and waiting for your respin
in that case.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian
From: Okash Khawaja @ 2018-07-10 17:49 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Martin KaFai Lau, Alexei Starovoitov, Yonghong Song,
Jakub Kicinski, David S. Miller, netdev, kernel-team,
linux-kernel
In-Reply-To: <58136182-0eb1-78c9-ceb9-402418c7d10c@iogearbox.net>
On Tue, Jul 10, 2018 at 10:21:02AM +0200, Daniel Borkmann wrote:
> On 07/09/2018 08:32 PM, Martin KaFai Lau wrote:
> > On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
> >> When extracting bitfield from a number, btf_int_bits_seq_show() builds
> >> a mask and accesses least significant byte of the number in a way
> >> specific to little-endian. This patch fixes that by checking endianness
> >> of the machine and then shifting left and right the unneeded bits.
> >>
> >> Thanks to Martin Lau for the help in navigating potential pitfalls when
> >> dealing with endianess and for the final solution.
> >>
> >> Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
> >> Signed-off-by: Okash Khawaja <osk@fb.com>
> >>
> >> ---
> >> kernel/bpf/btf.c | 32 +++++++++++++++-----------------
> >> 1 file changed, 15 insertions(+), 17 deletions(-)
> >>
> >> --- a/kernel/bpf/btf.c
> >> +++ b/kernel/bpf/btf.c
> >> @@ -162,6 +162,8 @@
> >> #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
> >> #define BITS_ROUNDUP_BYTES(bits) \
> >> (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
> >> +const int one = 1;
> >> +#define is_big_endian() ((*(char *)&one) == 0)
>
> Also here, in the kernel archs provide proper definitions.
Is this the __BIG_ENDIAN #define or are there better ways to check that?
Thanks,
Okash
^ permalink raw reply
* Re: [RFC] Add BPF_SYNCHRONIZE bpf(2) command
From: Joel Fernandes @ 2018-07-10 17:29 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Joel Fernandes, Mathieu Desnoyers, Alexei Starovoitov,
Daniel Colascione, Alexei Starovoitov, linux-kernel, Tim Murray,
Daniel Borkmann, netdev, fengc
In-Reply-To: <20180710171229.GZ3593@linux.vnet.ibm.com>
On Tue, Jul 10, 2018 at 10:12:29AM -0700, Paul E. McKenney wrote:
[..]
> > > > The other question I have is about the whole "nohz-full doesn't work" thing.
> > > > I didn't fully understand why. RCU is already tracking the state of nohz-full
> > > > CPUs because the rcu dynticks code in (kernel/rcu/tree.c) monitors
> > > > transitions to and from usermode even if the timer tick is turned off. So why
> > > > would it not work?
> > >
> > > In the nohz_full case, there is no need for sys_membarrier()'s call to
> > > synchronize_sched() to interact directly with the nohz_full CPU. It
> > > can instead look at the target CPU's dyntick-idle state, and that state
> > > would potentially have been set in the dim distant past, thus having
> > > no effect on the target CPU's current execution.
> >
> > In nohz-idle case though, there's nothing to promote the barrier() to
> > smp_mb() if you were to purely look at the dynticks-idle state on the
> > nohz-full CPU executing in user mode?
> >
> > So then it makes sense to me now that nohz-full needs something to IPI that
> > CPU inorder to enforce the needed memory barrier and pure synchronize_sched()
> > wouldn't work. So then makes me think the expedited versions of
> > synchronize_sched should be able to do the job but I could off on a different
> > track..
>
> The problem is that the expedited versions also check the dyntick-idle
> state and don't touch idle (or nohz_full usermode) CPUs. This is by
> design for the battery-powered embedded use case. ;-)
Oh ok! ;)
I guess there's also a MEMBARRIER_CMD_GLOBAL_EXPEDITED which seems to IPI
CPUs (I'm guessing regardless of dynticks state) and execute smp_mb within
the IPI so userspace can fallback to using that incase MEMBARRIER_CMD_GLOBAL
returns -EINVAL.
thanks!
-Joel
^ permalink raw reply
* Re: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian
From: Martin KaFai Lau @ 2018-07-10 17:18 UTC (permalink / raw)
To: David Laight
Cc: Okash Khawaja, Daniel Borkmann, Alexei Starovoitov, Yonghong Song,
Jakub Kicinski, David S. Miller, netdev@vger.kernel.org,
kernel-team@fb.com, linux-kernel@vger.kernel.org
In-Reply-To: <457ec72d8d7c4cdd8c112f9c8c91b0e7@AcuMS.aculab.com>
On Tue, Jul 10, 2018 at 04:35:04PM +0000, David Laight wrote:
> From: Martin KaFai Lau
> > Sent: 09 July 2018 19:33
> > On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
> > > When extracting bitfield from a number, btf_int_bits_seq_show() builds
> > > a mask and accesses least significant byte of the number in a way
> > > specific to little-endian. This patch fixes that by checking endianness
> > > of the machine and then shifting left and right the unneeded bits.
> > >
> > > Thanks to Martin Lau for the help in navigating potential pitfalls when
> > > dealing with endianess and for the final solution.
> > >
> > > Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
> > > Signed-off-by: Okash Khawaja <osk@fb.com>
> > >
> > > ---
> > > kernel/bpf/btf.c | 32 +++++++++++++++-----------------
> > > 1 file changed, 15 insertions(+), 17 deletions(-)
> > >
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -162,6 +162,8 @@
> > > #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
> > > #define BITS_ROUNDUP_BYTES(bits) \
> > > (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
> > > +const int one = 1;
> > > +#define is_big_endian() ((*(char *)&one) == 0)
> > >
> > > #define BTF_INFO_MASK 0x0f00ffff
> > > #define BTF_INT_MASK 0x0fffffff
> > > @@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
> > > void *data, u8 bits_offset,
> > > struct seq_file *m)
> > > {
> > > + u8 left_shift_bits, right_shift_bits;
> > Nit.
> > Although only max 64 bit int is allowed now (ensured by btf_int_check_meta),
> > it is better to use u16 such that it will be consistent to BTF_INT_BITS.
>
> Double-nit.
>
> Use 'int' or 'unsigned int'.
> Sub-word arithmetic will require extra instructions on almost everything
> except x86.
I would prefer to keep it as u16 which is the max width that is allowed for
this field in the wire format. Keeping the usage consistent can avoid
accidentally incorrect offsetting or writing wrong data out in other
cases.
^ permalink raw reply
* Re: [PATCH net-next 13/13] ARM: net: bpf: use double-word load/stores where available
From: Russell King - ARM Linux @ 2018-07-10 17:14 UTC (permalink / raw)
To: Olof Johansson
Cc: Network Development, Linux ARM Mailing List, Daniel Borkmann
In-Reply-To: <CAOesGMio4JTzAPQ3Bkx8LxhkhJStzOGC6t2U8aC6m4TiBtaunQ@mail.gmail.com>
On Tue, Jul 10, 2018 at 10:03:33AM -0700, Olof Johansson wrote:
> Hi Russell,
> > @@ -663,13 +679,27 @@ static inline void emit_a32_mov_r(const s8 dst, const s8 src,
> > static inline void emit_a32_mov_r64(const bool is64, const s8 dst[],
> > const s8 src[],
> > struct jit_ctx *ctx) {
> > - emit_a32_mov_r(dst_lo, src_lo, ctx);
> > - if (is64) {
> > + if (!is64) {
> > + emit_a32_mov_r(dst_lo, src_lo, ctx);
> > + /* Zero out high 4 bytes */
> > + emit_a32_mov_i(dst_hi, 0, ctx);
> > + } else if (__LINUX_ARM_ARCH__ < 6 &&
> > + ctx->cpu_architecture < CPU_ARCH_ARMv5) {
> > /* complete 8 byte move */
> > + emit_a32_mov_r(dst_lo, src_lo, ctx);
> > emit_a32_mov_r(dst_hi, src_hi, ctx);
>
>
> Tiny nit: Looks like you compare for >= ARMv5TE above and <ARMv5 here.
Good catch, I'll fix it, and it gives me some satisfaction that
someone is reviewing this JIT code closely! IMHO, JITs need a lot
of scrutiny.
> I'm not aware of any vanilla v5 implementations (all I can find are
> v5TE or <=v4T), so it doesn't seem like something actually causing
> problems. Mostly pointing it out for consistency's sake.
They're rare - I think the only one is an ARM1020 (ARMv5T) as opposed
to the ARM1020E (ARMv5TE). Whether any are in the wild or not is
another matter.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
^ permalink raw reply
* Re: [RFC] Add BPF_SYNCHRONIZE bpf(2) command
From: Paul E. McKenney @ 2018-07-10 17:12 UTC (permalink / raw)
To: Joel Fernandes
Cc: Joel Fernandes, Mathieu Desnoyers, Alexei Starovoitov,
Daniel Colascione, Alexei Starovoitov, linux-kernel, Tim Murray,
Daniel Borkmann, netdev, fengc
In-Reply-To: <20180710165744.GA99146@joelaf.mtv.corp.google.com>
On Tue, Jul 10, 2018 at 09:57:44AM -0700, Joel Fernandes wrote:
> On Tue, Jul 10, 2018 at 09:42:12AM -0700, Paul E. McKenney wrote:
> > On Mon, Jul 09, 2018 at 10:13:47PM -0700, Joel Fernandes wrote:
> > > On Sun, Jul 08, 2018 at 04:54:38PM -0400, Mathieu Desnoyers wrote:
> > > > ----- On Jul 7, 2018, at 4:33 PM, Joel Fernandes joelaf@google.com wrote:
> > > >
> > > > > On Fri, Jul 06, 2018 at 07:54:28PM -0700, Alexei Starovoitov wrote:
> > > > >> On Fri, Jul 06, 2018 at 06:56:16PM -0700, Daniel Colascione wrote:
> > > > >> > BPF_SYNCHRONIZE waits for any BPF programs active at the time of
> > > > >> > BPF_SYNCHRONIZE to complete, allowing userspace to ensure atomicity of
> > > > >> > RCU data structure operations with respect to active programs. For
> > > > >> > example, userspace can update a map->map entry to point to a new map,
> > > > >> > use BPF_SYNCHRONIZE to wait for any BPF programs using the old map to
> > > > >> > complete, and then drain the old map without fear that BPF programs
> > > > >> > may still be updating it.
> > > > >> >
> > > > >> > Signed-off-by: Daniel Colascione <dancol@google.com>
> > > > >> > ---
> > > > >> > include/uapi/linux/bpf.h | 1 +
> > > > >> > kernel/bpf/syscall.c | 14 ++++++++++++++
> > > > >> > 2 files changed, 15 insertions(+)
> > > > >> >
> > > > >> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > >> > index b7db3261c62d..4365c50e8055 100644
> > > > >> > --- a/include/uapi/linux/bpf.h
> > > > >> > +++ b/include/uapi/linux/bpf.h
> > > > >> > @@ -98,6 +98,7 @@ enum bpf_cmd {
> > > > >> > BPF_BTF_LOAD,
> > > > >> > BPF_BTF_GET_FD_BY_ID,
> > > > >> > BPF_TASK_FD_QUERY,
> > > > >> > + BPF_SYNCHRONIZE,
> > > > >> > };
> > > > >> >
> > > > >> > enum bpf_map_type {
> > > > >> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > > >> > index d10ecd78105f..60ec7811846e 100644
> > > > >> > --- a/kernel/bpf/syscall.c
> > > > >> > +++ b/kernel/bpf/syscall.c
> > > > >> > @@ -2272,6 +2272,20 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *,
> > > > >> > uattr, unsigned int, siz
> > > > >> > if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
> > > > >> > return -EPERM;
> > > > >> >
> > > > >> > + if (cmd == BPF_SYNCHRONIZE) {
> > > > >> > + if (uattr != NULL || size != 0)
> > > > >> > + return -EINVAL;
> > > > >> > + err = security_bpf(cmd, NULL, 0);
> > > > >> > + if (err < 0)
> > > > >> > + return err;
> > > > >> > + /* BPF programs are run with preempt disabled, so
> > > > >> > + * synchronize_sched is sufficient even with
> > > > >> > + * RCU_PREEMPT.
> > > > >> > + */
> > > > >> > + synchronize_sched();
> > > > >> > + return 0;
> > > > >>
> > > > >> I don't think it's necessary. sys_membarrier() can do this already
> > > > >> and some folks use it exactly for this use case.
> > > > >
> > > > > Alexei, the use of sys_membarrier for this purpose seems kind of weird to me
> > > > > though. No where does the manpage say membarrier should be implemented this
> > > > > way so what happens if the implementation changes?
> > > > >
> > > > > Further, membarrier manpage says that a memory barrier should be matched with
> > > > > a matching barrier. In this use case there is no matching barrier, so it
> > > > > makes it weirder.
> > > > >
> > > > > Lastly, sys_membarrier seems will not work on nohz-full systems, so its a bit
> > > > > fragile to depend on it for this?
> > > > >
> > > > > case MEMBARRIER_CMD_GLOBAL:
> > > > > /* MEMBARRIER_CMD_GLOBAL is not compatible with nohz_full. */
> > > > > if (tick_nohz_full_enabled())
> > > > > return -EINVAL;
> > > > > if (num_online_cpus() > 1)
> > > > > synchronize_sched();
> > > > > return 0;
> > > > >
> > > > >
> > > > > Adding Mathieu as well who I believe is author/maintainer of membarrier.
> > > >
> > > > See commit 907565337
> > > > "Fix: Disable sys_membarrier when nohz_full is enabled"
> > > >
> > > > "Userspace applications should be allowed to expect the membarrier system
> > > > call with MEMBARRIER_CMD_SHARED command to issue memory barriers on
> > > > nohz_full CPUs, but synchronize_sched() does not take those into
> > > > account."
> > > >
> > > > So AFAIU you'd want to re-use membarrier to issue synchronize_sched, and you
> > > > only care about kernel preempt off critical sections.
> > >
> > > Mathieu, Thanks a lot for your reply. I understand what you said and agree
> > > with you. Slight OT, but I tried to go back to first principles and
> > > understand how membarrier() uses synchronize_sched() for the "slow path" and
> > > it didn't make immediate sense to me. Let me clarify my dillema..
> > >
> > > My understanding is membarrier's MEMBARRIER_CMD_GLOBAL will employ
> > > synchronize_sched to make sure all other CPUs aren't executing anymore in an
> > > section of usercode that happen to be accessing memory that was written to
> > > before the membarrier call was made. To do this, the system call will use
> > > synchronize_sched to try to guarantee that all user-mode execution that
> > > started before the membarrier call would be completed when the membarrier
> > > call returns. This guarantees that without using a real memory barrier on the
> > > "fast path", things work just fine and everyone wins.
> > >
> > > But, going through RCU code, I see that a "RCU-sched quiecent state" on a CPU
> > > may be reached when the CPU receives a timer tick while executing in user
> > > mode:
> > >
> > > void rcu_check_callbacks(int user)
> > > {
> > > trace_rcu_utilization(TPS("Start scheduler-tick"));
> > > increment_cpu_stall_ticks();
> > > if (user || rcu_is_cpu_rrupt_from_idle()) {
> > > [...]
> > > rcu_sched_qs();
> > > rcu_bh_qs();
> > >
> > > The problem I see is the CPU could be executing usermode code at the time of
> > > the RCU sched-QS. This IMO is enough reason for synchronize_sched() to
> > > return, because the CPU in question just reported a QS (assuming all other
> > > CPUs also happen to do so if they needed to).
> >
> > This scenario will have inserted the needed smp_mb() into the userspace
> > instruction execution stream, as is required by the sys_membarrier
> > use cases.
>
> Oh ok, that makes sense!
>
> > > Then I am wondering how does the membarrier call even work, the tick could
> > > very well have interrupted the CPU while it was executing usermode code in
> > > the middle of a set of instructions performing memory accesses. Reporting a
> > > quiescent state at such an inopportune time would cause the membarrier call
> > > to prematurely return, no? Sorry if I missed something.
> >
> > One way to think of sys_membarrier() is as something that promotes a
> > barrier() to an smp_mb(). This barrier then separates the target CPU's
> > accesses that the caller saw before the sys_membarrier() from that same
> > CPU's accesses that the caller will see after the sys_membarrier().
>
> Got it!
>
> > > The other question I have is about the whole "nohz-full doesn't work" thing.
> > > I didn't fully understand why. RCU is already tracking the state of nohz-full
> > > CPUs because the rcu dynticks code in (kernel/rcu/tree.c) monitors
> > > transitions to and from usermode even if the timer tick is turned off. So why
> > > would it not work?
> >
> > In the nohz_full case, there is no need for sys_membarrier()'s call to
> > synchronize_sched() to interact directly with the nohz_full CPU. It
> > can instead look at the target CPU's dyntick-idle state, and that state
> > would potentially have been set in the dim distant past, thus having
> > no effect on the target CPU's current execution.
>
> In nohz-idle case though, there's nothing to promote the barrier() to
> smp_mb() if you were to purely look at the dynticks-idle state on the
> nohz-full CPU executing in user mode?
>
> So then it makes sense to me now that nohz-full needs something to IPI that
> CPU inorder to enforce the needed memory barrier and pure synchronize_sched()
> wouldn't work. So then makes me think the expedited versions of
> synchronize_sched should be able to do the job but I could off on a different
> track..
The problem is that the expedited versions also check the dyntick-idle
state and don't touch idle (or nohz_full usermode) CPUs. This is by
design for the battery-powered embedded use case. ;-)
Thanx, Paul
> Thanks a lot,
>
> -Joel
>
>
^ permalink raw reply
* Re: [PATCH net-next 13/13] ARM: net: bpf: use double-word load/stores where available
From: Olof Johansson @ 2018-07-10 17:03 UTC (permalink / raw)
To: Russell King; +Cc: Network Development, Linux ARM Mailing List, Daniel Borkmann
In-Reply-To: <E1fcrtG-0005Vh-DK@rmk-PC.armlinux.org.uk>
Hi Russell,
On Tue, Jul 10, 2018 at 5:37 AM, Russell King
<rmk+kernel@armlinux.org.uk> wrote:
> Use double-word load and stores where support for this instruction is
> supported by the CPU architecture.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> arch/arm/net/bpf_jit_32.c | 55 ++++++++++++++++++++++++++++++++++++-----------
> arch/arm/net/bpf_jit_32.h | 2 ++
> 2 files changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm/net/bpf_jit_32.c b/arch/arm/net/bpf_jit_32.c
> index ca6534cabfa9..7e1d1635c65b 100644
> --- a/arch/arm/net/bpf_jit_32.c
> +++ b/arch/arm/net/bpf_jit_32.c
> @@ -485,10 +489,16 @@ static const s8 *arm_bpf_get_reg64(const s8 *reg, const s8 *tmp,
> struct jit_ctx *ctx)
> {
> if (is_stacked(reg[1])) {
> - emit(ARM_LDR_I(tmp[1], ARM_FP, EBPF_SCRATCH_TO_ARM_FP(reg[1])),
> - ctx);
> - emit(ARM_LDR_I(tmp[0], ARM_FP, EBPF_SCRATCH_TO_ARM_FP(reg[0])),
> - ctx);
> + if (__LINUX_ARM_ARCH__ >= 6 ||
> + ctx->cpu_architecture >= CPU_ARCH_ARMv5TE) {
> + emit(ARM_LDRD_I(tmp[1], ARM_FP,
> + EBPF_SCRATCH_TO_ARM_FP(reg[1])), ctx);
> + } else {
> + emit(ARM_LDR_I(tmp[1], ARM_FP,
> + EBPF_SCRATCH_TO_ARM_FP(reg[1])), ctx);
> + emit(ARM_LDR_I(tmp[0], ARM_FP,
> + EBPF_SCRATCH_TO_ARM_FP(reg[0])), ctx);
> + }
> reg = tmp;
> }
> return reg;
> @@ -510,10 +520,16 @@ static void arm_bpf_put_reg64(const s8 *reg, const s8 *src,
> struct jit_ctx *ctx)
> {
> if (is_stacked(reg[1])) {
> - emit(ARM_STR_I(src[1], ARM_FP, EBPF_SCRATCH_TO_ARM_FP(reg[1])),
> - ctx);
> - emit(ARM_STR_I(src[0], ARM_FP, EBPF_SCRATCH_TO_ARM_FP(reg[0])),
> - ctx);
> + if (__LINUX_ARM_ARCH__ >= 6 ||
> + ctx->cpu_architecture >= CPU_ARCH_ARMv5TE) {
> + emit(ARM_STRD_I(src[1], ARM_FP,
> + EBPF_SCRATCH_TO_ARM_FP(reg[1])), ctx);
> + } else {
> + emit(ARM_STR_I(src[1], ARM_FP,
> + EBPF_SCRATCH_TO_ARM_FP(reg[1])), ctx);
> + emit(ARM_STR_I(src[0], ARM_FP,
> + EBPF_SCRATCH_TO_ARM_FP(reg[0])), ctx);
> + }
> } else {
> if (reg[1] != src[1])
> emit(ARM_MOV_R(reg[1], src[1]), ctx);
> @@ -663,13 +679,27 @@ static inline void emit_a32_mov_r(const s8 dst, const s8 src,
> static inline void emit_a32_mov_r64(const bool is64, const s8 dst[],
> const s8 src[],
> struct jit_ctx *ctx) {
> - emit_a32_mov_r(dst_lo, src_lo, ctx);
> - if (is64) {
> + if (!is64) {
> + emit_a32_mov_r(dst_lo, src_lo, ctx);
> + /* Zero out high 4 bytes */
> + emit_a32_mov_i(dst_hi, 0, ctx);
> + } else if (__LINUX_ARM_ARCH__ < 6 &&
> + ctx->cpu_architecture < CPU_ARCH_ARMv5) {
> /* complete 8 byte move */
> + emit_a32_mov_r(dst_lo, src_lo, ctx);
> emit_a32_mov_r(dst_hi, src_hi, ctx);
Tiny nit: Looks like you compare for >= ARMv5TE above and <ARMv5 here.
I'm not aware of any vanilla v5 implementations (all I can find are
v5TE or <=v4T), so it doesn't seem like something actually causing
problems. Mostly pointing it out for consistency's sake.
-Olof
^ permalink raw reply
* Re: [PATCH net-next] net: sched: refactor flower walk to iterate over idr
From: Vlad Buslov @ 2018-07-10 17:02 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev, davem, jhs, xiyou.wangcong, jiri
In-Reply-To: <20180710135543.pedl7dzhi7tmdpew@netronome.com>
On Tue 10 Jul 2018 at 13:55, Simon Horman <simon.horman@netronome.com> wrote:
> On Mon, Jul 09, 2018 at 01:29:11PM +0300, Vlad Buslov wrote:
>> Extend struct tcf_walker with additional 'cookie' field. It is intended to
>> be used by classifier walk implementations to continue iteration directly
>> from particular filter, instead of iterating 'skip' number of times.
>>
>> Change flower walk implementation to save filter handle in 'cookie'. Each
>> time flower walk is called, it looks up filter with saved handle directly
>> with idr, instead of iterating over filter linked list 'skip' number of
>> times. This change improves complexity of dumping flower classifier from
>> quadratic to linearithmic. (assuming idr lookup has logarithmic complexity)
>>
>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
>
> Reported-by: Simon Horman <simon.horman@netronome.com>
>
> Thanks, I'm very pleased to see this change. I would appreciate it if
> we could have a little time to test its impact on performance
> thoroughly.
For me it reduced time needed to dump 5m flows to ~50 seconds. Not a
very thorough benchmark, but performance improvement was so dramatic
that I decided to not investigate further.
>
> One question: will this work as expected (i.e. be at least backwards
> compatible) with existing user-space code?
I considered that and didn't find any reason why it would break
compatibility. Basically with current flower flows are dumped in
arbitrary order, and with this change dump(accidentally) outputs flows
in sorted ascending order. User space shouldn't expect any ordering in
dumped flows anyway, right?
^ permalink raw reply
* Re: [RFC] Add BPF_SYNCHRONIZE bpf(2) command
From: Joel Fernandes @ 2018-07-10 16:57 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Joel Fernandes, Mathieu Desnoyers, Alexei Starovoitov,
Daniel Colascione, Alexei Starovoitov, linux-kernel, Tim Murray,
Daniel Borkmann, netdev, fengc
In-Reply-To: <20180710164212.GY3593@linux.vnet.ibm.com>
On Tue, Jul 10, 2018 at 09:42:12AM -0700, Paul E. McKenney wrote:
> On Mon, Jul 09, 2018 at 10:13:47PM -0700, Joel Fernandes wrote:
> > On Sun, Jul 08, 2018 at 04:54:38PM -0400, Mathieu Desnoyers wrote:
> > > ----- On Jul 7, 2018, at 4:33 PM, Joel Fernandes joelaf@google.com wrote:
> > >
> > > > On Fri, Jul 06, 2018 at 07:54:28PM -0700, Alexei Starovoitov wrote:
> > > >> On Fri, Jul 06, 2018 at 06:56:16PM -0700, Daniel Colascione wrote:
> > > >> > BPF_SYNCHRONIZE waits for any BPF programs active at the time of
> > > >> > BPF_SYNCHRONIZE to complete, allowing userspace to ensure atomicity of
> > > >> > RCU data structure operations with respect to active programs. For
> > > >> > example, userspace can update a map->map entry to point to a new map,
> > > >> > use BPF_SYNCHRONIZE to wait for any BPF programs using the old map to
> > > >> > complete, and then drain the old map without fear that BPF programs
> > > >> > may still be updating it.
> > > >> >
> > > >> > Signed-off-by: Daniel Colascione <dancol@google.com>
> > > >> > ---
> > > >> > include/uapi/linux/bpf.h | 1 +
> > > >> > kernel/bpf/syscall.c | 14 ++++++++++++++
> > > >> > 2 files changed, 15 insertions(+)
> > > >> >
> > > >> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > >> > index b7db3261c62d..4365c50e8055 100644
> > > >> > --- a/include/uapi/linux/bpf.h
> > > >> > +++ b/include/uapi/linux/bpf.h
> > > >> > @@ -98,6 +98,7 @@ enum bpf_cmd {
> > > >> > BPF_BTF_LOAD,
> > > >> > BPF_BTF_GET_FD_BY_ID,
> > > >> > BPF_TASK_FD_QUERY,
> > > >> > + BPF_SYNCHRONIZE,
> > > >> > };
> > > >> >
> > > >> > enum bpf_map_type {
> > > >> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > > >> > index d10ecd78105f..60ec7811846e 100644
> > > >> > --- a/kernel/bpf/syscall.c
> > > >> > +++ b/kernel/bpf/syscall.c
> > > >> > @@ -2272,6 +2272,20 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *,
> > > >> > uattr, unsigned int, siz
> > > >> > if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
> > > >> > return -EPERM;
> > > >> >
> > > >> > + if (cmd == BPF_SYNCHRONIZE) {
> > > >> > + if (uattr != NULL || size != 0)
> > > >> > + return -EINVAL;
> > > >> > + err = security_bpf(cmd, NULL, 0);
> > > >> > + if (err < 0)
> > > >> > + return err;
> > > >> > + /* BPF programs are run with preempt disabled, so
> > > >> > + * synchronize_sched is sufficient even with
> > > >> > + * RCU_PREEMPT.
> > > >> > + */
> > > >> > + synchronize_sched();
> > > >> > + return 0;
> > > >>
> > > >> I don't think it's necessary. sys_membarrier() can do this already
> > > >> and some folks use it exactly for this use case.
> > > >
> > > > Alexei, the use of sys_membarrier for this purpose seems kind of weird to me
> > > > though. No where does the manpage say membarrier should be implemented this
> > > > way so what happens if the implementation changes?
> > > >
> > > > Further, membarrier manpage says that a memory barrier should be matched with
> > > > a matching barrier. In this use case there is no matching barrier, so it
> > > > makes it weirder.
> > > >
> > > > Lastly, sys_membarrier seems will not work on nohz-full systems, so its a bit
> > > > fragile to depend on it for this?
> > > >
> > > > case MEMBARRIER_CMD_GLOBAL:
> > > > /* MEMBARRIER_CMD_GLOBAL is not compatible with nohz_full. */
> > > > if (tick_nohz_full_enabled())
> > > > return -EINVAL;
> > > > if (num_online_cpus() > 1)
> > > > synchronize_sched();
> > > > return 0;
> > > >
> > > >
> > > > Adding Mathieu as well who I believe is author/maintainer of membarrier.
> > >
> > > See commit 907565337
> > > "Fix: Disable sys_membarrier when nohz_full is enabled"
> > >
> > > "Userspace applications should be allowed to expect the membarrier system
> > > call with MEMBARRIER_CMD_SHARED command to issue memory barriers on
> > > nohz_full CPUs, but synchronize_sched() does not take those into
> > > account."
> > >
> > > So AFAIU you'd want to re-use membarrier to issue synchronize_sched, and you
> > > only care about kernel preempt off critical sections.
> >
> > Mathieu, Thanks a lot for your reply. I understand what you said and agree
> > with you. Slight OT, but I tried to go back to first principles and
> > understand how membarrier() uses synchronize_sched() for the "slow path" and
> > it didn't make immediate sense to me. Let me clarify my dillema..
> >
> > My understanding is membarrier's MEMBARRIER_CMD_GLOBAL will employ
> > synchronize_sched to make sure all other CPUs aren't executing anymore in an
> > section of usercode that happen to be accessing memory that was written to
> > before the membarrier call was made. To do this, the system call will use
> > synchronize_sched to try to guarantee that all user-mode execution that
> > started before the membarrier call would be completed when the membarrier
> > call returns. This guarantees that without using a real memory barrier on the
> > "fast path", things work just fine and everyone wins.
> >
> > But, going through RCU code, I see that a "RCU-sched quiecent state" on a CPU
> > may be reached when the CPU receives a timer tick while executing in user
> > mode:
> >
> > void rcu_check_callbacks(int user)
> > {
> > trace_rcu_utilization(TPS("Start scheduler-tick"));
> > increment_cpu_stall_ticks();
> > if (user || rcu_is_cpu_rrupt_from_idle()) {
> > [...]
> > rcu_sched_qs();
> > rcu_bh_qs();
> >
> > The problem I see is the CPU could be executing usermode code at the time of
> > the RCU sched-QS. This IMO is enough reason for synchronize_sched() to
> > return, because the CPU in question just reported a QS (assuming all other
> > CPUs also happen to do so if they needed to).
>
> This scenario will have inserted the needed smp_mb() into the userspace
> instruction execution stream, as is required by the sys_membarrier
> use cases.
Oh ok, that makes sense!
> > Then I am wondering how does the membarrier call even work, the tick could
> > very well have interrupted the CPU while it was executing usermode code in
> > the middle of a set of instructions performing memory accesses. Reporting a
> > quiescent state at such an inopportune time would cause the membarrier call
> > to prematurely return, no? Sorry if I missed something.
>
> One way to think of sys_membarrier() is as something that promotes a
> barrier() to an smp_mb(). This barrier then separates the target CPU's
> accesses that the caller saw before the sys_membarrier() from that same
> CPU's accesses that the caller will see after the sys_membarrier().
Got it!
> > The other question I have is about the whole "nohz-full doesn't work" thing.
> > I didn't fully understand why. RCU is already tracking the state of nohz-full
> > CPUs because the rcu dynticks code in (kernel/rcu/tree.c) monitors
> > transitions to and from usermode even if the timer tick is turned off. So why
> > would it not work?
>
> In the nohz_full case, there is no need for sys_membarrier()'s call to
> synchronize_sched() to interact directly with the nohz_full CPU. It
> can instead look at the target CPU's dyntick-idle state, and that state
> would potentially have been set in the dim distant past, thus having
> no effect on the target CPU's current execution.
In nohz-idle case though, there's nothing to promote the barrier() to
smp_mb() if you were to purely look at the dynticks-idle state on the
nohz-full CPU executing in user mode?
So then it makes sense to me now that nohz-full needs something to IPI that
CPU inorder to enforce the needed memory barrier and pure synchronize_sched()
wouldn't work. So then makes me think the expedited versions of
synchronize_sched should be able to do the job but I could off on a different
track..
Thanks a lot,
-Joel
^ permalink raw reply
* Re: [PATCH net] ipv4: reset fnhe_mtu_locked after cache route flushed
From: David Miller @ 2018-07-10 16:49 UTC (permalink / raw)
To: sd; +Cc: liuhangbin, netdev, sbrivio
In-Reply-To: <20180710140042.GA1963@bistromath.localdomain>
From: Sabrina Dubroca <sd@queasysnail.net>
Date: Tue, 10 Jul 2018 16:00:42 +0200
> Would there be a way to detect this when you apply patches? "there's
> no tag above the first '^---', but there are some tags below it",
> something like that?
Ugh, yeah ping's output format keeps biting us like this, sorry.
^ permalink raw reply
* Re: [RFC] Add BPF_SYNCHRONIZE bpf(2) command
From: Paul E. McKenney @ 2018-07-10 16:42 UTC (permalink / raw)
To: Joel Fernandes
Cc: Mathieu Desnoyers, Joel Fernandes, Alexei Starovoitov,
Daniel Colascione, Alexei Starovoitov, linux-kernel, Tim Murray,
Daniel Borkmann, netdev, fengc
In-Reply-To: <20180710051347.GA180724@joelaf.mtv.corp.google.com>
On Mon, Jul 09, 2018 at 10:13:47PM -0700, Joel Fernandes wrote:
> On Sun, Jul 08, 2018 at 04:54:38PM -0400, Mathieu Desnoyers wrote:
> > ----- On Jul 7, 2018, at 4:33 PM, Joel Fernandes joelaf@google.com wrote:
> >
> > > On Fri, Jul 06, 2018 at 07:54:28PM -0700, Alexei Starovoitov wrote:
> > >> On Fri, Jul 06, 2018 at 06:56:16PM -0700, Daniel Colascione wrote:
> > >> > BPF_SYNCHRONIZE waits for any BPF programs active at the time of
> > >> > BPF_SYNCHRONIZE to complete, allowing userspace to ensure atomicity of
> > >> > RCU data structure operations with respect to active programs. For
> > >> > example, userspace can update a map->map entry to point to a new map,
> > >> > use BPF_SYNCHRONIZE to wait for any BPF programs using the old map to
> > >> > complete, and then drain the old map without fear that BPF programs
> > >> > may still be updating it.
> > >> >
> > >> > Signed-off-by: Daniel Colascione <dancol@google.com>
> > >> > ---
> > >> > include/uapi/linux/bpf.h | 1 +
> > >> > kernel/bpf/syscall.c | 14 ++++++++++++++
> > >> > 2 files changed, 15 insertions(+)
> > >> >
> > >> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > >> > index b7db3261c62d..4365c50e8055 100644
> > >> > --- a/include/uapi/linux/bpf.h
> > >> > +++ b/include/uapi/linux/bpf.h
> > >> > @@ -98,6 +98,7 @@ enum bpf_cmd {
> > >> > BPF_BTF_LOAD,
> > >> > BPF_BTF_GET_FD_BY_ID,
> > >> > BPF_TASK_FD_QUERY,
> > >> > + BPF_SYNCHRONIZE,
> > >> > };
> > >> >
> > >> > enum bpf_map_type {
> > >> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > >> > index d10ecd78105f..60ec7811846e 100644
> > >> > --- a/kernel/bpf/syscall.c
> > >> > +++ b/kernel/bpf/syscall.c
> > >> > @@ -2272,6 +2272,20 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *,
> > >> > uattr, unsigned int, siz
> > >> > if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
> > >> > return -EPERM;
> > >> >
> > >> > + if (cmd == BPF_SYNCHRONIZE) {
> > >> > + if (uattr != NULL || size != 0)
> > >> > + return -EINVAL;
> > >> > + err = security_bpf(cmd, NULL, 0);
> > >> > + if (err < 0)
> > >> > + return err;
> > >> > + /* BPF programs are run with preempt disabled, so
> > >> > + * synchronize_sched is sufficient even with
> > >> > + * RCU_PREEMPT.
> > >> > + */
> > >> > + synchronize_sched();
> > >> > + return 0;
> > >>
> > >> I don't think it's necessary. sys_membarrier() can do this already
> > >> and some folks use it exactly for this use case.
> > >
> > > Alexei, the use of sys_membarrier for this purpose seems kind of weird to me
> > > though. No where does the manpage say membarrier should be implemented this
> > > way so what happens if the implementation changes?
> > >
> > > Further, membarrier manpage says that a memory barrier should be matched with
> > > a matching barrier. In this use case there is no matching barrier, so it
> > > makes it weirder.
> > >
> > > Lastly, sys_membarrier seems will not work on nohz-full systems, so its a bit
> > > fragile to depend on it for this?
> > >
> > > case MEMBARRIER_CMD_GLOBAL:
> > > /* MEMBARRIER_CMD_GLOBAL is not compatible with nohz_full. */
> > > if (tick_nohz_full_enabled())
> > > return -EINVAL;
> > > if (num_online_cpus() > 1)
> > > synchronize_sched();
> > > return 0;
> > >
> > >
> > > Adding Mathieu as well who I believe is author/maintainer of membarrier.
> >
> > See commit 907565337
> > "Fix: Disable sys_membarrier when nohz_full is enabled"
> >
> > "Userspace applications should be allowed to expect the membarrier system
> > call with MEMBARRIER_CMD_SHARED command to issue memory barriers on
> > nohz_full CPUs, but synchronize_sched() does not take those into
> > account."
> >
> > So AFAIU you'd want to re-use membarrier to issue synchronize_sched, and you
> > only care about kernel preempt off critical sections.
>
> Mathieu, Thanks a lot for your reply. I understand what you said and agree
> with you. Slight OT, but I tried to go back to first principles and
> understand how membarrier() uses synchronize_sched() for the "slow path" and
> it didn't make immediate sense to me. Let me clarify my dillema..
>
> My understanding is membarrier's MEMBARRIER_CMD_GLOBAL will employ
> synchronize_sched to make sure all other CPUs aren't executing anymore in an
> section of usercode that happen to be accessing memory that was written to
> before the membarrier call was made. To do this, the system call will use
> synchronize_sched to try to guarantee that all user-mode execution that
> started before the membarrier call would be completed when the membarrier
> call returns. This guarantees that without using a real memory barrier on the
> "fast path", things work just fine and everyone wins.
>
> But, going through RCU code, I see that a "RCU-sched quiecent state" on a CPU
> may be reached when the CPU receives a timer tick while executing in user
> mode:
>
> void rcu_check_callbacks(int user)
> {
> trace_rcu_utilization(TPS("Start scheduler-tick"));
> increment_cpu_stall_ticks();
> if (user || rcu_is_cpu_rrupt_from_idle()) {
> [...]
> rcu_sched_qs();
> rcu_bh_qs();
>
> The problem I see is the CPU could be executing usermode code at the time of
> the RCU sched-QS. This IMO is enough reason for synchronize_sched() to
> return, because the CPU in question just reported a QS (assuming all other
> CPUs also happen to do so if they needed to).
This scenario will have inserted the needed smp_mb() into the userspace
instruction execution stream, as is required by the sys_membarrier
use cases.
> Then I am wondering how does the membarrier call even work, the tick could
> very well have interrupted the CPU while it was executing usermode code in
> the middle of a set of instructions performing memory accesses. Reporting a
> quiescent state at such an inopportune time would cause the membarrier call
> to prematurely return, no? Sorry if I missed something.
One way to think of sys_membarrier() is as something that promotes a
barrier() to an smp_mb(). This barrier then separates the target CPU's
accesses that the caller saw before the sys_membarrier() from that same
CPU's accesses that the caller will see after the sys_membarrier().
> The other question I have is about the whole "nohz-full doesn't work" thing.
> I didn't fully understand why. RCU is already tracking the state of nohz-full
> CPUs because the rcu dynticks code in (kernel/rcu/tree.c) monitors
> transitions to and from usermode even if the timer tick is turned off. So why
> would it not work?
In the nohz_full case, there is no need for sys_membarrier()'s call to
synchronize_sched() to interact directly with the nohz_full CPU. It
can instead look at the target CPU's dyntick-idle state, and that state
would potentially have been set in the dim distant past, thus having
no effect on the target CPU's current execution.
Thanx, Paul
^ permalink raw reply
* RE: [PATCH bpf 1/1] bpf: btf: Fix bitfield extraction for big endian
From: David Laight @ 2018-07-10 16:35 UTC (permalink / raw)
To: 'Martin KaFai Lau', Okash Khawaja
Cc: Daniel Borkmann, Alexei Starovoitov, Yonghong Song,
Jakub Kicinski, David S. Miller, netdev@vger.kernel.org,
kernel-team@fb.com, linux-kernel@vger.kernel.org
In-Reply-To: <20180709183236.r4b7gzmev5h4lcbw@kafai-mbp.dhcp.thefacebook.com>
From: Martin KaFai Lau
> Sent: 09 July 2018 19:33
> On Sun, Jul 08, 2018 at 05:22:03PM -0700, Okash Khawaja wrote:
> > When extracting bitfield from a number, btf_int_bits_seq_show() builds
> > a mask and accesses least significant byte of the number in a way
> > specific to little-endian. This patch fixes that by checking endianness
> > of the machine and then shifting left and right the unneeded bits.
> >
> > Thanks to Martin Lau for the help in navigating potential pitfalls when
> > dealing with endianess and for the final solution.
> >
> > Fixes: b00b8daec828 ("bpf: btf: Add pretty print capability for data with BTF type info")
> > Signed-off-by: Okash Khawaja <osk@fb.com>
> >
> > ---
> > kernel/bpf/btf.c | 32 +++++++++++++++-----------------
> > 1 file changed, 15 insertions(+), 17 deletions(-)
> >
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -162,6 +162,8 @@
> > #define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
> > #define BITS_ROUNDUP_BYTES(bits) \
> > (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
> > +const int one = 1;
> > +#define is_big_endian() ((*(char *)&one) == 0)
> >
> > #define BTF_INFO_MASK 0x0f00ffff
> > #define BTF_INT_MASK 0x0fffffff
> > @@ -991,16 +993,13 @@ static void btf_int_bits_seq_show(const
> > void *data, u8 bits_offset,
> > struct seq_file *m)
> > {
> > + u8 left_shift_bits, right_shift_bits;
> Nit.
> Although only max 64 bit int is allowed now (ensured by btf_int_check_meta),
> it is better to use u16 such that it will be consistent to BTF_INT_BITS.
Double-nit.
Use 'int' or 'unsigned int'.
Sub-word arithmetic will require extra instructions on almost everything
except x86.
David
^ 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