* Re: [PATCH net-next] virtio_net: force_napi_tx module param.
From: David Miller @ 2018-07-29 16:00 UTC (permalink / raw)
To: caleb.raitto; +Cc: mst, jasowang, netdev, caraitto
In-Reply-To: <20180723231119.142904-1-caleb.raitto@gmail.com>
From: Caleb Raitto <caleb.raitto@gmail.com>
Date: Mon, 23 Jul 2018 16:11:19 -0700
> From: Caleb Raitto <caraitto@google.com>
>
> The driver disables tx napi if it's not certain that completions will
> be processed affine with tx service.
>
> Its heuristic doesn't account for some scenarios where it is, such as
> when the queue pair count matches the core but not hyperthread count.
>
> Allow userspace to override the heuristic. This is an alternative
> solution to that in the linked patch. That added more logic in the
> kernel for these cases, but the agreement was that this was better left
> to user control.
>
> Do not expand the existing napi_tx variable to a ternary value,
> because doing so can break user applications that expect
> boolean ('Y'/'N') instead of integer output. Add a new param instead.
>
> Link: https://patchwork.ozlabs.org/patch/725249/
> Acked-by: Willem de Bruijn <willemb@google.com>
> Acked-by: Jon Olson <jonolson@google.com>
> Signed-off-by: Caleb Raitto <caraitto@google.com>
So I looked into the history surrounding these issues.
First of all, it's always ends up turning out crummy when drivers start
to set affinities themselves. The worst possible case is to do it
_conditionally_, and that is exactly what virtio_net is doing.
>From the user's perspective, this provides a really bad experience.
So if I have a 32-queue device and there are 32 cpus, you'll do all
the affinity settings, stopping Irqbalanced from doing anything
right?
So if I add one more cpu, you'll say "oops, no idea what to do in
this situation" and not touch the affinities at all?
That makes no sense at all.
If the driver is going to set affinities at all, OWN that decision
and set it all the time to something reasonable.
Or accept that you shouldn't be touching this stuff in the first place
and leave the affinities alone.
Right now we're kinda in a situation where the driver has been setting
affinities in the ncpus==nqueues cases for some time, so we can't stop
doing it.
Which means we have to set them in all cases to make the user
experience sane again.
I looked at the linked to patch again:
https://patchwork.ozlabs.org/patch/725249/
And I think the strategy should be made more generic, to get rid of
the hyperthreading assumptions. I also agree that the "assign
to first N cpus" logic doesn't make much sense either.
Just distribute across the available cpus evenly, and be done with it.
If you have 64 cpus and 32 queues, this assigns queues to every other
cpu.
Then we don't need this weird new module parameter.
^ permalink raw reply
* Re: [PATCH 4/4] net: dsa: Add Lantiq / Intel DSA driver for vrx200
From: Hauke Mehrtens @ 2018-07-29 16:25 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem, netdev, vivien.didelot, f.fainelli, john, linux-mips, dev,
hauke.mehrtens
In-Reply-To: <20180725161219.GC16819@lunn.ch>
On 07/25/2018 06:12 PM, Andrew Lunn wrote:
>> LANTIQ MIPS ARCHITECTURE
>> M: John Crispin <john@phrozen.org>
>> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
>> index 2b81b97e994f..f1280aa3f9bd 100644
>> --- a/drivers/net/dsa/Kconfig
>> +++ b/drivers/net/dsa/Kconfig
>> @@ -23,6 +23,14 @@ config NET_DSA_LOOP
>> This enables support for a fake mock-up switch chip which
>> exercises the DSA APIs.
>>
>> +config NET_DSA_GSWIP
>> + tristate "Intel / Lantiq GSWIP"
>
> Minor nit pick. Could you make this NET_DSA_LANTIQ_GSWIP. We generally
> have some manufacture ID in the name. And change the text to Lantiq /
> Intel GSWIP.
done
>> +static const struct gswip_rmon_cnt_desc gswip_rmon_cnt[] = {
>> + /** Receive Packet Count (only packets that are accepted and not discarded). */
>> + MIB_DESC(1, 0x1F, "RxGoodPkts"),
>> + /** Receive Unicast Packet Count. */
>> + MIB_DESC(1, 0x23, "RxUnicastPkts"),
>> + /** Receive Multicast Packet Count. */
>> + MIB_DESC(1, 0x22, "RxMulticastPkts"),
>> + /** Receive FCS Error Packet Count. */
>> + MIB_DESC(1, 0x21, "RxFCSErrorPkts"),
>> + /** Receive Undersize Good Packet Count. */
>> + MIB_DESC(1, 0x1D, "RxUnderSizeGoodPkts"),
>> + /** Receive Undersize Error Packet Count. */
>> + MIB_DESC(1, 0x1E, "RxUnderSizeErrorPkts"),
>> + /** Receive Oversize Good Packet Count. */
>> + MIB_DESC(1, 0x1B, "RxOversizeGoodPkts"),
>> + /** Receive Oversize Error Packet Count. */
>> + MIB_DESC(1, 0x1C, "RxOversizeErrorPkts"),
>> + /** Receive Good Pause Packet Count. */
>> + MIB_DESC(1, 0x20, "RxGoodPausePkts"),
>> + /** Receive Align Error Packet Count. */
>> + MIB_DESC(1, 0x1A, "RxAlignErrorPkts"),
>> + /** Receive Size 64 Packet Count. */
>> + MIB_DESC(1, 0x12, "Rx64BytePkts"),
>> + /** Receive Size 65-127 Packet Count. */
>> + MIB_DESC(1, 0x13, "Rx127BytePkts"),
>> + /** Receive Size 128-255 Packet Count. */
>> + MIB_DESC(1, 0x14, "Rx255BytePkts"),
>> + /** Receive Size 256-511 Packet Count. */
>> + MIB_DESC(1, 0x15, "Rx511BytePkts"),
>> + /** Receive Size 512-1023 Packet Count. */
>> + MIB_DESC(1, 0x16, "Rx1023BytePkts"),
>> + /** Receive Size 1024-1522 (or more, if configured) Packet Count. */
>> + MIB_DESC(1, 0x17, "RxMaxBytePkts"),
>> + /** Receive Dropped Packet Count. */
>> + MIB_DESC(1, 0x18, "RxDroppedPkts"),
>> + /** Filtered Packet Count. */
>> + MIB_DESC(1, 0x19, "RxFilteredPkts"),
>> + /** Receive Good Byte Count (64 bit). */
>> + MIB_DESC(2, 0x24, "RxGoodBytes"),
>> + /** Receive Bad Byte Count (64 bit). */
>> + MIB_DESC(2, 0x26, "RxBadBytes"),
>> + /** Transmit Dropped Packet Count, based on Congestion Management. */
>> + MIB_DESC(1, 0x11, "TxAcmDroppedPkts"),
>> + /** Transmit Packet Count. */
>> + MIB_DESC(1, 0x0C, "TxGoodPkts"),
>> + /** Transmit Unicast Packet Count. */
>> + MIB_DESC(1, 0x06, "TxUnicastPkts"),
>> + /** Transmit Multicast Packet Count. */
>> + MIB_DESC(1, 0x07, "TxMulticastPkts"),
>> + /** Transmit Size 64 Packet Count. */
>> + MIB_DESC(1, 0x00, "Tx64BytePkts"),
>> + /** Transmit Size 65-127 Packet Count. */
>> + MIB_DESC(1, 0x01, "Tx127BytePkts"),
>> + /** Transmit Size 128-255 Packet Count. */
>> + MIB_DESC(1, 0x02, "Tx255BytePkts"),
>> + /** Transmit Size 256-511 Packet Count. */
>> + MIB_DESC(1, 0x03, "Tx511BytePkts"),
>> + /** Transmit Size 512-1023 Packet Count. */
>> + MIB_DESC(1, 0x04, "Tx1023BytePkts"),
>> + /** Transmit Size 1024-1522 (or more, if configured) Packet Count. */
>> + MIB_DESC(1, 0x05, "TxMaxBytePkts"),
>> + /** Transmit Single Collision Count. */
>> + MIB_DESC(1, 0x08, "TxSingleCollCount"),
>> + /** Transmit Multiple Collision Count. */
>> + MIB_DESC(1, 0x09, "TxMultCollCount"),
>> + /** Transmit Late Collision Count. */
>> + MIB_DESC(1, 0x0A, "TxLateCollCount"),
>> + /** Transmit Excessive Collision Count. */
>> + MIB_DESC(1, 0x0B, "TxExcessCollCount"),
>> + /** Transmit Pause Packet Count. */
>> + MIB_DESC(1, 0x0D, "TxPauseCount"),
>> + /** Transmit Drop Packet Count. */
>> + MIB_DESC(1, 0x10, "TxDroppedPkts"),
>> + /** Transmit Good Byte Count (64 bit). */
>> + MIB_DESC(2, 0x0E, "TxGoodBytes"),
>
> Most of the comments here don't add anything useful. Maybe remove
> them?
Ok I removed them. Are the names ok, or should they follow any Linux
definition?
>> +};
>> +
>> +static u32 gswip_switch_r(struct gswip_priv *priv, u32 offset)
>> +{
>> + return __raw_readl(priv->gswip + (offset * 4));
>> +}
>> +
>> +static void gswip_switch_w(struct gswip_priv *priv, u32 val, u32 offset)
>> +{
>> + return __raw_writel(val, priv->gswip + (offset * 4));
>> +}
>
> Since this is MIPS, i assume re-ordering cannot happen, there are
> barriers, etc?
As far as I know this is not a problem on this bus and no barriers are
needed here.
>> +static int xrx200_mdio_poll(struct gswip_priv *priv)
>> +{
>> + int cnt = 10000;
>> +
>> + while (likely(cnt--)) {
>> + u32 ctrl = gswip_mdio_r(priv, GSWIP_MDIO_CTRL);
>> +
>> + if ((ctrl & GSWIP_MDIO_CTRL_BUSY) == 0)
>> + return 0;
>> + cpu_relax();
>> + }
>> +
>> + return 1;
>> +}
>
> Please return ETIMEOUT when needed. Maybe use one of the variants of
> readx_poll_timeout().
I am returning ETIMEOUT now.
When I would use readx_poll_timeout() I can not use the gswip_mdio_r()
function, because it takes two arguments and would have to use readl
directly. I do not think that this would make the code much better
readable.
>> +
>> +static int xrx200_mdio_wr(struct mii_bus *bus, int addr, int reg, u16 val)
>> +{
>> + struct gswip_priv *priv = bus->priv;
>> +
>> + if (xrx200_mdio_poll(priv))
>> + return -EIO;
>
> EIO is wrong, it should be a timeout. Solved above...
OK I replaced this now with this code:
err = gswip_mdio_poll(priv);
if (err)
return err;
>> +
>> + gswip_mdio_w(priv, val, GSWIP_MDIO_WRITE);
>> + gswip_mdio_w(priv, GSWIP_MDIO_CTRL_BUSY | GSWIP_MDIO_CTRL_WR |
>> + ((addr & GSWIP_MDIO_CTRL_PHYAD_MASK) << GSWIP_MDIO_CTRL_PHYAD_SHIFT) |
>> + (reg & GSWIP_MDIO_CTRL_REGAD_MASK),
>> + GSWIP_MDIO_CTRL);
>> +
>> + return 0;
>> +}
>> +
>
>> +static int gswip_mdio(struct gswip_priv *priv, struct device_node *mdio_np)
>> +{
>> + struct dsa_switch *ds = priv->ds;
>> +
>> + ds->slave_mii_bus = devm_mdiobus_alloc(priv->dev);
>> + if (!ds->slave_mii_bus)
>> + return -ENOMEM;
>> +
>> + ds->slave_mii_bus->priv = priv;
>> + ds->slave_mii_bus->read = xrx200_mdio_rd;
>> + ds->slave_mii_bus->write = xrx200_mdio_wr;
>> + ds->slave_mii_bus->name = "lantiq,xrx200-mdio";
>> + snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%x", 0);
>
> You should be able to do better than that.
I replaced this now with this code:
snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
dev_name(priv->dev));
>> + ds->slave_mii_bus->parent = priv->dev;
>> + ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
>> +
>> + return of_mdiobus_register(ds->slave_mii_bus, mdio_np);
>> +}
>> +
>> +static void gswip_wait_pce_tbl_ready(struct gswip_priv *priv)
>> +{
>> + while (gswip_switch_r(priv, GSWIP_PCE_TBL_CTRL) & GSWIP_PCE_TBL_CTRL_BAS)
>> + cond_resched();
>
> Please add some form of timeout.
I introduced a new function gswip_switch_r_timeout() which uses
readx_poll_timeout().
>> +}
>> +
>> +static int gswip_port_enable(struct dsa_switch *ds, int port,
>> + struct phy_device *phy)
>> +{
>> + struct gswip_priv *priv = (struct gswip_priv *)ds->priv;
>
> Casts like this are not needed.
removed
>> + /* RMON Counter Enable for port */
>> + gswip_switch_w(priv, GSWIP_BM_PCFG_CNTEN, GSWIP_BM_PCFGp(port));
>> +
>> + /* enable port fetch/store dma & VLAN Modification */
>> + gswip_switch_mask(priv, 0, GSWIP_FDMA_PCTRL_EN |
>> + GSWIP_FDMA_PCTRL_VLANMOD_BOTH,
>> + GSWIP_FDMA_PCTRLp(port));
>> + gswip_switch_mask(priv, 0, GSWIP_SDMA_PCTRL_EN,
>> + GSWIP_SDMA_PCTRLp(port));
>> + gswip_switch_mask(priv, 0, GSWIP_PCE_PCTRL_0_INGRESS,
>> + GSWIP_PCE_PCTRL_0p(port));
>> +
>> + return 0;
>> +}
>> +
>
>> +static int gswip_setup(struct dsa_switch *ds)
>> +{
>> + struct gswip_priv *priv = ds->priv;
>> + int i;
>> +
>> + gswip_switch_w(priv, GSWIP_ETHSW_SWRES_R0, GSWIP_ETHSW_SWRES);
>> + usleep_range(5000, 10000);
>> + gswip_switch_w(priv, 0, GSWIP_ETHSW_SWRES);
>> +
>> + /* disable port fetch/store dma, assume CPU port is last port */
>
> Since this comes from device tree, you should really verify that and
> return EINVAL if not, in the probe() function.
I defined a struct hw_info which describes the details of this switch,
max port numbers and CPU port, as this is hard coded and this is then
compared to the device tree settings.
>> + for (i = 0; i <= priv->cpu_port; i++)
>> + gswip_port_disable(ds, i, NULL);
>> +
>> + /* enable Switch */
>> + gswip_mdio_mask(priv, 0, GSWIP_MDIO_GLOB_ENABLE, GSWIP_MDIO_GLOB);
>> +
>> + xrx200_pci_microcode(priv);
>> +
>> + /* Default unknown Broadcast/Multicast/Unicast port maps */
>> + gswip_switch_w(priv, BIT(priv->cpu_port), GSWIP_PCE_PMAP1);
>> + gswip_switch_w(priv, BIT(priv->cpu_port), GSWIP_PCE_PMAP2);
>> + gswip_switch_w(priv, BIT(priv->cpu_port), GSWIP_PCE_PMAP3);
>> +
>> + /* disable auto polling */
>> + gswip_mdio_w(priv, 0x0, GSWIP_MDIO_MDC_CFG0);
>> +
>> + /* enable special tag insertion on cpu port */
>> + gswip_switch_mask(priv, 0, GSWIP_FDMA_PCTRL_STEN,
>> + GSWIP_FDMA_PCTRLp(priv->cpu_port));
>> +
>> + gswip_switch_mask(priv, 0, GSWIP_MAC_CTRL_2_MLEN,
>> + GSWIP_MAC_CTRL_2p(priv->cpu_port));
>> + gswip_switch_w(priv, VLAN_ETH_FRAME_LEN + 8, GSWIP_MAC_FLEN);
>> + gswip_switch_mask(priv, 0, GSWIP_BM_QUEUE_GCTRL_GL_MOD,
>> + GSWIP_BM_QUEUE_GCTRL);
>> +
>> + /* VLAN aware Switching */
>> + gswip_switch_mask(priv, 0, GSWIP_PCE_GCTRL_0_VLAN, GSWIP_PCE_GCTRL_0);
>> +
>> + /* Mac Address Table Lock */
>> + gswip_switch_mask(priv, 0, GSWIP_PCE_GCTRL_1_MAC_GLOCK |
>> + GSWIP_PCE_GCTRL_1_MAC_GLOCK_MOD,
>> + GSWIP_PCE_GCTRL_1);
>> +
>> + gswip_port_enable(ds, priv->cpu_port, NULL);
>> + return 0;
>> +}
>> +
>> +static void gswip_adjust_link(struct dsa_switch *ds, int port,
>> + struct phy_device *phydev)
>> +{
>> + struct gswip_priv *priv = (struct gswip_priv *)ds->priv;
>> + u16 phyaddr = phydev->mdio.addr & GSWIP_MDIO_PHY_ADDR_MASK;
>> + u16 miirate = 0;
>> + u16 miimode;
>> + u16 lcl_adv = 0, rmt_adv = 0;
>> + u8 flowctrl;
>> +
>> + /* do not run this for the CPU port 6 */
>> + if (port >= priv->cpu_port)
>
> Please use dsa_is_cpu_port(ds, port)
done
>> + return;
>> +
>> + miimode = gswip_mdio_r(priv, GSWIP_MII_CFGp(port));
>> + miimode &= GSWIP_MII_CFG_MODE_MASK;
>> +
>> + switch (phydev->speed) {
>> + case SPEED_1000:
>> + phyaddr |= GSWIP_MDIO_PHY_SPEED_G1;
>> + miirate = GSWIP_MII_CFG_RATE_M125;
>> + break;
>> +
>> + case SPEED_100:
>> + phyaddr |= GSWIP_MDIO_PHY_SPEED_M100;
>> + switch (miimode) {
>> + case GSWIP_MII_CFG_MODE_RMIIM:
>> + case GSWIP_MII_CFG_MODE_RMIIP:
>> + miirate = GSWIP_MII_CFG_RATE_M50;
>> + break;
>> + default:
>> + miirate = GSWIP_MII_CFG_RATE_M25;
>> + break;
>> + }
>> + break;
>> +
>> + default:
>> + phyaddr |= GSWIP_MDIO_PHY_SPEED_M10;
>> + miirate = GSWIP_MII_CFG_RATE_M2P5;
>> + break;
>> + }
>> +
>> + if (phydev->link)
>> + phyaddr |= GSWIP_MDIO_PHY_LINK_UP;
>> + else
>> + phyaddr |= GSWIP_MDIO_PHY_LINK_DOWN;
>> +
>> + if (phydev->duplex == DUPLEX_FULL)
>> + phyaddr |= GSWIP_MDIO_PHY_FDUP_EN;
>> + else
>> + phyaddr |= GSWIP_MDIO_PHY_FDUP_DIS;
>> +
>> + if (phydev->pause)
>> + rmt_adv = LPA_PAUSE_CAP;
>> + if (phydev->asym_pause)
>> + rmt_adv |= LPA_PAUSE_ASYM;
>> +
>> + if (phydev->advertising & ADVERTISED_Pause)
>> + lcl_adv |= ADVERTISE_PAUSE_CAP;
>> + if (phydev->advertising & ADVERTISED_Asym_Pause)
>> + lcl_adv |= ADVERTISE_PAUSE_ASYM;
>> +
>> + flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
>> +
>> + if (flowctrl & FLOW_CTRL_TX)
>> + phyaddr |= GSWIP_MDIO_PHY_FCONTX_EN;
>> + else
>> + phyaddr |= GSWIP_MDIO_PHY_FCONTX_DIS;
>> + if (flowctrl & FLOW_CTRL_RX)
>> + phyaddr |= GSWIP_MDIO_PHY_FCONRX_EN;
>> + else
>> + phyaddr |= GSWIP_MDIO_PHY_FCONRX_DIS;
>> +
>> + gswip_mdio_mask(priv, GSWIP_MDIO_PHY_MASK, phyaddr,
>> + GSWIP_MDIO_PHYp(port));
>
> The names make this unclear. The callback is used to configure the MAC
> layer when something happens at the PHY layer. phyaddr does not appear
> to be an address, not should it be doing anything to a PHY.
I renamed this to phyconf, as this contains multiple configuration
values. This tells the mac what settings the phy wants to use.
>
>> + gswip_mii_mask(priv, GSWIP_MII_CFG_RATE_MASK, miirate,
>> + GSWIP_MII_CFGp(port));
>> +}
>> +
>> +static u32 gswip_bcm_ram_entry_read(struct gswip_priv *priv, u32 table,
>> + u32 index)
>> +{
>> + u32 result;
>> +
>> + gswip_switch_w(priv, index, GSWIP_BM_RAM_ADDR);
>> + gswip_switch_mask(priv, GSWIP_BM_RAM_CTRL_ADDR_MASK |
>> + GSWIP_BM_RAM_CTRL_OPMOD,
>> + table | GSWIP_BM_RAM_CTRL_BAS,
>> + GSWIP_BM_RAM_CTRL);
>> +
>> + while (gswip_switch_r(priv, GSWIP_BM_RAM_CTRL) & GSWIP_BM_RAM_CTRL_BAS)
>> + cond_resched();
>
> Please add a timeout.
I introduced a new funtion gswip_switch_r_timeout() which uses
readx_poll_timeout().
>> +
>> + result = gswip_switch_r(priv, GSWIP_BM_RAM_VAL(0));
>> + result |= gswip_switch_r(priv, GSWIP_BM_RAM_VAL(1)) << 16;
>> +
>> + return result;
>> +}
>> +
>
>> +static int gswip_probe(struct platform_device *pdev)
>> +{
>> + struct gswip_priv *priv;
>> + struct resource *gswip_res, *mdio_res, *mii_res;
>> + struct device_node *mdio_np;
>> + struct device *dev = &pdev->dev;
>> + int err;
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + gswip_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + priv->gswip = devm_ioremap_resource(dev, gswip_res);
>> + if (!priv->gswip)
>> + return -ENOMEM;
>> +
>> + mdio_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + priv->mdio = devm_ioremap_resource(dev, mdio_res);
>> + if (!priv->mdio)
>> + return -ENOMEM;
>> +
>> + mii_res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> + priv->mii = devm_ioremap_resource(dev, mii_res);
>> + if (!priv->mii)
>> + return -ENOMEM;
>> +
>> + priv->ds = dsa_switch_alloc(dev, DSA_MAX_PORTS);
>
> If you know how many ports there are, it is better to pass it.
done
>> + if (!priv->ds)
>> + return -ENOMEM;
>> +
>> + priv->ds->priv = priv;
>> + priv->ds->ops = &gswip_switch_ops;
>> + priv->dev = dev;
>> + priv->cpu_port = 6;
>> +
>> + /* bring up the mdio bus */
>> + mdio_np = of_find_compatible_node(pdev->dev.of_node, NULL,
>> + "lantiq,xrx200-mdio");
>> + if (mdio_np) {
>> + err = gswip_mdio(priv, mdio_np);
>> + if (err) {
>> + dev_err(dev, "mdio probe failed\n");
>> + return err;
>> + }
>> + }
>> +
>> + platform_set_drvdata(pdev, priv);
>> +
>> + err = dsa_register_switch(priv->ds);
>> + if (err) {
>> + dev_err(dev, "dsa switch register failed: %i\n", err);
>> + if (mdio_np)
>> + mdiobus_unregister(priv->ds->slave_mii_bus);
>> + }
>> +
>> + return err;
>> +}
>
>> +static const struct of_device_id gswip_of_match[] = {
>> + { .compatible = "lantiq,xrx200-gswip" },
>> + {},
>> +};
>
> Please add device tree documentation.
Will do that.
>
>> +MODULE_DEVICE_TABLE(of, xrx200_match);
>> +
>> +static struct platform_driver gswip_driver = {
>> + .probe = gswip_probe,
>> + .remove = gswip_remove,
>> + .driver = {
>> + .name = "gswip",
>> + .of_match_table = gswip_of_match,
>> + },
>> +};
>> +#define MC_ENTRY(val, msk, ns, out, len, type, flags, ipv4_len) \
>> + { val, msk, (ns << 10 | out << 4 | len >> 1),\
>> + (len & 1) << 15 | type << 13 | flags << 9 | ipv4_len << 8 }
>> +static const struct gswip_pce_microcode gswip_pce_microcode[] = {
>
> How big is this table? I'm wondering if it should be loaded from
> /lib/firmware. Or can it be marked __initdata?
This is sort of a firmware, but it is also in the GPL driver.
Currently the probe function is not marked __init so we can not make
this easily __initdata.
It has 64 entries of 8 bytes each so, 512 bytes, I think we can put this
into the code.
Hauke
^ permalink raw reply
* Re: [PATCH 3/4] net: lantiq: Add Lantiq / Intel vrx200 Ethernet driver
From: Andrew Lunn @ 2018-07-29 16:40 UTC (permalink / raw)
To: Hauke Mehrtens
Cc: davem, netdev, vivien.didelot, f.fainelli, john, linux-mips, dev,
hauke.mehrtens
In-Reply-To: <18f8bbd5-0623-7bed-c96a-c7b10f1e2cd2@hauke-m.de>
> I am thinking about merging this into the switch driver, then we do not
> have to configure the dependency any more.
Hi Hauke
Are there any PHYs which are not part of the switch?
Making it part of the switch driver would make sense. Are there any
backwards compatibility issues? I don't actually see any boards in
mailine using the compatible strings.
Another option would be to write an independent mdio driver, and make
firmware download part of that. That gives the advantage of supporting
PHYs which are not part of the switch.
Andrew
^ permalink raw reply
* Re: [PATCH 4/4] net: dsa: Add Lantiq / Intel DSA driver for vrx200
From: Andrew Lunn @ 2018-07-29 16:49 UTC (permalink / raw)
To: Hauke Mehrtens
Cc: davem, netdev, vivien.didelot, f.fainelli, john, linux-mips, dev,
hauke.mehrtens
In-Reply-To: <df047f7a-de85-372a-7bce-1f78839f98fe@hauke-m.de>
> >> +static const struct gswip_rmon_cnt_desc gswip_rmon_cnt[] = {
> >> + /** Receive Packet Count (only packets that are accepted and not discarded). */
> >> + MIB_DESC(1, 0x1F, "RxGoodPkts"),
> >> + /** Receive Size 1024-1522 (or more, if configured) Packet Count. */
> >> + MIB_DESC(1, 0x17, "RxMaxBytePkts"),
> >> + /** Transmit Size 1024-1522 (or more, if configured) Packet Count. */
> >> + MIB_DESC(1, 0x05, "TxMaxBytePkts"),
> >
> > Most of the comments here don't add anything useful. Maybe remove
> > them?
>
> Ok I removed them.
The comments i left above are useful, since they give additional
information which is not obvious from the name.
> Are the names ok, or should they follow any Linux definition?
There are no standard names. So each driver tends to be different.
> > Please return ETIMEOUT when needed. Maybe use one of the variants of
> > readx_poll_timeout().
>
> I am returning ETIMEOUT now.
>
> When I would use readx_poll_timeout() I can not use the gswip_mdio_r()
> function, because it takes two arguments and would have to use readl
> directly.
Yes, they don't always fit, which is why is said "maybe".
> > The names make this unclear. The callback is used to configure the MAC
> > layer when something happens at the PHY layer. phyaddr does not appear
> > to be an address, not should it be doing anything to a PHY.
>
> I renamed this to phyconf, as this contains multiple configuration
> values. This tells the mac what settings the phy wants to use.
macconf might be better, since this is configuring the MAC, not the
PHY.
> This is sort of a firmware, but it is also in the GPL driver.
> Currently the probe function is not marked __init so we can not make
> this easily __initdata.
> It has 64 entries of 8 bytes each so, 512 bytes, I think we can put this
> into the code.
512 bytes is fine.
Andrew
^ permalink raw reply
* Re: [PATCH bpf] tools/bpftool: fix a percpu_array map dump problem
From: Yonghong Song @ 2018-07-29 17:16 UTC (permalink / raw)
To: Daniel Borkmann, ast, netdev; +Cc: kernel-team
In-Reply-To: <98bd07f3-a132-5477-3108-ac4e0114cd1c@iogearbox.net>
On 7/28/18 12:14 PM, Daniel Borkmann wrote:
> On 07/28/2018 01:11 AM, Yonghong Song wrote:
>> I hit the following problem when I tried to use bpftool
>> to dump a percpu array.
>>
>> $ sudo ./bpftool map show
>> 61: percpu_array name stub flags 0x0
>> key 4B value 4B max_entries 1 memlock 4096B
>> ...
>> $ sudo ./bpftool map dump id 61
>> bpftool: malloc.c:2406: sysmalloc: Assertion
>> `(old_top == initial_top (av) && old_size == 0) || \
>> ((unsigned long) (old_size) >= MINSIZE && \
>> prev_inuse (old_top) && \
>> ((unsigned long) old_end & (pagesize - 1)) == 0)'
>> failed.
>> Aborted
>>
>> Further debugging revealed that this is due to
>> miscommunication between bpftool and kernel.
>> For example, for the above percpu_array with value size of 4B.
>> The map info returned to user space has value size of 4B.
>>
>> In bpftool, the values array for lookup is allocated like:
>> info->value_size * get_possible_cpus() = 4 * get_possible_cpus()
>> In kernel (kernel/bpf/syscall.c), the values array size is
>> rounded up to multiple of 8.
>> round_up(map->value_size, 8) * num_possible_cpus()
>> = 8 * num_possible_cpus()
>> So when kernel copies the values to user buffer, the kernel will
>> overwrite beyond user buffer boundary.
>>
>> This patch fixed the issue by allocating and stepping through
>> percpu map value array properly in bpftool.
>>
>> Fixes: 71bb428fe2c19 ("tools: bpf: add bpftool")
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>> tools/bpf/bpftool/map.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
>> index 0ee3ba479d87..92bc55f98c4c 100644
>> --- a/tools/bpf/bpftool/map.c
>> +++ b/tools/bpf/bpftool/map.c
>> @@ -35,6 +35,7 @@
>> #include <errno.h>
>> #include <fcntl.h>
>> #include <linux/err.h>
>> +#include <linux/kernel.h>
>> #include <stdbool.h>
>> #include <stdio.h>
>> #include <stdlib.h>
>> @@ -91,7 +92,8 @@ static bool map_is_map_of_progs(__u32 type)
>> static void *alloc_value(struct bpf_map_info *info)
>> {
>> if (map_is_per_cpu(info->type))
>> - return malloc(info->value_size * get_possible_cpus());
>> + return malloc(round_up(info->value_size, 8) *
>> + get_possible_cpus());
>> else
>> return malloc(info->value_size);
>> }
>> @@ -273,9 +275,10 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>> do_dump_btf(&d, info, key, value);
>> }
>> } else {
>> - unsigned int i, n;
>> + unsigned int i, n, step;
>>
>> n = get_possible_cpus();
>> + step = round_up(info->value_size, 8);
>>
>> jsonw_name(json_wtr, "key");
>> print_hex_data_json(key, info->key_size);
>> @@ -288,7 +291,7 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
>> jsonw_int_field(json_wtr, "cpu", i);
>>
>> jsonw_name(json_wtr, "value");
>> - print_hex_data_json(value + i * info->value_size,
>> + print_hex_data_json(value + i * step,
>> info->value_size);
>>
>> jsonw_end_object(json_wtr);
>
> Fix looks correct to me, but you would also need the same fix in print_entry_plain(), no?
Thanks for pointing this out. will submit v2 soon to fix the issue.
>
> Thanks,
> Daniel
>
^ permalink raw reply
* [PATCH bpf v2] tools/bpftool: fix a percpu_array map dump problem
From: Yonghong Song @ 2018-07-29 17:20 UTC (permalink / raw)
To: ast, daniel, netdev; +Cc: kernel-team
I hit the following problem when I tried to use bpftool
to dump a percpu array.
$ sudo ./bpftool map show
61: percpu_array name stub flags 0x0
key 4B value 4B max_entries 1 memlock 4096B
...
$ sudo ./bpftool map dump id 61
bpftool: malloc.c:2406: sysmalloc: Assertion
`(old_top == initial_top (av) && old_size == 0) || \
((unsigned long) (old_size) >= MINSIZE && \
prev_inuse (old_top) && \
((unsigned long) old_end & (pagesize - 1)) == 0)'
failed.
Aborted
Further debugging revealed that this is due to
miscommunication between bpftool and kernel.
For example, for the above percpu_array with value size of 4B.
The map info returned to user space has value size of 4B.
In bpftool, the values array for lookup is allocated like:
info->value_size * get_possible_cpus() = 4 * get_possible_cpus()
In kernel (kernel/bpf/syscall.c), the values array size is
rounded up to multiple of 8.
round_up(map->value_size, 8) * num_possible_cpus()
= 8 * num_possible_cpus()
So when kernel copies the values to user buffer, the kernel will
overwrite beyond user buffer boundary.
This patch fixed the issue by allocating and stepping through
percpu map value array properly in bpftool.
Fixes: 71bb428fe2c19 ("tools: bpf: add bpftool")
Signed-off-by: Yonghong Song <yhs@fb.com>
---
tools/bpf/bpftool/map.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
Changelogs:
v1 -> v2:
. Added missing fix in function print_entry_plain().
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index 0ee3ba479d87..0a63842e9cb4 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -35,6 +35,7 @@
#include <errno.h>
#include <fcntl.h>
#include <linux/err.h>
+#include <linux/kernel.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
@@ -91,7 +92,8 @@ static bool map_is_map_of_progs(__u32 type)
static void *alloc_value(struct bpf_map_info *info)
{
if (map_is_per_cpu(info->type))
- return malloc(info->value_size * get_possible_cpus());
+ return malloc(round_up(info->value_size, 8) *
+ get_possible_cpus());
else
return malloc(info->value_size);
}
@@ -273,9 +275,10 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
do_dump_btf(&d, info, key, value);
}
} else {
- unsigned int i, n;
+ unsigned int i, n, step;
n = get_possible_cpus();
+ step = round_up(info->value_size, 8);
jsonw_name(json_wtr, "key");
print_hex_data_json(key, info->key_size);
@@ -288,7 +291,7 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
jsonw_int_field(json_wtr, "cpu", i);
jsonw_name(json_wtr, "value");
- print_hex_data_json(value + i * info->value_size,
+ print_hex_data_json(value + i * step,
info->value_size);
jsonw_end_object(json_wtr);
@@ -319,9 +322,10 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
printf("\n");
} else {
- unsigned int i, n;
+ unsigned int i, n, step;
n = get_possible_cpus();
+ step = round_up(info->value_size, 8);
printf("key:\n");
fprint_hex(stdout, key, info->key_size, " ");
@@ -329,7 +333,7 @@ static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
for (i = 0; i < n; i++) {
printf("value (CPU %02d):%c",
i, info->value_size > 16 ? '\n' : ' ');
- fprint_hex(stdout, value + i * info->value_size,
+ fprint_hex(stdout, value + i * step,
info->value_size, " ");
printf("\n");
}
--
2.14.3
^ permalink raw reply related
* Re: [PATCH 3/4] net: lantiq: Add Lantiq / Intel vrx200 Ethernet driver
From: Hauke Mehrtens @ 2018-07-29 17:44 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem, netdev, vivien.didelot, f.fainelli, john, linux-mips, dev,
hauke.mehrtens
In-Reply-To: <20180729164052.GA14420@lunn.ch>
On 07/29/2018 06:40 PM, Andrew Lunn wrote:
>> I am thinking about merging this into the switch driver, then we do not
>> have to configure the dependency any more.
>
> Hi Hauke
>
> Are there any PHYs which are not part of the switch?
The embedded PHYs are only connected to the switch in this SoC and on
all other SoCs from this line I am aware of.
> Making it part of the switch driver would make sense. Are there any
> backwards compatibility issues? I don't actually see any boards in
> mailine using the compatible strings.
There is currently no device tree file added for any board in mainline.
I would then prefer to add this to the switch driver.
I have to make sure the firmware gets loaded before we scan the MDIO
bus. When no FW is loaded they do not get detected.
More recent SoC have more embedded Ethernet PHYs so I would like to
support a variable number of these PHYs.
The firmware is 64KBytes big and we have to load that into continuous
memory which is then used by the PHY itself. When we are late in the
boot process we could run into memory problems, most devices have 64MB
or 128MB of RAM.
How should the device tree binding should look like?
Should I create an extra sub node:
gswip: gswip@E108000 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "lantiq,xrx200-gswip";
reg = < 0xE108000 0x3000 /* switch */
0xE10B100 0x70 /* mdio */
0xE10B1D8 0x30 /* mii */
>;
dsa,member = <0 0>;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
label = "lan3";
phy-mode = "rgmii";
phy-handle = <&phy0>;
};
....
};
mdio@0 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "lantiq,xrx200-mdio";
reg = <0>;
phy0: ethernet-phy@0 {
reg = <0x0>;
};
....
};
# this would be the new part
phys {
gphy0: gphy@20 {
compatible = "lantiq,xrx200a2x-gphy";
reg = <0x20 0x4>;
rcu = <&rcu0>;
resets = <&reset0 31 30>, <&reset1 7 7>;
reset-names = "gphy", "gphy2";
clocks = <&pmu0 XRX200_PMU_GATE_GPHY>;
lantiq,gphy-mode = <GPHY_MODE_GE>;
};
....
};
};
> Another option would be to write an independent mdio driver, and make
> firmware download part of that. That gives the advantage of supporting
> PHYs which are not part of the switch.
>
> Andrew
>
^ permalink raw reply
* Re: [PATCH] NET: stmmac: align DMA stuff to largest cache line length
From: David Miller @ 2018-07-29 19:35 UTC (permalink / raw)
To: Eugeniy.Paltsev
Cc: netdev, linux-kernel, linux-snps-arc, Jose.Abreu,
alexandre.torgue, peppe.cavallaro
In-Reply-To: <20180726120537.4664-1-Eugeniy.Paltsev@synopsys.com>
From: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
Date: Thu, 26 Jul 2018 15:05:37 +0300
> As for today STMMAC_ALIGN macro (which is used to align DMA stuff)
> relies on L1 line length (L1_CACHE_BYTES).
> This isn't correct in case of system with several cache levels
> which might have L1 cache line length smaller than L2 line. This
> can lead to sharing one cache line between DMA buffer and other
> data, so we can lose this data while invalidate DMA buffer before
> DMA transaction.
>
> Fix that by using SMP_CACHE_BYTES instead of L1_CACHE_BYTES for
> aligning.
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
This is definitely an improvement, so applied and queued up for
-stable.
There is also dma_get_cache_alignment(), so maybe we can eventually
use that here instead.
^ permalink raw reply
* Re: [PATCH 3/4] net: lantiq: Add Lantiq / Intel vrx200 Ethernet driver
From: Andrew Lunn @ 2018-07-29 18:10 UTC (permalink / raw)
To: Hauke Mehrtens
Cc: davem, netdev, vivien.didelot, f.fainelli, john, linux-mips, dev,
hauke.mehrtens
In-Reply-To: <7c866f1b-70f3-8221-debd-be1bd0b6f7dd@hauke-m.de>
> The embedded PHYs are only connected to the switch in this SoC and on
> all other SoCs from this line I am aware of.
Hi Hauke
O.K, then it makes sense to have it part of the switch driver.
> The firmware is 64KBytes big and we have to load that into continuous
> memory which is then used by the PHY itself. When we are late in the
> boot process we could run into memory problems, most devices have 64MB
> or 128MB of RAM.
You might want to look at using CMA. I've never used it myself, so
cannot help much.
> How should the device tree binding should look like?
>
> Should I create an extra sub node:
>
> gswip: gswip@E108000 {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "lantiq,xrx200-gswip";
> reg = < 0xE108000 0x3000 /* switch */
> 0xE10B100 0x70 /* mdio */
> 0xE10B1D8 0x30 /* mii */
> >;
> dsa,member = <0 0>;
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@0 {
> reg = <0>;
> label = "lan3";
> phy-mode = "rgmii";
> phy-handle = <&phy0>;
> };
> ....
> };
>
> mdio@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "lantiq,xrx200-mdio";
> reg = <0>;
>
> phy0: ethernet-phy@0 {
> reg = <0x0>;
> };
> ....
> };
>
> # this would be the new part
> phys {
> gphy0: gphy@20 {
> compatible = "lantiq,xrx200a2x-gphy";
It would be good to make it clear this is for firmware download. So
scatter "firmware" or "fw" in some of these names. What we don't want
is a mix up with phy's within the mdio subtree. Otherwise this looks
good. But you should cross post the device tree binding to the device
tree mailing list.
Andrew
^ permalink raw reply
* Re: [PATCH rdma] IB/cache: Restore compatibility for ib_query_gid
From: Jason Gunthorpe @ 2018-07-29 19:42 UTC (permalink / raw)
To: linux-rdma, Stephen Rothwell
Cc: David Miller, Networking, Doug Ledford, Linux-Next Mailing List,
Linux Kernel Mailing List, Parav Pandit, Ursula Braun,
Leon Romanovsky
In-Reply-To: <20180727155402.GA15253@ziepe.ca>
On Fri, Jul 27, 2018 at 09:54:02AM -0600, Jason Gunthorpe wrote:
> Code changes in smc have become so complicated this cycle that the RDMA
> patches to remove ib_query_gid in smc create too complex merge conflicts.
> Allow those conflicts to be resolved by using the net/smc hunks by
> providing a compatibility wrapper. During the second phase of the merge
> window this wrapper will be deleted and smc updated to use the new API.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> include/rdma/ib_cache.h | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> The resolution Stephen had to make is too complicated, I think we
> should go this way instead. Parav can send a patch to DaveM during the
> 2nd half of the merge window to safely update SMC and delete this
> wrapper.
>
> Parav, can you check this?
>
> Ursula, this is only for SMC, are you able to test SMC with it? You
> will need to apply it to the RDMA tree here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=for-next
>
> Thanks,
> Jason
I applied this to rdma for-next with Parav's notes.
Jason
^ permalink raw reply
* Re: [PATCH] netlink: Do not subscribe to non-existent groups
From: David Miller @ 2018-07-29 19:51 UTC (permalink / raw)
To: dima; +Cc: linux-kernel, herbert, steffen.klassert, netdev
In-Reply-To: <20180727155444.17081-1-dima@arista.com>
From: Dmitry Safonov <dima@arista.com>
Date: Fri, 27 Jul 2018 16:54:44 +0100
> Make ABI more strict about subscribing to group > ngroups.
> Code doesn't check for that and it looks bogus.
> (one can subscribe to non-existing group)
> Still, it's possible to bind() to all possible groups with (-1)
>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Dmitry Safonov <dima@arista.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: ethernet: ti: cpsw: add missed RX_CTAG feature for second slave
From: David Miller @ 2018-07-29 19:52 UTC (permalink / raw)
To: ivan.khoronzhuk; +Cc: grygorii.strashko, linux-omap, netdev, linux-kernel
In-Reply-To: <20180727165439.16933-1-ivan.khoronzhuk@linaro.org>
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Fri, 27 Jul 2018 19:54:39 +0300
> Seems it was missed while adding for first net dev in dual-emac mode.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Applied to net-next.
^ permalink raw reply
* Re: [PATCH net-next] sysfs: Fix regression when adding a file to an existing group
From: David Miller @ 2018-07-29 20:11 UTC (permalink / raw)
To: tyhicks; +Cc: gregkh, hkallweit1, dmitry.torokhov, linux-kernel, netdev
In-Reply-To: <1532727207-10912-1-git-send-email-tyhicks@canonical.com>
From: Tyler Hicks <tyhicks@canonical.com>
Date: Fri, 27 Jul 2018 21:33:27 +0000
> Commit 5f81880d5204 ("sysfs, kobject: allow creating kobject belonging
> to arbitrary users") incorrectly changed the argument passed as the
> parent parameter when calling sysfs_add_file_mode_ns(). This caused some
> sysfs attribute files to not be added correctly to certain groups.
>
> Fixes: 5f81880d5204 ("sysfs, kobject: allow creating kobject belonging to arbitrary users")
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> Reported-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>
> Note, this was a regression exclusively in linux-next and never made it to
> Linus' tree. There's no need to queue it up for linux-stable.
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net-next 1/2] net-next: mediatek: improve more with using dma_zalloc_coherent
From: David Miller @ 2018-07-29 20:16 UTC (permalink / raw)
To: sean.wang; +Cc: john, nbd, netdev, linux-kernel, linux-mediatek
In-Reply-To: <658dfd485c035cdae5452bc5d572125076b47a48.1532755541.git.sean.wang@mediatek.com>
From: <sean.wang@mediatek.com>
Date: Sat, 28 Jul 2018 13:35:55 +0800
> From: Sean Wang <sean.wang@mediatek.com>
>
> Improve more in the existing code by reusing dma_zalloc_coherent instead
> of dma_alloc_coherent with __GFP_ZERO or superfluous zeroing buffer.
>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Applied.
^ permalink raw reply
* Re: [PATCH] net: ethernet: realtek: r8169: Add spaces after commas coding style issue
From: David Miller @ 2018-07-29 20:17 UTC (permalink / raw)
To: supertakumi86; +Cc: nic_swsd, netdev, linux-kernel
In-Reply-To: <20180728090217.GA19065@zenus>
From: Sohil Ladhani <supertakumi86@gmail.com>
Date: Sat, 28 Jul 2018 14:32:21 +0530
> Fixed a coding style issue
>
> Signed-off-by: Sohil Ladhani <supertakumi86@gmail.com>
This doesn't apply to the current net-next tree.
^ permalink raw reply
* Re: [PATCH net-next] qed: remove redundant functions qed_get_cm_pq_idx_rl
From: David Miller @ 2018-07-29 20:18 UTC (permalink / raw)
To: yuehaibing; +Cc: Ariel.Elior, everest-linux-l2, linux-kernel, netdev
In-Reply-To: <20180728102336.16956-1-yuehaibing@huawei.com>
From: YueHaibing <yuehaibing@huawei.com>
Date: Sat, 28 Jul 2018 18:23:36 +0800
> There are no in-tree callers of qed_get_cm_pq_idx_rl since it be there,
> so it can be removed.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied.
^ permalink raw reply
* Re: [net-next v1] net/ipv6: allow any source address for sendmsg pktinfo with ip_nonlocal_bind
From: David Miller @ 2018-07-29 19:28 UTC (permalink / raw)
To: vincent; +Cc: kuznet, yoshfuji, netdev, tom
In-Reply-To: <20180725111914.28093-1-vincent@bernat.im>
From: Vincent Bernat <vincent@bernat.im>
Date: Wed, 25 Jul 2018 13:19:13 +0200
> When freebind feature is set of an IPv6 socket, any source address can
> be used when sending UDP datagrams using IPv6 PKTINFO ancillary
> message. Global non-local bind feature was added in commit
> 35a256fee52c ("ipv6: Nonlocal bind") for IPv6. This commit also allows
> IPv6 source address spoofing when non-local bind feature is enabled.
>
> Signed-off-by: Vincent Bernat <vincent@bernat.im>
This definitely seems to make sense. And is consistent with the other
tests involving freebind and transparent.
This test involving ip_nonlocal_bind, freeebind, and transparent happens
in several locations. Perhaps we should add a helper function for this?
Thanks.
^ permalink raw reply
* Re: [PATCHv4 net-next 0/2] route: add support and selftests for directed broadcast forwarding
From: David Miller @ 2018-07-29 19:37 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, dsahern, dcaratti, idosch
In-Reply-To: <cover.1532680283.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Fri, 27 Jul 2018 16:37:27 +0800
> Patch 1/2 is the feature and 2/2 is the selftest. Check the changelog
> on each of them to know the details.
>
> v1->v2:
> - fix a typo in changelog.
> - fix an uapi break that Davide noticed.
> - flush route cache when bc_forwarding is changed.
> - add the selftest for this patch as Ido's suggestion.
>
> v2->v3:
> - fix an incorrect 'if check' in devinet_conf_proc as David Ahern
> noticed.
> - extend the selftest after one David Ahern fix for vrf.
>
> v3->v4:
> - improve the output log in the selftest as David Ahern suggested.
Series applied, thanks Xin.
^ permalink raw reply
* Re: [PATCH net-next] net: dcb: add DSCP to comment about priority selector types
From: David Miller @ 2018-07-29 19:54 UTC (permalink / raw)
To: jakub.kicinski
Cc: idosch, petrm, huyn, john.fastabend, mlxsw, jiri, netdev,
oss-drivers
In-Reply-To: <20180727201100.26891-1-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Fri, 27 Jul 2018 13:11:00 -0700
> Commit ee2059819450 ("net/dcb: Add dscp to priority selector type")
> added a define for the new DSCP selector type created by
> IEEE 802.1Qcd, but missed the comment enumerating all selector types.
> Update the comment.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 0/3] mtu related changes
From: David Miller @ 2018-07-29 19:57 UTC (permalink / raw)
To: stephen; +Cc: netdev, sthemmin
In-Reply-To: <20180727204323.19408-1-sthemmin@microsoft.com>
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Fri, 27 Jul 2018 13:43:20 -0700
> While looking at other MTU issues, noticed a couple oppurtunties
> for improving user experience.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] selftests: mlxsw: qos_dscp_bridge: Fix
From: David Miller @ 2018-07-29 20:00 UTC (permalink / raw)
To: petrm; +Cc: netdev, linux-kselftest, jiri, idosch, shuah
In-Reply-To: <0265c1caa420b594b88f2b43ca0cd8df100aa627.1532731316.git.petrm@mellanox.com>
From: Petr Machata <petrm@mellanox.com>
Date: Sat, 28 Jul 2018 00:48:13 +0200
> There are two problems in this test case:
>
> - When indexing in bash associative array, the subscript is interpreted as
> string, not as a variable name to be expanded.
>
> - The keys stored to t0s and t1s are not DSCP values, but priority +
> base (i.e. the logical DSCP value, not the full bitfield value).
>
> In combination these two bugs conspire to make the test just work,
> except it doesn't really test anything and always passes.
>
> Fix the above two problems in obvious manner.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH 1/5] net: remove bogus RCU annotations on socket.wq
From: David Miller @ 2018-07-29 20:05 UTC (permalink / raw)
To: hch; +Cc: netdev
In-Reply-To: <20180727140214.1938-2-hch@lst.de>
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 27 Jul 2018 16:02:10 +0200
> We never use RCU protection for it, just a lot of cargo-cult
> rcu_deference_protects calls.
>
> Note that we do keep the kfree_rcu call for it, as the references through
> struct sock are RCU protected and thus might require a grace period before
> freeing.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
These were added by Eric Dumazet and I would never accuse him of cargo
cult programming.
All of the rcu_dereference_protects() calls are legit, even though some
of them use '1' as the protects condition because in fact we know the
object is dead and gone through an RCU cycle at that point.
Let's skip this for now. The rest of your series looks fine so why
don't you resubmit this series with just #2-#5?
Thanks.
^ permalink raw reply
* Re: [pull request][net-next V2 00/12] Mellanox, mlx5 updates 2018-07-27 (Vxlan updates)
From: David Miller @ 2018-07-29 20:09 UTC (permalink / raw)
To: saeedm; +Cc: netdev
In-Reply-To: <20180728000622.4123-1-saeedm@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Fri, 27 Jul 2018 17:06:10 -0700
> This series from Gal and Saeed provides updates to mlx5 vxlan implementation.
>
> For more information please see tag log below.
>
> Please pull and let me know if there's any problem.
>
> V1->V2:
> - Drop the rw lock patch.
Looks good, pulled, thank you!
^ permalink raw reply
* Re: [PATCH net-next 2/2] net-next: mediatek: cleanup unnecessary get chip id and its user
From: David Miller @ 2018-07-29 20:16 UTC (permalink / raw)
To: sean.wang; +Cc: john, nbd, netdev, linux-kernel, linux-mediatek
In-Reply-To: <d4fa77f13190aefa5cf3f609a3ae555aeef38259.1532755541.git.sean.wang@mediatek.com>
From: <sean.wang@mediatek.com>
Date: Sat, 28 Jul 2018 13:35:56 +0800
> From: Sean Wang <sean.wang@mediatek.com>
>
> Since driver is devicetree-based, all device type and charateristic can be
> determined by the compatible string and its data. It's unnecessary to
> create another dependent function to check chip ID and then decide whether
> the specific funciton is being supported on a certain device. It can be
> totally replaced by the existing flag, so a cleanup is made by removing
> the function and the only user, HWLRO.
>
> MT2701 also have a missing HWLRO support in old code, so add it the same
> patch.
>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: force_napi_tx module param.
From: Michael S. Tsirkin @ 2018-07-29 20:33 UTC (permalink / raw)
To: David Miller; +Cc: caleb.raitto, jasowang, netdev, caraitto
In-Reply-To: <20180729.090027.1373538625446665385.davem@davemloft.net>
On Sun, Jul 29, 2018 at 09:00:27AM -0700, David Miller wrote:
> From: Caleb Raitto <caleb.raitto@gmail.com>
> Date: Mon, 23 Jul 2018 16:11:19 -0700
>
> > From: Caleb Raitto <caraitto@google.com>
> >
> > The driver disables tx napi if it's not certain that completions will
> > be processed affine with tx service.
> >
> > Its heuristic doesn't account for some scenarios where it is, such as
> > when the queue pair count matches the core but not hyperthread count.
> >
> > Allow userspace to override the heuristic. This is an alternative
> > solution to that in the linked patch. That added more logic in the
> > kernel for these cases, but the agreement was that this was better left
> > to user control.
> >
> > Do not expand the existing napi_tx variable to a ternary value,
> > because doing so can break user applications that expect
> > boolean ('Y'/'N') instead of integer output. Add a new param instead.
> >
> > Link: https://patchwork.ozlabs.org/patch/725249/
> > Acked-by: Willem de Bruijn <willemb@google.com>
> > Acked-by: Jon Olson <jonolson@google.com>
> > Signed-off-by: Caleb Raitto <caraitto@google.com>
>
> So I looked into the history surrounding these issues.
>
> First of all, it's always ends up turning out crummy when drivers start
> to set affinities themselves. The worst possible case is to do it
> _conditionally_, and that is exactly what virtio_net is doing.
>
> >From the user's perspective, this provides a really bad experience.
>
> So if I have a 32-queue device and there are 32 cpus, you'll do all
> the affinity settings, stopping Irqbalanced from doing anything
> right?
>
> So if I add one more cpu, you'll say "oops, no idea what to do in
> this situation" and not touch the affinities at all?
>
> That makes no sense at all.
>
> If the driver is going to set affinities at all, OWN that decision
> and set it all the time to something reasonable.
>
> Or accept that you shouldn't be touching this stuff in the first place
> and leave the affinities alone.
>
> Right now we're kinda in a situation where the driver has been setting
> affinities in the ncpus==nqueues cases for some time, so we can't stop
> doing it.
>
> Which means we have to set them in all cases to make the user
> experience sane again.
>
> I looked at the linked to patch again:
>
> https://patchwork.ozlabs.org/patch/725249/
>
> And I think the strategy should be made more generic, to get rid of
> the hyperthreading assumptions. I also agree that the "assign
> to first N cpus" logic doesn't make much sense either.
>
> Just distribute across the available cpus evenly, and be done with it.
> If you have 64 cpus and 32 queues, this assigns queues to every other
> cpu.
>
> Then we don't need this weird new module parameter.
Can't we set affinity to a set of CPUs?
The point really is that tx irq handler needs a lock on the tx queue to
free up skbs, so processing it on another CPU while tx is active causes
cache line bounces. So we want affinity to CPUs that submit to this
queue on the theory they have these cache line(s) anyway.
I suspect it's not a uniqueue property of virtio.
--
MST
^ 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