* [PATCH net-next] net: phy: Trigger state machine on state change and not polling.
From: Andrew Lunn @ 2016-10-12 20:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Florian Fainelli, Andrew Lunn, Kyle Roeschley
The phy_start() is used to indicate the PHY is now ready to do its
work. The state is changed, normally to PHY_UP which means that both
the MAC and the PHY are ready.
If the phy driver is using polling, when the next poll happens, the
state machine notices the PHY is now in PHY_UP, and kicks off
auto-negotiation, if needed.
If however, the PHY is using interrupts, there is no polling. The phy
is stuck in PHY_UP until the next interrupt comes along. And there is
no reason for the PHY to interrupt.
Have phy_start() schedule the state machine to run, which both speeds
up the polling use case, and makes the interrupt use case actually
work.
This problems exists whenever there is a state change which will not
cause an interrupt. Trigger the state machine in these cases,
e.g. phy_error().
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Cc: Kyle Roeschley <kyle.roeschley@ni.com>
---
This should be applied to stable, but i've no idea what fixes: tag to
use. It could be phylib has been broken since interrupts were added?
drivers/net/phy/phy.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c6f66832a1a6..f424b867f73e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -608,6 +608,21 @@ void phy_start_machine(struct phy_device *phydev)
}
/**
+ * phy_trigger_machine - trigger the state machine to run
+ *
+ * @phydev: the phy_device struct
+ *
+ * Description: There has been a change in state which requires that the
+ * state machine runs.
+ */
+
+static void phy_trigger_machine(struct phy_device *phydev)
+{
+ cancel_delayed_work_sync(&phydev->state_queue);
+ queue_delayed_work(system_power_efficient_wq, &phydev->state_queue, 0);
+}
+
+/**
* phy_stop_machine - stop the PHY state machine tracking
* @phydev: target phy_device struct
*
@@ -639,6 +654,8 @@ static void phy_error(struct phy_device *phydev)
mutex_lock(&phydev->lock);
phydev->state = PHY_HALTED;
mutex_unlock(&phydev->lock);
+
+ phy_trigger_machine(phydev);
}
/**
@@ -800,8 +817,7 @@ void phy_change(struct work_struct *work)
}
/* reschedule state queue work to run as soon as possible */
- cancel_delayed_work_sync(&phydev->state_queue);
- queue_delayed_work(system_power_efficient_wq, &phydev->state_queue, 0);
+ phy_trigger_machine(phydev);
return;
ignore:
@@ -890,6 +906,8 @@ void phy_start(struct phy_device *phydev)
/* if phy was suspended, bring the physical link up again */
if (do_resume)
phy_resume(phydev);
+
+ phy_trigger_machine(phydev);
}
EXPORT_SYMBOL(phy_start);
--
2.9.3
^ permalink raw reply related
* Re: [RFC] net: phy: smsc: Disable auto-negotiation on startup
From: Andrew Lunn @ 2016-10-12 19:55 UTC (permalink / raw)
To: Kyle Roeschley; +Cc: Florian Fainelli, netdev, linux-kernel
In-Reply-To: <20161012150533.dctakcjczclk2mml@senary>
On Wed, Oct 12, 2016 at 10:05:33AM -0500, Kyle Roeschley wrote:
> On Wed, Oct 12, 2016 at 02:13:06AM -0700, Florian Fainelli wrote:
> > On 10/10/2016 10:41 AM, Kyle Roeschley wrote:
> > > Because the SMSC PHY completes auto-negotiation before the driver is
> > > ready to handle interrupts, the PHY state machine never realizes that we
> > > have a link. Clear the ANENABLE bit on initialization, which lets
> > > genphy_config_aneg do its thing when that code is hit later.
> > >
> > > While this patch does fix the problem we see (no link on boot without
> > > re-plugging the cable), it seems like the generic PHY code should be
> > > able to handle auto-negotiation completing before interrupts are
> > > enabled. Submitted as an RFC in the hopes that someone has an idea as to
> > > how that could be done.
> > >
> > > This fix is copied from commit 99f81afc139c ("phy: micrel: Disable auto
> > > negotiation on startup").
> >
> > Do you mind trying:
> >
> > https://www.spinics.net/lists/netdev/msg397857.html
> >
> > and see if you do get link interrupts without your patch applied? Thanks!
>
> Yep, that fixes it. I figured there was some state machine issue I was missing.
> Thanks very much!
Humm, O.K, time to pull that patch out of the series and make it
standalone.
Andrew
^ permalink raw reply
* [PATCH] net: wan: slic_ds26522: Allow driver to built if COMPILE_TEST is enabled
From: Javier Martinez Canillas @ 2016-10-12 19:05 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, netdev, Zhao Qiang, David S. Miller
The driver only has runtime but no build time dependency with FSL_SOC ||
ARCH_MXC || ARCH_LAYERSCAPE. So it can be built for testing purposes if
the COMPILE_TEST option is enabled.
This is useful to have more build coverage and make sure that the driver
is not affected by changes that could cause build regressions.
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
drivers/net/wan/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig
index 33ab3345d333..4e9fe75d7067 100644
--- a/drivers/net/wan/Kconfig
+++ b/drivers/net/wan/Kconfig
@@ -294,7 +294,7 @@ config FSL_UCC_HDLC
config SLIC_DS26522
tristate "Slic Maxim ds26522 card support"
depends on SPI
- depends on FSL_SOC || ARCH_MXC || ARCH_LAYERSCAPE
+ depends on FSL_SOC || ARCH_MXC || ARCH_LAYERSCAPE || COMPILE_TEST
help
This module initializes and configures the slic maxim card
in T1 or E1 mode.
--
2.7.4
^ permalink raw reply related
* [PATCH 2/2] net: wan: slic_ds26522: Export OF module alias information
From: Javier Martinez Canillas @ 2016-10-12 18:55 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, netdev, Zhao Qiang, David S. Miller
In-Reply-To: <1476298541-7354-1-git-send-email-javier@osg.samsung.com>
When the device is registered via OF, the OF table is used to match the
driver instead of the SPI device ID table, but the entries in the later
are used as aliasses to load the module if the driver was not built-in.
This is because the SPI core always reports an SPI module alias instead
of an OF one, but that could change so it's better to always export it.
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
drivers/net/wan/slic_ds26522.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wan/slic_ds26522.c b/drivers/net/wan/slic_ds26522.c
index 53366a2232f0..b776a0ab106c 100644
--- a/drivers/net/wan/slic_ds26522.c
+++ b/drivers/net/wan/slic_ds26522.c
@@ -235,6 +235,7 @@ static const struct of_device_id slic_ds26522_match[] = {
},
{},
};
+MODULE_DEVICE_TABLE(of, slic_ds26522_match);
static struct spi_driver slic_ds26522_driver = {
.driver = {
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] net: wan: slic_ds26522: add SPI device ID table to fix module autoload
From: Javier Martinez Canillas @ 2016-10-12 18:55 UTC (permalink / raw)
To: linux-kernel
Cc: Javier Martinez Canillas, netdev, Zhao Qiang, David S. Miller
If the driver is built as a module, module alias information isn't filled
so the module won't be autoloaded. Add a SPI device ID table and use the
MODULE_DEVICE_TABLE() macro so the information is exported in the module.
Before this patch:
$ modinfo drivers/net/wan/slic_ds26522.ko | grep alias
$
After this patch:
$ modinfo drivers/net/wan/slic_ds26522.ko | grep alias
alias: spi:ds26522
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
drivers/net/wan/slic_ds26522.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/wan/slic_ds26522.c b/drivers/net/wan/slic_ds26522.c
index d06a887a2352..53366a2232f0 100644
--- a/drivers/net/wan/slic_ds26522.c
+++ b/drivers/net/wan/slic_ds26522.c
@@ -223,6 +223,12 @@ static int slic_ds26522_probe(struct spi_device *spi)
return ret;
}
+static const struct spi_device_id slic_ds26522_id[] = {
+ { .name = "ds26522" },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(spi, slic_ds26522_id);
+
static const struct of_device_id slic_ds26522_match[] = {
{
.compatible = "maxim,ds26522",
@@ -239,6 +245,7 @@ static struct spi_driver slic_ds26522_driver = {
},
.probe = slic_ds26522_probe,
.remove = slic_ds26522_remove,
+ .id_table = slic_ds26522_id,
};
static int __init slic_ds26522_init(void)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning
From: Chris Rorvick @ 2016-10-12 18:36 UTC (permalink / raw)
To: Paul Bolle
Cc: Luca Coelho, Intel Linux Wireless, Emmanuel Grumbach,
Johannes Berg, Kalle Valo, Oren Givon, linux-wireless, netdev,
linux-kernel
In-Reply-To: <1476295541.9670.26.camel@tiscali.nl>
On Wed, Oct 12, 2016 at 1:05 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> On Wed, 2016-10-12 at 12:50 -0500, Chris Rorvick wrote:
>> This may already be apparent, but Dell sells two versions of the 9350:
>> one with the Broadcom adapter and one with the AC 8260.
>
> Off topic, for most readers: my version (with the AC 8260) came with
> Ubuntu preinstalled. Perhaps Chris' version came preinstalled with
> something else?
Exactly. Mine came with Windows 10 and the Broadcom adapter, while
the "Developer Edition" comes preloaded with Ubuntu and has the AC
8260. The Broadcom adapter has been supported since 4.4 but still
seems to have issues. For example, I had it working at home but I was
not able to connect to a friend's AT&T U-Verse wireless gateway;
something was failing with -EBUSY. I ordered the AC 8260 and it works
fine after the upgrade.
Chris
^ permalink raw reply
* Re: igb driver can cause cache invalidation of non-owned memory?
From: Alexander Duyck @ 2016-10-12 18:30 UTC (permalink / raw)
To: Nikita Yushchenko
Cc: David Laight, Eric Dumazet, David Miller, Jeff Kirsher,
intel-wired-lan, Netdev, linux-kernel@vger.kernel.org,
Chris Healy
In-Reply-To: <d34b7175-1024-8cc9-d422-d9f75f56c069@cogentembedded.com>
On Wed, Oct 12, 2016 at 11:12 AM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>> It would make more sense to update the DMA API for
>> __dma_page_cpu_to_dev on ARM so that you don't invalidate the cache if
>> the direction is DMA_FROM_DEVICE.
>
> No, in generic case it's unsafe.
>
> If CPU issued a write to a location, and sometime later that location is
> used as DMA buffer, there is danger that write is still in cache only,
> and writeback is pending. Later this writeback can overwrite data
> written to memory via DMA, causing corruption.
Okay so if I understand it correctly then the invalidation in
sync_for_device is to force any writes to be flushed out, and the
invalidation in sync_for_cpu is to flush out any speculative reads.
So without speculative reads then the sync_for_cpu portion is not
needed. You might want to determine if the core you are using
supports the speculative reads, if not you might be able to get away
without having to do the sync_for_cpu at all.
>> The point I was trying to make is that you are invalidating the cache
>> in both the sync_for_device and sync_for_cpu. Do you really need that
>> for ARM or do you need to perform the invalidation on sync_for_device
>> if that may be pushed out anyway? If you aren't running with with
>> speculative look-ups do you even need the invalidation in
>> sync_for_cpu?
>
> I'm not lowlevel arm guru and I don't know for sure. Probably another
> CPU core can be accessing locations neighbor page, causing specilative
> load of locations in DMA page.
>
>
>> Changing the driver code for this won't necessarily work on all
>> architectures, and on top of it we have some changes planned which
>> will end up making the pages writable in the future to support the
>> ongoing XDP effort. That is one of the reasons why I would not be
>> okay with changing the driver to make this work.
>
> Well I was not really serious about removing that sync_for_device() in
> mainline :) Although >20% throughput win that this provides is
> impressive...
I agree the improvement is pretty impressive. The think is there are
similar gains we can generate on x86 by stripping out bits and pieces
that are needed for other architectures. I'm just wanting to make
certain we aren't optimizing for one architecture at the detriment of
others.
> But what about doing something safer, e.g. adding a bit of tracking and
> only sync_for_device() what was previously sync_for_cpu()ed? Will you
> accept that?
>
> Nikita
The problem is that as we move things over for XDP we will be looking
at having the CPU potentially write to any spot in the region that was
mapped as we could append headers to the front or pad data onto the
end of the frame. It is probably safest for us to invalidate the
entire region just to make sure we don't have a collision with
something that is writing to the page.
So for example in the near future I am planning to expand out the
DMA_ATTR_SKIP_CPU_SYNC DMA attribute beyond just the ARM architecture
to see if I can expand it for use with SWIOTLB. If we can use this on
unmap we might be able to solve some of the existing problems that
required us to make the page read-only since we could unmap the page
without invalidating any existing writes on the page.
- Alex
^ permalink raw reply
* Re: [PATCH] IB/ipoib: move back the IB LL address into the hard header
From: Doug Ledford @ 2016-10-12 18:21 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Paolo Abeni, linux-rdma, Sean Hefty, Hal Rosenstock, netdev
In-Reply-To: <9bed7085-949b-c68b-91ed-f930702a6a94@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 1339 bytes --]
On 10/11/2016 2:50 PM, Doug Ledford wrote:
> On 10/11/2016 2:30 PM, Jason Gunthorpe wrote:
>> On Tue, Oct 11, 2016 at 02:17:51PM -0400, Doug Ledford wrote:
>>
>>> Well, not exactly. Even if we put 65520 into the scripts, the kernel
>>> will silently drop it down to 65504. It actually won't require anyone
>>> change anything, they just won't get the full value. I experimented
>>> with this in the past for other reasons and an overly large MTU setting
>>> just resulted in the max MTU. I don't know if that's changed, but if it
>>> still works that way, this is much less of an issue than it might
>>> otherwise be.
>>
>> So it is just docs and relying on PMTU? That is not as bad..
>>
>> Still would be nice to avoid if at all possible..
>
> I agree, but we have a test getting ready to commence. We'll know
> shortly how much the reduced MTU effects things because they aren't
> going to alter any of their setup, just put the new kernel in place, and
> see what happens.
>
>
Long story short on the MTU stuff, the setups whined a bit about not
being able to set the desired MTU, used the new max MTU instead, and
things otherwise worked fine. But, Paolo submitted a v2 patch that
removes this change, so it's all moot anyway.
--
Doug Ledford <dledford@redhat.com>
GPG Key ID: 0E572FDD
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]
^ permalink raw reply
* Re: igb driver can cause cache invalidation of non-owned memory?
From: Nikita Yushchenko @ 2016-10-12 18:12 UTC (permalink / raw)
To: Alexander Duyck
Cc: David Laight, Eric Dumazet, David Miller, Jeff Kirsher,
intel-wired-lan, Netdev, linux-kernel@vger.kernel.org,
Chris Healy
In-Reply-To: <CAKgT0UdqwpjP0b4knt2d4DqS1Z9dR+vg-smkgBTZDeACGGdx=Q@mail.gmail.com>
> It would make more sense to update the DMA API for
> __dma_page_cpu_to_dev on ARM so that you don't invalidate the cache if
> the direction is DMA_FROM_DEVICE.
No, in generic case it's unsafe.
If CPU issued a write to a location, and sometime later that location is
used as DMA buffer, there is danger that write is still in cache only,
and writeback is pending. Later this writeback can overwrite data
written to memory via DMA, causing corruption.
> The point I was trying to make is that you are invalidating the cache
> in both the sync_for_device and sync_for_cpu. Do you really need that
> for ARM or do you need to perform the invalidation on sync_for_device
> if that may be pushed out anyway? If you aren't running with with
> speculative look-ups do you even need the invalidation in
> sync_for_cpu?
I'm not lowlevel arm guru and I don't know for sure. Probably another
CPU core can be accessing locations neighbor page, causing specilative
load of locations in DMA page.
> Changing the driver code for this won't necessarily work on all
> architectures, and on top of it we have some changes planned which
> will end up making the pages writable in the future to support the
> ongoing XDP effort. That is one of the reasons why I would not be
> okay with changing the driver to make this work.
Well I was not really serious about removing that sync_for_device() in
mainline :) Although >20% throughput win that this provides is
impressive...
But what about doing something safer, e.g. adding a bit of tracking and
only sync_for_device() what was previously sync_for_cpu()ed? Will you
accept that?
Nikita
^ permalink raw reply
* Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning
From: Paul Bolle @ 2016-10-12 18:05 UTC (permalink / raw)
To: Chris Rorvick, Luca Coelho
Cc: Intel Linux Wireless, Emmanuel Grumbach, Johannes Berg,
Kalle Valo, Oren Givon, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAEUsAPYYJ3Gmh0T16veCn3wnzdD8bTxE+_U-AUYQpMo3TUd4Mg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Wed, 2016-10-12 at 12:50 -0500, Chris Rorvick wrote:
> This may already be apparent, but Dell sells two versions of the 9350:
> one with the Broadcom adapter and one with the AC 8260.
Off topic, for most readers: my version (with the AC 8260) came with
Ubuntu preinstalled. Perhaps Chris' version came preinstalled with
something else?
Thanks,
Paul Bolle
^ permalink raw reply
* Re: igb driver can cause cache invalidation of non-owned memory?
From: Alexander Duyck @ 2016-10-12 17:56 UTC (permalink / raw)
To: Nikita Yushchenko
Cc: David Laight, Eric Dumazet, David Miller, Jeff Kirsher,
intel-wired-lan, Netdev, linux-kernel@vger.kernel.org,
Chris Healy
In-Reply-To: <cbd0f41d-bc01-3f93-9171-2795c61afdc9@cogentembedded.com>
On Wed, Oct 12, 2016 at 9:11 AM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>>>> To get some throughput improvement, I propose removal of that
>>>> sync_for_device() before reusing buffer. Will you accept such a patch ;)
>>>
>>> Not one that gets rid of sync_for_device() in the driver. From what I
>>> can tell there are some DMA APIs that use that to perform the
>>> invalidation on the region of memory so that it can be DMAed into.
>>> Without that we run the risk of having a race between something the
>>> CPU might have placed in the cache and something the device wrote into
>>> memory. The sync_for_device() call is meant to invalidate the cache
>>> for the region so that when the device writes into memory there is no
>>> risk of that race.
>>
>> I'm not expert, but some thought...
>>
>> Just remember that the cpu can do speculative memory and cache line reads.
>> So you need to ensure there are no dirty cache lines when the receive
>> buffer is setup and no cache lines at all at before looking at the frame.
>
> Exactly.
>
> And because of that, arm does cache invalidation both in sync_for_cpu()
> and sync_for_device().
>
>> If you can 100% guarantee the cpu hasn't dirtied the cache then I
>> think the invalidate prior to reusing the buffer can be skipped.
>> But I wouldn't want to debug that going wrong.
>
> I've written earlier in this thread why it is the case for igb - as long
> as Alexander's statement that igb's buffers are readonly for the stack
> is true. If Alexander's statement is wrong, then igb is vulnerable to
> issue I've described in the first message of this thread.
>
> Btw, we are unable get any breakage with that sync_to_device() removed
> already for a day. And - our tests run with software checksumming,
> because on our hardware i210 is wired connected to DSA switch and in
> this config, no i210 offloading works (i210 hardware gets frames with
> eDSA headers that it can't parse). Thus any breakage should be
> immediately discovered.
>
> And throughput measured by iperf raises from 650 to 850 Mbps.
>
> Nikita
The point I was trying to make is that you are invalidating the cache
in both the sync_for_device and sync_for_cpu. Do you really need that
for ARM or do you need to perform the invalidation on sync_for_device
if that may be pushed out anyway? If you aren't running with with
speculative look-ups do you even need the invalidation in
sync_for_cpu? The underlying problem lives in the code for
__dma_page_cpu_to_dev and __dma_page_dev_to_cpu. It seems like they
are trying to solve for both speculative and non-speculative setups
and having "FIXME" comments in the code related to this kind of points
to your problems being there.
Changing the driver code for this won't necessarily work on all
architectures, and on top of it we have some changes planned which
will end up making the pages writable in the future to support the
ongoing XDP effort. That is one of the reasons why I would not be
okay with changing the driver to make this work.
It would make more sense to update the DMA API for
__dma_page_cpu_to_dev on ARM so that you don't invalidate the cache if
the direction is DMA_FROM_DEVICE.
- Alex
^ permalink raw reply
* Re: [PATCH] iwlwifi: pcie: reduce "unsupported splx" to a warning
From: Chris Rorvick @ 2016-10-12 17:50 UTC (permalink / raw)
To: Paul Bolle, Luca Coelho
Cc: Intel Linux Wireless, Emmanuel Grumbach, Johannes Berg,
Kalle Valo, Oren Givon, linux-wireless, netdev, linux-kernel
In-Reply-To: <1476255135.1972.5.camel@tiscali.nl>
Hi Luca,
FYI, It seems that Google does not like your email as I'm not
receiving any of your messages in gmail. Some responses below:
On Wed, 2016-10-12 at 15:24 +0300, Luca Coelho wrote:
> Hi Chris,
> On Tue, 2016-10-11 at 09:09 -0500, Chris Rorvick wrote:
> > On Tue, Oct 11, 2016 at 5:11 AM, Paul Bolle <pebolle [at] tiscali> wrote:
> > > > This is not coming from the NIC itself, but from the platform's ACPI
> > > > tables. Can you tell us which platform you are using?
> >
> > Interesting. I'm running a Dell XPS 13 9350. I replaced the
> > factory-provided Broadcom card with an AC 8260. I can update the
> > commit log to reflect this.
>
> Okay, so this makes sense. Those entries are probably formatted for
> the Broadcom card, which the iwlwifi driver obviously doesn't
> understand. The best we can do, as I already said, is to ignore values
> we don't understand.
This may already be apparent, but Dell sells two versions of the 9350:
one with the Broadcom adapter and one with the AC 8260. I just
happened to find the former version at a deep discount at Costco so
decided to chance it. Turns out the Broadcom card is not so good even
with new kernels so I upgraded. Anyway, since Paul is seeing the same
issue I don't think the values are intended to be Broadcom-specific.
On Wed, 2016-10-12 at 17:21 +0300, Luca Coelho wrote:
> And, the values in the SPLX structs are being changed here, to DOM1,
> LIM1, TIM1 etc., before being returned. Â This also matches your
> description that, at runtime, you got something different than the pure
> dump. Â If you follow these DOM*, LIM*, TIM* symbols, you'll probably
> end up getting the values you observed at runtime.
Probably not important, but it seems that there is some additional
indirection. The only values I'm seeing associated with those symbols
are 8 and 16:
$ grep -e 'DOM[0-9]' -e 'LIM[0-9]' -e 'TIM[0-9]' dsdt.dsl | grep -v Store
DOM1, 8,
LIM1, 16,
TIM1, 16,
DOM2, 8,
LIM2, 16,
TIM2, 16,
DOM3, 8,
LIM3, 16,
TIM3, 16,
> I'll send you a patch for testing soon.
I will keep an eye on the list archive, thanks!
Chris
^ permalink raw reply
* Re: [RFC] net: phy: smsc: Disable auto-negotiation on startup
From: Kyle Roeschley @ 2016-10-12 15:09 UTC (permalink / raw)
To: Jeremy Linton; +Cc: f.fainelli, netdev, linux-kernel
In-Reply-To: <4ced57d0-febe-181c-5f41-41ff3e018857@arm.com>
On Tue, Oct 11, 2016 at 09:32:30AM -0500, Jeremy Linton wrote:
> On 10/10/2016 12:41 PM, Kyle Roeschley wrote:
> > Because the SMSC PHY completes auto-negotiation before the driver is
> > ready to handle interrupts, the PHY state machine never realizes that we
> > have a link. Clear the ANENABLE bit on initialization, which lets
> > genphy_config_aneg do its thing when that code is hit later.
> >
> > While this patch does fix the problem we see (no link on boot without
> > re-plugging the cable), it seems like the generic PHY code should be
> > able to handle auto-negotiation completing before interrupts are
> > enabled. Submitted as an RFC in the hopes that someone has an idea as to
> > how that could be done.
>
> Hi,
>
> Which smsc chip/driver? Maybe assuring the device interrupts are enabled
> before the phy is started is a solution?
>
> The whole problem sounds similar to what was recently happening in the
> smsc911x driver, but AFAIK that driver is basically only polling at this
> point so connecting the phy before the interrupts are enabled shouldn't be a
> problem.
>
We're using the SMSC LAN8720A with the Cadence MACB ethernet controller.
Interrupts are enabled before the phy is started, but it looks like the patch
Florian pointed me to (https://www.spinics.net/lists/netdev/msg397857.html)
fixes my interrupt problem.
> >
> > This fix is copied from commit 99f81afc139c ("phy: micrel: Disable auto
> > negotiation on startup").
> >
> > Signed-off-by: Kyle Roeschley <kyle.roeschley@ni.com>
> > ---
> > drivers/net/phy/smsc.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> > index b62c4aa..8de8011 100644
> > --- a/drivers/net/phy/smsc.c
> > +++ b/drivers/net/phy/smsc.c
> > @@ -62,6 +62,16 @@ static int smsc_phy_config_init(struct phy_device *phydev)
> > return rc;
> > }
> >
> > + if (phy_interrupt_is_valid(phydev)) {
> > + rc = phy_read(phydev, MII_BMCR);
> > + if (rc < 0)
> > + return rc;
> > +
> > + rc = phy_write(phydev, MII_BMCR, rc & ~BMCR_ANENABLE);
> > + if (rc < 0)
> > + return rc;
> > + }
> > +
> > return smsc_phy_ack_interrupt(phydev);
> > }
> >
> >
>
--
Kyle Roeschley
Software Engineer
National Instruments
^ permalink raw reply
* [PATCH] net: limit a number of namespaces which can be cleaned up concurrently
From: Andrei Vagin @ 2016-10-12 17:32 UTC (permalink / raw)
To: netdev-u79uwXL29TY76Z2rM5mHXA
Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Eric W. Biederman, Andrey Vagin, David S. Miller
From: Andrey Vagin <avagin-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
The operation of destroying netns is heavy and it is executed under
net_mutex. If many namespaces are destroyed concurrently, net_mutex can
be locked for a long time. It is impossible to create a new netns during
this period of time.
In our days when userns allows to create network namespaces to
unprivilaged users, it may be a real problem.
On my laptop (fedora 24, i5-5200U, 12GB) 1000 namespaces requires about
300MB of RAM and are being destroyed for 8 seconds.
In this patch, a number of namespaces which can be cleaned up
concurrently is limited by 32. net_mutex is released after handling each
portion of net namespaces and then it is locked again to handle the next
one. It allows other users to lock it without waiting for a long
time.
I am not sure whether we need to add a sysctl to costomize this limit.
Let me know if you think it's required.
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Andrei Vagin <avagin-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
---
net/core/net_namespace.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index 989434f..33dd3b7 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -406,10 +406,20 @@ static void cleanup_net(struct work_struct *work)
struct net *net, *tmp;
struct list_head net_kill_list;
LIST_HEAD(net_exit_list);
+ int i = 0;
/* Atomically snapshot the list of namespaces to cleanup */
spin_lock_irq(&cleanup_list_lock);
- list_replace_init(&cleanup_list, &net_kill_list);
+ list_for_each_entry_safe(net, tmp, &cleanup_list, cleanup_list)
+ if (++i == 32)
+ break;
+ if (i == 32) {
+ list_cut_position(&net_kill_list,
+ &cleanup_list, &net->cleanup_list);
+ queue_work(netns_wq, work);
+ } else {
+ list_replace_init(&cleanup_list, &net_kill_list);
+ }
spin_unlock_irq(&cleanup_list_lock);
mutex_lock(&net_mutex);
--
2.7.4
^ permalink raw reply related
* [PATCH net] ipv6: tcp: restore IP6CB for pktoptions skbs
From: Eric Dumazet @ 2016-10-12 17:01 UTC (permalink / raw)
To: Baozeng Ding, David Miller; +Cc: netdev
In-Reply-To: <21019976-9e40-5e6d-b733-fa9eced710f0@gmail.com>
From: Eric Dumazet <edumazet@google.com>
Baozeng Ding reported following KASAN splat :
BUG: KASAN: use-after-free in ip6_datagram_recv_specific_ctl+0x13f1/0x15c0 at addr ffff880029c84ec8
Read of size 1 by task poc/25548
Call Trace:
[<ffffffff82cf43c9>] dump_stack+0x12e/0x185 /lib/dump_stack.c:15
[< inline >] print_address_description /mm/kasan/report.c:204
[<ffffffff817ced3b>] kasan_report_error+0x48b/0x4b0 /mm/kasan/report.c:283
[< inline >] kasan_report /mm/kasan/report.c:303
[<ffffffff817ced9e>] __asan_report_load1_noabort+0x3e/0x40 /mm/kasan/report.c:321
[<ffffffff85c71da1>] ip6_datagram_recv_specific_ctl+0x13f1/0x15c0 /net/ipv6/datagram.c:687
[<ffffffff85c734c3>] ip6_datagram_recv_ctl+0x33/0x40
[<ffffffff85c0b07c>] do_ipv6_getsockopt.isra.4+0xaec/0x2150
[<ffffffff85c0c7f6>] ipv6_getsockopt+0x116/0x230
[<ffffffff859b5a12>] tcp_getsockopt+0x82/0xd0 /net/ipv4/tcp.c:3035
[<ffffffff855fb385>] sock_common_getsockopt+0x95/0xd0 /net/core/sock.c:2647
[< inline >] SYSC_getsockopt /net/socket.c:1776
[<ffffffff855f8ba2>] SyS_getsockopt+0x142/0x230 /net/socket.c:1758
[<ffffffff8685cdc5>] entry_SYSCALL_64_fastpath+0x23/0xc6
Memory state around the buggy address:
ffff880029c84d80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff880029c84e00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ffff880029c84e80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
^
ffff880029c84f00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff880029c84f80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
He also provided a syzkaller reproducer.
Issue is that ip6_datagram_recv_specific_ctl() expects to find IP6CB
data that was moved at a different place in tcp_v6_rcv()
This patch moves tcp_v6_restore_cb() up and calls it from
tcp_v6_do_rcv() when np->pktoptions is set.
Fixes: 971f10eca186 ("tcp: better TCP_SKB_CB layout to reduce cache line misses")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Baozeng Ding <sploving1@gmail.com>
---
net/ipv6/tcp_ipv6.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 54cf7197c7ab..5a27ab4eab39 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1190,6 +1190,16 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
return NULL;
}
+static void tcp_v6_restore_cb(struct sk_buff *skb)
+{
+ /* We need to move header back to the beginning if xfrm6_policy_check()
+ * and tcp_v6_fill_cb() are going to be called again.
+ * ip6_datagram_recv_specific_ctl() also expects IP6CB to be there.
+ */
+ memmove(IP6CB(skb), &TCP_SKB_CB(skb)->header.h6,
+ sizeof(struct inet6_skb_parm));
+}
+
/* The socket must have it's spinlock held when we get
* here, unless it is a TCP_LISTEN socket.
*
@@ -1319,6 +1329,7 @@ static int tcp_v6_do_rcv(struct sock *sk, struct sk_buff *skb)
np->flow_label = ip6_flowlabel(ipv6_hdr(opt_skb));
if (ipv6_opt_accepted(sk, opt_skb, &TCP_SKB_CB(opt_skb)->header.h6)) {
skb_set_owner_r(opt_skb, sk);
+ tcp_v6_restore_cb(opt_skb);
opt_skb = xchg(&np->pktoptions, opt_skb);
} else {
__kfree_skb(opt_skb);
@@ -1352,15 +1363,6 @@ static void tcp_v6_fill_cb(struct sk_buff *skb, const struct ipv6hdr *hdr,
TCP_SKB_CB(skb)->sacked = 0;
}
-static void tcp_v6_restore_cb(struct sk_buff *skb)
-{
- /* We need to move header back to the beginning if xfrm6_policy_check()
- * and tcp_v6_fill_cb() are going to be called again.
- */
- memmove(IP6CB(skb), &TCP_SKB_CB(skb)->header.h6,
- sizeof(struct inet6_skb_parm));
-}
-
static int tcp_v6_rcv(struct sk_buff *skb)
{
const struct tcphdr *th;
^ permalink raw reply related
* Re: [PATCH iproute2 1/1] tc filters: add support to get individual filters by handle
From: Cong Wang @ 2016-10-12 16:46 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Stephen Hemminger, Linux Kernel Network Developers, Eric Dumazet,
Roman Mashak
In-Reply-To: <1476117914-32207-1-git-send-email-jhs@emojatatu.com>
On Mon, Oct 10, 2016 at 9:45 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> tc/tc_filter.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 175 insertions(+), 10 deletions(-)
Please update man/man8/tc.8 too, but can be a separated patch. ;)
^ permalink raw reply
* [PATCH net v2] netvsc: fix checksum on UDP IPV6
From: Stephen Hemminger @ 2016-10-12 16:58 UTC (permalink / raw)
To: David Miller, K. Y. Srinivasan, Haiyang Zhang; +Cc: netdev
In-Reply-To: <20161011140307.15fd390b@xeon-e3>
From: Stephen Hemminger <sthemmin@microsoft.com>
The software calculation of UDP checksum in Netvsc driver was
only handling IPv4 case. Instead, use common code to recompute
checksum as needed for all protocols.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
v2 remove accidental udp.h inclusion
drivers/net/hyperv/netvsc_drv.c | 71 ++++++++++++-----------------------------
1 file changed, 21 insertions(+), 50 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 52eeb2f..f0919bd 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -442,8 +442,6 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
}
net_trans_info = get_net_transport_info(skb, &hdr_offset);
- if (net_trans_info == TRANSPORT_INFO_NOT_IP)
- goto do_send;
/*
* Setup the sendside checksum offload only if this is not a
@@ -478,56 +476,29 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
}
lso_info->lso_v2_transmit.tcp_header_offset = hdr_offset;
lso_info->lso_v2_transmit.mss = skb_shinfo(skb)->gso_size;
- goto do_send;
- }
-
- if ((skb->ip_summed == CHECKSUM_NONE) ||
- (skb->ip_summed == CHECKSUM_UNNECESSARY))
- goto do_send;
-
- rndis_msg_size += NDIS_CSUM_PPI_SIZE;
- ppi = init_ppi_data(rndis_msg, NDIS_CSUM_PPI_SIZE,
- TCPIP_CHKSUM_PKTINFO);
-
- csum_info = (struct ndis_tcp_ip_checksum_info *)((void *)ppi +
- ppi->ppi_offset);
-
- if (net_trans_info & (INFO_IPV4 << 16))
- csum_info->transmit.is_ipv4 = 1;
- else
- csum_info->transmit.is_ipv6 = 1;
-
- if (net_trans_info & INFO_TCP) {
- csum_info->transmit.tcp_checksum = 1;
- csum_info->transmit.tcp_header_offset = hdr_offset;
- } else if (net_trans_info & INFO_UDP) {
- /* UDP checksum offload is not supported on ws2008r2.
- * Furthermore, on ws2012 and ws2012r2, there are some
- * issues with udp checksum offload from Linux guests.
- * (these are host issues).
- * For now compute the checksum here.
- */
- struct udphdr *uh;
- u16 udp_len;
-
- ret = skb_cow_head(skb, 0);
- if (ret)
- goto no_memory;
-
- uh = udp_hdr(skb);
- udp_len = ntohs(uh->len);
- uh->check = 0;
- uh->check = csum_tcpudp_magic(ip_hdr(skb)->saddr,
- ip_hdr(skb)->daddr,
- udp_len, IPPROTO_UDP,
- csum_partial(uh, udp_len, 0));
- if (uh->check == 0)
- uh->check = CSUM_MANGLED_0;
-
- csum_info->transmit.udp_checksum = 0;
+ } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+ if (net_trans_info & INFO_TCP) {
+ rndis_msg_size += NDIS_CSUM_PPI_SIZE;
+ ppi = init_ppi_data(rndis_msg, NDIS_CSUM_PPI_SIZE,
+ TCPIP_CHKSUM_PKTINFO);
+
+ csum_info = (struct ndis_tcp_ip_checksum_info *)((void *)ppi +
+ ppi->ppi_offset);
+
+ if (net_trans_info & (INFO_IPV4 << 16))
+ csum_info->transmit.is_ipv4 = 1;
+ else
+ csum_info->transmit.is_ipv6 = 1;
+
+ csum_info->transmit.tcp_checksum = 1;
+ csum_info->transmit.tcp_header_offset = hdr_offset;
+ } else {
+ /* UDP checksum (and other) offload is not supported. */
+ if (skb_checksum_help(skb))
+ goto drop;
+ }
}
-do_send:
/* Start filling in the page buffers with the rndis hdr */
rndis_msg->msg_len += rndis_msg_size;
packet->total_data_buflen = rndis_msg->msg_len;
--
2.9.3
^ permalink raw reply related
* Re: [PATCH net] net_sched: do not broadcast RTM_GETTFILTER result
From: Cong Wang @ 2016-10-12 16:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, Jamal Hadi Salim
In-Reply-To: <1476069955.28155.292.camel@edumazet-glaptop3.roam.corp.google.com>
On Sun, Oct 9, 2016 at 8:25 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> + if (unicast)
> + return netlink_unicast(net->rtnl, skb, portid, MSG_DONTWAIT);
Nit: rtnl_unicast() is simpler.
^ permalink raw reply
* [PATCH net-next v11 0/1] net: phy: Rebase Edge-Rate clean up
From: Allan W. Nielsen @ 2016-10-12 13:47 UTC (permalink / raw)
To: netdev; +Cc: andrew, f.fainelli, raju.lakkaraju, allan.nielsen
Hi,
I can see that the "Add Wake-on-LAN driver for Microsemi PHYs" (0a55c12f97)
patch has been applied to net-next, and that it is causing the Edge-Rate patch
to conflict.
I have therefore rebased v10 of the patch to fit on top of net-next.
/Allan
--
2.7.3
^ permalink raw reply
* Re: igb driver can cause cache invalidation of non-owned memory?
From: Nikita Yushchenko @ 2016-10-12 16:11 UTC (permalink / raw)
To: David Laight, 'Alexander Duyck'
Cc: Eric Dumazet, David Miller, Jeff Kirsher, intel-wired-lan, Netdev,
linux-kernel@vger.kernel.org, Chris Healy
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DB01E6F0F@AcuExch.aculab.com>
>>> To get some throughput improvement, I propose removal of that
>>> sync_for_device() before reusing buffer. Will you accept such a patch ;)
>>
>> Not one that gets rid of sync_for_device() in the driver. From what I
>> can tell there are some DMA APIs that use that to perform the
>> invalidation on the region of memory so that it can be DMAed into.
>> Without that we run the risk of having a race between something the
>> CPU might have placed in the cache and something the device wrote into
>> memory. The sync_for_device() call is meant to invalidate the cache
>> for the region so that when the device writes into memory there is no
>> risk of that race.
>
> I'm not expert, but some thought...
>
> Just remember that the cpu can do speculative memory and cache line reads.
> So you need to ensure there are no dirty cache lines when the receive
> buffer is setup and no cache lines at all at before looking at the frame.
Exactly.
And because of that, arm does cache invalidation both in sync_for_cpu()
and sync_for_device().
> If you can 100% guarantee the cpu hasn't dirtied the cache then I
> think the invalidate prior to reusing the buffer can be skipped.
> But I wouldn't want to debug that going wrong.
I've written earlier in this thread why it is the case for igb - as long
as Alexander's statement that igb's buffers are readonly for the stack
is true. If Alexander's statement is wrong, then igb is vulnerable to
issue I've described in the first message of this thread.
Btw, we are unable get any breakage with that sync_to_device() removed
already for a day. And - our tests run with software checksumming,
because on our hardware i210 is wired connected to DSA switch and in
this config, no i210 offloading works (i210 hardware gets frames with
eDSA headers that it can't parse). Thus any breakage should be
immediately discovered.
And throughput measured by iperf raises from 650 to 850 Mbps.
Nikita
^ permalink raw reply
* RE: igb driver can cause cache invalidation of non-owned memory?
From: David Laight @ 2016-10-12 16:03 UTC (permalink / raw)
To: 'Alexander Duyck', Nikita Yushchenko
Cc: Eric Dumazet, David Miller, Jeff Kirsher, intel-wired-lan, Netdev,
linux-kernel@vger.kernel.org, Chris Healy
In-Reply-To: <CAKgT0UfPC_uAn5=+0iiAOGyY7QA-4NWAmedVmWf6TGnywjJR8A@mail.gmail.com>
From: Alexander Duyck
> Sent: 12 October 2016 16:33
...
> > To get some throughput improvement, I propose removal of that
> > sync_for_device() before reusing buffer. Will you accept such a patch ;)
>
> Not one that gets rid of sync_for_device() in the driver. From what I
> can tell there are some DMA APIs that use that to perform the
> invalidation on the region of memory so that it can be DMAed into.
> Without that we run the risk of having a race between something the
> CPU might have placed in the cache and something the device wrote into
> memory. The sync_for_device() call is meant to invalidate the cache
> for the region so that when the device writes into memory there is no
> risk of that race.
I'm not expert, but some thought...
Just remember that the cpu can do speculative memory and cache line reads.
So you need to ensure there are no dirty cache lines when the receive
buffer is setup and no cache lines at all at before looking at the frame.
So unless you know the exact rules for these speculative cache line reads
you have to invalidate the cache after the buffer is written to by the
hardware even it was invalidated when the buffer was set up.
If you can 100% guarantee the cpu hasn't dirtied the cache then I
think the invalidate prior to reusing the buffer can be skipped.
But I wouldn't want to debug that going wrong.
Might be provable safe in the 'copybreak' path.
David
^ permalink raw reply
* Re: [PATCH iproute2] tc: cls_bpf: handle skip_sw and skip_hw flags
From: Daniel Borkmann @ 2016-10-12 16:03 UTC (permalink / raw)
To: Jakub Kicinski, netdev
In-Reply-To: <1476287196-28931-1-git-send-email-jakub.kicinski@netronome.com>
On 10/12/2016 05:46 PM, Jakub Kicinski wrote:
> Add support for controling hardware offload using (now standard)
> skip_sw and skip_hw flags in cls_bpf.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* Re: [RFC v3 3/3] phy,leds: add support for led triggers on phy link state change
From: Zach Brown @ 2016-10-12 15:57 UTC (permalink / raw)
To: Florian Fainelli
Cc: mlindner, stephen, netdev, linux-kernel, devel,
florian.c.schilhabel, Larry.Finger, gregkh, rpurdie, j.anaszewski,
linux-leds, Andrew Lunn
In-Reply-To: <1eb3afdb-0236-65e3-64be-09cc6eeda533@gmail.com>
On Mon, Oct 10, 2016 at 02:03:32AM -0700, Florian Fainelli wrote:
> > +
> > +#ifdef CONFIG_LED_TRIGGER_PHY
> > +
> > +#include <linux/leds.h>
> > +#include <linux/phy.h>
> > +
> > +#define PHY_LINK_LED_MAX_TRIGGERS 5
> > +#define PHY_LED_TRIGGER_SPEED_SUFFIX_SIZE 7
> > +#define PHY_MII_BUS_ID_SIZE (20 - 3)
>
> This particular constant may be something worth moving to
> include/linux/phy.h eventually.
> --
> Florian
MII_BUS_ID_SIZE is defined in include/linux/phy.h but it's defined after
phy_led_triggers.h is included so phy_led_triggers.h doesn't have access.
I could move the definition of MII_BUS_ID_SIZE above the include, but that
seemed ugly. Do you have any suggestions?
^ permalink raw reply
* [PATCH iproute2] tc: cls_bpf: handle skip_sw and skip_hw flags
From: Jakub Kicinski @ 2016-10-12 15:46 UTC (permalink / raw)
To: netdev; +Cc: daniel, Jakub Kicinski
Add support for controling hardware offload using (now standard)
skip_sw and skip_hw flags in cls_bpf.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
Hi Stephen!
This requires header rebase to get TCA_BPF_FLAGS_GEN in pkt_cls.h.
The patch is for 4.9 release so it is targeted at master branch.
Thanks!
man/man8/tc-bpf.8 | 14 ++++++++++++++
tc/f_bpf.c | 21 +++++++++++++++++++--
2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/man/man8/tc-bpf.8 b/man/man8/tc-bpf.8
index c8d5c5f94da8..e371964d06ab 100644
--- a/man/man8/tc-bpf.8
+++ b/man/man8/tc-bpf.8
@@ -14,6 +14,10 @@ CLS_NAME ] [
UDS_FILE ] [
.B verbose
] [
+.B skip_hw
+|
+.B skip_sw
+] [
.B police
POLICE_SPEC ] [
.B action
@@ -137,6 +141,16 @@ if set, it will dump the eBPF verifier output, even if loading the eBPF
program was successful. By default, only on error, the verifier log is
being emitted to the user.
+.SS skip_hw | skip_sw
+hardware offload control flags. By default TC will try to offload
+filters to hardware if possible.
+.B skip_hw
+explicitly disables the attempt to offload.
+.B skip_sw
+forces the offload and disables running the eBPF program in the kernel.
+If hardware offload is not possible and this flag was set kernel will
+report an error and filter will not be installed at all.
+
.SS police
is an optional parameter for an eBPF/cBPF classifier that specifies a
police in
diff --git a/tc/f_bpf.c b/tc/f_bpf.c
index 5c97c863d15a..665bc6612eeb 100644
--- a/tc/f_bpf.c
+++ b/tc/f_bpf.c
@@ -37,8 +37,8 @@ static void explain(void)
fprintf(stderr, "\n");
fprintf(stderr, "eBPF use case:\n");
fprintf(stderr, " object-file FILE [ section CLS_NAME ] [ export UDS_FILE ]");
- fprintf(stderr, " [ verbose ] [ direct-action ]\n");
- fprintf(stderr, " object-pinned FILE [ direct-action ]\n");
+ fprintf(stderr, " [ verbose ] [ direct-action ] [ skip_hw | skip_sw ]\n");
+ fprintf(stderr, " object-pinned FILE [ direct-action ] [ skip_hw | skip_sw ]\n");
fprintf(stderr, "\n");
fprintf(stderr, "Common remaining options:\n");
fprintf(stderr, " [ action ACTION_SPEC ]\n");
@@ -66,6 +66,7 @@ static int bpf_parse_opt(struct filter_util *qu, char *handle,
{
const char *bpf_obj = NULL, *bpf_uds_name = NULL;
struct tcmsg *t = NLMSG_DATA(n);
+ unsigned int bpf_gen_flags = 0;
unsigned int bpf_flags = 0;
bool seen_run = false;
struct rtattr *tail;
@@ -107,6 +108,10 @@ static int bpf_parse_opt(struct filter_util *qu, char *handle,
} else if (matches(*argv, "direct-action") == 0 ||
matches(*argv, "da") == 0) {
bpf_flags |= TCA_BPF_FLAG_ACT_DIRECT;
+ } else if (matches(*argv, "skip_hw") == 0) {
+ bpf_gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
+ } else if (matches(*argv, "skip_sw") == 0) {
+ bpf_gen_flags |= TCA_CLS_FLAGS_SKIP_SW;
} else if (matches(*argv, "action") == 0) {
NEXT_ARG();
if (parse_action(&argc, &argv, TCA_BPF_ACT, n)) {
@@ -136,6 +141,8 @@ static int bpf_parse_opt(struct filter_util *qu, char *handle,
NEXT_ARG_FWD();
}
+ if (bpf_gen_flags)
+ addattr32(n, MAX_MSG, TCA_BPF_FLAGS_GEN, bpf_gen_flags);
if (bpf_obj && bpf_flags)
addattr32(n, MAX_MSG, TCA_BPF_FLAGS, bpf_flags);
@@ -178,6 +185,16 @@ static int bpf_print_opt(struct filter_util *qu, FILE *f,
fprintf(f, "direct-action ");
}
+ if (tb[TCA_BPF_FLAGS_GEN]) {
+ unsigned int flags =
+ rta_getattr_u32(tb[TCA_BPF_FLAGS_GEN]);
+
+ if (flags & TCA_CLS_FLAGS_SKIP_HW)
+ fprintf(f, "skip_hw ");
+ if (flags & TCA_CLS_FLAGS_SKIP_SW)
+ fprintf(f, "skip_sw ");
+ }
+
if (tb[TCA_BPF_OPS] && tb[TCA_BPF_OPS_LEN]) {
bpf_print_ops(f, tb[TCA_BPF_OPS],
rta_getattr_u16(tb[TCA_BPF_OPS_LEN]));
--
1.9.1
^ permalink raw reply related
* Re: igb driver can cause cache invalidation of non-owned memory?
From: Alexander Duyck @ 2016-10-12 15:32 UTC (permalink / raw)
To: Nikita Yushchenko
Cc: Eric Dumazet, David Miller, Jeff Kirsher, intel-wired-lan, Netdev,
linux-kernel@vger.kernel.org, Chris Healy
In-Reply-To: <a0779e4d-3228-8649-1e68-21dffd4249bd@cogentembedded.com>
On Tue, Oct 11, 2016 at 11:55 PM, Nikita Yushchenko
<nikita.yoush@cogentembedded.com> wrote:
>>>> The main reason why this isn't a concern for the igb driver is because
>>>> we currently pass the page up as read-only. We don't allow the stack
>>>> to write into the page by keeping the page count greater than 1 which
>>>> means that the page is shared. It isn't until we unmap the page that
>>>> the page count is allowed to drop to 1 indicating that it is writable.
>>>
>>> Doesn't that mean that sync_to_device() in igb_reuse_rx_page() can be
>>> avoided? If page is read only for entire world, then it can't be dirty
>>> in cache and thus device can safely write to it without preparation step.
>>
>> For the sake of correctness we were adding the
>> dma_sync_single_range_for_device.
>
> Could you please elaborate this "for sake of correctness"?
>
> If by "correctness" you mean ensuring that buffer gets frame DMAed by
> device and that's not broken by cache activity, then:
> - on first use of this buffer after page allocation, sync_for_device()
> is not needed due to previous dma_page_map() call,
> - on later uses of the same buffer, sync_for_device() is not needed due
> to buffer being read-only since dma_page_map() call, thus it can't be
> dirty in cache and thus no writebacks of this area can be possible.
>
> If by "correctness" you mean strict following "ownership" concept - i.e.
> memory area is "owned" either by cpu or by device, and "ownersip" must
> be passed to device before DMA and back to cpu after DMA - then, igb
> driver already breaks these rules anyway:
Sort of. Keep in mind the recent changes to only sync what the device
had DMAed into is a recent change and was provided by a third party.
> - igb calls dma_map_page() at page allocation time, thus entire page
> becomes "owned" by device,
> - and then, on first use of second buffer inside the page, igb calls
> sync_for_device() for buffer area, despite of that area is already
> "owned" by device,
Right. However there is nothing wrong with assigning a buffer to the
device twice especially since we are just starting out. If we wanted
to be more correct we would probably be allocating and deallocating
the pages with the DMA_ATTR_SKIP_CPU_SYNC attribute and then just do
the sync_for_device before reassigning the page back to the device.
> - and later, if a buffer within page gets reused, igb calls
> sync_for_device() for entire buffer, despite of only part of buffer was
> sync_for_cpu()'ed at time of completing receive of previous frame into
> this buffer,
This is going to come into play with future changes we have planned.
If we update things to use build_skb we are going to be writing into
other parts of the page as we will be using that for shared info.
> - and later, igb calls dma_unmap_page(), despite of that part of page
> was sync_for_cpu()'ed and thus is "owned" by CPU.
Right we are looking into finding a fix for that.
> Given all that, not calling sync_for_device() before reusing buffer
> won't make picture much worse :)
Actually it will since you are suggesting taking things in a different
direction then the rest of the community. What we plan to do is
weaken the dma_map/dma_unmap semantics by likely using
DMA_ATTR_SKIP_CPU_SYNC on the map and unmap calls.
>> Since it is an DMA_FROM_DEVICE
>> mapping calling it should really have no effect for most DMA mapping
>> interfaces.
>
> Unfortunately dma_sync_single_range_for_device() *is* slow on imx6q - it
> does cache invalidation. I don't really understand why invalidating
> cache can be slow - it only removes data from cache, it should not
> access slow outer memory - but cache invalidation *is* in top of perf
> profiles.
>
> To get some throughput improvement, I propose removal of that
> sync_for_device() before reusing buffer. Will you accept such a patch ;)
Not one that gets rid of sync_for_device() in the driver. From what I
can tell there are some DMA APIs that use that to perform the
invalidation on the region of memory so that it can be DMAed into.
Without that we run the risk of having a race between something the
CPU might have placed in the cache and something the device wrote into
memory. The sync_for_device() call is meant to invalidate the cache
for the region so that when the device writes into memory there is no
risk of that race.
What you may want to do is look at the DMA API you are using and
determine if it is functioning correctly. Most DMA APIs I am familiar
with will either sync Rx data on the sync_for_cpu() or
sync_for_device() but it should not sync on both. The fact that it is
syncing on both makes me wonder if the API was modified to work around
a buggy driver that didn't follow the proper semantics for buffer
ownership instead of just fixing the buggy driver.
>> Also you may want to try updating to the 4.8 version of the driver.
>> It reduces the size of the dma_sync_single_range_for_cpu loops by
>> reducing the sync size down to the size that was DMAed into the
>> buffer.
>
> Actually that patch came out of the work I'm currently participating in
> ;). Sure I have it.
>
>> Specifically I believe
>> the 0->100% accounting problem is due to the way this is all tracked.
>
> Thanks for this hint - shame on me not realizing this earlier...
>
>> You may want to try pulling the most recent net-next kernel and
>> testing that to see if you still see the same behavior as Eric has
>> recently added a fix that is meant to allow for better sharing between
>> softirq polling and applications when dealing with stuff like UDP
>> traffic.
>>
>> As far as identifying the problem areas your best bet would be to push
>> the CPU to 100% and then identify the hot spots.
>
> Thanks for hints
>
>
> Nikita
^ 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