* Re: [PATCH net-next,v2] net: rename ndo_setup_tc to ndo_setup_offload
From: Pablo Neira Ayuso @ 2018-07-19 20:52 UTC (permalink / raw)
To: Alexander Duyck
Cc: Netdev, Jiri Pirko, Tom Lendacky, Florian Fainelli, Ariel Elior,
Michael Chan, Santosh Raspatur, madalin.bucur, yisen.zhuang,
salil.mehta, Jeff Kirsher, Tariq Toukan, Saeed Mahameed,
Jiri Pirko, Ido Schimmel, Ganesh Goudar, Jakub Kicinski,
linux-net-drivers, peppe.cavallaro, alexandre.torgue, joabreu,
grygorii.strashko, Andrew Lunn
In-Reply-To: <CAKgT0UcTihfkgzO=AZ=J_hS-3gg_wjMYr+kJGAWobQ_Wkdfp9g@mail.gmail.com>
On Thu, Jul 19, 2018 at 08:18:20AM -0700, Alexander Duyck wrote:
> On Wed, Jul 18, 2018 at 5:11 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > One of the recurring complaints is that we do not have, as a driver
> > writer, a central location from which we would be fed offloading rules
> > into a NIC. This was brought up again during Netconf'18 in Boston.
> >
> > This patch just renames ndo_setup_tc to ndo_setup_offload as a very
> > early initial work to prepare for follow up patch that discuss unified
> > flow representation for the existing offload programming APIs.
> >
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > Acked-by: Jiri Pirko <jiri@mellanox.com>
> > Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>
> One request I would have here is to not bother updating the individual
> driver function names. For now I would say we could leave the
> "_setup_tc" in the naming of the driver functions itself and just
> update the name of the net device operation. Renaming the driver
> functions just adds unnecessary overhead and complexity to the patch
> and will make it more difficult to maintain. When we get around to
> adding additional functionality that relates to the rename we could
> address renaming the function on a per driver basis in the future.
Plan was to follow up patch will rename enum tc_setup_type too:
https://marc.info/?l=linux-netdev&m=153193158512556&w=2
that will result in more renames in the driver side.
I would expect this will happen sooner or later, and out of tree
patches will end up needing a rebase sooner or later, if that is the
concern.
^ permalink raw reply
* Re: [PATCH 4/5] net: add support for nvmem to eth_platform_get_mac_address()
From: Andrew Lunn @ 2018-07-19 21:48 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Russell King - ARM Linux, Sekhar Nori, Kevin Hilman,
Grygorii Strashko, David S . Miller, Srinivas Kandagatla,
Lukas Wunner, Rob Herring, Florian Fainelli, Dan Carpenter,
Ivan Khoronzhuk, David Lechner, Greg Kroah-Hartman, Linux ARM,
Linux Kernel Mailing List, Linux-OMAP, netdev,
Bartosz Golaszewski
In-Reply-To: <CAMRc=MfYp8Q4KNUMhLqf5jgd+C-_bWnfHworTbaCj0CZgRqMeA@mail.gmail.com>
> Hi Russell,
>
> this issue is gone now in v3 but thanks for the example - I somehow
> never thought about it.
Please don't send patchset so fast. It wastes people time, reviewing
outdated versions. No more than one version of a patchset per day.
Andrew
^ permalink raw reply
* Re: [PATCH net-next,v2] net: rename ndo_setup_tc to ndo_setup_offload
From: Alexander Duyck @ 2018-07-19 21:04 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Netdev, Jiri Pirko, Tom Lendacky, Florian Fainelli, Ariel Elior,
Michael Chan, Santosh Raspatur, madalin.bucur, yisen.zhuang,
salil.mehta, Jeff Kirsher, Tariq Toukan, Saeed Mahameed,
Jiri Pirko, Ido Schimmel, Ganesh Goudar, Jakub Kicinski,
linux-net-drivers, peppe.cavallaro, alexandre.torgue, joabreu,
grygorii.strashko, Andrew Lunn
In-Reply-To: <20180719205212.f4invmtqvtcjzuid@salvia>
On Thu, Jul 19, 2018 at 1:52 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Jul 19, 2018 at 08:18:20AM -0700, Alexander Duyck wrote:
>> On Wed, Jul 18, 2018 at 5:11 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > One of the recurring complaints is that we do not have, as a driver
>> > writer, a central location from which we would be fed offloading rules
>> > into a NIC. This was brought up again during Netconf'18 in Boston.
>> >
>> > This patch just renames ndo_setup_tc to ndo_setup_offload as a very
>> > early initial work to prepare for follow up patch that discuss unified
>> > flow representation for the existing offload programming APIs.
>> >
>> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> > Acked-by: Jiri Pirko <jiri@mellanox.com>
>> > Acked-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>
>> One request I would have here is to not bother updating the individual
>> driver function names. For now I would say we could leave the
>> "_setup_tc" in the naming of the driver functions itself and just
>> update the name of the net device operation. Renaming the driver
>> functions just adds unnecessary overhead and complexity to the patch
>> and will make it more difficult to maintain. When we get around to
>> adding additional functionality that relates to the rename we could
>> address renaming the function on a per driver basis in the future.
>
> Plan was to follow up patch will rename enum tc_setup_type too:
>
> https://marc.info/?l=linux-netdev&m=153193158512556&w=2
>
> that will result in more renames in the driver side.
>
> I would expect this will happen sooner or later, and out of tree
> patches will end up needing a rebase sooner or later, if that is the
> concern.
I was just thinking that renaming the functions themselves adds noise
and makes it harder to debug functions later when they get renamed. As
far as the out-of-tree driver I agree we will still have to deal with
it due to the enum and NDO function rename. I just figured that using
things like LXR is a bit easier when the function name stays the same
and you have to move between versions.
- Alex
^ permalink raw reply
* Re: [PATCH net-next] net: phy: add GBit master / slave error detection
From: Heiner Kallweit @ 2018-07-19 21:11 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <20180719144637.GA9119@lunn.ch>
On 19.07.2018 16:46, Andrew Lunn wrote:
>>> AFAIR there was a patch a while ago from Mellanox guys that was possibly
>>> extending the link notification with an error cause, this sounds like
>>> something that could be useful to report to user space somehow to help
>>> troubleshoot link down events.
>>>
>> Do you by chance have a reference to this patch? There's heavy development
>> on the Mellanox drivers with a lot of patches.
>
> Hi Heiner, Florian
>
> A general mechanism has been added to allow error messages to be
> reported via netlink sockets. I think wifi was the first to actually
> make use of it, since i think Johannes Berg did the core work, but
> other parts of the stack have also started using it.
>
I think you mean the devlink interface. With regard to nl80211 I'm aware
that it's used for communication between Wifi core and userspace tools /
daemons like hostapd and wpa_supplicant.
The devlink use case reminds me of swconfig for switch port configuration
under OpenWRT.
For our use case I think this is overkill, and I wonder what the
userspace tool would be that consumes our link down error info.
Heiner
> Just picking a commit at random, maybe not the best of examples:
>
> Fixes: 768075ebc238 ("nl80211: add a few extended error strings to key parsing")
>
> Andrew
>
^ permalink raw reply
* Re: [PATCH net] net/xdp: Fix suspicious RCU usage warning
From: Alexei Starovoitov @ 2018-07-19 21:36 UTC (permalink / raw)
To: Tariq Toukan
Cc: Daniel Borkmann, David S. Miller, netdev, Eran Ben Elisha,
Jesper Dangaard Brouer
In-Reply-To: <6ed9d782-13dd-d5b5-6b65-e583356c06d5@mellanox.com>
On Wed, Jul 18, 2018 at 05:13:54PM +0300, Tariq Toukan wrote:
>
>
> On 17/07/2018 10:27 PM, Daniel Borkmann wrote:
> > On 07/17/2018 06:47 PM, Alexei Starovoitov wrote:
> > > On Tue, Jul 17, 2018 at 06:10:38PM +0300, Tariq Toukan wrote:
> > > > Fix the warning below by calling rhashtable_lookup under
> > > > RCU read lock.
> > > >
>
> ...
>
> > > > mutex_lock(&mem_id_lock);
> > > > + rcu_read_lock();
> > > > xa = rhashtable_lookup(mem_id_ht, &id, mem_id_rht_params);
> > > > + rcu_read_unlock();
> > > > if (!xa) {
> > >
> > > if it's an actual bug rcu_read_unlock seems to be misplaced.
> > > It silences the warn, but rcu section looks wrong.
> >
> > I think that whole piece in __xdp_rxq_info_unreg_mem_model() should be:
> >
> > mutex_lock(&mem_id_lock);
> > xa = rhashtable_lookup_fast(mem_id_ht, &id, mem_id_rht_params);
> > if (xa && rhashtable_remove_fast(mem_id_ht, &xa->node, mem_id_rht_params) == 0)
> > call_rcu(&xa->rcu, __xdp_mem_allocator_rcu_free);
> > mutex_unlock(&mem_id_lock);
> >
> > Technically the RCU read side plus rhashtable_lookup() is the same, but lets
> > use proper api. From the doc (https://lwn.net/Articles/751374/) object removal
> > is wrapped around the RCU read side additionally, but in our case we're behind
> > mem_id_lock for insertion/removal serialization.
> >
> > Cheers,
> > Daniel
> >
>
> Just as Daniel stated, I think there's no actual bug here, but we still want
> to silence the RCU warning.
>
> Alexei, did you mean getting the if statement into the RCU lock critical
> section?
If what Daniel proposes silences the warn, I'd rather do that.
Pattern like:
rcu_lock;
val = lookup();
rcu_unlock;
if (val)
will cause people to question the quality of the code and whether
authors of the code understand rcu.
There should be a way to silence the warn without adding
"wrong on the first glance" code.
^ permalink raw reply
* Re: [PATCH net-next] net: phy: add GBit master / slave error detection
From: Andrew Lunn @ 2018-07-19 21:45 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <048869a5-7fbe-f68d-8d9c-123b2ac38bd5@gmail.com>
On Thu, Jul 19, 2018 at 11:11:53PM +0200, Heiner Kallweit wrote:
> On 19.07.2018 16:46, Andrew Lunn wrote:
> >>> AFAIR there was a patch a while ago from Mellanox guys that was possibly
> >>> extending the link notification with an error cause, this sounds like
> >>> something that could be useful to report to user space somehow to help
> >>> troubleshoot link down events.
> >>>
> >> Do you by chance have a reference to this patch? There's heavy development
> >> on the Mellanox drivers with a lot of patches.
> >
> > Hi Heiner, Florian
> >
> > A general mechanism has been added to allow error messages to be
> > reported via netlink sockets. I think wifi was the first to actually
> > make use of it, since i think Johannes Berg did the core work, but
> > other parts of the stack have also started using it.
> >
> I think you mean the devlink interface.
devlink uses netlink, so it can use these extended error message. But
all forms of netlink sockets can use this.
What Florian might be referring to, is that when netif_carrier_off()
or netif_carrier_on() is called, a netlink message is sent to
userspace. Tools link ip monitor can be used to display these events.
I think Florian is suggesting a more detailed message could be display
about master/slave issues. However, how you get that information to
rtnl_fill_ifinfo() i don't know.
Andrew
^ permalink raw reply
* Re: linux-next: build warning after merge of the net-next tree
From: Guenter Roeck @ 2018-07-19 22:35 UTC (permalink / raw)
To: Stephen Rothwell
Cc: David Miller, Networking, Linux-Next Mailing List,
Linux Kernel Mailing List, Masahiro Yamada, Andrew Lunn
In-Reply-To: <20180720080906.7a30c573@canb.auug.org.au>
On Fri, Jul 20, 2018 at 08:09:06AM +1000, Stephen Rothwell wrote:
> Hi Guenter,
>
> On Thu, 19 Jul 2018 06:49:01 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 07/18/2018 10:29 PM, Stephen Rothwell wrote:
> > >
> > > On Wed, 18 Jul 2018 20:52:56 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> > >>
> > >> On 07/18/2018 07:04 PM, Stephen Rothwell wrote:
> > >>>
> > >>> After merging the net-next tree, today's linux-next build (x86_64
> > >>> allmodconfig) produced this warning:
> > >>>
> > >>> *
> > >>> * Restart config...
> > >>> *
> > >>> ....
> > >>>
> > >>> This is output by my "make allmodconfig" and only started after merging
> > >>> the net-next tree today. It has continued for further merges/builds.
> > >>>
> > >>> I suspect commit
> > >>>
> > >>> 1323061a018a ("net: phy: sfp: Add HWMON support for module sensors")
> > >>>
> > >>> which added an "imply" clause.
> > >>>
> > >> I thought "imply" was better than "depends on HWMON || HWMON=n", but maybe
> > >> not. Is that a caveat when using "imply", and does it mean that "imply"
> > >> should better not be used ?
> > >
> > > I don't know, sorry. It was just my best guess from what I could see
> > > had changed.
> > >
> > > I wonder if it makes a difference that I am doing my "make
> > > allmodconfig" on top of a previous "make allmodconfig" and some symbols
> > > are marked as "NEW" (though they are not symbols related to the changes
> > > that happened during the net-next tree merge)?
> > >
> >
> > I tried to reproduce the problem, but I don't see the message.
> >
> > What I do see, though, is that "make allmodconfig" on a clean tree,
> > followed by "make menuconfig", results in configuration changes.
> > Specifically,
> >
> > > CONFIG_ARC_EMAC_CORE=m
> > > CONFIG_ARC_EMAC=m
> > > CONFIG_EMAC_ROCKCHIP=m
> >
> > is removed by menuconfig, and a large number of "# ... is not set"
> > configuration lines are added. Weird and bad, since several of the
> > disabled configurations _should_ be enabled by "make allmodconfig",
> > and a large number of hwmon drivers are affected. Bisect does point
> > to "net: phy: sfp: Add HWMON support for module sensors", meaning
> > "imply hwmon" does have severe side effects and can not be used.
> >
> > I'll try to find a fix.
>
> OK, my mistake, the "make allmodconfig" works, the following "make"
> causes the config restart. (I am actually doing cross builds and using
> an external object directory, in case that matters.)
>
> I removed the "imply HWMON" line added by the above commit and the
> problem went away. Also, using "depends on HWMON || HWMON=n" instead
> of the imply fixes it.
Yes, replacing imply with the dependency is what I did in the fixup patch.
Sorry, I should have copied you: https://patchwork.kernel.org/patch/10534925/
It is a bit different - imply was supposed to enforce HWMON={y,n} if SFP=y,
and the depends line enforces SFP={n,m} if HWMON=m. I have no idea why
imply doesn't work, but I think I'll stay away from it in the future.
Guenter
^ permalink raw reply
* Re: linux-next: build warning after merge of the net-next tree
From: Stephen Rothwell @ 2018-07-19 22:09 UTC (permalink / raw)
To: Guenter Roeck
Cc: David Miller, Networking, Linux-Next Mailing List,
Linux Kernel Mailing List, Masahiro Yamada, Andrew Lunn
In-Reply-To: <6dabf12b-e3db-dfe8-101c-02772039e21a@roeck-us.net>
[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]
Hi Guenter,
On Thu, 19 Jul 2018 06:49:01 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 07/18/2018 10:29 PM, Stephen Rothwell wrote:
> >
> > On Wed, 18 Jul 2018 20:52:56 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 07/18/2018 07:04 PM, Stephen Rothwell wrote:
> >>>
> >>> After merging the net-next tree, today's linux-next build (x86_64
> >>> allmodconfig) produced this warning:
> >>>
> >>> *
> >>> * Restart config...
> >>> *
> >>> ....
> >>>
> >>> This is output by my "make allmodconfig" and only started after merging
> >>> the net-next tree today. It has continued for further merges/builds.
> >>>
> >>> I suspect commit
> >>>
> >>> 1323061a018a ("net: phy: sfp: Add HWMON support for module sensors")
> >>>
> >>> which added an "imply" clause.
> >>>
> >> I thought "imply" was better than "depends on HWMON || HWMON=n", but maybe
> >> not. Is that a caveat when using "imply", and does it mean that "imply"
> >> should better not be used ?
> >
> > I don't know, sorry. It was just my best guess from what I could see
> > had changed.
> >
> > I wonder if it makes a difference that I am doing my "make
> > allmodconfig" on top of a previous "make allmodconfig" and some symbols
> > are marked as "NEW" (though they are not symbols related to the changes
> > that happened during the net-next tree merge)?
> >
>
> I tried to reproduce the problem, but I don't see the message.
>
> What I do see, though, is that "make allmodconfig" on a clean tree,
> followed by "make menuconfig", results in configuration changes.
> Specifically,
>
> > CONFIG_ARC_EMAC_CORE=m
> > CONFIG_ARC_EMAC=m
> > CONFIG_EMAC_ROCKCHIP=m
>
> is removed by menuconfig, and a large number of "# ... is not set"
> configuration lines are added. Weird and bad, since several of the
> disabled configurations _should_ be enabled by "make allmodconfig",
> and a large number of hwmon drivers are affected. Bisect does point
> to "net: phy: sfp: Add HWMON support for module sensors", meaning
> "imply hwmon" does have severe side effects and can not be used.
>
> I'll try to find a fix.
OK, my mistake, the "make allmodconfig" works, the following "make"
causes the config restart. (I am actually doing cross builds and using
an external object directory, in case that matters.)
I removed the "imply HWMON" line added by the above commit and the
problem went away. Also, using "depends on HWMON || HWMON=n" instead
of the imply fixes it.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 6/7] net: systemport: Add support for WAKE_FILTER
From: Andrew Lunn @ 2018-07-19 22:25 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev, linville, davem, vivien.didelot
In-Reply-To: <1eec25bd-2cc1-3fa7-08b5-fa4ab7c5e777@gmail.com>
> In both of your examples, only one bit is set, what will change is the
> value being programmed to RXHCK_BRCM_TAG(i), which will be either 0, or
> 1, but the value programmed to RXCHK_CONTROL as far as which filter is
> enabled will be the same because we can use filter position 0.
>
> What the code basically does is look at how many bits are set in the
> filters bitmap, and then it starts populating the filters from filter 0
> up to filter 7 with the value of the bit.
O.K. Now it get it. Sorry for being so slow.
Andrew
^ permalink raw reply
* Re: [PATCH net] net: phy: consider PHY_IGNORE_INTERRUPT in phy_start_aneg_priv
From: Andrew Lunn @ 2018-07-19 22:33 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <e6770015-4b52-d0ff-fc98-9e0b8f34be6b@gmail.com>
On Thu, Jul 19, 2018 at 08:15:16AM +0200, Heiner Kallweit wrote:
> The situation described in the comment can occur also with
> PHY_IGNORE_INTERRUPT, therefore change the condition to include it.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: linux-next: build warning after merge of the net-next tree
From: Masahiro Yamada @ 2018-07-19 23:19 UTC (permalink / raw)
To: Guenter Roeck
Cc: Stephen Rothwell, David Miller, Networking,
Linux-Next Mailing List, Linux Kernel Mailing List, Andrew Lunn
In-Reply-To: <20180719223521.GA30287@roeck-us.net>
2018-07-20 7:35 GMT+09:00 Guenter Roeck <linux@roeck-us.net>:
> On Fri, Jul 20, 2018 at 08:09:06AM +1000, Stephen Rothwell wrote:
>> Hi Guenter,
>>
>> On Thu, 19 Jul 2018 06:49:01 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
>> >
>> > On 07/18/2018 10:29 PM, Stephen Rothwell wrote:
>> > >
>> > > On Wed, 18 Jul 2018 20:52:56 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
>> > >>
>> > >> On 07/18/2018 07:04 PM, Stephen Rothwell wrote:
>> > >>>
>> > >>> After merging the net-next tree, today's linux-next build (x86_64
>> > >>> allmodconfig) produced this warning:
>> > >>>
>> > >>> *
>> > >>> * Restart config...
>> > >>> *
>> > >>> ....
>> > >>>
>> > >>> This is output by my "make allmodconfig" and only started after merging
>> > >>> the net-next tree today. It has continued for further merges/builds.
>> > >>>
>> > >>> I suspect commit
>> > >>>
>> > >>> 1323061a018a ("net: phy: sfp: Add HWMON support for module sensors")
>> > >>>
>> > >>> which added an "imply" clause.
>> > >>>
>> > >> I thought "imply" was better than "depends on HWMON || HWMON=n", but maybe
>> > >> not. Is that a caveat when using "imply", and does it mean that "imply"
>> > >> should better not be used ?
>> > >
>> > > I don't know, sorry. It was just my best guess from what I could see
>> > > had changed.
>> > >
>> > > I wonder if it makes a difference that I am doing my "make
>> > > allmodconfig" on top of a previous "make allmodconfig" and some symbols
>> > > are marked as "NEW" (though they are not symbols related to the changes
>> > > that happened during the net-next tree merge)?
>> > >
>> >
>> > I tried to reproduce the problem, but I don't see the message.
>> >
>> > What I do see, though, is that "make allmodconfig" on a clean tree,
>> > followed by "make menuconfig", results in configuration changes.
>> > Specifically,
>> >
>> > > CONFIG_ARC_EMAC_CORE=m
>> > > CONFIG_ARC_EMAC=m
>> > > CONFIG_EMAC_ROCKCHIP=m
>> >
>> > is removed by menuconfig, and a large number of "# ... is not set"
>> > configuration lines are added. Weird and bad, since several of the
>> > disabled configurations _should_ be enabled by "make allmodconfig",
>> > and a large number of hwmon drivers are affected. Bisect does point
>> > to "net: phy: sfp: Add HWMON support for module sensors", meaning
>> > "imply hwmon" does have severe side effects and can not be used.
>> >
>> > I'll try to find a fix.
>>
>> OK, my mistake, the "make allmodconfig" works, the following "make"
>> causes the config restart. (I am actually doing cross builds and using
>> an external object directory, in case that matters.)
>>
>> I removed the "imply HWMON" line added by the above commit and the
>> problem went away. Also, using "depends on HWMON || HWMON=n" instead
>> of the imply fixes it.
>
> Yes, replacing imply with the dependency is what I did in the fixup patch.
> Sorry, I should have copied you: https://patchwork.kernel.org/patch/10534925/
>
> It is a bit different - imply was supposed to enforce HWMON={y,n} if SFP=y,
> and the depends line enforces SFP={n,m} if HWMON=m. I have no idea why
> imply doesn't work, but I think I'll stay away from it in the future.
>
> Guenter
Hmm, this could be a Kconfig bug.
I will take a look.
BTW,
'depends on HWMON || HWMON=n' is pointless
because it is always true.
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: linux-next: build warning after merge of the net-next tree
From: Guenter Roeck @ 2018-07-19 23:37 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Stephen Rothwell, David Miller, Networking,
Linux-Next Mailing List, Linux Kernel Mailing List, Andrew Lunn
In-Reply-To: <CAK7LNARfZH=ZEdFuRq_g1HsJ+Pv1m=v343JLyE-6+uM_d+3eWA@mail.gmail.com>
On Fri, Jul 20, 2018 at 08:19:34AM +0900, Masahiro Yamada wrote:
> 2018-07-20 7:35 GMT+09:00 Guenter Roeck <linux@roeck-us.net>:
> > On Fri, Jul 20, 2018 at 08:09:06AM +1000, Stephen Rothwell wrote:
> >> Hi Guenter,
> >>
> >> On Thu, 19 Jul 2018 06:49:01 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> >> >
> >> > On 07/18/2018 10:29 PM, Stephen Rothwell wrote:
> >> > >
> >> > > On Wed, 18 Jul 2018 20:52:56 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> >> > >>
> >> > >> On 07/18/2018 07:04 PM, Stephen Rothwell wrote:
> >> > >>>
> >> > >>> After merging the net-next tree, today's linux-next build (x86_64
> >> > >>> allmodconfig) produced this warning:
> >> > >>>
> >> > >>> *
> >> > >>> * Restart config...
> >> > >>> *
> >> > >>> ....
> >> > >>>
> >> > >>> This is output by my "make allmodconfig" and only started after merging
> >> > >>> the net-next tree today. It has continued for further merges/builds.
> >> > >>>
> >> > >>> I suspect commit
> >> > >>>
> >> > >>> 1323061a018a ("net: phy: sfp: Add HWMON support for module sensors")
> >> > >>>
> >> > >>> which added an "imply" clause.
> >> > >>>
> >> > >> I thought "imply" was better than "depends on HWMON || HWMON=n", but maybe
> >> > >> not. Is that a caveat when using "imply", and does it mean that "imply"
> >> > >> should better not be used ?
> >> > >
> >> > > I don't know, sorry. It was just my best guess from what I could see
> >> > > had changed.
> >> > >
> >> > > I wonder if it makes a difference that I am doing my "make
> >> > > allmodconfig" on top of a previous "make allmodconfig" and some symbols
> >> > > are marked as "NEW" (though they are not symbols related to the changes
> >> > > that happened during the net-next tree merge)?
> >> > >
> >> >
> >> > I tried to reproduce the problem, but I don't see the message.
> >> >
> >> > What I do see, though, is that "make allmodconfig" on a clean tree,
> >> > followed by "make menuconfig", results in configuration changes.
> >> > Specifically,
> >> >
> >> > > CONFIG_ARC_EMAC_CORE=m
> >> > > CONFIG_ARC_EMAC=m
> >> > > CONFIG_EMAC_ROCKCHIP=m
> >> >
> >> > is removed by menuconfig, and a large number of "# ... is not set"
> >> > configuration lines are added. Weird and bad, since several of the
> >> > disabled configurations _should_ be enabled by "make allmodconfig",
> >> > and a large number of hwmon drivers are affected. Bisect does point
> >> > to "net: phy: sfp: Add HWMON support for module sensors", meaning
> >> > "imply hwmon" does have severe side effects and can not be used.
> >> >
> >> > I'll try to find a fix.
> >>
> >> OK, my mistake, the "make allmodconfig" works, the following "make"
> >> causes the config restart. (I am actually doing cross builds and using
> >> an external object directory, in case that matters.)
> >>
> >> I removed the "imply HWMON" line added by the above commit and the
> >> problem went away. Also, using "depends on HWMON || HWMON=n" instead
> >> of the imply fixes it.
> >
> > Yes, replacing imply with the dependency is what I did in the fixup patch.
> > Sorry, I should have copied you: https://patchwork.kernel.org/patch/10534925/
> >
> > It is a bit different - imply was supposed to enforce HWMON={y,n} if SFP=y,
> > and the depends line enforces SFP={n,m} if HWMON=m. I have no idea why
> > imply doesn't work, but I think I'll stay away from it in the future.
> >
> > Guenter
>
>
> Hmm, this could be a Kconfig bug.
>
> I will take a look.
>
>
> BTW,
> 'depends on HWMON || HWMON=n' is pointless
> because it is always true.
>
I disagree. Its purpose is to enforce SFP=m if HWMON=m while still
permitting HWMON=n.
Guenter
^ permalink raw reply
* [PATCH net] net: skb_segment() should not return NULL
From: Eric Dumazet @ 2018-07-19 23:04 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, Alexander Duyck
syzbot caught a NULL deref [1], caused by skb_segment()
skb_segment() has many "goto err;" that assume the @err variable
contains -ENOMEM.
A successful call to __skb_linearize() should not clear @err,
otherwise a subsequent memory allocation error could return NULL.
While we are at it, we might use -EINVAL instead of -ENOMEM when
MAX_SKB_FRAGS limit is reached.
[1]
kasan: CONFIG_KASAN_INLINE enabled
kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN
CPU: 0 PID: 13285 Comm: syz-executor3 Not tainted 4.18.0-rc4+ #146
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:tcp_gso_segment+0x3dc/0x1780 net/ipv4/tcp_offload.c:106
Code: f0 ff ff 0f 87 1c fd ff ff e8 00 88 0b fb 48 8b 75 d0 48 b9 00 00 00 00 00 fc ff df 48 8d be 90 00 00 00 48 89 f8 48 c1 e8 03 <0f> b6 14 08 48 8d 86 94 00 00 00 48 89 c6 83 e0 07 48 c1 ee 03 0f
RSP: 0018:ffff88019b7fd060 EFLAGS: 00010206
RAX: 0000000000000012 RBX: 0000000000000020 RCX: dffffc0000000000
RDX: 0000000000040000 RSI: 0000000000000000 RDI: 0000000000000090
RBP: ffff88019b7fd0f0 R08: ffff88019510e0c0 R09: ffffed003b5c46d6
R10: ffffed003b5c46d6 R11: ffff8801dae236b3 R12: 0000000000000001
R13: ffff8801d6c581f4 R14: 0000000000000000 R15: ffff8801d6c58128
FS: 00007fcae64d6700(0000) GS:ffff8801dae00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000004e8664 CR3: 00000001b669b000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
tcp4_gso_segment+0x1c3/0x440 net/ipv4/tcp_offload.c:54
inet_gso_segment+0x64e/0x12d0 net/ipv4/af_inet.c:1342
inet_gso_segment+0x64e/0x12d0 net/ipv4/af_inet.c:1342
skb_mac_gso_segment+0x3b5/0x740 net/core/dev.c:2792
__skb_gso_segment+0x3c3/0x880 net/core/dev.c:2865
skb_gso_segment include/linux/netdevice.h:4099 [inline]
validate_xmit_skb+0x640/0xf30 net/core/dev.c:3104
__dev_queue_xmit+0xc14/0x3910 net/core/dev.c:3561
dev_queue_xmit+0x17/0x20 net/core/dev.c:3602
neigh_hh_output include/net/neighbour.h:473 [inline]
neigh_output include/net/neighbour.h:481 [inline]
ip_finish_output2+0x1063/0x1860 net/ipv4/ip_output.c:229
ip_finish_output+0x841/0xfa0 net/ipv4/ip_output.c:317
NF_HOOK_COND include/linux/netfilter.h:276 [inline]
ip_output+0x223/0x880 net/ipv4/ip_output.c:405
dst_output include/net/dst.h:444 [inline]
ip_local_out+0xc5/0x1b0 net/ipv4/ip_output.c:124
iptunnel_xmit+0x567/0x850 net/ipv4/ip_tunnel_core.c:91
ip_tunnel_xmit+0x1598/0x3af1 net/ipv4/ip_tunnel.c:778
ipip_tunnel_xmit+0x264/0x2c0 net/ipv4/ipip.c:308
__netdev_start_xmit include/linux/netdevice.h:4148 [inline]
netdev_start_xmit include/linux/netdevice.h:4157 [inline]
xmit_one net/core/dev.c:3034 [inline]
dev_hard_start_xmit+0x26c/0xc30 net/core/dev.c:3050
__dev_queue_xmit+0x29ef/0x3910 net/core/dev.c:3569
dev_queue_xmit+0x17/0x20 net/core/dev.c:3602
neigh_direct_output+0x15/0x20 net/core/neighbour.c:1403
neigh_output include/net/neighbour.h:483 [inline]
ip_finish_output2+0xa67/0x1860 net/ipv4/ip_output.c:229
ip_finish_output+0x841/0xfa0 net/ipv4/ip_output.c:317
NF_HOOK_COND include/linux/netfilter.h:276 [inline]
ip_output+0x223/0x880 net/ipv4/ip_output.c:405
dst_output include/net/dst.h:444 [inline]
ip_local_out+0xc5/0x1b0 net/ipv4/ip_output.c:124
ip_queue_xmit+0x9df/0x1f80 net/ipv4/ip_output.c:504
tcp_transmit_skb+0x1bf9/0x3f10 net/ipv4/tcp_output.c:1168
tcp_write_xmit+0x1641/0x5c20 net/ipv4/tcp_output.c:2363
__tcp_push_pending_frames+0xb2/0x290 net/ipv4/tcp_output.c:2536
tcp_push+0x638/0x8c0 net/ipv4/tcp.c:735
tcp_sendmsg_locked+0x2ec5/0x3f00 net/ipv4/tcp.c:1410
tcp_sendmsg+0x2f/0x50 net/ipv4/tcp.c:1447
inet_sendmsg+0x1a1/0x690 net/ipv4/af_inet.c:798
sock_sendmsg_nosec net/socket.c:641 [inline]
sock_sendmsg+0xd5/0x120 net/socket.c:651
__sys_sendto+0x3d7/0x670 net/socket.c:1797
__do_sys_sendto net/socket.c:1809 [inline]
__se_sys_sendto net/socket.c:1805 [inline]
__x64_sys_sendto+0xe1/0x1a0 net/socket.c:1805
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x455ab9
Code: 1d ba fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 eb b9 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fcae64d5c68 EFLAGS: 00000246 ORIG_RAX: 000000000000002c
RAX: ffffffffffffffda RBX: 00007fcae64d66d4 RCX: 0000000000455ab9
RDX: 0000000000000001 RSI: 0000000020000200 RDI: 0000000000000013
RBP: 000000000072bea0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000014
R13: 00000000004c1145 R14: 00000000004d1818 R15: 0000000000000006
Modules linked in:
Dumping ftrace buffer:
(ftrace buffer empty)
Fixes: ddff00d42043 ("net: Move skb_has_shared_frag check out of GRE code and into segmentation")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Alexander Duyck <alexander.h.duyck@intel.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
net/core/skbuff.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 8e51f8555e11b95bc48ab334f50571048f705101..fb35b62af2724025f743d61de24f9fb7eb9186a8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -3720,6 +3720,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
net_warn_ratelimited(
"skb_segment: too many frags: %u %u\n",
pos, mss);
+ err = -EINVAL;
goto err;
}
@@ -3753,11 +3754,10 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
perform_csum_check:
if (!csum) {
- if (skb_has_shared_frag(nskb)) {
- err = __skb_linearize(nskb);
- if (err)
- goto err;
- }
+ if (skb_has_shared_frag(nskb) &&
+ __skb_linearize(nskb))
+ goto err;
+
if (!nskb->remcsum_offload)
nskb->ip_summed = CHECKSUM_NONE;
SKB_GSO_CB(nskb)->csum =
--
2.18.0.233.g985f88cf7e-goog
^ permalink raw reply related
* [PATCH] net: wimax: stack: fixed multi line comment issue
From: Mark Railton @ 2018-07-19 23:11 UTC (permalink / raw)
To: inaky.perez-gonzalez, davem
Cc: linux-wimax, netdev, linux-kernel, Mark Railton
Moved end of comment to it's own line per guide
Signed-off-by: Mark Railton <mark@markrailton.com>
---
net/wimax/stack.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/wimax/stack.c b/net/wimax/stack.c
index 5db731512014..73dba9c077bb 100644
--- a/net/wimax/stack.c
+++ b/net/wimax/stack.c
@@ -486,7 +486,8 @@ int wimax_dev_add(struct wimax_dev *wimax_dev, struct net_device *net_dev)
d_fnstart(3, dev, "(wimax_dev %p net_dev %p)\n", wimax_dev, net_dev);
/* Do the RFKILL setup before locking, as RFKILL will call
- * into our functions. */
+ * into our functions.
+ */
wimax_dev->net_dev = net_dev;
result = wimax_rfkill_add(wimax_dev);
if (result < 0)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf 0/2] BPF fix and test case
From: Alexei Starovoitov @ 2018-07-19 23:12 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: naveen.n.rao, sandipan, netdev
In-Reply-To: <20180719161836.24468-1-daniel@iogearbox.net>
On Thu, Jul 19, 2018 at 06:18:34PM +0200, Daniel Borkmann wrote:
> This set adds a ppc64 JIT fix for xadd as well as a missing test
> case for verifying whether xadd messes with src/dst reg. Thanks!
Applied, Thanks
^ permalink raw reply
* [PATCH net-next 0/9] TX used ring batched updating for vhost
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
Hi:
This series implement batch updating of used ring for TX. This help to
reduce the cache contention on used ring. The idea is first split
datacopy path from zerocopy, and do only batching for datacopy. This
is because zercopy had already supported its own batching.
TX PPS was increased 25.8% and Netperf TCP does not show obvious
differences.
The split of datapath will also be helpful for future implementation
like in order completion.
Please review.
Thanks
Jason Wang (9):
vhost_net: drop unnecessary parameter
vhost_net: introduce helper to initialize tx iov iter
vhost_net: introduce vhost_exceeds_weight()
vhost_net: introduce get_tx_bufs()
vhost_net: introduce tx_can_batch()
vhost_net: split out datacopy logic
vhost_net: rename vhost_rx_signal_used() to vhost_net_signal_used()
vhost_net: rename VHOST_RX_BATCH to VHOST_NET_BATCH
vhost_net: batch update used ring for datacopy TX
drivers/vhost/net.c | 249 +++++++++++++++++++++++++++++++++++++---------------
1 file changed, 179 insertions(+), 70 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH net-next 1/9] vhost_net: drop unnecessary parameter
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
In-Reply-To: <1532045721-4958-1-git-send-email-jasowang@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b224036..1a8175a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -430,7 +430,6 @@ static int vhost_net_enable_vq(struct vhost_net *n,
static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
struct vhost_virtqueue *vq,
- struct iovec iov[], unsigned int iov_size,
unsigned int *out_num, unsigned int *in_num,
bool *busyloop_intr)
{
@@ -512,9 +511,8 @@ static void handle_tx(struct vhost_net *net)
vhost_zerocopy_signal_used(net, vq);
busyloop_intr = false;
- head = vhost_net_tx_get_vq_desc(net, vq, vq->iov,
- ARRAY_SIZE(vq->iov),
- &out, &in, &busyloop_intr);
+ head = vhost_net_tx_get_vq_desc(net, vq, &out, &in,
+ &busyloop_intr);
/* On error, stop handling until the next kick. */
if (unlikely(head < 0))
break;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 2/9] vhost_net: introduce helper to initialize tx iov iter
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
In-Reply-To: <1532045721-4958-1-git-send-email-jasowang@redhat.com>
Introduce init_iov_iter() in order to be reused by future patch.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 1a8175a..cac28fd 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -466,6 +466,18 @@ static bool vhost_exceeds_maxpend(struct vhost_net *net)
min_t(unsigned int, VHOST_MAX_PEND, vq->num >> 2);
}
+static size_t init_iov_iter(struct vhost_virtqueue *vq, struct iov_iter *iter,
+ size_t hdr_size, int out)
+{
+ /* Skip header. TODO: support TSO. */
+ size_t len = iov_length(vq->iov, out);
+
+ iov_iter_init(iter, WRITE, vq->iov, out, len);
+ iov_iter_advance(iter, hdr_size);
+
+ return iov_iter_count(iter);
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_tx(struct vhost_net *net)
@@ -531,18 +543,14 @@ static void handle_tx(struct vhost_net *net)
"out %d, int %d\n", out, in);
break;
}
- /* Skip header. TODO: support TSO. */
- len = iov_length(vq->iov, out);
- iov_iter_init(&msg.msg_iter, WRITE, vq->iov, out, len);
- iov_iter_advance(&msg.msg_iter, hdr_size);
+
/* Sanity check */
- if (!msg_data_left(&msg)) {
- vq_err(vq, "Unexpected header len for TX: "
- "%zd expected %zd\n",
- len, hdr_size);
+ len = init_iov_iter(vq, &msg.msg_iter, hdr_size, out);
+ if (!len) {
+ vq_err(vq, "Unexpected header len for TX: %zd expected %zd\n",
+ len, hdr_size);
break;
}
- len = msg_data_left(&msg);
zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
&& !vhost_exceeds_maxpend(net)
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 3/9] vhost_net: introduce vhost_exceeds_weight()
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
In-Reply-To: <1532045721-4958-1-git-send-email-jasowang@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index cac28fd..b9e1674 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -478,6 +478,12 @@ static size_t init_iov_iter(struct vhost_virtqueue *vq, struct iov_iter *iter,
return iov_iter_count(iter);
}
+static bool vhost_exceeds_weight(int pkts, int total_len)
+{
+ return total_len >= VHOST_NET_WEIGHT ||
+ pkts >= VHOST_NET_PKT_WEIGHT;
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_tx(struct vhost_net *net)
@@ -576,7 +582,6 @@ static void handle_tx(struct vhost_net *net)
msg.msg_control = NULL;
ubufs = NULL;
}
-
total_len += len;
if (total_len < VHOST_NET_WEIGHT &&
!vhost_vq_avail_empty(&net->dev, vq) &&
@@ -606,8 +611,7 @@ static void handle_tx(struct vhost_net *net)
else
vhost_zerocopy_signal_used(net, vq);
vhost_net_tx_packet(net);
- if (unlikely(total_len >= VHOST_NET_WEIGHT) ||
- unlikely(++sent_pkts >= VHOST_NET_PKT_WEIGHT)) {
+ if (unlikely(vhost_exceeds_weight(++sent_pkts, total_len))) {
vhost_poll_queue(&vq->poll);
break;
}
@@ -918,8 +922,7 @@ static void handle_rx(struct vhost_net *net)
if (unlikely(vq_log))
vhost_log_write(vq, vq_log, log, vhost_len);
total_len += vhost_len;
- if (unlikely(total_len >= VHOST_NET_WEIGHT) ||
- unlikely(++recv_pkts >= VHOST_NET_PKT_WEIGHT)) {
+ if (unlikely(vhost_exceeds_weight(++recv_pkts, total_len))) {
vhost_poll_queue(&vq->poll);
goto out;
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 4/9] vhost_net: introduce get_tx_bufs()
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
In-Reply-To: <1532045721-4958-1-git-send-email-jasowang@redhat.com>
Factor out logic of getting tx buffer and iov iter
initialization. This will be used for reducing codes duplication in
the future.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 49 ++++++++++++++++++++++++++++++++-----------------
1 file changed, 32 insertions(+), 17 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b9e1674..a014ca0 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -484,6 +484,36 @@ static bool vhost_exceeds_weight(int pkts, int total_len)
pkts >= VHOST_NET_PKT_WEIGHT;
}
+static int get_tx_bufs(struct vhost_net *net,
+ struct vhost_net_virtqueue *nvq,
+ struct msghdr *msg,
+ unsigned int *out, unsigned int *in,
+ size_t *len, bool *busyloop_intr)
+{
+ struct vhost_virtqueue *vq = &nvq->vq;
+ int ret;
+
+ ret = vhost_net_tx_get_vq_desc(net, vq, out, in, busyloop_intr);
+ if (ret < 0 || ret == vq->num)
+ return ret;
+
+ if (*in) {
+ vq_err(vq, "Unexpected descriptor format for TX: out %d, int %d\n",
+ *out, *in);
+ return -EFAULT;
+ }
+
+ /* Sanity check */
+ *len = init_iov_iter(vq, &msg->msg_iter, nvq->vhost_hlen, *out);
+ if (*len == 0) {
+ vq_err(vq, "Unexpected header len for TX: %zd expected %zd\n",
+ *len, nvq->vhost_hlen);
+ return -EFAULT;
+ }
+
+ return ret;
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_tx(struct vhost_net *net)
@@ -501,7 +531,6 @@ static void handle_tx(struct vhost_net *net)
};
size_t len, total_len = 0;
int err;
- size_t hdr_size;
struct socket *sock;
struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
bool zcopy, zcopy_used;
@@ -518,7 +547,6 @@ static void handle_tx(struct vhost_net *net)
vhost_disable_notify(&net->dev, vq);
vhost_net_disable_vq(net, vq);
- hdr_size = nvq->vhost_hlen;
zcopy = nvq->ubufs;
for (;;) {
@@ -529,8 +557,8 @@ static void handle_tx(struct vhost_net *net)
vhost_zerocopy_signal_used(net, vq);
busyloop_intr = false;
- head = vhost_net_tx_get_vq_desc(net, vq, &out, &in,
- &busyloop_intr);
+ head = get_tx_bufs(net, nvq, &msg, &out, &in, &len,
+ &busyloop_intr);
/* On error, stop handling until the next kick. */
if (unlikely(head < 0))
break;
@@ -544,19 +572,6 @@ static void handle_tx(struct vhost_net *net)
}
break;
}
- if (in) {
- vq_err(vq, "Unexpected descriptor format for TX: "
- "out %d, int %d\n", out, in);
- break;
- }
-
- /* Sanity check */
- len = init_iov_iter(vq, &msg.msg_iter, hdr_size, out);
- if (!len) {
- vq_err(vq, "Unexpected header len for TX: %zd expected %zd\n",
- len, hdr_size);
- break;
- }
zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
&& !vhost_exceeds_maxpend(net)
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 5/9] vhost_net: introduce tx_can_batch()
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
In-Reply-To: <1532045721-4958-1-git-send-email-jasowang@redhat.com>
Introduce tx_can_batch() to determine whether TX could be
batched. This will help to reduce the code duplication in the future.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index a014ca0..f59b615 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -514,6 +514,12 @@ static int get_tx_bufs(struct vhost_net *net,
return ret;
}
+static bool tx_can_batch(struct vhost_virtqueue *vq, size_t total_len)
+{
+ return total_len < VHOST_NET_WEIGHT &&
+ !vhost_vq_avail_empty(vq->dev, vq);
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_tx(struct vhost_net *net)
@@ -598,8 +604,7 @@ static void handle_tx(struct vhost_net *net)
ubufs = NULL;
}
total_len += len;
- if (total_len < VHOST_NET_WEIGHT &&
- !vhost_vq_avail_empty(&net->dev, vq) &&
+ if (tx_can_batch(vq, total_len) &&
likely(!vhost_exceeds_maxpend(net))) {
msg.msg_flags |= MSG_MORE;
} else {
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 6/9] vhost_net: split out datacopy logic
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
In-Reply-To: <1532045721-4958-1-git-send-email-jasowang@redhat.com>
Instead of mixing zerocopy and datacopy logics, this patch tries to
split datacopy logic out. This results for a more compact code and
ad-hoc optimization could be done on top more easily.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 110 ++++++++++++++++++++++++++++++++++++++++++----------
1 file changed, 90 insertions(+), 20 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index f59b615..9cef0b2 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -520,9 +520,7 @@ static bool tx_can_batch(struct vhost_virtqueue *vq, size_t total_len)
!vhost_vq_avail_empty(vq->dev, vq);
}
-/* Expects to be always run from workqueue - which acts as
- * read-size critical section for our kind of RCU. */
-static void handle_tx(struct vhost_net *net)
+static void handle_tx_copy(struct vhost_net *net, struct socket *sock)
{
struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
struct vhost_virtqueue *vq = &nvq->vq;
@@ -537,30 +535,76 @@ static void handle_tx(struct vhost_net *net)
};
size_t len, total_len = 0;
int err;
- struct socket *sock;
- struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
- bool zcopy, zcopy_used;
int sent_pkts = 0;
- mutex_lock(&vq->mutex);
- sock = vq->private_data;
- if (!sock)
- goto out;
+ for (;;) {
+ bool busyloop_intr = false;
- if (!vq_iotlb_prefetch(vq))
- goto out;
+ head = get_tx_bufs(net, nvq, &msg, &out, &in, &len,
+ &busyloop_intr);
+ /* On error, stop handling until the next kick. */
+ if (unlikely(head < 0))
+ break;
+ /* Nothing new? Wait for eventfd to tell us they refilled. */
+ if (head == vq->num) {
+ if (unlikely(busyloop_intr)) {
+ vhost_poll_queue(&vq->poll);
+ } else if (unlikely(vhost_enable_notify(&net->dev,
+ vq))) {
+ vhost_disable_notify(&net->dev, vq);
+ continue;
+ }
+ break;
+ }
- vhost_disable_notify(&net->dev, vq);
- vhost_net_disable_vq(net, vq);
+ total_len += len;
+ if (tx_can_batch(vq, total_len))
+ msg.msg_flags |= MSG_MORE;
+ else
+ msg.msg_flags &= ~MSG_MORE;
+
+ /* TODO: Check specific error and bomb out unless ENOBUFS? */
+ err = sock->ops->sendmsg(sock, &msg, len);
+ if (unlikely(err < 0)) {
+ vhost_discard_vq_desc(vq, 1);
+ vhost_net_enable_vq(net, vq);
+ break;
+ }
+ if (err != len)
+ pr_debug("Truncated TX packet: len %d != %zd\n",
+ err, len);
+ vhost_add_used_and_signal(&net->dev, vq, head, 0);
+ if (vhost_exceeds_weight(++sent_pkts, total_len)) {
+ vhost_poll_queue(&vq->poll);
+ break;
+ }
+ }
+}
- zcopy = nvq->ubufs;
+static void handle_tx_zerocopy(struct vhost_net *net, struct socket *sock)
+{
+ struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
+ struct vhost_virtqueue *vq = &nvq->vq;
+ unsigned out, in;
+ int head;
+ struct msghdr msg = {
+ .msg_name = NULL,
+ .msg_namelen = 0,
+ .msg_control = NULL,
+ .msg_controllen = 0,
+ .msg_flags = MSG_DONTWAIT,
+ };
+ size_t len, total_len = 0;
+ int err;
+ struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
+ bool zcopy_used;
+ int sent_pkts = 0;
for (;;) {
bool busyloop_intr;
/* Release DMAs done buffers first */
- if (zcopy)
- vhost_zerocopy_signal_used(net, vq);
+ vhost_zerocopy_signal_used(net, vq);
busyloop_intr = false;
head = get_tx_bufs(net, nvq, &msg, &out, &in, &len,
@@ -579,9 +623,9 @@ static void handle_tx(struct vhost_net *net)
break;
}
- zcopy_used = zcopy && len >= VHOST_GOODCOPY_LEN
- && !vhost_exceeds_maxpend(net)
- && vhost_net_tx_select_zcopy(net);
+ zcopy_used = len >= VHOST_GOODCOPY_LEN
+ && !vhost_exceeds_maxpend(net)
+ && vhost_net_tx_select_zcopy(net);
/* use msg_control to pass vhost zerocopy ubuf info to skb */
if (zcopy_used) {
@@ -636,6 +680,32 @@ static void handle_tx(struct vhost_net *net)
break;
}
}
+}
+
+/* Expects to be always run from workqueue - which acts as
+ * read-size critical section for our kind of RCU. */
+static void handle_tx(struct vhost_net *net)
+{
+ struct vhost_net_virtqueue *nvq = &net->vqs[VHOST_NET_VQ_TX];
+ struct vhost_virtqueue *vq = &nvq->vq;
+ struct socket *sock;
+
+ mutex_lock(&vq->mutex);
+ sock = vq->private_data;
+ if (!sock)
+ goto out;
+
+ if (!vq_iotlb_prefetch(vq))
+ goto out;
+
+ vhost_disable_notify(&net->dev, vq);
+ vhost_net_disable_vq(net, vq);
+
+ if (vhost_sock_zcopy(sock))
+ handle_tx_zerocopy(net, sock);
+ else
+ handle_tx_copy(net, sock);
+
out:
mutex_unlock(&vq->mutex);
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 7/9] vhost_net: rename vhost_rx_signal_used() to vhost_net_signal_used()
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
In-Reply-To: <1532045721-4958-1-git-send-email-jasowang@redhat.com>
Rename for reusing this for TX.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 9cef0b2..53d305b 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -741,7 +741,7 @@ static int sk_has_rx_data(struct sock *sk)
return skb_queue_empty(&sk->sk_receive_queue);
}
-static void vhost_rx_signal_used(struct vhost_net_virtqueue *nvq)
+static void vhost_net_signal_used(struct vhost_net_virtqueue *nvq)
{
struct vhost_virtqueue *vq = &nvq->vq;
struct vhost_dev *dev = vq->dev;
@@ -765,7 +765,7 @@ static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
if (!len && tvq->busyloop_timeout) {
/* Flush batched heads first */
- vhost_rx_signal_used(rnvq);
+ vhost_net_signal_used(rnvq);
/* Both tx vq and rx socket were polled here */
mutex_lock_nested(&tvq->mutex, 1);
vhost_disable_notify(&net->dev, tvq);
@@ -1008,7 +1008,7 @@ static void handle_rx(struct vhost_net *net)
}
nvq->done_idx += headcount;
if (nvq->done_idx > VHOST_RX_BATCH)
- vhost_rx_signal_used(nvq);
+ vhost_net_signal_used(nvq);
if (unlikely(vq_log))
vhost_log_write(vq, vq_log, log, vhost_len);
total_len += vhost_len;
@@ -1022,7 +1022,7 @@ static void handle_rx(struct vhost_net *net)
else
vhost_net_enable_vq(net, vq);
out:
- vhost_rx_signal_used(nvq);
+ vhost_net_signal_used(nvq);
mutex_unlock(&vq->mutex);
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 8/9] vhost_net: rename VHOST_RX_BATCH to VHOST_NET_BATCH
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
In-Reply-To: <1532045721-4958-1-git-send-email-jasowang@redhat.com>
A more generic name which could be used for TX as well.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 53d305b..2fd2f0e3 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -94,7 +94,7 @@ struct vhost_net_ubuf_ref {
struct vhost_virtqueue *vq;
};
-#define VHOST_RX_BATCH 64
+#define VHOST_NET_BATCH 64
struct vhost_net_buf {
void **queue;
int tail;
@@ -168,7 +168,7 @@ static int vhost_net_buf_produce(struct vhost_net_virtqueue *nvq)
rxq->head = 0;
rxq->tail = ptr_ring_consume_batched(nvq->rx_ring, rxq->queue,
- VHOST_RX_BATCH);
+ VHOST_NET_BATCH);
return rxq->tail;
}
@@ -1007,7 +1007,7 @@ static void handle_rx(struct vhost_net *net)
goto out;
}
nvq->done_idx += headcount;
- if (nvq->done_idx > VHOST_RX_BATCH)
+ if (nvq->done_idx > VHOST_NET_BATCH)
vhost_net_signal_used(nvq);
if (unlikely(vq_log))
vhost_log_write(vq, vq_log, log, vhost_len);
@@ -1075,7 +1075,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
return -ENOMEM;
}
- queue = kmalloc_array(VHOST_RX_BATCH, sizeof(void *),
+ queue = kmalloc_array(VHOST_NET_BATCH, sizeof(void *),
GFP_KERNEL);
if (!queue) {
kfree(vqs);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 9/9] vhost_net: batch update used ring for datacopy TX
From: Jason Wang @ 2018-07-20 0:15 UTC (permalink / raw)
To: mst, jasowang, netdev; +Cc: kvm, virtualization, linux-kernel
In-Reply-To: <1532045721-4958-1-git-send-email-jasowang@redhat.com>
Like commit e2b3b35eb989 ("vhost_net: batch used ring update in rx"),
this patches implements batch used ring update for datacopy TX
(zerocopy has already done some kind of batching).
Testpmd transmission from guest to host (XDP_DROP on tap) shows 25.8%
improvement (from ~3.1Mpps to ~3.9Mpps) on Broadwell i7-5600U CPU @
2.60GHz machine. Netperf TCP tests does not show obvious differences.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 40 +++++++++++++++++++++++++---------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 2fd2f0e3..367d802 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -428,16 +428,31 @@ static int vhost_net_enable_vq(struct vhost_net *n,
return vhost_poll_start(poll, sock->file);
}
+static void vhost_net_signal_used(struct vhost_net_virtqueue *nvq)
+{
+ struct vhost_virtqueue *vq = &nvq->vq;
+ struct vhost_dev *dev = vq->dev;
+
+ if (!nvq->done_idx)
+ return;
+
+ vhost_add_used_and_signal_n(dev, vq, vq->heads, nvq->done_idx);
+ nvq->done_idx = 0;
+}
+
static int vhost_net_tx_get_vq_desc(struct vhost_net *net,
- struct vhost_virtqueue *vq,
+ struct vhost_net_virtqueue *nvq,
unsigned int *out_num, unsigned int *in_num,
bool *busyloop_intr)
{
+ struct vhost_virtqueue *vq = &nvq->vq;
unsigned long uninitialized_var(endtime);
int r = vhost_get_vq_desc(vq, vq->iov, ARRAY_SIZE(vq->iov),
out_num, in_num, NULL, NULL);
if (r == vq->num && vq->busyloop_timeout) {
+ if (!vhost_sock_zcopy(vq->private_data))
+ vhost_net_signal_used(nvq);
preempt_disable();
endtime = busy_clock() + vq->busyloop_timeout;
while (vhost_can_busy_poll(endtime)) {
@@ -493,7 +508,8 @@ static int get_tx_bufs(struct vhost_net *net,
struct vhost_virtqueue *vq = &nvq->vq;
int ret;
- ret = vhost_net_tx_get_vq_desc(net, vq, out, in, busyloop_intr);
+ ret = vhost_net_tx_get_vq_desc(net, nvq, out, in, busyloop_intr);
+
if (ret < 0 || ret == vq->num)
return ret;
@@ -557,6 +573,9 @@ static void handle_tx_copy(struct vhost_net *net, struct socket *sock)
break;
}
+ vq->heads[nvq->done_idx].id = cpu_to_vhost32(vq, head);
+ vq->heads[nvq->done_idx].len = 0;
+
total_len += len;
if (tx_can_batch(vq, total_len))
msg.msg_flags |= MSG_MORE;
@@ -573,12 +592,15 @@ static void handle_tx_copy(struct vhost_net *net, struct socket *sock)
if (err != len)
pr_debug("Truncated TX packet: len %d != %zd\n",
err, len);
- vhost_add_used_and_signal(&net->dev, vq, head, 0);
+ if (++nvq->done_idx >= VHOST_NET_BATCH)
+ vhost_net_signal_used(nvq);
if (vhost_exceeds_weight(++sent_pkts, total_len)) {
vhost_poll_queue(&vq->poll);
break;
}
}
+
+ vhost_net_signal_used(nvq);
}
static void handle_tx_zerocopy(struct vhost_net *net, struct socket *sock)
@@ -741,18 +763,6 @@ static int sk_has_rx_data(struct sock *sk)
return skb_queue_empty(&sk->sk_receive_queue);
}
-static void vhost_net_signal_used(struct vhost_net_virtqueue *nvq)
-{
- struct vhost_virtqueue *vq = &nvq->vq;
- struct vhost_dev *dev = vq->dev;
-
- if (!nvq->done_idx)
- return;
-
- vhost_add_used_and_signal_n(dev, vq, vq->heads, nvq->done_idx);
- nvq->done_idx = 0;
-}
-
static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
bool *busyloop_intr)
{
--
2.7.4
^ permalink raw reply related
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