* [PATCH net-next 0/2] net: phy: Eliminate unnecessary soft
From: Florian Fainelli @ 2018-09-25 18:28 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, Andrew Lunn, David S. Miller, open list,
dongsheng.wang, cphealy, clemens.gruber, hkallweit1, nbd,
harini.katakam
Hi all,
This patch series eliminates unnecessary software resets of the PHY.
This should hopefully not break anybody's hardware; but I would
appreciate testing to make sure this is is the case.
Sorry for this long email list, I wanted to make sure I reached out to
all people who made changes to the Marvell PHY driver.
Thank you!
Changes since RFT:
- added Tested-by tags from Wang, Dongsheng, Andrew, Chris and Clemens
Florian Fainelli (2):
net: phy: Stop with excessive soft reset
net: phy: marvell: Avoid unnecessary soft reset
drivers/net/phy/marvell.c | 63 ++++++++++++------------------------
drivers/net/phy/phy_device.c | 2 --
2 files changed, 21 insertions(+), 44 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH net-next 1/2] net: phy: Stop with excessive soft reset
From: Florian Fainelli @ 2018-09-25 18:28 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, Andrew Lunn, David S. Miller, open list,
dongsheng.wang, cphealy, clemens.gruber, hkallweit1, nbd,
harini.katakam
In-Reply-To: <20180925182846.30042-1-f.fainelli@gmail.com>
While consolidating the PHY reset in phy_init_hw() an unconditionaly
BMCR soft-reset I became quite trigger happy with those. This was later
on deactivated for the Generic PHY driver on the premise that a prior
software entity (e.g: bootloader) might have applied workarounds in
commit 0878fff1f42c ("net: phy: Do not perform software reset for
Generic PHY").
Since we have a hook to wire-up a soft_reset callback, just use that and
get rid of the call to genphy_soft_reset() entirely. This speeds up
initialization and link establishment for most PHYs out there that do
not require a reset.
Fixes: 87aa9f9c61ad ("net: phy: consolidate PHY reset in phy_init_hw()")
Tested-by: Wang, Dongsheng <dongsheng.wang@hxt-semitech.com>
Tested-by: Chris Healy <cphealy@gmail.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/phy_device.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index af64a9320fb0..ee676d75fe02 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -880,8 +880,6 @@ int phy_init_hw(struct phy_device *phydev)
if (phydev->drv->soft_reset)
ret = phydev->drv->soft_reset(phydev);
- else
- ret = genphy_soft_reset(phydev);
if (ret < 0)
return ret;
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 2/2] net: phy: marvell: Avoid unnecessary soft reset
From: Florian Fainelli @ 2018-09-25 18:28 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, Andrew Lunn, David S. Miller, open list,
dongsheng.wang, cphealy, clemens.gruber, hkallweit1, nbd,
harini.katakam
In-Reply-To: <20180925182846.30042-1-f.fainelli@gmail.com>
The BMCR.RESET bit on the Marvell PHYs has a special meaning in that
it commits the register writes into the HW for it to latch and be
configured appropriately. Doing software resets causes link drops, and
this is unnecessary disruption if nothing changed.
Determine from marvell_set_polarity()'s return code whether the register value
was changed and if it was, propagate that to the logic that hits the software
reset bit.
This avoids doing unnecessary soft reset if the PHY is configured in
the same state it was previously, this also eliminates the need for a
m88e1111_config_aneg() function since it now is the same as
marvell_config_aneg().
Tested-by: Wang, Dongsheng <dongsheng.wang@hxt-semitech.com>
Tested-by: Chris Healy <cphealy@gmail.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Clemens Gruber <clemens.gruber@pqgruber.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
drivers/net/phy/marvell.c | 63 +++++++++++++--------------------------
1 file changed, 21 insertions(+), 42 deletions(-)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f7c69ca34056..b55a7376bfdc 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -265,7 +265,7 @@ static int marvell_set_polarity(struct phy_device *phydev, int polarity)
return err;
}
- return 0;
+ return val != reg;
}
static int marvell_set_downshift(struct phy_device *phydev, bool enable,
@@ -287,12 +287,15 @@ static int marvell_set_downshift(struct phy_device *phydev, bool enable,
static int marvell_config_aneg(struct phy_device *phydev)
{
+ int changed = 0;
int err;
err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
if (err < 0)
return err;
+ changed = err;
+
err = phy_write(phydev, MII_M1111_PHY_LED_CONTROL,
MII_M1111_PHY_LED_DIRECT);
if (err < 0)
@@ -302,7 +305,7 @@ static int marvell_config_aneg(struct phy_device *phydev)
if (err < 0)
return err;
- if (phydev->autoneg != AUTONEG_ENABLE) {
+ if (phydev->autoneg != AUTONEG_ENABLE || changed) {
/* A write to speed/duplex bits (that is performed by
* genphy_config_aneg() call above) must be followed by
* a software reset. Otherwise, the write has no effect.
@@ -350,42 +353,6 @@ static int m88e1101_config_aneg(struct phy_device *phydev)
return marvell_config_aneg(phydev);
}
-static int m88e1111_config_aneg(struct phy_device *phydev)
-{
- int err;
-
- /* The Marvell PHY has an errata which requires
- * that certain registers get written in order
- * to restart autonegotiation
- */
- err = genphy_soft_reset(phydev);
-
- err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
- if (err < 0)
- return err;
-
- err = phy_write(phydev, MII_M1111_PHY_LED_CONTROL,
- MII_M1111_PHY_LED_DIRECT);
- if (err < 0)
- return err;
-
- err = genphy_config_aneg(phydev);
- if (err < 0)
- return err;
-
- if (phydev->autoneg != AUTONEG_ENABLE) {
- /* A write to speed/duplex bits (that is performed by
- * genphy_config_aneg() call above) must be followed by
- * a software reset. Otherwise, the write has no effect.
- */
- err = genphy_soft_reset(phydev);
- if (err < 0)
- return err;
- }
-
- return 0;
-}
-
#ifdef CONFIG_OF_MDIO
/* Set and/or override some configuration registers based on the
* marvell,reg-init property stored in the of_node for the phydev.
@@ -479,6 +446,7 @@ static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
static int m88e1121_config_aneg(struct phy_device *phydev)
{
+ int changed = 0;
int err = 0;
if (phy_interface_is_rgmii(phydev)) {
@@ -487,15 +455,26 @@ static int m88e1121_config_aneg(struct phy_device *phydev)
return err;
}
- err = genphy_soft_reset(phydev);
+ err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
if (err < 0)
return err;
- err = marvell_set_polarity(phydev, phydev->mdix_ctrl);
+ changed = err;
+
+ err = genphy_config_aneg(phydev);
if (err < 0)
return err;
- return genphy_config_aneg(phydev);
+ if (phydev->autoneg != autoneg || changed) {
+ /* A software reset is used to ensure a "commit" of the
+ * changes is done.
+ */
+ err = genphy_soft_reset(phydev);
+ if (err < 0)
+ return err;
+ }
+
+ return 0;
}
static int m88e1318_config_aneg(struct phy_device *phydev)
@@ -2067,7 +2046,7 @@ static struct phy_driver marvell_drivers[] = {
.flags = PHY_HAS_INTERRUPT,
.probe = marvell_probe,
.config_init = &m88e1111_config_init,
- .config_aneg = &m88e1111_config_aneg,
+ .config_aneg = &marvell_config_aneg,
.read_status = &marvell_read_status,
.ack_interrupt = &marvell_ack_interrupt,
.config_intr = &marvell_config_intr,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next v6 02/23] zinc: introduce minimal cryptography library
From: Joe Perches @ 2018-09-25 18:33 UTC (permalink / raw)
To: Jason A. Donenfeld, linux-kernel, netdev, linux-crypto, davem,
gregkh
Cc: Samuel Neves, Andy Lutomirski, Jean-Philippe Aumasson
In-Reply-To: <20180925145622.29959-3-Jason@zx2c4.com>
On Tue, 2018-09-25 at 16:56 +0200, Jason A. Donenfeld wrote:
> Zinc stands for "Zinc Is Neat Crypto" or "Zinc as IN Crypto" or maybe
> just "Zx2c4's INsane Cryptolib." It's also short, easy to type, and
> plays nicely with the recent trend of naming crypto libraries after
> elements. The guiding principle is "don't overdo it". It's less of a
> library and more of a directory tree for organizing well-curated direct
> implementations of cryptography primitives.
[]
> diff --git a/lib/zinc/Makefile b/lib/zinc/Makefile
> new file mode 100644
> index 000000000000..a61c80d676cb
> --- /dev/null
> +++ b/lib/zinc/Makefile
> @@ -0,0 +1,3 @@
> +ccflags-y := -O2
> +ccflags-y += -D'pr_fmt(fmt)="zinc: " fmt'
> +ccflags-$(CONFIG_ZINC_DEBUG) += -DDEBUG
I think the -Dpr_fmt is especially odd and not
really acceptable as it not used anywhere else
in the kernel.
^ permalink raw reply
* Re: [PATCH net 3/7] lan78xx: Check for supported Wake-on-LAN modes
From: Florian Fainelli @ 2018-09-25 18:35 UTC (permalink / raw)
To: Woojung.Huh, netdev
Cc: davem, UNGLinuxDriver, steve.glendinning, keescook, akurz,
hayeswang, kai.heng.feng, grundler, zhongjiang, bigeasy,
ran.wang_1, edumazet, linux-usb, linux-kernel
In-Reply-To: <BN6PR1101MB2130D18F03C79BC1E54C9FBAE7160@BN6PR1101MB2130.namprd11.prod.outlook.com>
On 09/25/2018 10:32 AM, Woojung.Huh@microchip.com wrote:
> Hi Florian,
>
>>>> + if (pdata->wol == 0)
>>>> + return -EINVAL;
>>>> +
>>> It will make function return when disabling WOL.
>>
>> Huh, yes, good point.
>>
>>> Is there other place handling this scenario?
>>
>> How do you mean?
>>
> I meant there is another path I might miss when disabling WOL
> than this xxx_set_wol().
I don't think so, at least not from the ethtool perspective, this should
fix the issue before, and simplifying the code, since all we are doing
it taking a bitmask, checking each bit we support, and again, make it
the same bitmask in pdata->wol, can you test that? If you have a new
enough version of ethtool, try using: ethtool -s <iface> wol f, which
was added recently and which this driver does not support:
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index a9991c5f4736..2e37028ef6ca 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1401,19 +1401,10 @@ static int lan78xx_set_wol(struct net_device
*netdev,
if (ret < 0)
return ret;
- pdata->wol = 0;
- if (wol->wolopts & WAKE_UCAST)
- pdata->wol |= WAKE_UCAST;
- if (wol->wolopts & WAKE_MCAST)
- pdata->wol |= WAKE_MCAST;
- if (wol->wolopts & WAKE_BCAST)
- pdata->wol |= WAKE_BCAST;
- if (wol->wolopts & WAKE_MAGIC)
- pdata->wol |= WAKE_MAGIC;
- if (wol->wolopts & WAKE_PHY)
- pdata->wol |= WAKE_PHY;
- if (wol->wolopts & WAKE_ARP)
- pdata->wol |= WAKE_ARP;
+ if (pdata->wol & ~WAKE_ALL)
+ return -EINVAL;
+
+ pdata->wol = wol->wolopts;
device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts);
--
Florian
^ permalink raw reply related
* Re: [PATCH net-next v6 17/23] zinc: Curve25519 generic C implementations and selftest
From: Joe Perches @ 2018-09-25 18:38 UTC (permalink / raw)
To: Jason A. Donenfeld, linux-kernel, netdev, linux-crypto, davem,
gregkh
Cc: Samuel Neves, Andy Lutomirski, Jean-Philippe Aumasson,
Karthikeyan Bhargavan
In-Reply-To: <20180925145622.29959-18-Jason@zx2c4.com>
On Tue, 2018-09-25 at 16:56 +0200, Jason A. Donenfeld wrote:
> This contains two formally verified C implementations of the Curve25519
> scalar multiplication function, one for 32-bit systems, and one for
> 64-bit systems whose compiler supports efficient 128-bit integer types.
> Not only are these implementations formally verified, but they are also
> the fastest available C implementations. They have been modified to be
> friendly to kernel space and to be generally less horrendous looking,
> but still an effort has been made to retain their formally verified
> characteristic, and so the C might look slightly unidiomatic.
[]
> diff --git a/lib/zinc/curve25519/curve25519-fiat32.h b/lib/zinc/curve25519/curve25519-fiat32.h
[]
> +static __always_inline void fe_freeze(u32 out[10], const u32 in1[10])
> +{
> + { const u32 x17 = in1[9];
> + { const u32 x18 = in1[8];
> + { const u32 x16 = in1[7];
> + { const u32 x14 = in1[6];
> + { const u32 x12 = in1[5];
> + { const u32 x10 = in1[4];
> + { const u32 x8 = in1[3];
> + { const u32 x6 = in1[2];
> + { const u32 x4 = in1[1];
> + { const u32 x2 = in1[0];
> + { u32 x20; u8/*bool*/ x21 = subborrow_u26(0x0, x2, 0x3ffffed, &x20);
> + { u32 x23; u8/*bool*/ x24 = subborrow_u25(x21, x4, 0x1ffffff, &x23);
> + { u32 x26; u8/*bool*/ x27 = subborrow_u26(x24, x6, 0x3ffffff, &x26);
> + { u32 x29; u8/*bool*/ x30 = subborrow_u25(x27, x8, 0x1ffffff, &x29);
> + { u32 x32; u8/*bool*/ x33 = subborrow_u26(x30, x10, 0x3ffffff, &x32);
> + { u32 x35; u8/*bool*/ x36 = subborrow_u25(x33, x12, 0x1ffffff, &x35);
> + { u32 x38; u8/*bool*/ x39 = subborrow_u26(x36, x14, 0x3ffffff, &x38);
> + { u32 x41; u8/*bool*/ x42 = subborrow_u25(x39, x16, 0x1ffffff, &x41);
> + { u32 x44; u8/*bool*/ x45 = subborrow_u26(x42, x18, 0x3ffffff, &x44);
> + { u32 x47; u8/*bool*/ x48 = subborrow_u25(x45, x17, 0x1ffffff, &x47);
> + { u32 x49 = cmovznz32(x48, 0x0, 0xffffffff);
> + { u32 x50 = (x49 & 0x3ffffed);
> + { u32 x52; u8/*bool*/ x53 = addcarryx_u26(0x0, x20, x50, &x52);
> + { u32 x54 = (x49 & 0x1ffffff);
> + { u32 x56; u8/*bool*/ x57 = addcarryx_u25(x53, x23, x54, &x56);
> + { u32 x58 = (x49 & 0x3ffffff);
> + { u32 x60; u8/*bool*/ x61 = addcarryx_u26(x57, x26, x58, &x60);
> + { u32 x62 = (x49 & 0x1ffffff);
> + { u32 x64; u8/*bool*/ x65 = addcarryx_u25(x61, x29, x62, &x64);
> + { u32 x66 = (x49 & 0x3ffffff);
> + { u32 x68; u8/*bool*/ x69 = addcarryx_u26(x65, x32, x66, &x68);
> + { u32 x70 = (x49 & 0x1ffffff);
> + { u32 x72; u8/*bool*/ x73 = addcarryx_u25(x69, x35, x70, &x72);
> + { u32 x74 = (x49 & 0x3ffffff);
> + { u32 x76; u8/*bool*/ x77 = addcarryx_u26(x73, x38, x74, &x76);
> + { u32 x78 = (x49 & 0x1ffffff);
> + { u32 x80; u8/*bool*/ x81 = addcarryx_u25(x77, x41, x78, &x80);
> + { u32 x82 = (x49 & 0x3ffffff);
> + { u32 x84; u8/*bool*/ x85 = addcarryx_u26(x81, x44, x82, &x84);
> + { u32 x86 = (x49 & 0x1ffffff);
> + { u32 x88; addcarryx_u25(x85, x47, x86, &x88);
> + out[0] = x52;
> + out[1] = x56;
> + out[2] = x60;
> + out[3] = x64;
> + out[4] = x68;
> + out[5] = x72;
> + out[6] = x76;
> + out[7] = x80;
> + out[8] = x84;
> + out[9] = x88;
> + }}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}
> +}
Unidiomatic might be a stretch here...
[]
and elsewhere...
> +static void fe_add_impl(u32 out[10], const u32 in1[10], const u32 in2[10])
> +{
> + { const u32 x20 = in1[9];
> + { const u32 x21 = in1[8];
> + { const u32 x19 = in1[7];
> + { const u32 x17 = in1[6];
> + { const u32 x15 = in1[5];
> + { const u32 x13 = in1[4];
> + { const u32 x11 = in1[3];
> + { const u32 x9 = in1[2];
> + { const u32 x7 = in1[1];
> + { const u32 x5 = in1[0];
> + { const u32 x38 = in2[9];
> + { const u32 x39 = in2[8];
> + { const u32 x37 = in2[7];
> + { const u32 x35 = in2[6];
> + { const u32 x33 = in2[5];
> + { const u32 x31 = in2[4];
> + { const u32 x29 = in2[3];
> + { const u32 x27 = in2[2];
> + { const u32 x25 = in2[1];
> + { const u32 x23 = in2[0];
> + out[0] = (x5 + x23);
> + out[1] = (x7 + x25);
> + out[2] = (x9 + x27);
> + out[3] = (x11 + x29);
> + out[4] = (x13 + x31);
> + out[5] = (x15 + x33);
> + out[6] = (x17 + x35);
> + out[7] = (x19 + x37);
> + out[8] = (x21 + x39);
> + out[9] = (x20 + x38);
> + }}}}}}}}}}}}}}}}}}}}
> +}
etc...
^ permalink raw reply
* [REBASE PATCH net-next v9 0/4] net: vhost: improve performance when enable busyloop
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This patches improve the guest receive performance.
On the handle_tx side, we poll the sock receive queue
at the same time. handle_rx do that in the same way.
For more performance report, see patch 4
Tonghao Zhang (4):
net: vhost: lock the vqs one by one
net: vhost: replace magic number of lock annotation
net: vhost: factor out busy polling logic to vhost_net_busy_poll()
net: vhost: add rx busy polling in tx path
drivers/vhost/net.c | 147 +++++++++++++++++++++++++++++---------------------
drivers/vhost/vhost.c | 24 +++------
2 files changed, 92 insertions(+), 79 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [REBASE PATCH net-next v9 1/4] net: vhost: lock the vqs one by one
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
In-Reply-To: <1537879012-20859-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This patch changes the way that lock all vqs
at the same, to lock them one by one. It will
be used for next patch to avoid the deadlock.
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/vhost.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b13c6b4..f52008b 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
{
int i;
- for (i = 0; i < d->nvqs; ++i)
+ for (i = 0; i < d->nvqs; ++i) {
+ mutex_lock(&d->vqs[i]->mutex);
__vhost_vq_meta_reset(d->vqs[i]);
+ mutex_unlock(&d->vqs[i]->mutex);
+ }
}
static void vhost_vq_reset(struct vhost_dev *dev,
@@ -891,20 +894,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
#define vhost_get_used(vq, x, ptr) \
vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
-static void vhost_dev_lock_vqs(struct vhost_dev *d)
-{
- int i = 0;
- for (i = 0; i < d->nvqs; ++i)
- mutex_lock_nested(&d->vqs[i]->mutex, i);
-}
-
-static void vhost_dev_unlock_vqs(struct vhost_dev *d)
-{
- int i = 0;
- for (i = 0; i < d->nvqs; ++i)
- mutex_unlock(&d->vqs[i]->mutex);
-}
-
static int vhost_new_umem_range(struct vhost_umem *umem,
u64 start, u64 size, u64 end,
u64 userspace_addr, int perm)
@@ -954,7 +943,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
if (msg->iova <= vq_msg->iova &&
msg->iova + msg->size - 1 >= vq_msg->iova &&
vq_msg->type == VHOST_IOTLB_MISS) {
+ mutex_lock(&node->vq->mutex);
vhost_poll_queue(&node->vq->poll);
+ mutex_unlock(&node->vq->mutex);
+
list_del(&node->node);
kfree(node);
}
@@ -986,7 +978,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
int ret = 0;
mutex_lock(&dev->mutex);
- vhost_dev_lock_vqs(dev);
switch (msg->type) {
case VHOST_IOTLB_UPDATE:
if (!dev->iotlb) {
@@ -1020,7 +1011,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
break;
}
- vhost_dev_unlock_vqs(dev);
mutex_unlock(&dev->mutex);
return ret;
--
1.8.3.1
^ permalink raw reply related
* [REBASE PATCH net-next v9 2/4] net: vhost: replace magic number of lock annotation
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
In-Reply-To: <1537879012-20859-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Use the VHOST_NET_VQ_XXX as a subclass for mutex_lock_nested.
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 1bff6bc..5fe57ab 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -856,7 +856,7 @@ static void handle_tx(struct vhost_net *net)
struct vhost_virtqueue *vq = &nvq->vq;
struct socket *sock;
- mutex_lock(&vq->mutex);
+ mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_TX);
sock = vq->private_data;
if (!sock)
goto out;
@@ -921,7 +921,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
/* Flush batched heads first */
vhost_net_signal_used(rnvq);
/* Both tx vq and rx socket were polled here */
- mutex_lock_nested(&tvq->mutex, 1);
+ mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
vhost_disable_notify(&net->dev, tvq);
preempt_disable();
@@ -1063,7 +1063,7 @@ static void handle_rx(struct vhost_net *net)
__virtio16 num_buffers;
int recv_pkts = 0;
- mutex_lock_nested(&vq->mutex, 0);
+ mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
sock = vq->private_data;
if (!sock)
goto out;
--
1.8.3.1
^ permalink raw reply related
* [REBASE PATCH net-next v9 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
In-Reply-To: <1537879012-20859-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Factor out generic busy polling logic and will be
used for in tx path in the next patch. And with the patch,
qemu can set differently the busyloop_timeout for rx queue.
To avoid duplicate codes, introduce the helper functions:
* sock_has_rx_data(changed from sk_has_rx_data)
* vhost_net_busy_poll_try_queue
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
drivers/vhost/net.c | 110 +++++++++++++++++++++++++++++++++-------------------
1 file changed, 70 insertions(+), 40 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 5fe57ab..ac0b954 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -480,6 +480,74 @@ static void vhost_tx_batch(struct vhost_net *net,
nvq->batched_xdp = 0;
}
+static int sock_has_rx_data(struct socket *sock)
+{
+ if (unlikely(!sock))
+ return 0;
+
+ if (sock->ops->peek_len)
+ return sock->ops->peek_len(sock);
+
+ return skb_queue_empty(&sock->sk->sk_receive_queue);
+}
+
+static void vhost_net_busy_poll_try_queue(struct vhost_net *net,
+ struct vhost_virtqueue *vq)
+{
+ if (!vhost_vq_avail_empty(&net->dev, vq)) {
+ vhost_poll_queue(&vq->poll);
+ } else if (unlikely(vhost_enable_notify(&net->dev, vq))) {
+ vhost_disable_notify(&net->dev, vq);
+ vhost_poll_queue(&vq->poll);
+ }
+}
+
+static void vhost_net_busy_poll(struct vhost_net *net,
+ struct vhost_virtqueue *rvq,
+ struct vhost_virtqueue *tvq,
+ bool *busyloop_intr,
+ bool poll_rx)
+{
+ unsigned long busyloop_timeout;
+ unsigned long endtime;
+ struct socket *sock;
+ struct vhost_virtqueue *vq = poll_rx ? tvq : rvq;
+
+ mutex_lock_nested(&vq->mutex, poll_rx ? VHOST_NET_VQ_TX: VHOST_NET_VQ_RX);
+ vhost_disable_notify(&net->dev, vq);
+ sock = rvq->private_data;
+
+ busyloop_timeout = poll_rx ? rvq->busyloop_timeout:
+ tvq->busyloop_timeout;
+
+ preempt_disable();
+ endtime = busy_clock() + busyloop_timeout;
+
+ while (vhost_can_busy_poll(endtime)) {
+ if (vhost_has_work(&net->dev)) {
+ *busyloop_intr = true;
+ break;
+ }
+
+ if ((sock_has_rx_data(sock) &&
+ !vhost_vq_avail_empty(&net->dev, rvq)) ||
+ !vhost_vq_avail_empty(&net->dev, tvq))
+ break;
+
+ cpu_relax();
+ }
+
+ preempt_enable();
+
+ if (poll_rx || sock_has_rx_data(sock))
+ vhost_net_busy_poll_try_queue(net, vq);
+ else if (!poll_rx) /* On tx here, sock has no rx data. */
+ vhost_enable_notify(&net->dev, rvq);
+
+ mutex_unlock(&vq->mutex);
+}
+
+
static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
struct vhost_net_virtqueue *nvq,
unsigned int *out_num, unsigned int *in_num,
@@ -897,16 +965,6 @@ static int peek_head_len(struct vhost_net_virtqueue *rvq, struct sock *sk)
return len;
}
-static int sk_has_rx_data(struct sock *sk)
-{
- struct socket *sock = sk->sk_socket;
-
- if (sock->ops->peek_len)
- return sock->ops->peek_len(sock);
-
- return skb_queue_empty(&sk->sk_receive_queue);
-}
-
static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
bool *busyloop_intr)
{
@@ -914,41 +972,13 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
struct vhost_net_virtqueue *tnvq = &net->vqs[VHOST_NET_VQ_TX];
struct vhost_virtqueue *rvq = &rnvq->vq;
struct vhost_virtqueue *tvq = &tnvq->vq;
- unsigned long uninitialized_var(endtime);
int len = peek_head_len(rnvq, sk);
- if (!len && tvq->busyloop_timeout) {
+ if (!len && rvq->busyloop_timeout) {
/* Flush batched heads first */
vhost_net_signal_used(rnvq);
/* Both tx vq and rx socket were polled here */
- mutex_lock_nested(&tvq->mutex, VHOST_NET_VQ_TX);
- vhost_disable_notify(&net->dev, tvq);
-
- preempt_disable();
- endtime = busy_clock() + tvq->busyloop_timeout;
-
- while (vhost_can_busy_poll(endtime)) {
- if (vhost_has_work(&net->dev)) {
- *busyloop_intr = true;
- break;
- }
- if ((sk_has_rx_data(sk) &&
- !vhost_vq_avail_empty(&net->dev, rvq)) ||
- !vhost_vq_avail_empty(&net->dev, tvq))
- break;
- cpu_relax();
- }
-
- preempt_enable();
-
- if (!vhost_vq_avail_empty(&net->dev, tvq)) {
- vhost_poll_queue(&tvq->poll);
- } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
- vhost_disable_notify(&net->dev, tvq);
- vhost_poll_queue(&tvq->poll);
- }
-
- mutex_unlock(&tvq->mutex);
+ vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, true);
len = peek_head_len(rnvq, sk);
}
--
1.8.3.1
^ permalink raw reply related
* [REBASE PATCH net-next v9 4/4] net: vhost: add rx busy polling in tx path
From: xiangxia.m.yue @ 2018-09-25 12:36 UTC (permalink / raw)
To: jasowang, mst, makita.toshiaki, davem
Cc: netdev, virtualization, Tonghao Zhang
In-Reply-To: <1537879012-20859-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This patch improves the guest receive performance.
On the handle_tx side, we poll the sock receive queue at the
same time. handle_rx do that in the same way.
We set the poll-us=100us and use the netperf to test throughput
and mean latency. When running the tests, the vhost-net kthread
of that VM, is alway 100% CPU. The commands are shown as below.
Rx performance is greatly improved by this patch. There is not
notable performance change on tx with this series though. This
patch is useful for bi-directional traffic.
netperf -H IP -t TCP_STREAM -l 20 -- -O "THROUGHPUT, THROUGHPUT_UNITS, MEAN_LATENCY"
Topology:
[Host] ->linux bridge -> tap vhost-net ->[Guest]
TCP_STREAM:
* Without the patch: 19842.95 Mbps, 6.50 us mean latency
* With the patch: 37598.20 Mbps, 3.43 us mean latency
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
drivers/vhost/net.c | 35 ++++++++++++++---------------------
1 file changed, 14 insertions(+), 21 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index ac0b954..015abf3 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -547,34 +547,27 @@ static void vhost_net_busy_poll(struct vhost_net *net,
mutex_unlock(&vq->mutex);
}
-
static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
- struct vhost_net_virtqueue *nvq,
+ struct vhost_net_virtqueue *tnvq,
unsigned int *out_num, unsigned int *in_num,
struct msghdr *msghdr, bool *busyloop_intr)
{
- struct vhost_virtqueue *vq = &nvq->vq;
- unsigned long uninitialized_var(endtime);
- int r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
+ struct vhost_net_virtqueue *rnvq = &net->vqs[VHOST_NET_VQ_RX];
+ struct vhost_virtqueue *rvq = &rnvq->vq;
+ struct vhost_virtqueue *tvq = &tnvq->vq;
+
+ int r = vhost_get_vq_desc(tvq, tvq->iov, ARRAY_SIZE(tvq->iov),
out_num, in_num, NULL, NULL);
- if (r == vq->num && vq->busyloop_timeout) {
+ if (r == tvq->num && tvq->busyloop_timeout) {
/* Flush batched packets first */
- if (!vhost_sock_zcopy(vq->private_data))
- vhost_tx_batch(net, nvq, vq->private_data, msghdr);
- preempt_disable();
- endtime = busy_clock() + vq->busyloop_timeout;
- while (vhost_can_busy_poll(endtime)) {
- if (vhost_has_work(vq->dev)) {
- *busyloop_intr = true;
- break;
- }
- if (!vhost_vq_avail_empty(vq->dev, vq))
- break;
- cpu_relax();
- }
- preempt_enable();
- r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
+ if (!vhost_sock_zcopy(tvq->private_data))
+ // vhost_net_signal_used(tnvq);
+ vhost_tx_batch(net, tnvq, tvq->private_data, msghdr);
+
+ vhost_net_busy_poll(net, rvq, tvq, busyloop_intr, false);
+
+ r = vhost_get_vq_desc(tvq, tvq->iov, ARRAY_SIZE(tvq->iov),
out_num, in_num, NULL, NULL);
}
--
1.8.3.1
^ permalink raw reply related
* requesting stable backport of BPF security fix (commit dd066823db2ac4e22f721ec85190817b58059a54)
From: Jann Horn @ 2018-09-25 12:46 UTC (permalink / raw)
To: Network Development, David S. Miller
Cc: Alexei Starovoitov, Daniel Borkmann, security
Hi!
Per the policy at Documentation/networking/netdev-FAQ.rst, I'm sending
this to netdev@ and davem, rather than stable@; with a CC to security@
because I believe that this is a security process issue.
Upstream commit dd066823db2ac4e22f721ec85190817b58059a54
("bpf/verifier: disallow pointer subtraction") fixes a security bug
(kernel pointer leak to unprivileged userspace). The fix has been in
Linus' tree since about a week ago, but the patch still doesn't appear
in Greg's linux-4.18.y linux-stable-rc repo, in Greg's 4.18
stable-queue, or in davem's stable queue at
http://patchwork.ozlabs.org/bundle/davem/stable/?state=* .
Please queue it up for backporting.
I am curious: Why was this not queued up for stable immediately? Or
was it, and I looked in the wrong place?
^ permalink raw reply
* Re: [PATCH v2 bpf-next 03/10] bpf: introduce per-cpu cgroup local storage
From: Song Liu @ 2018-09-25 18:54 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Kernel Team,
Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <20180925152114.13537-4-guro@fb.com>
> On Sep 25, 2018, at 8:21 AM, Roman Gushchin <guro@fb.com> wrote:
>
> This commit introduced per-cpu cgroup local storage.
>
> Per-cpu cgroup local storage is very similar to simple cgroup storage
> (let's call it shared), except all the data is per-cpu.
>
> The main goal of per-cpu variant is to implement super fast
> counters (e.g. packet counters), which don't require neither
> lookups, neither atomic operations.
>
> From userspace's point of view, accessing a per-cpu cgroup storage
> is similar to other per-cpu map types (e.g. per-cpu hashmaps and
> arrays).
>
> Writing to a per-cpu cgroup storage is not atomic, but is performed
> by copying longs, so some minimal atomicity is here, exactly
> as with other per-cpu maps.
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
> ---
> include/linux/bpf-cgroup.h | 20 ++++-
> include/linux/bpf.h | 1 +
> include/linux/bpf_types.h | 1 +
> include/uapi/linux/bpf.h | 1 +
> kernel/bpf/helpers.c | 8 +-
> kernel/bpf/local_storage.c | 148 ++++++++++++++++++++++++++++++++-----
> kernel/bpf/syscall.c | 11 ++-
> kernel/bpf/verifier.c | 15 +++-
> 8 files changed, 177 insertions(+), 28 deletions(-)
>
> diff --git a/include/linux/bpf-cgroup.h b/include/linux/bpf-cgroup.h
> index 7e0c9a1d48b7..9bd907657f9b 100644
> --- a/include/linux/bpf-cgroup.h
> +++ b/include/linux/bpf-cgroup.h
> @@ -37,7 +37,10 @@ struct bpf_storage_buffer {
> };
>
> struct bpf_cgroup_storage {
> - struct bpf_storage_buffer *buf;
> + union {
> + struct bpf_storage_buffer *buf;
> + char __percpu *percpu_buf;
"char *" here looks weird. Did you mean to use "void *"?
> + };
> struct bpf_cgroup_storage_map *map;
> struct bpf_cgroup_storage_key key;
> struct list_head list;
> @@ -109,6 +112,9 @@ int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
> static inline enum bpf_cgroup_storage_type cgroup_storage_type(
> struct bpf_map *map)
> {
> + if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> + return BPF_CGROUP_STORAGE_PERCPU;
> +
> return BPF_CGROUP_STORAGE_SHARED;
> }
>
> @@ -131,6 +137,10 @@ void bpf_cgroup_storage_unlink(struct bpf_cgroup_storage *storage);
> int bpf_cgroup_storage_assign(struct bpf_prog *prog, struct bpf_map *map);
> void bpf_cgroup_storage_release(struct bpf_prog *prog, struct bpf_map *map);
>
> +int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key, void *value);
> +int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
> + void *value, u64 flags);
> +
> /* Wrappers for __cgroup_bpf_run_filter_skb() guarded by cgroup_bpf_enabled. */
> #define BPF_CGROUP_RUN_PROG_INET_INGRESS(sk, skb) \
> ({ \
> @@ -285,6 +295,14 @@ static inline struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(
> struct bpf_prog *prog, enum bpf_cgroup_storage_type stype) { return 0; }
> static inline void bpf_cgroup_storage_free(
> struct bpf_cgroup_storage *storage) {}
> +static inline int bpf_percpu_cgroup_storage_copy(struct bpf_map *map, void *key,
> + void *value) {
> + return 0;
> +}
> +static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
> + void *key, void *value, u64 flags) {
> + return 0;
> +}
>
> #define cgroup_bpf_enabled (0)
> #define BPF_CGROUP_PRE_CONNECT_ENABLED(sk) (0)
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b457fbe7b70b..018299a595c8 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -274,6 +274,7 @@ struct bpf_prog_offload {
>
> enum bpf_cgroup_storage_type {
> BPF_CGROUP_STORAGE_SHARED,
> + BPF_CGROUP_STORAGE_PERCPU,
> __BPF_CGROUP_STORAGE_MAX
> };
>
> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> index c9bd6fb765b0..5432f4c9f50e 100644
> --- a/include/linux/bpf_types.h
> +++ b/include/linux/bpf_types.h
> @@ -43,6 +43,7 @@ BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_ARRAY, cgroup_array_map_ops)
> #endif
> #ifdef CONFIG_CGROUP_BPF
> BPF_MAP_TYPE(BPF_MAP_TYPE_CGROUP_STORAGE, cgroup_storage_map_ops)
> +BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE, cgroup_storage_map_ops)
> #endif
> BPF_MAP_TYPE(BPF_MAP_TYPE_HASH, htab_map_ops)
> BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_HASH, htab_percpu_map_ops)
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index aa5ccd2385ed..e2070d819e04 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -127,6 +127,7 @@ enum bpf_map_type {
> BPF_MAP_TYPE_SOCKHASH,
> BPF_MAP_TYPE_CGROUP_STORAGE,
> BPF_MAP_TYPE_REUSEPORT_SOCKARRAY,
> + BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE,
> };
>
> enum bpf_prog_type {
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index e42f8789b7ea..1f21ef1c4ad3 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -206,10 +206,16 @@ BPF_CALL_2(bpf_get_local_storage, struct bpf_map *, map, u64, flags)
> */
> enum bpf_cgroup_storage_type stype = cgroup_storage_type(map);
> struct bpf_cgroup_storage *storage;
> + void *ptr = NULL;
Not necessary to initialize to NULL.
>
> storage = this_cpu_read(bpf_cgroup_storage[stype]);
>
> - return (unsigned long)&READ_ONCE(storage->buf)->data[0];
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + ptr = &READ_ONCE(storage->buf)->data[0];
> + else
> + ptr = this_cpu_ptr(storage->percpu_buf);
> +
> + return (unsigned long)ptr;
> }
>
> const struct bpf_func_proto bpf_get_local_storage_proto = {
> diff --git a/kernel/bpf/local_storage.c b/kernel/bpf/local_storage.c
> index 6742292fb39e..d991355b5b46 100644
> --- a/kernel/bpf/local_storage.c
> +++ b/kernel/bpf/local_storage.c
> @@ -152,6 +152,71 @@ static int cgroup_storage_update_elem(struct bpf_map *map, void *_key,
> return 0;
> }
>
> +int bpf_percpu_cgroup_storage_copy(struct bpf_map *_map, void *_key,
> + void *value)
> +{
> + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> + struct bpf_cgroup_storage_key *key = _key;
> + struct bpf_cgroup_storage *storage;
> + int cpu, off = 0;
> + u32 size;
> +
> + rcu_read_lock();
> + storage = cgroup_storage_lookup(map, key, false);
> + if (!storage) {
> + rcu_read_unlock();
> + return -ENOENT;
> + }
> +
> + /* per_cpu areas are zero-filled and bpf programs can only
> + * access 'value_size' of them, so copying rounded areas
> + * will not leak any kernel data
> + */
> + size = round_up(_map->value_size, 8);
> + for_each_possible_cpu(cpu) {
> + bpf_long_memcpy(value + off,
> + per_cpu_ptr(storage->percpu_buf, cpu), size);
> + off += size;
> + }
> + rcu_read_unlock();
> + return 0;
> +}
> +
> +int bpf_percpu_cgroup_storage_update(struct bpf_map *_map, void *_key,
> + void *value, u64 map_flags)
> +{
> + struct bpf_cgroup_storage_map *map = map_to_storage(_map);
> + struct bpf_cgroup_storage_key *key = _key;
> + struct bpf_cgroup_storage *storage;
> + int cpu, off = 0;
> + u32 size;
> +
> + if (unlikely(map_flags & BPF_EXIST))
> + return -EINVAL;
> +
> + rcu_read_lock();
> + storage = cgroup_storage_lookup(map, key, false);
> + if (!storage) {
> + rcu_read_unlock();
> + return -ENOENT;
> + }
> +
> + /* the user space will provide round_up(value_size, 8) bytes that
> + * will be copied into per-cpu area. bpf programs can only access
> + * value_size of it. During lookup the same extra bytes will be
> + * returned or zeros which were zero-filled by percpu_alloc,
> + * so no kernel data leaks possible
> + */
> + size = round_up(_map->value_size, 8);
> + for_each_possible_cpu(cpu) {
> + bpf_long_memcpy(per_cpu_ptr(storage->percpu_buf, cpu),
> + value + off, size);
> + off += size;
> + }
> + rcu_read_unlock();
> + return 0;
> +}
> +
> static int cgroup_storage_get_next_key(struct bpf_map *_map, void *_key,
> void *_next_key)
> {
> @@ -292,55 +357,98 @@ struct bpf_cgroup_storage *bpf_cgroup_storage_alloc(struct bpf_prog *prog,
> {
> struct bpf_cgroup_storage *storage;
> struct bpf_map *map;
> + gfp_t flags;
> + size_t size;
> u32 pages;
>
> map = prog->aux->cgroup_storage[stype];
> if (!map)
> return NULL;
>
> - pages = round_up(sizeof(struct bpf_cgroup_storage) +
> - sizeof(struct bpf_storage_buffer) +
> - map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + if (stype == BPF_CGROUP_STORAGE_SHARED) {
> + size = sizeof(struct bpf_storage_buffer) + map->value_size;
> + pages = round_up(sizeof(struct bpf_cgroup_storage) + size,
> + PAGE_SIZE) >> PAGE_SHIFT;
> + } else {
> + size = map->value_size;
> + pages = round_up(round_up(size, 8) * num_possible_cpus(),
> + PAGE_SIZE) >> PAGE_SHIFT;
> + }
> +
> if (bpf_map_charge_memlock(map, pages))
> return ERR_PTR(-EPERM);
>
> storage = kmalloc_node(sizeof(struct bpf_cgroup_storage),
> __GFP_ZERO | GFP_USER, map->numa_node);
> - if (!storage) {
> - bpf_map_uncharge_memlock(map, pages);
> - return ERR_PTR(-ENOMEM);
> - }
> + if (!storage)
> + goto enomem;
>
> - storage->buf = kmalloc_node(sizeof(struct bpf_storage_buffer) +
> - map->value_size, __GFP_ZERO | GFP_USER,
> - map->numa_node);
> - if (!storage->buf) {
> - bpf_map_uncharge_memlock(map, pages);
> - kfree(storage);
> - return ERR_PTR(-ENOMEM);
> + flags = __GFP_ZERO | GFP_USER;
> +
> + if (stype == BPF_CGROUP_STORAGE_SHARED) {
> + storage->buf = kmalloc_node(size, flags, map->numa_node);
> + if (!storage->buf)
> + goto enomem;
> + } else {
> + storage->percpu_buf = __alloc_percpu_gfp(size, 8, flags);
> + if (!storage->percpu_buf)
> + goto enomem;
> }
>
> storage->map = (struct bpf_cgroup_storage_map *)map;
>
> return storage;
> +
> +enomem:
> + bpf_map_uncharge_memlock(map, pages);
> + kfree(storage);
> + return ERR_PTR(-ENOMEM);
> +}
> +
> +static void free_cgroup_storage_rcu(struct rcu_head *rcu)
Maybe rename as free_shared_cgroup_storage_rcu()?
> +{
> + struct bpf_cgroup_storage *storage =
> + container_of(rcu, struct bpf_cgroup_storage, rcu);
> +
> + kfree(storage->buf);
> + kfree(storage);
> +}
> +
> +static void free_percpu_cgroup_storage_rcu(struct rcu_head *rcu)
> +{
> + struct bpf_cgroup_storage *storage =
> + container_of(rcu, struct bpf_cgroup_storage, rcu);
> +
> + free_percpu(storage->percpu_buf);
> + kfree(storage);
> }
>
> void bpf_cgroup_storage_free(struct bpf_cgroup_storage *storage)
> {
> - u32 pages;
> + enum bpf_cgroup_storage_type stype;
> struct bpf_map *map;
> + u32 pages;
>
> if (!storage)
> return;
>
> map = &storage->map->map;
> - pages = round_up(sizeof(struct bpf_cgroup_storage) +
> - sizeof(struct bpf_storage_buffer) +
> - map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + stype = cgroup_storage_type(map);
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + pages = round_up(sizeof(struct bpf_cgroup_storage) +
> + sizeof(struct bpf_storage_buffer) +
> + map->value_size, PAGE_SIZE) >> PAGE_SHIFT;
> + else
> + pages = round_up(round_up(map->value_size, 8) *
> + num_possible_cpus(),
> + PAGE_SIZE) >> PAGE_SHIFT;
> +
> bpf_map_uncharge_memlock(map, pages);
>
> - kfree_rcu(storage->buf, rcu);
> - kfree_rcu(storage, rcu);
> + if (stype == BPF_CGROUP_STORAGE_SHARED)
> + call_rcu(&storage->rcu, free_cgroup_storage_rcu);
> + else
> + call_rcu(&storage->rcu, free_percpu_cgroup_storage_rcu);
> }
>
> void bpf_cgroup_storage_link(struct bpf_cgroup_storage *storage,
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 8c91d2b41b1e..5742df21598c 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -686,7 +686,8 @@ static int map_lookup_elem(union bpf_attr *attr)
>
> if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
> map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
> - map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
> + map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY ||
> + map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> value_size = round_up(map->value_size, 8) * num_possible_cpus();
> else if (IS_FD_MAP(map))
> value_size = sizeof(u32);
> @@ -705,6 +706,8 @@ static int map_lookup_elem(union bpf_attr *attr)
> err = bpf_percpu_hash_copy(map, key, value);
> } else if (map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY) {
> err = bpf_percpu_array_copy(map, key, value);
> + } else if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE) {
> + err = bpf_percpu_cgroup_storage_copy(map, key, value);
> } else if (map->map_type == BPF_MAP_TYPE_STACK_TRACE) {
> err = bpf_stackmap_copy(map, key, value);
> } else if (IS_FD_ARRAY(map)) {
> @@ -774,7 +777,8 @@ static int map_update_elem(union bpf_attr *attr)
>
> if (map->map_type == BPF_MAP_TYPE_PERCPU_HASH ||
> map->map_type == BPF_MAP_TYPE_LRU_PERCPU_HASH ||
> - map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY)
> + map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY ||
> + map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> value_size = round_up(map->value_size, 8) * num_possible_cpus();
> else
> value_size = map->value_size;
> @@ -809,6 +813,9 @@ static int map_update_elem(union bpf_attr *attr)
> err = bpf_percpu_hash_update(map, key, value, attr->flags);
> } else if (map->map_type == BPF_MAP_TYPE_PERCPU_ARRAY) {
> err = bpf_percpu_array_update(map, key, value, attr->flags);
> + } else if (map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE) {
> + err = bpf_percpu_cgroup_storage_update(map, key, value,
> + attr->flags);
> } else if (IS_FD_ARRAY(map)) {
> rcu_read_lock();
> err = bpf_fd_array_map_update_elem(map, f.file, key, value,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e75f36de91d6..d94073deb68a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2074,6 +2074,7 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
> goto error;
> break;
> case BPF_MAP_TYPE_CGROUP_STORAGE:
> + case BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE:
> if (func_id != BPF_FUNC_get_local_storage)
> goto error;
> break;
> @@ -2164,7 +2165,8 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
> goto error;
> break;
> case BPF_FUNC_get_local_storage:
> - if (map->map_type != BPF_MAP_TYPE_CGROUP_STORAGE)
> + if (map->map_type != BPF_MAP_TYPE_CGROUP_STORAGE &&
> + map->map_type != BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE)
> goto error;
> break;
> case BPF_FUNC_sk_select_reuseport:
> @@ -5049,6 +5051,12 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
> return 0;
> }
>
> +static bool bpf_map_is_cgroup_storage(struct bpf_map *map)
> +{
> + return (map->map_type == BPF_MAP_TYPE_CGROUP_STORAGE ||
> + map->map_type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE);
> +}
> +
> /* look for pseudo eBPF instructions that access map FDs and
> * replace them with actual map pointers
> */
> @@ -5139,10 +5147,9 @@ static int replace_map_fd_with_map_ptr(struct bpf_verifier_env *env)
> }
> env->used_maps[env->used_map_cnt++] = map;
>
> - if (map->map_type == BPF_MAP_TYPE_CGROUP_STORAGE &&
> + if (bpf_map_is_cgroup_storage(map) &&
> bpf_cgroup_storage_assign(env->prog, map)) {
> - verbose(env,
> - "only one cgroup storage is allowed\n");
> + verbose(env, "only one cgroup storage of each type is allowed\n");
> fdput(f);
> return -EBUSY;
> }
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH iproute2 1/1] DEBUG: Fix make check when need build generate_nlmsg
From: Petr Vorel @ 2018-09-25 12:49 UTC (permalink / raw)
To: netdev; +Cc: Petr Vorel, Stephen Hemminger
make check from top level Makefile defines several flags which break
building generate_nlmsg:
$ make check
make -C tools
gcc -Wall -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wold-style-definition -Wformat=2 -O2 -I../include -I../include/uapi -DRESOLVE_HOSTNAMES -DLIBDIR=\"/usr/lib\" -DCONFDIR=\"/etc/iproute2\" -DNETNS_RUN_DIR=\"/var/run/netns\" -DNETNS_ETC_DIR=\"/etc/netns\" -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -DHAVE_SETNS -DHAVE_SELINUX -DHAVE_ELF -DHAVE_LIBMNL -I/usr/include/libmnl -DNEED_STRLCPY -DHAVE_LIBCAP ../lib/libutil.a ../lib/libnetlink.a -lselinux -lelf -lmnl -lcap -I../../include -include../../include/uapi/linux/netlink.h -o generate_nlmsg generate_nlmsg.c ../../lib/libnetlink.c -lmnl
gcc: error: ../lib/libutil.a: No such file or directory
gcc: error: ../lib/libnetlink.a: No such file or directory
make[2]: *** [Makefile:5: generate_nlmsg] Error 1
make[1]: *** [Makefile:40: generate_nlmsg] Error 2
To fix it reset CFLAGS in sub Makefile and remove LDLIBS entirely (as
required -lmnl flag was specified in 5dc2204c ("testsuite: add libmnl").
Fixes: 8804a8c0 ("Makefile: Add check target")
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Hi Stephen,
I'm sorry for this regression.
Kind regards,
Petr
---
testsuite/tools/Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/testsuite/tools/Makefile b/testsuite/tools/Makefile
index 7d53d226..e1d9bfef 100644
--- a/testsuite/tools/Makefile
+++ b/testsuite/tools/Makefile
@@ -1,8 +1,9 @@
# SPDX-License-Identifier: GPL-2.0
+CFLAGS=
include ../../config.mk
generate_nlmsg: generate_nlmsg.c ../../lib/libnetlink.c
- $(CC) $(CPPFLAGS) $(CFLAGS) $(LDLIBS) $(EXTRA_CFLAGS) -I../../include -include../../include/uapi/linux/netlink.h -o $@ $^ -lmnl
+ $(CC) $(CPPFLAGS) $(CFLAGS) $(EXTRA_CFLAGS) -I../../include -include../../include/uapi/linux/netlink.h -o $@ $^ -lmnl
clean:
rm -f generate_nlmsg
--
2.19.0
^ permalink raw reply related
* Re: [PATCH v2 bpf-next 00/10] bpf: per-cpu cgroup local storage
From: Song Liu @ 2018-09-25 19:05 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Kernel Team
In-Reply-To: <20180925152114.13537-1-guro@fb.com>
> On Sep 25, 2018, at 8:21 AM, Roman Gushchin <guro@fb.com> wrote:
>
> This patchset implements per-cpu cgroup local storage and provides
> an example how per-cpu and shared cgroup local storage can be used
> for efficient accounting of network traffic.
>
> v2->v1:
> 1) added a selftest implementing network counters
> 2) added a missing free() in cgroup local storage selftest
>
> Roman Gushchin (10):
> bpf: extend cgroup bpf core to allow multiple cgroup storage types
> bpf: rework cgroup storage pointer passing
> bpf: introduce per-cpu cgroup local storage
> bpf: don't allow create maps of per-cpu cgroup local storages
> bpf: sync include/uapi/linux/bpf.h to tools/include/uapi/linux/bpf.h
> bpftool: add support for PERCPU_CGROUP_STORAGE maps
> selftests/bpf: add verifier per-cpu cgroup storage tests
> selftests/bpf: extend the storage test to test per-cpu cgroup storage
> samples/bpf: extend test_cgrp2_attach2 test to use per-cpu cgroup
> storage
> selftests/bpf: cgroup local storage-based network counters
>
> include/linux/bpf-cgroup.h | 55 ++++--
> include/linux/bpf.h | 12 +-
> include/linux/bpf_types.h | 1 +
> include/uapi/linux/bpf.h | 1 +
> kernel/bpf/cgroup.c | 74 +++++---
> kernel/bpf/helpers.c | 25 ++-
> kernel/bpf/local_storage.c | 167 +++++++++++++++---
> kernel/bpf/map_in_map.c | 3 +-
> kernel/bpf/syscall.c | 20 ++-
> kernel/bpf/verifier.c | 23 ++-
> net/bpf/test_run.c | 20 ++-
> samples/bpf/test_cgrp2_attach2.c | 19 +-
> tools/bpf/bpftool/map.c | 4 +-
> tools/include/uapi/linux/bpf.h | 1 +
> tools/testing/selftests/bpf/Makefile | 6 +-
> tools/testing/selftests/bpf/netcnt_common.h | 23 +++
> tools/testing/selftests/bpf/netcnt_prog.c | 71 ++++++++
> .../selftests/bpf/test_cgroup_storage.c | 60 ++++++-
> tools/testing/selftests/bpf/test_netcnt.c | 153 ++++++++++++++++
> tools/testing/selftests/bpf/test_verifier.c | 139 ++++++++++++++-
> 20 files changed, 778 insertions(+), 99 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/netcnt_common.h
> create mode 100644 tools/testing/selftests/bpf/netcnt_prog.c
> create mode 100644 tools/testing/selftests/bpf/test_netcnt.c
>
> --
> 2.17.1
>
For 04/10 to 10/10:
Acked-by: Song Liu <songliubraving@fb.com>
^ permalink raw reply
* Re: netlink: 16 bytes leftover after parsing attributes in process `ip'.
From: Stephen Hemminger @ 2018-09-25 13:16 UTC (permalink / raw)
To: Christian Brauner; +Cc: David Ahern, netdev@vger.kernel.org, David Miller
In-Reply-To: <CAHrFyr55vt78akhC+WE5maDPLzmurUeB6wT-DmHcCmnAfPKrgw@mail.gmail.com>
On Tue, 25 Sep 2018 14:34:08 +0200
Christian Brauner <christian@brauner.io> wrote:
> On Tue, Sep 25, 2018, 14:07 Stephen Hemminger <stephen@networkplumber.org>
> wrote:
>
> > On Tue, 25 Sep 2018 11:49:10 +0200
> > Christian Brauner <christian@brauner.io> wrote:
> >
> > > On Mon, Sep 24, 2018 at 09:19:06PM -0600, David Ahern wrote:
> > > > On top of net-next I am see a dmesg error:
> > > >
> > > > netlink: 16 bytes leftover after parsing attributes in process `ip'.
> > > >
> > > > I traced it to address lists and commit:
> > > >
> > > > commit 6ecf4c37eb3e89b0832c9616089a5cdca3747da7
> > > > Author: Christian Brauner <christian@brauner.io>
> > > > Date: Tue Sep 4 21:53:50 2018 +0200
> > > >
> > > > ipv6: enable IFA_TARGET_NETNSID for RTM_GETADDR
> > > >
> > > > Per the commit you are trying to guess whether the ancillary header is
> > > > an ifinfomsg or a ifaddrmsg. I am guessing you are guessing wrong.
> > :-)
> > >
> > > Well, I currently don't guess at all. :) I'm parsing with struct
> > > ifaddrmsg as assumed header size but ignore parsing errors when that
> > > fails. You don't get the niceties of the new property if you don't pack
> >
> > There are legacy parts of netlink interface with kernel.
> > The ABI has evolved over time but some old parts are stuck in the past.
> >
> >
> > > > I don't have time to take this to ground, but address listing is not
> > the
> > > > only area subject to iproute2's SNAFU of infomsg everywhere on dumps. I
> > > > have thought about this for route dumps, but its solution does not work
> > > > here. You'll need to find something because the current warning on
> > every
> > > > address dump is not acceptable.
> > >
> > > Two points before I propose a migitation:
> > >
> > > 1. The burded of seeing pr_warn_ratelimited() messages in dmesg when
> > > userspace is doing something wrong is imho justifiable.
> > > Actually, I would argue that we should not hide the problem from
> > > userspace at all. The rate-limited (so no logging DOS afaict) warning
> > > messages are a perfect indicator that a tool is doing something wrong
> > > *without* introducing any regressions.
> > > The rtnetlink manpage clearly indicates that ifaddrmsg is supposed to
> > > be used too. Additionally, userspace stuffs an ifinfomsg in there but
> > > expects to receive ifaddrmsg. They should be warned loudly. :) So I
> > > actually like the warning messages.
> > > 2. Userspace should be fixed. Especially such an important standard tool
> > > as iproute2 that is maintained on git.kernel.org (glibc is already
> > > doing the right.).
> > >
> > > So if people really want to hide this issue as much as we can then we
> > > can play the guessing game. I could send a patch that roughly does the
> > > following:
> > >
> > > if (nlmsg_len(cb->nlh) < sizeof(struct ifinfomsg))
> > > guessed_header_len = sizeof(struct ifaddrmsg);
> > > else
> > > guessed_header_len = sizeof(struct ifinfomsg);
> > >
> > > This will work since sizeof(ifaddrmsg) == 8 and sizeof(ifinfomsg) == 16.
> > > The only valid property for RTM_GETADDR requests is IFA_TARGET_NETNSID.
> > > This propert is a __s32 which should bring the message up to 12 bytes
> > > (not sure about alignment requiremnts and where we might wend up ten)
> > > which is still less than the 16 bytes without that property from
> > > ifinfomsg. That's a hacky hacky hack-hack and will likely work but will
> > > break when ifaddrmsg grows a new member or we introduce another property
> > > that is valid in RTM_GETADDR requests. It also will not work cleanly
> > > when users stuff additional properties in there that are valid for the
> > > address family but are not used int RTM_GETADDR requests.
> > >
> > > I would like to hear what other people and davem think we should do.
> > > Patch it away or print the warning.
> > >
> > > Christian
> >
> > You can't break old programs. That is one of the rules of kernel.
> > Therefore, please either revert the kernel change or put the new attribute
> > in a place where old versions do not cause problem.
> >
>
> Sorry, there's a misunderstanding here. The code doesn't regress anything.
> The patch was written in a backward compatible way. The only thing it
> causes are rate-limited logging messages when the wrong struct is passed.
> But the request still succeeds. The issue is with the logging afaict.
>
> Christian
That still means enterprise distributions that use the current kernel will
get customer complaints. You need to remove the warning.
^ permalink raw reply
* Re: requesting stable backport of BPF security fix (commit dd066823db2ac4e22f721ec85190817b58059a54)
From: Daniel Borkmann @ 2018-09-25 13:27 UTC (permalink / raw)
To: Jann Horn, Network Development, David S. Miller
Cc: Alexei Starovoitov, security
In-Reply-To: <CAG48ez1=zOgmdsuE38HkG73EA4en+5QOTLTMZMe+PGcthhwt8g@mail.gmail.com>
On 09/25/2018 02:46 PM, Jann Horn wrote:
> Hi!
>
> Per the policy at Documentation/networking/netdev-FAQ.rst, I'm sending
> this to netdev@ and davem, rather than stable@; with a CC to security@
> because I believe that this is a security process issue.
>
> Upstream commit dd066823db2ac4e22f721ec85190817b58059a54
> ("bpf/verifier: disallow pointer subtraction") fixes a security bug
> (kernel pointer leak to unprivileged userspace). The fix has been in
> Linus' tree since about a week ago, but the patch still doesn't appear
> in Greg's linux-4.18.y linux-stable-rc repo, in Greg's 4.18
> stable-queue, or in davem's stable queue at
> http://patchwork.ozlabs.org/bundle/davem/stable/?state=* .
>
> Please queue it up for backporting.
Done & flushed out now, sorry for the delay.
Thanks,
Daniel
^ permalink raw reply
* Re: r8169 hang on 4.18
From: Gabriel C @ 2018-09-25 19:37 UTC (permalink / raw)
To: hkallweit1; +Cc: odi, LKML, netdev
In-Reply-To: <9fb89abb-917a-44b6-5dff-9c4a0ed1fd6b@gmail.com>
Am Di., 25. Sep. 2018 um 20:44 Uhr schrieb Heiner Kallweit
<hkallweit1@gmail.com>:
> As I said, you could try running 4.18.4 instead of 4.18.8 for a while (if you have this option).
> You faced the hang roughly once per day, so after a few days you should be sure.
I don't think this will fix anything.
See: https://lkml.org/lkml/2018/9/4/174
Probably all broken by 4fd48c4ac0a0 ("r8169: move common
initializations to tp->hw_start").
BR
^ permalink raw reply
* Re: [PATCH net-next v6 02/23] zinc: introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-09-25 19:43 UTC (permalink / raw)
To: Joe Perches
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <2b5f2df2f1278c118a913259a44efdf589719293.camel@perches.com>
On Tue, Sep 25, 2018 at 8:33 PM Joe Perches <joe@perches.com> wrote:
> I think the -Dpr_fmt is especially odd and not
> really acceptable as it not used anywhere else
> in the kernel.
There are about 2000 cases in the kernel of the same '#define
pr_fmt...' being pasted into the top of the file, which seems a bit
cumbersome. Rather than having to paste that into each and every file
that I pr_err from, why can't I just do this from the makefile, since
I want that same pr_fmt to copy the whole directory?
^ permalink raw reply
* [net 1/1] tipc: reset bearer if device carrier not ok
From: Jon Maloy @ 2018-09-25 19:56 UTC (permalink / raw)
To: davem, netdev; +Cc: tipc-discussion
From: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
If we detect that under lying carrier detects errors and goes down,
we reset the bearer.
Signed-off-by: Parthasarathy Bhuvaragan <parthasarathy.bhuvaragan@ericsson.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/bearer.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 418f03d..645c160 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -609,16 +609,18 @@ static int tipc_l2_device_event(struct notifier_block *nb, unsigned long evt,
switch (evt) {
case NETDEV_CHANGE:
- if (netif_carrier_ok(dev))
+ if (netif_carrier_ok(dev) && netif_oper_up(dev)) {
+ test_and_set_bit_lock(0, &b->up);
break;
- /* else: fall through */
- case NETDEV_UP:
- test_and_set_bit_lock(0, &b->up);
- break;
+ }
+ /* fall through */
case NETDEV_GOING_DOWN:
clear_bit_unlock(0, &b->up);
tipc_reset_bearer(net, b);
break;
+ case NETDEV_UP:
+ test_and_set_bit_lock(0, &b->up);
+ break;
case NETDEV_CHANGEMTU:
if (tipc_mtu_bad(dev, 0)) {
bearer_disable(net, b);
--
2.1.4
^ permalink raw reply related
* Re: [PATCH net-next v6 02/23] zinc: introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-09-25 20:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Joe Perches, LKML, Netdev, Linux Crypto Mailing List,
David Miller, Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <1BA7DB23-7BB6-46BA-A3E2-D65BC3CAF11C@amacapital.net>
On Tue, Sep 25, 2018 at 10:00 PM Andy Lutomirski <luto@amacapital.net> wrote:
> > On Sep 25, 2018, at 12:43 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> >> On Tue, Sep 25, 2018 at 8:33 PM Joe Perches <joe@perches.com> wrote:
> >> I think the -Dpr_fmt is especially odd and not
> >> really acceptable as it not used anywhere else
> >> in the kernel.
> >
> > There are about 2000 cases in the kernel of the same '#define
> > pr_fmt...' being pasted into the top of the file, which seems a bit
> > cumbersome. Rather than having to paste that into each and every file
> > that I pr_err from, why can't I just do this from the makefile, since
> > I want that same pr_fmt to copy the whole directory?
>
> Because people like to be able to just read the C file to figure out what it does. Or we could adopt the Makefile approach kernel-wide, since this use of it seems reasonable.
It would indeed be nice to see this done tree-wide. I can recall
various random bugs over the year where some dmesg messages are
missing a prefix because the author forgot to copy and paste the thing
to /yet another file/ in the same directory.
^ permalink raw reply
* Re: [PATCH net-next v6 02/23] zinc: introduce minimal cryptography library
From: Joe Perches @ 2018-09-25 20:05 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <CAHmME9qLzFHPm2aaEKOmjBA95-c3CZuT+kQiuksLRRWQ_ad2sw@mail.gmail.com>
On Tue, 2018-09-25 at 21:43 +0200, Jason A. Donenfeld wrote:
> On Tue, Sep 25, 2018 at 8:33 PM Joe Perches <joe@perches.com> wrote:
> > I think the -Dpr_fmt is especially odd and not
> > really acceptable as it not used anywhere else
> > in the kernel.
>
> There are about 2000 cases in the kernel of the same '#define
> pr_fmt...' being pasted into the top of the file, which seems a bit
> cumbersome. Rather than having to paste that into each and every file
> that I pr_err from,
As far as I can tell, zinc doesn't use pr_err, just
pr_info in all the selftest blocks which are only
used when DEBUG is #defined.
Perhaps all the pr_info uses should use pr_debug
instead as well as dynamic debugging which already
optionally adds KBUILD_MODINFO.
> why can't I just do this from the makefile, since
> I want that same pr_fmt to copy the whole directory?
Ideally all of those
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
uses will be removed one day soon and the default
will change.
https://lore.kernel.org/patchwork/cover/904507/
This slightly odd use might complicate that.
^ permalink raw reply
* Re: [PATCH net 00/15] netpoll: avoid capture effects for NAPI drivers
From: Michael Chan @ 2018-09-25 14:02 UTC (permalink / raw)
To: Song Liu
Cc: Eric Dumazet, David Miller, Netdev, Ariel Elior, Eric Dumazet,
tariqt, Saeed Mahameed, jeffrey.t.kirsher, Jakub Kicinski,
j.vosburgh, vfalico, Andy Gospodarek
In-Reply-To: <ACD08072-B5A0-4A5D-8E02-30E5F763124D@fb.com>
On Mon, Sep 24, 2018 at 2:18 PM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 24, 2018, at 2:05 PM, Eric Dumazet <edumazet@google.com> wrote:
> >
> >>
> >> Interesting, maybe a bnxt specific issue.
> >>
> >> It seems their model is to process TX/RX notification in the same queue,
> >> they throw away RX events if budget == 0
> >>
> >> It means commit e7b9569102995ebc26821789628eef45bd9840d8 is wrong and
> >> must be reverted.
> >>
> >> Otherwise, we have a possibility of blocking a queue under netpoll pressure.
> >
> > Hmm, actually a revert might not be enough, since code at lines 2030-2031
> > would fire and we might not call napi_complete_done() anyway.
> >
> > Unfortunately this driver logic is quite complex.
> >
> > Could you test on other NIC eventually ?
> >
>
> It actually runs OK on ixgbe.
>
> @Michael, could you please help us with this?
>
I've taken a quick look using today's net tree plus Eric's
poll_one_napi() patch. The problem I'm seeing is that netpoll calls
bnxt_poll() with budget 0. And since work_done >= budget of 0, we
return without calling napi_complete_done() and without arming the
interrupt. netpoll doesn't always call us back until we call
napi_complete_done(), right? So I think if there are in-flight TX
completions, we'll miss those.
^ permalink raw reply
* Re: [PATCH net-next v6 02/23] zinc: introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-09-25 20:12 UTC (permalink / raw)
To: Joe Perches
Cc: LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <748429eee2fc77afd01630c24bd2cf66a545aec5.camel@perches.com>
Hi Joe,
On Tue, Sep 25, 2018 at 10:05 PM Joe Perches <joe@perches.com> wrote:
> As far as I can tell, zinc doesn't use pr_err, just
> pr_info
Yes, pr_info, not pr_err. Apologies for my imprecision. But the
distinction does not matter at all, since they both use pr_fmt in
exactly the same way, which is sufficient for the purposes of this
discussion.
> > why can't I just do this from the makefile, since
> > I want that same pr_fmt to copy the whole directory?
>
> Ideally all of those
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> uses will be removed one day soon and the default
> will change.
>
> https://lore.kernel.org/patchwork/cover/904507/
>
> This slightly odd use might complicate that.
Oh, that's good to see. In this case, I'm prefixing all of them with
zinc: instead of zinc_chacha20: (the modname), but I wouldn't object
to that changing to KBUILD_MODNAME if your patch lands. IOW, once your
patch lands, I'm happy to revisit this discussion and fall back to
using the sane defaults you're in the process of setting up for
everyone.
Jason
^ permalink raw reply
* Re: [PATCH v2 net-next 9/9] devlink: Add Documentation/networking/devlink-params-bnxt.txt
From: Jakub Kicinski @ 2018-09-25 14:05 UTC (permalink / raw)
To: Vasundhara Volam; +Cc: David Miller, michael.chan@broadcom.com, Netdev
In-Reply-To: <CAACQVJpxp=C8TdiAbOZCKhZ=Pgo-kX8aD148pYMM2613GdEQjQ@mail.gmail.com>
On Tue, 25 Sep 2018 09:37:20 +0530, Vasundhara Volam wrote:
> > Why duplicate the description of the generic parameters?
> Not all generic parameters are used by all drivers. So, I want to add
> information about
> type and configuration mode about generic parameters used by bnxt_en driver.
> I can remove description part keeping type and configuration mode, if
> it looks duplication.
That'd be better.
^ 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