* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Andreas Schwab @ 2018-11-19 16:32 UTC (permalink / raw)
To: Andrew Lunn
Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
linux-riscv
In-Reply-To: <20181119162529.GA31955@lunn.ch>
On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> Lock debugging: prove locking correctness
404
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply
* [PATCH 4.18 088/171] bonding/802.3ad: fix link_failure_count tracking
From: Greg Kroah-Hartman @ 2018-11-19 16:28 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Mahesh Bandewar, David S. Miller,
netdev, Jarod Wilson
In-Reply-To: <20181119162618.909354448@linuxfoundation.org>
4.18-stable review patch. If anyone has any objections, please let me know.
------------------
From: Jarod Wilson <jarod@redhat.com>
commit ea53abfab960909d622ca37bcfb8e1c5378d21cc upstream.
Commit 4d2c0cda07448ea6980f00102dc3964eb25e241c set slave->link to
BOND_LINK_DOWN for 802.3ad bonds whenever invalid speed/duplex values
were read, to fix a problem with slaves getting into weird states, but
in the process, broke tracking of link failures, as going straight to
BOND_LINK_DOWN when a link is indeed down (cable pulled, switch rebooted)
means we broke out of bond_miimon_inspect()'s BOND_LINK_DOWN case because
!link_state was already true, we never incremented commit, and never got
a chance to call bond_miimon_commit(), where slave->link_failure_count
would be incremented. I believe the simple fix here is to mark the slave
as BOND_LINK_FAIL, and let bond_miimon_inspect() transition the link from
_FAIL to either _UP or _DOWN, and in the latter case, we now get proper
incrementing of link_failure_count again.
Fixes: 4d2c0cda0744 ("bonding: speed/duplex update at NETDEV_UP event")
CC: Mahesh Bandewar <maheshb@google.com>
CC: David S. Miller <davem@davemloft.net>
CC: netdev@vger.kernel.org
CC: stable@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/net/bonding/bond_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3118,13 +3118,13 @@ static int bond_slave_netdev_event(unsig
case NETDEV_CHANGE:
/* For 802.3ad mode only:
* Getting invalid Speed/Duplex values here will put slave
- * in weird state. So mark it as link-down for the time
+ * in weird state. So mark it as link-fail for the time
* being and let link-monitoring (miimon) set it right when
* correct speeds/duplex are available.
*/
if (bond_update_speed_duplex(slave) &&
BOND_MODE(bond) == BOND_MODE_8023AD)
- slave->link = BOND_LINK_DOWN;
+ slave->link = BOND_LINK_FAIL;
if (BOND_MODE(bond) == BOND_MODE_8023AD)
bond_3ad_adapter_speed_duplex_changed(slave);
^ permalink raw reply
* [PATCH 4.19 107/205] bonding/802.3ad: fix link_failure_count tracking
From: Greg Kroah-Hartman @ 2018-11-19 16:26 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Mahesh Bandewar, David S. Miller,
netdev, Jarod Wilson
In-Reply-To: <20181119162616.586062722@linuxfoundation.org>
4.19-stable review patch. If anyone has any objections, please let me know.
------------------
From: Jarod Wilson <jarod@redhat.com>
commit ea53abfab960909d622ca37bcfb8e1c5378d21cc upstream.
Commit 4d2c0cda07448ea6980f00102dc3964eb25e241c set slave->link to
BOND_LINK_DOWN for 802.3ad bonds whenever invalid speed/duplex values
were read, to fix a problem with slaves getting into weird states, but
in the process, broke tracking of link failures, as going straight to
BOND_LINK_DOWN when a link is indeed down (cable pulled, switch rebooted)
means we broke out of bond_miimon_inspect()'s BOND_LINK_DOWN case because
!link_state was already true, we never incremented commit, and never got
a chance to call bond_miimon_commit(), where slave->link_failure_count
would be incremented. I believe the simple fix here is to mark the slave
as BOND_LINK_FAIL, and let bond_miimon_inspect() transition the link from
_FAIL to either _UP or _DOWN, and in the latter case, we now get proper
incrementing of link_failure_count again.
Fixes: 4d2c0cda0744 ("bonding: speed/duplex update at NETDEV_UP event")
CC: Mahesh Bandewar <maheshb@google.com>
CC: David S. Miller <davem@davemloft.net>
CC: netdev@vger.kernel.org
CC: stable@vger.kernel.org
Signed-off-by: Jarod Wilson <jarod@redhat.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/net/bonding/bond_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3111,13 +3111,13 @@ static int bond_slave_netdev_event(unsig
case NETDEV_CHANGE:
/* For 802.3ad mode only:
* Getting invalid Speed/Duplex values here will put slave
- * in weird state. So mark it as link-down for the time
+ * in weird state. So mark it as link-fail for the time
* being and let link-monitoring (miimon) set it right when
* correct speeds/duplex are available.
*/
if (bond_update_speed_duplex(slave) &&
BOND_MODE(bond) == BOND_MODE_8023AD)
- slave->link = BOND_LINK_DOWN;
+ slave->link = BOND_LINK_FAIL;
if (BOND_MODE(bond) == BOND_MODE_8023AD)
bond_3ad_adapter_speed_duplex_changed(slave);
^ permalink raw reply
* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Andrew Lunn @ 2018-11-19 16:25 UTC (permalink / raw)
To: Andreas Schwab
Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
linux-riscv
In-Reply-To: <mvmva4t9h8x.fsf@suse.de>
On Mon, Nov 19, 2018 at 05:14:38PM +0100, Andreas Schwab wrote:
> On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > Could you turn on lockdep and see if it reports a deadlock.
>
> How do I "turn on lockdep"?
make menuconfig
Kernel hacking
Lock Debugging (spinlocks, mutexes, etc...)
Lock debugging: prove locking correctness
It has changes its name at some point. it used to be called
CONFIG_LOCKDEP, for locking dependencies. Anybody who has been kernel
hacking for a while still calls it lockdep.
Andrew
^ permalink raw reply
* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Andreas Schwab @ 2018-11-19 16:14 UTC (permalink / raw)
To: Andrew Lunn
Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
linux-riscv
In-Reply-To: <20181119161204.GC2393@lunn.ch>
On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> Could you turn on lockdep and see if it reports a deadlock.
How do I "turn on lockdep"?
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Andrew Lunn @ 2018-11-19 16:12 UTC (permalink / raw)
To: Andreas Schwab
Cc: Alexandre Belloni, Quentin Schulz, davem, f.fainelli,
allan.nielsen, linux-kernel, netdev, thomas.petazzoni,
linux-riscv
In-Reply-To: <mvmzhu59idl.fsf@suse.de>
On Mon, Nov 19, 2018 at 04:50:14PM +0100, Andreas Schwab wrote:
> On Nov 19 2018, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
>
> > My first intuition is that he mac driver quentin is using does
> > phy_connect when the interface is opened while macb is doing it at probe
> > time. I didn't investigate but maybe this can help :)
>
> The phy probing of macb is very unreliable, perhaps it needs to be
> rewritten.
Hi Andreas
I still don't see why that would cause a hang.
Could you turn on lockdep and see if it reports a deadlock.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Andreas Schwab @ 2018-11-19 15:50 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Andrew Lunn, Quentin Schulz, davem, f.fainelli, allan.nielsen,
linux-kernel, netdev, thomas.petazzoni, linux-riscv
In-Reply-To: <20181119154005.GA8367@piout.net>
On Nov 19 2018, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote:
> My first intuition is that he mac driver quentin is using does
> phy_connect when the interface is opened while macb is doing it at probe
> time. I didn't investigate but maybe this can help :)
The phy probing of macb is very unreliable, perhaps it needs to be
rewritten.
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Alexandre Belloni @ 2018-11-19 15:40 UTC (permalink / raw)
To: Andrew Lunn
Cc: Andreas Schwab, Quentin Schulz, davem, f.fainelli, allan.nielsen,
linux-kernel, netdev, thomas.petazzoni, linux-riscv
In-Reply-To: <20181119152830.GE26852@lunn.ch>
On 19/11/2018 16:28:30+0100, Andrew Lunn wrote:
> On Mon, Nov 19, 2018 at 04:13:10PM +0100, Andreas Schwab wrote:
> > On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote:
> > >> On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> > >>
> > >> > The Microsemi PHYs have multiple banks of registers (called pages).
> > >> > Registers can only be accessed from one page, if we need a register from
> > >> > another page, we need to switch the page and the registers of all other
> > >> > pages are not accessible anymore.
> > >> >
> > >> > Basically, to read register 5 from page 0, 1, 2, etc., you do the same
> > >> > phy_read(phydev, 5); but you need to set the desired page beforehand.
> > >> >
> > >> > In order to guarantee that two concurrent functions do not change the
> > >> > page, we need to do some locking per page. This can be achieved with the
> > >> > use of phy_select_page and phy_restore_page functions but phy_write/read
> > >> > calls in-between those two functions shall be replaced by their
> > >> > lock-free alternative __phy_write/read.
> > >> >
> > >> > Let's migrate this driver to those functions.
> > >>
> > >> This has some serious locking problem.
> > >
> > > Hi Andreas
> > >
> > > Could you be more specific. Are you getting a deadlock? A WARN_ON?
> >
> > See the stack trace. That's where it hangs.
>
> So you never said it hangs. The stacktrace helps, but a description of
> what actually happens also helps. And i expect Quentin has booted this
> code lots of times and not had a hang. So some hits how to reproduce
> it would also help. Maybe your kernel config?
>
> I'm interested because he is using the core mdio locking
> primitives. If those are broken, i want to know.
>
My first intuition is that he mac driver quentin is using does
phy_connect when the interface is opened while macb is doing it at probe
time. I didn't investigate but maybe this can help :)
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Andrew Lunn @ 2018-11-19 15:28 UTC (permalink / raw)
To: Andreas Schwab
Cc: Quentin Schulz, davem, f.fainelli, allan.nielsen, linux-kernel,
netdev, thomas.petazzoni, alexandre.belloni, linux-riscv
In-Reply-To: <mvm4lcdaynt.fsf@suse.de>
On Mon, Nov 19, 2018 at 04:13:10PM +0100, Andreas Schwab wrote:
> On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
>
> > On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote:
> >> On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> >>
> >> > The Microsemi PHYs have multiple banks of registers (called pages).
> >> > Registers can only be accessed from one page, if we need a register from
> >> > another page, we need to switch the page and the registers of all other
> >> > pages are not accessible anymore.
> >> >
> >> > Basically, to read register 5 from page 0, 1, 2, etc., you do the same
> >> > phy_read(phydev, 5); but you need to set the desired page beforehand.
> >> >
> >> > In order to guarantee that two concurrent functions do not change the
> >> > page, we need to do some locking per page. This can be achieved with the
> >> > use of phy_select_page and phy_restore_page functions but phy_write/read
> >> > calls in-between those two functions shall be replaced by their
> >> > lock-free alternative __phy_write/read.
> >> >
> >> > Let's migrate this driver to those functions.
> >>
> >> This has some serious locking problem.
> >
> > Hi Andreas
> >
> > Could you be more specific. Are you getting a deadlock? A WARN_ON?
>
> See the stack trace. That's where it hangs.
So you never said it hangs. The stacktrace helps, but a description of
what actually happens also helps. And i expect Quentin has booted this
code lots of times and not had a hang. So some hits how to reproduce
it would also help. Maybe your kernel config?
I'm interested because he is using the core mdio locking
primitives. If those are broken, i want to know.
Thanks
Andrew
^ permalink raw reply
* RE: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC ethernet drivers
From: Claudiu Manoil @ 2018-11-19 15:25 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Alexandru Marginean, Catalin Horghidan
In-Reply-To: <20181117.120800.2083117509893220159.davem@davemloft.net>
>-----Original Message-----
>From: David Miller <davem@davemloft.net>
>Sent: Saturday, November 17, 2018 10:08 PM
>To: Claudiu Manoil <claudiu.manoil@nxp.com>
>Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Alexandru
>Marginean <alexandru.marginean@nxp.com>; Catalin Horghidan
><catalin.horghidan@nxp.com>
>Subject: Re: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC
>ethernet drivers
>
>From: Claudiu Manoil <claudiu.manoil@nxp.com>
>Date: Thu, 15 Nov 2018 18:13:53 +0200
>
>> + if (!nr_frags && !(flags & ENETC_TXBD_FLAGS_EX))
>> + flags |= ENETC_TXBD_FLAGS_F;
>> +
>> + txbd->flags = flags;
>> +
>> + if (flags & ENETC_TXBD_FLAGS_EX) {
> ...
>> + if (!nr_frags)
> ...
>> + }
>> +
>> + frag = &skb_shinfo(skb)->frags[0];
>> + for (f = 0; f < nr_frags; f++, frag++) {
> ...
>> + }
>> +
>> + if (nr_frags)
>> + /* last BD needs 'F' bit set */
>> + txbd->flags = ENETC_TXBD_FLAGS_F;
>
>Isn't it so much simpler to just have an unconditional:
>
> txbd->flags |= ENETC_TXBD_FLAGS_F;
>
>at the end of this code segment?
>
>Then all of this code conditional on nr_frags can go away.
True, but this code also avoids reading from the buffer descriptor memory
area (dma coherent), which is not cheap. As it is now, the driver only writes
the txbd (excepting the extension descriptor). I'll look into how big this penalty
really is. I'm also in favor of more readable code.
Thanks,
Claudiu
^ permalink raw reply
* Re: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC ethernet drivers
From: Andrew Lunn @ 2018-11-19 15:20 UTC (permalink / raw)
To: Claudiu Manoil
Cc: David S . Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Alexandru Marginean,
Catalin Horghidan
In-Reply-To: <VI1PR04MB48809F132A5DFFC71DA5F6BD96D80@VI1PR04MB4880.eurprd04.prod.outlook.com>
> >> +static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb);
> >> +static void enetc_unmap_tx_buff(struct enetc_bdr *tx_ring,
> >> + struct enetc_tx_swbd *tx_swbd);
> >> +static int enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int budget);
> >> +
> >> +static struct sk_buff *enetc_map_rx_buff_to_skb(struct enetc_bdr *rx_ring,
> >> + int i, u16 size);
> >> +static void enetc_add_rx_buff_to_skb(struct enetc_bdr *rx_ring, int i,
> >> + u16 size, struct sk_buff *skb);
> >> +static void enetc_process_skb(struct enetc_bdr *rx_ring, struct sk_buff *skb);
> >> +static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
> >> + struct napi_struct *napi, int work_limit);
> >> +
> >
> >Please try to avoid forward declarations. Move the code around so you
> >don't need them.
> >
>
> Maybe I could drop some of these, but...
> For some performance critical functions on the datapath this would be an
> attempt to improve icache hit rate by having the caller function located in
> memory just before it's children (callees).
That is kind of the point of moving the functions. GCC can only inline
a function when it has already seen it, at least as far as i know. So
by having the functions in the correct order, you increase the
likelihood gcc inlines it, making the icache layout better, no
function calls, etc.
Try building the code both ways, and then disassemble it. See which
looks better.
> >> +static bool enetc_tx_csum(struct sk_buff *skb, union enetc_tx_bd *txbd)
> >> +{
> >> + int l3_start, l3_hsize;
> >> + u16 l3_flags, l4_flags;
> >> +
> >> + if (skb->ip_summed != CHECKSUM_PARTIAL)
> >> + return false;
> >> +
> >> + switch (skb->csum_offset) {
> >> + case offsetof(struct tcphdr, check):
> >> + l4_flags = ENETC_TXBD_L4_TCP;
> >> + break;
> >> + case offsetof(struct udphdr, check):
> >> + l4_flags = ENETC_TXBD_L4_UDP;
> >> + break;
> >> + default:
> >> + skb_checksum_help(skb);
> >> + return false;
> >> + }
> >> +
> >> + l3_start = skb_network_offset(skb);
> >> + l3_hsize = skb_network_header_len(skb);
> >> +
> >> + l3_flags = 0;
> >> + if (skb->protocol == htons(ETH_P_IPV6))
> >> + l3_flags = ENETC_TXBD_L3_IPV6;
> >
> >Is there no need to do anything with IPv4?
> >
>
> No, 0 for this bit means IPv4. Only UDP and TCP over IPv4 and IPv6 supported.
Ah, O.K. Some i assume there is some way to tell it this is actually
an IP packet, not an IPX packet, etc.
> This code path is also reached by the VF driver (via the open() hook which is common to both
> PF and VF drivers, and VFs don't manage PHYs).
> Also, the driver may be exercised in MAC loopback mode (ethtool -K loopback on), when the
> PHY nodes are removed. And it's always good to be able to run the driver on a simulator or
> an emulator without having to modify it.
O.K, that is fine.
> >> +int enetc_pci_probe(struct pci_dev *pdev, const char *name, int sizeof_priv)
> >> +{
> >> + struct enetc_si *si, *p;
> >> + struct enetc_hw *hw;
> >> + size_t alloc_size;
> >> + int err, len;
> >> +
> >> + err = pci_enable_device_mem(pdev);
> >> + if (err) {
> >> + dev_err(&pdev->dev, "device enable failed\n");
> >> + return err;
> >> + }
> >> +
> >> + /* set up for high or low dma */
> >> + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> >> + if (err) {
> >> + err = dma_set_mask_and_coherent(&pdev->dev,
> >DMA_BIT_MASK(32));
> >> + if (err) {
> >> + dev_err(&pdev->dev,
> >> + "DMA configuration failed: 0x%x\n", err);
> >> + goto err_dma;
> >> + }
> >> + }
> >
> >Humm, i thought drivers were not supposed to do this. The architecture
> >code should be setting masks. But i've not followed all those
> >conversations...
> >
>
> Documentation/DMA-API-HOWTO.txt still states:
> " For correct operation, you must interrogate the kernel in your device
> probe routine to see if the DMA controller on the machine can properly
> support the DMA addressing limitation your device has. It is good
> style to do this even if your device holds the default setting, [...]"
O.K, thanks for the reference.
Andrew
^ permalink raw reply
* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Andreas Schwab @ 2018-11-19 15:13 UTC (permalink / raw)
To: Andrew Lunn
Cc: Quentin Schulz, davem, f.fainelli, allan.nielsen, linux-kernel,
netdev, thomas.petazzoni, alexandre.belloni, linux-riscv
In-Reply-To: <20181119151057.GC26852@lunn.ch>
On Nov 19 2018, Andrew Lunn <andrew@lunn.ch> wrote:
> On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote:
>> On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
>>
>> > The Microsemi PHYs have multiple banks of registers (called pages).
>> > Registers can only be accessed from one page, if we need a register from
>> > another page, we need to switch the page and the registers of all other
>> > pages are not accessible anymore.
>> >
>> > Basically, to read register 5 from page 0, 1, 2, etc., you do the same
>> > phy_read(phydev, 5); but you need to set the desired page beforehand.
>> >
>> > In order to guarantee that two concurrent functions do not change the
>> > page, we need to do some locking per page. This can be achieved with the
>> > use of phy_select_page and phy_restore_page functions but phy_write/read
>> > calls in-between those two functions shall be replaced by their
>> > lock-free alternative __phy_write/read.
>> >
>> > Let's migrate this driver to those functions.
>>
>> This has some serious locking problem.
>
> Hi Andreas
>
> Could you be more specific. Are you getting a deadlock? A WARN_ON?
See the stack trace. That's where it hangs.
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Andrew Lunn @ 2018-11-19 15:10 UTC (permalink / raw)
To: Andreas Schwab
Cc: Quentin Schulz, davem, f.fainelli, allan.nielsen, linux-kernel,
netdev, thomas.petazzoni, alexandre.belloni, linux-riscv
In-Reply-To: <mvm8t1pazea.fsf@suse.de>
On Mon, Nov 19, 2018 at 03:57:17PM +0100, Andreas Schwab wrote:
> On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
>
> > The Microsemi PHYs have multiple banks of registers (called pages).
> > Registers can only be accessed from one page, if we need a register from
> > another page, we need to switch the page and the registers of all other
> > pages are not accessible anymore.
> >
> > Basically, to read register 5 from page 0, 1, 2, etc., you do the same
> > phy_read(phydev, 5); but you need to set the desired page beforehand.
> >
> > In order to guarantee that two concurrent functions do not change the
> > page, we need to do some locking per page. This can be achieved with the
> > use of phy_select_page and phy_restore_page functions but phy_write/read
> > calls in-between those two functions shall be replaced by their
> > lock-free alternative __phy_write/read.
> >
> > Let's migrate this driver to those functions.
>
> This has some serious locking problem.
Hi Andreas
Could you be more specific. Are you getting a deadlock? A WARN_ON?
Thanks,
Andrew
^ permalink raw reply
* RE: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC ethernet drivers
From: Claudiu Manoil @ 2018-11-19 15:09 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S . Miller, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Alexandru Marginean,
Catalin Horghidan
In-Reply-To: <20181117003002.GC752@lunn.ch>
>-----Original Message-----
>From: Andrew Lunn <andrew@lunn.ch>
>Sent: Saturday, November 17, 2018 2:30 AM
>To: Claudiu Manoil <claudiu.manoil@nxp.com>
>Cc: David S . Miller <davem@davemloft.net>; netdev@vger.kernel.org; linux-
>kernel@vger.kernel.org; Alexandru Marginean
><alexandru.marginean@nxp.com>; Catalin Horghidan
><catalin.horghidan@nxp.com>
>Subject: Re: [PATCH net-next 1/4] enetc: Introduce basic PF and VF ENETC
>ethernet drivers
>
[...]
>> +
>> +static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb);
>> +static void enetc_unmap_tx_buff(struct enetc_bdr *tx_ring,
>> + struct enetc_tx_swbd *tx_swbd);
>> +static int enetc_clean_tx_ring(struct enetc_bdr *tx_ring, int budget);
>> +
>> +static struct sk_buff *enetc_map_rx_buff_to_skb(struct enetc_bdr *rx_ring,
>> + int i, u16 size);
>> +static void enetc_add_rx_buff_to_skb(struct enetc_bdr *rx_ring, int i,
>> + u16 size, struct sk_buff *skb);
>> +static void enetc_process_skb(struct enetc_bdr *rx_ring, struct sk_buff *skb);
>> +static int enetc_clean_rx_ring(struct enetc_bdr *rx_ring,
>> + struct napi_struct *napi, int work_limit);
>> +
>
>Please try to avoid forward declarations. Move the code around so you
>don't need them.
>
Maybe I could drop some of these, but...
For some performance critical functions on the datapath this would be an
attempt to improve icache hit rate by having the caller function located in
memory just before it's children (callees). As you can see, only some perf
critical functions are forward declared here, for the xmit() and rx/tx cleanup paths.
Does this make sense to you? Could you point me to some reading on this topic otherwise?
>> +static bool enetc_tx_csum(struct sk_buff *skb, union enetc_tx_bd *txbd)
>> +{
>> + int l3_start, l3_hsize;
>> + u16 l3_flags, l4_flags;
>> +
>> + if (skb->ip_summed != CHECKSUM_PARTIAL)
>> + return false;
>> +
>> + switch (skb->csum_offset) {
>> + case offsetof(struct tcphdr, check):
>> + l4_flags = ENETC_TXBD_L4_TCP;
>> + break;
>> + case offsetof(struct udphdr, check):
>> + l4_flags = ENETC_TXBD_L4_UDP;
>> + break;
>> + default:
>> + skb_checksum_help(skb);
>> + return false;
>> + }
>> +
>> + l3_start = skb_network_offset(skb);
>> + l3_hsize = skb_network_header_len(skb);
>> +
>> + l3_flags = 0;
>> + if (skb->protocol == htons(ETH_P_IPV6))
>> + l3_flags = ENETC_TXBD_L3_IPV6;
>
>Is there no need to do anything with IPv4?
>
No, 0 for this bit means IPv4. Only UDP and TCP over IPv4 and IPv6 supported.
[...]
>> +static void adjust_link(struct net_device *ndev)
>> +{
>> + struct phy_device *phydev = ndev->phydev;
>> +
>> + phy_print_status(phydev);
>
>You normally need more than that. Maybe later patches add to this?
>
Yes, later patches. The first boards will be soon available.
Once we validate the remaining support for PHYs (including MDIO support which
is on the todo list) the patches will be available. Also, we're looking at phylink too.
Once we can confirm whether it works on our boards, the driver will be updated accordingly.
>> +static int enetc_phy_connect(struct net_device *ndev)
>> +{
>> + struct enetc_ndev_priv *priv = netdev_priv(ndev);
>> + struct phy_device *phydev;
>> +
>> + if (!priv->phy_node)
>> + return 0; /* phy-less mode */
>
>What are your user-cases for phy-less? If you don't have a PHY it is
>mostly because you are connected to an Ethernet switch. In that case
>you use fixed-link, which gives you a phy.
>
This code path is also reached by the VF driver (via the open() hook which is common to both
PF and VF drivers, and VFs don't manage PHYs).
Also, the driver may be exercised in MAC loopback mode (ethtool -K loopback on), when the
PHY nodes are removed. And it's always good to be able to run the driver on a simulator or
an emulator without having to modify it.
[...]
>> +int enetc_pci_probe(struct pci_dev *pdev, const char *name, int sizeof_priv)
>> +{
>> + struct enetc_si *si, *p;
>> + struct enetc_hw *hw;
>> + size_t alloc_size;
>> + int err, len;
>> +
>> + err = pci_enable_device_mem(pdev);
>> + if (err) {
>> + dev_err(&pdev->dev, "device enable failed\n");
>> + return err;
>> + }
>> +
>> + /* set up for high or low dma */
>> + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>> + if (err) {
>> + err = dma_set_mask_and_coherent(&pdev->dev,
>DMA_BIT_MASK(32));
>> + if (err) {
>> + dev_err(&pdev->dev,
>> + "DMA configuration failed: 0x%x\n", err);
>> + goto err_dma;
>> + }
>> + }
>
>Humm, i thought drivers were not supposed to do this. The architecture
>code should be setting masks. But i've not followed all those
>conversations...
>
Documentation/DMA-API-HOWTO.txt still states:
" For correct operation, you must interrogate the kernel in your device
probe routine to see if the DMA controller on the machine can properly
support the DMA addressing limitation your device has. It is good
style to do this even if your device holds the default setting, [...]"
For the rest of the comments I didn't include in this reply: OK.
Thanks,
Claudiu
^ permalink raw reply
* Re: [PATCH v2 RESEND 1/2] mm/page_alloc: free order-0 pages through PCP in page_frag_free()
From: Tariq Toukan @ 2018-11-19 15:00 UTC (permalink / raw)
To: Aaron Lu, linux-mm@kvack.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org
Cc: Andrew Morton, Paweł Staszewski, Jesper Dangaard Brouer,
Eric Dumazet, Tariq Toukan, Ilias Apalodimas, Yoel Caspersen,
Mel Gorman, Saeed Mahameed, Michal Hocko, Vlastimil Babka,
Dave Hansen, Alexander Duyck, Ian Kumlien
In-Reply-To: <20181119134834.17765-2-aaron.lu@intel.com>
On 19/11/2018 3:48 PM, Aaron Lu wrote:
> page_frag_free() calls __free_pages_ok() to free the page back to
> Buddy. This is OK for high order page, but for order-0 pages, it
> misses the optimization opportunity of using Per-Cpu-Pages and can
> cause zone lock contention when called frequently.
>
> Paweł Staszewski recently shared his result of 'how Linux kernel
> handles normal traffic'[1] and from perf data, Jesper Dangaard Brouer
> found the lock contention comes from page allocator:
>
> mlx5e_poll_tx_cq
> |
> --16.34%--napi_consume_skb
> |
> |--12.65%--__free_pages_ok
> | |
> | --11.86%--free_one_page
> | |
> | |--10.10%--queued_spin_lock_slowpath
> | |
> | --0.65%--_raw_spin_lock
> |
> |--1.55%--page_frag_free
> |
> --1.44%--skb_release_data
>
> Jesper explained how it happened: mlx5 driver RX-page recycle
> mechanism is not effective in this workload and pages have to go
> through the page allocator. The lock contention happens during
> mlx5 DMA TX completion cycle. And the page allocator cannot keep
> up at these speeds.[2]
>
> I thought that __free_pages_ok() are mostly freeing high order
> pages and thought this is an lock contention for high order pages
> but Jesper explained in detail that __free_pages_ok() here are
> actually freeing order-0 pages because mlx5 is using order-0 pages
> to satisfy its page pool allocation request.[3]
>
> The free path as pointed out by Jesper is:
> skb_free_head()
> -> skb_free_frag()
> -> page_frag_free()
> And the pages being freed on this path are order-0 pages.
>
> Fix this by doing similar things as in __page_frag_cache_drain() -
> send the being freed page to PCP if it's an order-0 page, or
> directly to Buddy if it is a high order page.
>
> With this change, Paweł hasn't noticed lock contention yet in
> his workload and Jesper has noticed a 7% performance improvement
> using a micro benchmark and lock contention is gone. Ilias' test
> on a 'low' speed 1Gbit interface on an cortex-a53 shows ~11%
> performance boost testing with 64byte packets and __free_pages_ok()
> disappeared from perf top.
>
> [1]: https://www.spinics.net/lists/netdev/msg531362.html
> [2]: https://www.spinics.net/lists/netdev/msg531421.html
> [3]: https://www.spinics.net/lists/netdev/msg531556.html
>
> Reported-by: Paweł Staszewski <pstaszewski@itcare.pl>
> Analysed-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Acked-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> Acked-by: Tariq Toukan <tariqt@mellanox.com
missing '>' sign in my email tag.
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
^ permalink raw reply
* Re: [PATCH net-next v3 1/6] net: phy: mscc: migrate to phy_select/restore_page functions
From: Andreas Schwab @ 2018-11-19 14:57 UTC (permalink / raw)
To: Quentin Schulz
Cc: davem, andrew, f.fainelli, allan.nielsen, linux-kernel, netdev,
thomas.petazzoni, alexandre.belloni, linux-riscv
In-Reply-To: <20181008100728.24959-2-quentin.schulz@bootlin.com>
On Okt 08 2018, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> The Microsemi PHYs have multiple banks of registers (called pages).
> Registers can only be accessed from one page, if we need a register from
> another page, we need to switch the page and the registers of all other
> pages are not accessible anymore.
>
> Basically, to read register 5 from page 0, 1, 2, etc., you do the same
> phy_read(phydev, 5); but you need to set the desired page beforehand.
>
> In order to guarantee that two concurrent functions do not change the
> page, we need to do some locking per page. This can be achieved with the
> use of phy_select_page and phy_restore_page functions but phy_write/read
> calls in-between those two functions shall be replaced by their
> lock-free alternative __phy_write/read.
>
> Let's migrate this driver to those functions.
This has some serious locking problem.
[<0>] __schedule+0x25e/0x74c
[<0>] schedule+0x1a/0x58
[<0>] schedule_preempt_disabled+0xc/0x14
[<0>] __mutex_lock.isra.0+0x10e/0x22e
[<0>] __mutex_lock_slowpath+0xe/0x16
[<0>] mutex_lock+0x22/0x2a
[<0>] mdiobus_read+0x36/0x60
[<0>] vsc85xx_config_init+0x4c/0x1e2
[<0>] phy_init_hw+0x3c/0x52
[<0>] phy_attach_direct+0xec/0x1dc
[<0>] phy_connect_direct+0x1a/0x56
[<0>] macb_probe+0x800/0xb5e [macb]
[<0>] platform_drv_probe+0x3e/0x7e
[<0>] really_probe+0xba/0x310
[<0>] driver_probe_device+0x54/0xf2
[<0>] __driver_attach+0xde/0x106
[<0>] bus_for_each_dev+0x4a/0x72
[<0>] driver_attach+0x1a/0x22
[<0>] bus_add_driver+0x1ce/0x212
[<0>] driver_register+0x3a/0xd0
[<0>] __platform_driver_register+0x3a/0x42
[<0>] macb_driver_init+0x20/0x28 [macb]
[<0>] do_one_initcall+0x48/0x128
[<0>] do_init_module+0x4a/0x186
[<0>] load_module+0xd6a/0xe6a
[<0>] sys_finit_module+0xc6/0xfc
[<0>] check_syscall_nr+0x22/0x22
Andreas.
--
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply
* Re: "WARNING: CPU: 4 PID: 1 at ./include/net/genetlink.h:294 genlmsg_multicast_netns.isra.10.constprop.19+0x34/0x5e" with mlx4 under 4.9.137
From: Krzysztof Olędzki @ 2018-11-19 4:31 UTC (permalink / raw)
To: tariqt, jiri; +Cc: netdev, johannes.berg
In-Reply-To: <201b8368-f5d5-c76e-59ab-f9c3e6de5c7a@ans.pl>
-> jiri@mellanox.com
> Hi,
>
> I just upgraded kernel from 4.4.161 to 4.9.137 on one of my servers (Dell PowerEdge T110 II) and found this in the dmesg:
>
> [ 0.796066] mlx4_core: Mellanox ConnectX core driver v2.2-1 (Feb, 2014)
> [ 0.796251] ------------[ cut here ]------------
> [ 0.796435] WARNING: CPU: 4 PID: 1 at ./include/net/genetlink.h:294 genlmsg_multicast_netns.isra.10.constprop.19+0x34/0x5e
> [ 0.796724] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.9.137-o2 #1
> [ 0.796898] Hardware name: Dell Inc. PowerEdge T110 II/0PM2CW, BIOS 2.10.0 05/24/2018
> [ 0.797188] ffffc90000013c08 ffffffff813ea8f6 0000000000000000 0000000000000000
> [ 0.797679] ffffc90000013c48 ffffffff8110df65 0000012600000ec0 00000000ffffffea
> [ 0.798157] ffff88081a320000 0000000000000003 00000000fffffff4 ffffffff818eb360
> [ 0.798648] Call Trace:
> [ 0.798819] [<ffffffff813ea8f6>] dump_stack+0x4f/0x65
> [ 0.798993] [<ffffffff8110df65>] __warn+0xce/0xe9
> [ 0.799164] [<ffffffff8110e028>] warn_slowpath_null+0x18/0x1a
> [ 0.799343] [<ffffffff81688b88>] genlmsg_multicast_netns.isra.10.constprop.19+0x34/0x5e
> [ 0.799637] [<ffffffff816890d5>] devlink_notify+0x54/0x5b
> [ 0.799813] [<ffffffff816896f7>] devlink_register+0x46/0x59
> [ 0.799989] [<ffffffff8155ef29>] mlx4_init_one+0x104/0x4c7
> [ 0.800163] [<ffffffff8112b4d6>] ? preempt_latency_start+0x21/0x57
> [ 0.800341] [<ffffffff8112d4e4>] ? preempt_count_add+0x70/0x73
> [ 0.800515] [<ffffffff814202db>] ? pci_match_id+0x32/0x57
> [ 0.800689] [<ffffffff81420739>] pci_device_probe+0x6e/0xb9
> [ 0.800867] [<ffffffff814cdd67>] driver_probe_device+0xf9/0x222
> [ 0.804465] [<ffffffff814cdefd>] __driver_attach+0x6d/0x8f
> [ 0.804638] [<ffffffff814cde90>] ? driver_probe_device+0x222/0x222
> [ 0.804814] [<ffffffff814cc5e8>] bus_for_each_dev+0x6a/0x82
> [ 0.804988] [<ffffffff814ce0df>] driver_attach+0x19/0x1b
> [ 0.805164] [<ffffffff814ccd96>] bus_add_driver+0xee/0x1c9
> [ 0.805342] [<ffffffff81d486df>] ? do_early_param+0x90/0x90
> [ 0.805519] [<ffffffff814ce502>] driver_register+0x83/0xba
> [ 0.805689] [<ffffffff81d90724>] ? tg3_driver_init+0x1b/0x1b
> [ 0.805862] [<ffffffff8142058d>] __pci_register_driver+0x44/0x46
> [ 0.806034] [<ffffffff81d9082d>] mlx4_init+0x109/0x12a
> [ 0.806200] [<ffffffff81d90724>] ? tg3_driver_init+0x1b/0x1b
> [ 0.806377] [<ffffffff81d48efb>] do_one_initcall+0x8b/0x10e
> [ 0.806547] [<ffffffff81d486df>] ? do_early_param+0x90/0x90
> [ 0.806717] [<ffffffff81d490cf>] kernel_init_freeable+0x151/0x1dd
> [ 0.806890] [<ffffffff817dbe16>] ? rest_init+0x7d/0x7d
> [ 0.807061] [<ffffffff817dbe1f>] kernel_init+0x9/0x105
> [ 0.807232] [<ffffffff817e01d4>] ret_from_fork+0x54/0x60
> [ 0.807415] ---[ end trace 34e550e5d3d83d0a ]---
> [ 0.807582] mlx4_core: Initializing 0000:01:00.0
>
> The NIC seems to work correctly, but the warning appears after each boot.
> There is also BCM5722 (tg3) NIC in the system. The only thing that changes
> between boots is the CPU reported in the message.
>
> I can try bisec or test a more recent kernel, but this will have to wait after
> I come back from my Thanksgiving travel.
>
> $ lspci -nn shows:
> 01:00.0 Ethernet controller [0200]: Mellanox Technologies MT26448 [ConnectX EN 10GigE, PCIe 2.0 5GT/s] [15b3:6750] (rev b0)
>
> Thanks,
> Krzysztof
>
^ permalink raw reply
* "WARNING: CPU: 4 PID: 1 at ./include/net/genetlink.h:294 genlmsg_multicast_netns.isra.10.constprop.19+0x34/0x5e" with mlx4 under 4.9.137
From: Krzysztof Olędzki @ 2018-11-19 3:52 UTC (permalink / raw)
To: tariqt, jiri; +Cc: netdev, johannes.berg
Hi,
I just upgraded kernel from 4.4.161 to 4.9.137 on one of my servers (Dell PowerEdge T110 II) and found this in the dmesg:
[ 0.796066] mlx4_core: Mellanox ConnectX core driver v2.2-1 (Feb, 2014)
[ 0.796251] ------------[ cut here ]------------
[ 0.796435] WARNING: CPU: 4 PID: 1 at ./include/net/genetlink.h:294 genlmsg_multicast_netns.isra.10.constprop.19+0x34/0x5e
[ 0.796724] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 4.9.137-o2 #1
[ 0.796898] Hardware name: Dell Inc. PowerEdge T110 II/0PM2CW, BIOS 2.10.0 05/24/2018
[ 0.797188] ffffc90000013c08 ffffffff813ea8f6 0000000000000000 0000000000000000
[ 0.797679] ffffc90000013c48 ffffffff8110df65 0000012600000ec0 00000000ffffffea
[ 0.798157] ffff88081a320000 0000000000000003 00000000fffffff4 ffffffff818eb360
[ 0.798648] Call Trace:
[ 0.798819] [<ffffffff813ea8f6>] dump_stack+0x4f/0x65
[ 0.798993] [<ffffffff8110df65>] __warn+0xce/0xe9
[ 0.799164] [<ffffffff8110e028>] warn_slowpath_null+0x18/0x1a
[ 0.799343] [<ffffffff81688b88>] genlmsg_multicast_netns.isra.10.constprop.19+0x34/0x5e
[ 0.799637] [<ffffffff816890d5>] devlink_notify+0x54/0x5b
[ 0.799813] [<ffffffff816896f7>] devlink_register+0x46/0x59
[ 0.799989] [<ffffffff8155ef29>] mlx4_init_one+0x104/0x4c7
[ 0.800163] [<ffffffff8112b4d6>] ? preempt_latency_start+0x21/0x57
[ 0.800341] [<ffffffff8112d4e4>] ? preempt_count_add+0x70/0x73
[ 0.800515] [<ffffffff814202db>] ? pci_match_id+0x32/0x57
[ 0.800689] [<ffffffff81420739>] pci_device_probe+0x6e/0xb9
[ 0.800867] [<ffffffff814cdd67>] driver_probe_device+0xf9/0x222
[ 0.804465] [<ffffffff814cdefd>] __driver_attach+0x6d/0x8f
[ 0.804638] [<ffffffff814cde90>] ? driver_probe_device+0x222/0x222
[ 0.804814] [<ffffffff814cc5e8>] bus_for_each_dev+0x6a/0x82
[ 0.804988] [<ffffffff814ce0df>] driver_attach+0x19/0x1b
[ 0.805164] [<ffffffff814ccd96>] bus_add_driver+0xee/0x1c9
[ 0.805342] [<ffffffff81d486df>] ? do_early_param+0x90/0x90
[ 0.805519] [<ffffffff814ce502>] driver_register+0x83/0xba
[ 0.805689] [<ffffffff81d90724>] ? tg3_driver_init+0x1b/0x1b
[ 0.805862] [<ffffffff8142058d>] __pci_register_driver+0x44/0x46
[ 0.806034] [<ffffffff81d9082d>] mlx4_init+0x109/0x12a
[ 0.806200] [<ffffffff81d90724>] ? tg3_driver_init+0x1b/0x1b
[ 0.806377] [<ffffffff81d48efb>] do_one_initcall+0x8b/0x10e
[ 0.806547] [<ffffffff81d486df>] ? do_early_param+0x90/0x90
[ 0.806717] [<ffffffff81d490cf>] kernel_init_freeable+0x151/0x1dd
[ 0.806890] [<ffffffff817dbe16>] ? rest_init+0x7d/0x7d
[ 0.807061] [<ffffffff817dbe1f>] kernel_init+0x9/0x105
[ 0.807232] [<ffffffff817e01d4>] ret_from_fork+0x54/0x60
[ 0.807415] ---[ end trace 34e550e5d3d83d0a ]---
[ 0.807582] mlx4_core: Initializing 0000:01:00.0
The NIC seems to work correctly, but the warning appears after each boot.
There is also BCM5722 (tg3) NIC in the system. The only thing that changes
between boots is the CPU reported in the message.
I can try bisec or test a more recent kernel, but this will have to wait after
I come back from my Thanksgiving travel.
$ lspci -nn shows:
01:00.0 Ethernet controller [0200]: Mellanox Technologies MT26448 [ConnectX EN 10GigE, PCIe 2.0 5GT/s] [15b3:6750] (rev b0)
Thanks,
Krzysztof
^ permalink raw reply
* Re: [PATCH] rhashtable: detect when object movement between tables might have invalidated a lookup
From: David Miller @ 2018-11-19 4:22 UTC (permalink / raw)
To: herbert; +Cc: neilb, tgraf, netdev, linux-kernel, eric.dumazet
In-Reply-To: <20181119040634.qlvsrviznd5npiek@gondor.apana.org.au>
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 19 Nov 2018 12:06:34 +0800
> On Mon, Nov 19, 2018 at 11:56:35AM +0800, Herbert Xu wrote:
>>
>> I take that back. Because of your shift which cancels out the
>> shift in NULLS_MARKER, it would appear that this should work just
>> fine with RHT_NULLS_MARRKER(0), no? IOW, it would appear that
>>
>> RHT_NULLS_MARKER(0) = RHT_NULLS_MARKER(RHT_NULLS_MARKER(0))
>
> My emails to Neil are bouncing:
>
> neilb@suse.com
> host smtp.glb1.softwaregrp.com [15.124.2.87]
> SMTP error from remote mail server after RCPT TO:<neilb@suse.com>:
> 550 Cannot process address
Yeah this just started happening 2 days ago.
^ permalink raw reply
* Re: [PATCH net] sctp: not allow to set asoc prsctp_enable by sockopt
From: Marcelo Ricardo Leitner @ 2018-11-19 4:14 UTC (permalink / raw)
To: Xin Long; +Cc: Neil Horman, network dev, linux-sctp, davem
In-Reply-To: <CADvbK_dkuMn_v_0vpvB9VJjUUsa66Po44EDevEzqACjveY94Tg@mail.gmail.com>
On Sun, Nov 18, 2018 at 04:02:25PM +0900, Xin Long wrote:
> On Sat, Nov 17, 2018 at 12:12 AM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Thu, Nov 15, 2018 at 09:41:01PM -0200, Marcelo Ricardo Leitner wrote:
> > > [ re-sending, without html this time ]
> > >
> > > On Thu, Nov 15, 2018, 15:26 Neil Horman <nhorman@tuxdriver.com wrote:
> > >
> > > > On Thu, Nov 15, 2018 at 08:25:36PM -0200, Marcelo Ricardo Leitner wrote:
> > > > > On Thu, Nov 15, 2018 at 04:43:10PM -0500, Neil Horman wrote:
> > > > > > On Thu, Nov 15, 2018 at 03:22:21PM -0200, Marcelo Ricardo Leitner
> > > > wrote:
> > > > > > > On Thu, Nov 15, 2018 at 07:14:28PM +0800, Xin Long wrote:
> > > > > > > > As rfc7496#section4.5 says about SCTP_PR_SUPPORTED:
> > > > > > > >
> > > > > > > > This socket option allows the enabling or disabling of the
> > > > > > > > negotiation of PR-SCTP support for future associations. For
> > > > existing
> > > > > > > > associations, it allows one to query whether or not PR-SCTP
> > > > support
> > > > > > > > was negotiated on a particular association.
> > > > > > > >
> > > > > > > > It means only sctp sock's prsctp_enable can be set.
> > > > > > > >
> > > > > > > > Note that for the limitation of SCTP_{CURRENT|ALL}_ASSOC, we will
> > > > > > > > add it when introducing SCTP_{FUTURE|CURRENT|ALL}_ASSOC for linux
> > > > > > > > sctp in another patchset.
> > > > > > > >
> > > > > > > > Fixes: 28aa4c26fce2 ("sctp: add SCTP_PR_SUPPORTED on sctp sockopt")
> > > > > > > > Reported-by: Ying Xu <yinxu@redhat.com>
> > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > > ---
> > > > > > > > net/sctp/socket.c | 13 +++----------
> > > > > > > > 1 file changed, 3 insertions(+), 10 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > > > > index 739f3e5..e9b8232 100644
> > > > > > > > --- a/net/sctp/socket.c
> > > > > > > > +++ b/net/sctp/socket.c
> > > > > > > > @@ -3940,7 +3940,6 @@ static int
> > > > sctp_setsockopt_pr_supported(struct sock *sk,
> > > > > > > > unsigned int optlen)
> > > > > > > > {
> > > > > > > > struct sctp_assoc_value params;
> > > > > > > > - struct sctp_association *asoc;
> > > > > > > > int retval = -EINVAL;
> > > > > > > >
> > > > > > > > if (optlen != sizeof(params))
> > > > > > > > @@ -3951,16 +3950,10 @@ static int
> > > > sctp_setsockopt_pr_supported(struct sock *sk,
> > > > > > > > goto out;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - asoc = sctp_id2assoc(sk, params.assoc_id);
> > > > > > > > - if (asoc) {
> > > > > > > > - asoc->prsctp_enable = !!params.assoc_value;
> > > > > > > > - } else if (!params.assoc_id) {
> > > > > > > > - struct sctp_sock *sp = sctp_sk(sk);
> > > > > > > > -
> > > > > > > > - sp->ep->prsctp_enable = !!params.assoc_value;
> > > > > > > > - } else {
> > > > > > > > + if (sctp_style(sk, UDP) && sctp_id2assoc(sk,
> > > > params.assoc_id))
> > > > > > >
> > > > > > > This would allow using a non-existent assoc id on UDP-style sockets
> > > > to
> > > > > > > set it at the socket, which is not expected. It should be more like:
> > > > > > >
> > > > > > > + if (sctp_style(sk, UDP) && params.assoc_id)
> > > > > > How do you see that to be the case? sctp_id2assoc will return NULL if
> > > > an
> > > > > > association isn't found, so the use of sctp_id2assoc should work just
> > > > fine.
> > > > >
> > > > > Right, it will return NULL, and because of that it won't bail out as
> > > > > it should and will adjust the socket config instead.
> > > > >
> > > >
> > > > Oh, duh, you're absolutely right, NULL will evalutate to false there, and
> > > > skip
> > > > the conditional goto out;
> > > >
> > > > that said, It would make more sense to me to just change the sense of the
> > > > second
> > > > condition to !sctp_id2assoc(sk, params.assoc_id), so that we goto out if no
> > > > association is found. it still seems a
> > >
> > >
> > > That would break setting it on the socket without an assoc so far.
> > >
> > ok, yes, I see what xin is getting at now. The RFC indicates that the
> > setsockopt method for this socket option is meant to set the prsctp enabled
> > value on _future_ associations, implying that we should not operate at all on
> > already existing associations (i.e. we should ignore the assoc_id in the passed
> > in structure and only operate on the socket). That said, heres the entire text
> > of the RFC section:
> >
> > 4.5. Socket Option for Getting and Setting the PR-SCTP Support
> > (SCTP_PR_SUPPORTED)
> >
> > This socket option allows the enabling or disabling of the
> > negotiation of PR-SCTP support for future associations. For existing
> > associations, it allows one to query whether or not PR-SCTP support
> > was negotiated on a particular association.
> >
> > Whether or not PR-SCTP is enabled by default is implementation
> > specific.
> >
> > This socket option uses IPPROTO_SCTP as its level and
> > SCTP_PR_SUPPORTED as its name. It can be used with getsockopt() and
> > setsockopt(). The socket option value uses the following structure
> > defined in [RFC6458]:
> >
> > struct sctp_assoc_value {
> > sctp_assoc_t assoc_id;
> > uint32_t assoc_value;
> > };
> >
> > assoc_id: This parameter is ignored for one-to-one style sockets.
> > For one-to-many style sockets, this parameter indicates upon which
> > association the user is performing an action. The special
> > sctp_assoc_t SCTP_FUTURE_ASSOC can also be used; it is an error to
> > use SCTP_{CURRENT|ALL}_ASSOC in assoc_id.
> >
> > assoc_value: A non-zero value encodes the enabling of PR-SCTP,
> > whereas a value of 0 encodes the disabling of PR-SCTP.
> >
> > sctp_opt_info() needs to be extended to support SCTP_PR_SUPPORTED
> >
> > My read of this suggests that for setting the prsctp_enabled flag, we only need
> > a valid socket (the presence or lack of associations is irrelevant), its only
> > for the getsockopt method that we need to specify an assoc_id, as the getsockopt
> > method operates on associations, while the setsockopt method operates at the
> > socket level (to be inherited as association init).
> >
> > Given that, I'd argue that we can skip the check entirely, and just assign
> > sctp_sock(sk)->prsctp_enabled = !!param.assoc_value
> >
> > directly.
> RFC seems to have no clear demands for this, I will just drop the check
> in this patch, thanks.
RFC may not have clear demands, but I still don't see a reason for not
rejecting bogus arguments that can potentially lead to confusion.
We usually do argument parsing in the other way around: restrict as
much as possible, and relax when needed. That avoids applications to
build bad behaviors that we would end up having to cope with it.
Anyhow, I won't oppose to this any further.
@Dave: please give me till Tue to review the other patches. I'm
traveling and will be offline till Mon night. Thanks.
Marcelo
^ permalink raw reply
* Re: [Patch net] net: invert the check of detecting hardware RX checksum fault
From: Herbert Xu @ 2018-11-19 4:01 UTC (permalink / raw)
To: Cong Wang; +Cc: Linux Kernel Network Developers, Tom Herbert, Eric Dumazet
In-Reply-To: <CAM_iQpXxc5BuC5m_Ut7DxPpkVgEgEmZc=D8=NjzMSfZxNw06EA@mail.gmail.com>
On Fri, Nov 16, 2018 at 01:32:50PM -0800, Cong Wang wrote:
>
> This is true only when there is a skb_checksum_init*() or
> skb_checksum_validate*() prior to it, it seems not true for
> nf_ip_checksum() where skb->csum is correctly set to pesudo header
> checksum but there is no validation of the original skb->csum.
> So this check should be still inverted there??
>
> Or am I still missing anything here?
What do you mean? My copy of nf_ip_checksum seems to be doing the
right thing as far as verifying CHECKSUM_COMPLETED goes.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* netns_id in bpf_sk_lookup_{tcp,udp}
From: David Ahern @ 2018-11-19 3:26 UTC (permalink / raw)
To: joe; +Cc: netdev@vger.kernel.org
Hi Joe:
The netns_id to the bpf_sk_lookup_{tcp,udp} functions in
net/core/filter.c is a u64, yet the APIs in include/uapi/linux/bpf.h
shows a u32. Is that intentional or an oversight through the iterations?
David
^ permalink raw reply
* [PATCH v2 RESEND 1/2] mm/page_alloc: free order-0 pages through PCP in page_frag_free()
From: Aaron Lu @ 2018-11-19 13:48 UTC (permalink / raw)
To: linux-mm, linux-kernel, netdev
Cc: Andrew Morton, Paweł Staszewski, Jesper Dangaard Brouer,
Eric Dumazet, Tariq Toukan, Ilias Apalodimas, Yoel Caspersen,
Mel Gorman, Saeed Mahameed, Michal Hocko, Vlastimil Babka,
Dave Hansen, Alexander Duyck, Ian Kumlien
In-Reply-To: <20181119134834.17765-1-aaron.lu@intel.com>
page_frag_free() calls __free_pages_ok() to free the page back to
Buddy. This is OK for high order page, but for order-0 pages, it
misses the optimization opportunity of using Per-Cpu-Pages and can
cause zone lock contention when called frequently.
Paweł Staszewski recently shared his result of 'how Linux kernel
handles normal traffic'[1] and from perf data, Jesper Dangaard Brouer
found the lock contention comes from page allocator:
mlx5e_poll_tx_cq
|
--16.34%--napi_consume_skb
|
|--12.65%--__free_pages_ok
| |
| --11.86%--free_one_page
| |
| |--10.10%--queued_spin_lock_slowpath
| |
| --0.65%--_raw_spin_lock
|
|--1.55%--page_frag_free
|
--1.44%--skb_release_data
Jesper explained how it happened: mlx5 driver RX-page recycle
mechanism is not effective in this workload and pages have to go
through the page allocator. The lock contention happens during
mlx5 DMA TX completion cycle. And the page allocator cannot keep
up at these speeds.[2]
I thought that __free_pages_ok() are mostly freeing high order
pages and thought this is an lock contention for high order pages
but Jesper explained in detail that __free_pages_ok() here are
actually freeing order-0 pages because mlx5 is using order-0 pages
to satisfy its page pool allocation request.[3]
The free path as pointed out by Jesper is:
skb_free_head()
-> skb_free_frag()
-> page_frag_free()
And the pages being freed on this path are order-0 pages.
Fix this by doing similar things as in __page_frag_cache_drain() -
send the being freed page to PCP if it's an order-0 page, or
directly to Buddy if it is a high order page.
With this change, Paweł hasn't noticed lock contention yet in
his workload and Jesper has noticed a 7% performance improvement
using a micro benchmark and lock contention is gone. Ilias' test
on a 'low' speed 1Gbit interface on an cortex-a53 shows ~11%
performance boost testing with 64byte packets and __free_pages_ok()
disappeared from perf top.
[1]: https://www.spinics.net/lists/netdev/msg531362.html
[2]: https://www.spinics.net/lists/netdev/msg531421.html
[3]: https://www.spinics.net/lists/netdev/msg531556.html
Reported-by: Paweł Staszewski <pstaszewski@itcare.pl>
Analysed-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Acked-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
Acked-by: Tariq Toukan <tariqt@mellanox.com
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
mm/page_alloc.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 421c5b652708..8f8c6b33b637 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4677,8 +4677,14 @@ void page_frag_free(void *addr)
{
struct page *page = virt_to_head_page(addr);
- if (unlikely(put_page_testzero(page)))
- __free_pages_ok(page, compound_order(page));
+ if (unlikely(put_page_testzero(page))) {
+ unsigned int order = compound_order(page);
+
+ if (order == 0)
+ free_unref_page(page);
+ else
+ __free_pages_ok(page, order);
+ }
}
EXPORT_SYMBOL(page_frag_free);
--
2.17.2
^ permalink raw reply related
* [PATCH RESEND 0/2] free order-0 pages through PCP in page_frag_free() and cleanup
From: Aaron Lu @ 2018-11-19 13:48 UTC (permalink / raw)
To: linux-mm, linux-kernel, netdev
Cc: Andrew Morton, Paweł Staszewski, Jesper Dangaard Brouer,
Eric Dumazet, Tariq Toukan, Ilias Apalodimas, Yoel Caspersen,
Mel Gorman, Saeed Mahameed, Michal Hocko, Vlastimil Babka,
Dave Hansen, Alexander Duyck, Ian Kumlien
This is a resend of the two patches.
Patch 1 is the same as:
[PATCH v2 1/2] mm/page_alloc: free order-0 pages through PCP in page_frag_free()
https://lkml.kernel.org/r/20181106052833.GC6203@intel.com
With one more ack from Tariq Toukan.
Patch 2 is the same as:
[PATCH v3 2/2] mm/page_alloc: use a single function to free page
https://lkml.kernel.org/r/20181106113149.GC24198@intel.com
With some changelog rewording.
Applies on top of v4.20-rc2-mmotm-2018-11-16-14-52.
Aaron Lu (2):
mm/page_alloc: free order-0 pages through PCP in page_frag_free()
mm/page_alloc: use a single function to free page
mm/page_alloc.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
--
2.17.2
^ permalink raw reply
* Re: DSA support for Marvell 88e6065 switch
From: Andrew Lunn @ 2018-11-19 3:25 UTC (permalink / raw)
To: Pavel Machek; +Cc: netdev, f.fainelli, buytenh, buytenh, nico
In-Reply-To: <20181118201528.GA18518@amd>
> If I wanted it to work, what do I need to do? AFAICT phy autoprobing
> should just attach it as soon as it is compiled in?
Nope. It is a switch, not a PHY. Switches are never auto-probed
because they are not guaranteed to have ID registers.
You need to use the legacy device tree binding. Look in
Documentation/devicetree/bindings/net/dsa/dsa.txt, section Deprecated
Binding. You can get more examples if you checkout old kernels. Or
kirkwood-rd88f6281.dtsi, the dsa { } node which is disabled.
Andrew
^ 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