* Re: [PATCH net-next] virtio_net: force_napi_tx module param.
From: Michael S. Tsirkin @ 2018-07-29 20:33 UTC (permalink / raw)
To: David Miller; +Cc: caleb.raitto, jasowang, netdev, caraitto
In-Reply-To: <20180729.090027.1373538625446665385.davem@davemloft.net>
On Sun, Jul 29, 2018 at 09:00:27AM -0700, David Miller wrote:
> From: Caleb Raitto <caleb.raitto@gmail.com>
> Date: Mon, 23 Jul 2018 16:11:19 -0700
>
> > From: Caleb Raitto <caraitto@google.com>
> >
> > The driver disables tx napi if it's not certain that completions will
> > be processed affine with tx service.
> >
> > Its heuristic doesn't account for some scenarios where it is, such as
> > when the queue pair count matches the core but not hyperthread count.
> >
> > Allow userspace to override the heuristic. This is an alternative
> > solution to that in the linked patch. That added more logic in the
> > kernel for these cases, but the agreement was that this was better left
> > to user control.
> >
> > Do not expand the existing napi_tx variable to a ternary value,
> > because doing so can break user applications that expect
> > boolean ('Y'/'N') instead of integer output. Add a new param instead.
> >
> > Link: https://patchwork.ozlabs.org/patch/725249/
> > Acked-by: Willem de Bruijn <willemb@google.com>
> > Acked-by: Jon Olson <jonolson@google.com>
> > Signed-off-by: Caleb Raitto <caraitto@google.com>
>
> So I looked into the history surrounding these issues.
>
> First of all, it's always ends up turning out crummy when drivers start
> to set affinities themselves. The worst possible case is to do it
> _conditionally_, and that is exactly what virtio_net is doing.
>
> >From the user's perspective, this provides a really bad experience.
>
> So if I have a 32-queue device and there are 32 cpus, you'll do all
> the affinity settings, stopping Irqbalanced from doing anything
> right?
>
> So if I add one more cpu, you'll say "oops, no idea what to do in
> this situation" and not touch the affinities at all?
>
> That makes no sense at all.
>
> If the driver is going to set affinities at all, OWN that decision
> and set it all the time to something reasonable.
>
> Or accept that you shouldn't be touching this stuff in the first place
> and leave the affinities alone.
>
> Right now we're kinda in a situation where the driver has been setting
> affinities in the ncpus==nqueues cases for some time, so we can't stop
> doing it.
>
> Which means we have to set them in all cases to make the user
> experience sane again.
>
> I looked at the linked to patch again:
>
> https://patchwork.ozlabs.org/patch/725249/
>
> And I think the strategy should be made more generic, to get rid of
> the hyperthreading assumptions. I also agree that the "assign
> to first N cpus" logic doesn't make much sense either.
>
> Just distribute across the available cpus evenly, and be done with it.
> If you have 64 cpus and 32 queues, this assigns queues to every other
> cpu.
>
> Then we don't need this weird new module parameter.
Can't we set affinity to a set of CPUs?
The point really is that tx irq handler needs a lock on the tx queue to
free up skbs, so processing it on another CPU while tx is active causes
cache line bounces. So we want affinity to CPUs that submit to this
queue on the theory they have these cache line(s) anyway.
I suspect it's not a uniqueue property of virtio.
--
MST
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: force_napi_tx module param.
From: David Miller @ 2018-07-29 20:36 UTC (permalink / raw)
To: mst; +Cc: caleb.raitto, jasowang, netdev, caraitto
In-Reply-To: <20180729231247-mutt-send-email-mst@kernel.org>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Sun, 29 Jul 2018 23:33:20 +0300
> The point really is that tx irq handler needs a lock on the tx queue to
> free up skbs, so processing it on another CPU while tx is active causes
> cache line bounces. So we want affinity to CPUs that submit to this
> queue on the theory they have these cache line(s) anyway.
>
> I suspect it's not a uniqueue property of virtio.
It certainly is not.
I think the objectives are clear, someone just needs to put them
together cleanly into a patch :)
^ permalink raw reply
* Re: [PATCH v2] Add BPF_SYNCHRONIZE_MAPS bpf(2) command
From: Daniel Colascione @ 2018-07-29 20:46 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Joel Fernandes, LKML, Tim Murray, Network Development,
Lorenzo Colitti, Chenbo Feng, Mathieu Desnoyers,
Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <CAADnVQL4twmGF3d26TgLsQJvnqLnM0F6hqL_42DSs95OuZ1y9A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]
On Sun, Jul 29, 2018 at 8:51 AM, Alexei Starovoitov <
alexei.starovoitov@gmail.com> wrote:
> On Thu, Jul 26, 2018 at 7:51 PM, Daniel Colascione <dancol@google.com>
> wrote:
> > BPF_SYNCHRONIZE_MAPS waits for the release of any references to a BPF
> > map made by a BPF program that is running at the time the
> > BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command is
> > to provide a means for userspace to replace a BPF map with another,
> > newer version, then ensure that no component is still using the "old"
> > map before manipulating the "old" map in some way.
> >
> > Signed-off-by: Daniel Colascione <dancol@google.com>
> > ---
> > include/uapi/linux/bpf.h | 9 +++++++++
> > kernel/bpf/syscall.c | 13 +++++++++++++
> > 2 files changed, 22 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index b7db3261c62d..5b27e9117d3e 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -75,6 +75,14 @@ struct bpf_lpm_trie_key {
> > __u8 data[0]; /* Arbitrary size */
> > };
> >
> > +/* BPF_SYNCHRONIZE_MAPS waits for the release of any references to a
> > + * BPF map made by a BPF program that is running at the time the
> > + * BPF_SYNCHRONIZE_MAPS command is issued. The purpose of this command
>
> that doesn't sound right to me.
> such command won't wait for the release of the references.
> in case of map-in-map the program does not hold
> the references to inner map (only to outer map).
>
Well, today that's the guarantee it provides. :-) I figured it'd be simpler
to explain the guarantee this way.
How about "waits for the release of any reference to a map obtained from
another map"?
>
> > + * is to provide a means for userspace to replace a BPF map with
> > + * another, newer version, then ensure that no component is still
> > + * using the "old" map before manipulating the "old" map in some way.
> > + */
>
> that's correct, but the whole paragraph still reads too
> 'generic' which will lead to confusion,
> whereas the use case is map-in-map only.
> I think bpf_sync_inner_map or
> bpf_sync_map_in_map would be better
> choices for the name.
I worry that a name like that would be too specific.
[-- Attachment #2: Type: text/html, Size: 3163 bytes --]
^ permalink raw reply
* [PATCH v3] Add BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES bpf(2) command
From: Daniel Colascione @ 2018-07-29 20:58 UTC (permalink / raw)
To: joelaf
Cc: linux-kernel, timmurray, netdev, Alexei Starovoitov,
Lorenzo Colitti, Chenbo Feng, Mathieu Desnoyers,
Alexei Starovoitov, Daniel Borkmann, Daniel Colascione
In-Reply-To: <CAKOZuesQ6GdNTGDFsNi4o8LYzxLBtYZ=Cz4=aZbqqCNia+QFnQ@mail.gmail.com>
BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES waits for the release of all
references to maps active at the instant the
BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES is
issued. BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES waits only for the
expiration of map references obtained by BPF programs from other maps.
The purpose of this command is to provide a means for userspace to
replace a BPF map with another, newer version, then ensure that no
component is still using the "old" map before manipulating the "old"
map in some way.
Signed-off-by: Daniel Colascione <dancol@google.com>
---
include/uapi/linux/bpf.h | 14 ++++++++++++++
kernel/bpf/syscall.c | 13 +++++++++++++
2 files changed, 27 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index b7db3261c62d..ca3cfca76edc 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -75,6 +75,19 @@ struct bpf_lpm_trie_key {
__u8 data[0]; /* Arbitrary size */
};
+/* BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES waits for the release of all
+ * references to maps active at the instant the
+ * BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES is
+ * issued. BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES waits only for the
+ * expiration of map references obtained by BPF programs from
+ * other maps.
+ *
+ * The purpose of this command is to provide a means for userspace to
+ * replace a BPF map with another, newer version, then ensure that no
+ * component is still using the "old" map before manipulating the
+ * "old" map in some way.
+ */
+
/* BPF syscall commands, see bpf(2) man-page for details. */
enum bpf_cmd {
BPF_MAP_CREATE,
@@ -98,6 +111,7 @@ enum bpf_cmd {
BPF_BTF_LOAD,
BPF_BTF_GET_FD_BY_ID,
BPF_TASK_FD_QUERY,
+ BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES,
};
enum bpf_map_type {
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index a31a1ba0f8ea..bc9a0713f47d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -2274,6 +2274,19 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
if (sysctl_unprivileged_bpf_disabled && !capable(CAP_SYS_ADMIN))
return -EPERM;
+ if (cmd == BPF_SYNCHRONIZE_MAP_TO_MAP_REFERENCES) {
+ if (uattr != NULL || size != 0)
+ return -EINVAL;
+ err = security_bpf(cmd, NULL, 0);
+ if (err < 0)
+ return err;
+ /* BPF programs always enter a critical section while
+ * they have a map reference outstanding.
+ */
+ synchronize_rcu();
+ return 0;
+ }
+
err = bpf_check_uarg_tail_zero(uattr, sizeof(attr), size);
if (err)
return err;
--
2.18.0.345.g5c9ce644c3-goog
^ permalink raw reply related
* Re: [PATCH net-next] virtio_net: force_napi_tx module param.
From: Willem de Bruijn @ 2018-07-29 21:09 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Miller, caleb.raitto, Jason Wang, Network Development,
Caleb Raitto, Benjamin Serebrin
In-Reply-To: <20180729231247-mutt-send-email-mst@kernel.org>
On Sun, Jul 29, 2018 at 4:48 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Jul 29, 2018 at 09:00:27AM -0700, David Miller wrote:
> > From: Caleb Raitto <caleb.raitto@gmail.com>
> > Date: Mon, 23 Jul 2018 16:11:19 -0700
> >
> > > From: Caleb Raitto <caraitto@google.com>
> > >
> > > The driver disables tx napi if it's not certain that completions will
> > > be processed affine with tx service.
> > >
> > > Its heuristic doesn't account for some scenarios where it is, such as
> > > when the queue pair count matches the core but not hyperthread count.
> > >
> > > Allow userspace to override the heuristic. This is an alternative
> > > solution to that in the linked patch. That added more logic in the
> > > kernel for these cases, but the agreement was that this was better left
> > > to user control.
> > >
> > > Do not expand the existing napi_tx variable to a ternary value,
> > > because doing so can break user applications that expect
> > > boolean ('Y'/'N') instead of integer output. Add a new param instead.
> > >
> > > Link: https://patchwork.ozlabs.org/patch/725249/
> > > Acked-by: Willem de Bruijn <willemb@google.com>
> > > Acked-by: Jon Olson <jonolson@google.com>
> > > Signed-off-by: Caleb Raitto <caraitto@google.com>
> >
> > So I looked into the history surrounding these issues.
> >
> > First of all, it's always ends up turning out crummy when drivers start
> > to set affinities themselves. The worst possible case is to do it
> > _conditionally_, and that is exactly what virtio_net is doing.
> >
> > >From the user's perspective, this provides a really bad experience.
> >
> > So if I have a 32-queue device and there are 32 cpus, you'll do all
> > the affinity settings, stopping Irqbalanced from doing anything
> > right?
> >
> > So if I add one more cpu, you'll say "oops, no idea what to do in
> > this situation" and not touch the affinities at all?
> >
> > That makes no sense at all.
> >
> > If the driver is going to set affinities at all, OWN that decision
> > and set it all the time to something reasonable.
> >
> > Or accept that you shouldn't be touching this stuff in the first place
> > and leave the affinities alone.
> >
> > Right now we're kinda in a situation where the driver has been setting
> > affinities in the ncpus==nqueues cases for some time, so we can't stop
> > doing it.
> >
> > Which means we have to set them in all cases to make the user
> > experience sane again.
> >
> > I looked at the linked to patch again:
> >
> > https://patchwork.ozlabs.org/patch/725249/
> >
> > And I think the strategy should be made more generic, to get rid of
> > the hyperthreading assumptions. I also agree that the "assign
> > to first N cpus" logic doesn't make much sense either.
> >
> > Just distribute across the available cpus evenly, and be done with it.
> > If you have 64 cpus and 32 queues, this assigns queues to every other
> > cpu.
> >
> > Then we don't need this weird new module parameter.
>
> Can't we set affinity to a set of CPUs?
>
> The point really is that tx irq handler needs a lock on the tx queue to
> free up skbs, so processing it on another CPU while tx is active causes
> cache line bounces. So we want affinity to CPUs that submit to this
> queue on the theory they have these cache line(s) anyway.
>
> I suspect it's not a uniqueue property of virtio.
It is a good heuristic. But as Jon pointed out, there is a trade-off
with other costs, such as increased interrupt load with additional
tx queues. This is particularly stark when multiple hyperthreads
share a cache, but have independent tx queues.
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: force_napi_tx module param.
From: Willem de Bruijn @ 2018-07-29 21:32 UTC (permalink / raw)
To: David Miller
Cc: caleb.raitto, Michael S. Tsirkin, Jason Wang, Network Development,
Caleb Raitto
In-Reply-To: <20180729.090027.1373538625446665385.davem@davemloft.net>
On Sun, Jul 29, 2018 at 12:01 PM David Miller <davem@davemloft.net> wrote:
>
> From: Caleb Raitto <caleb.raitto@gmail.com>
> Date: Mon, 23 Jul 2018 16:11:19 -0700
>
> > From: Caleb Raitto <caraitto@google.com>
> >
> > The driver disables tx napi if it's not certain that completions will
> > be processed affine with tx service.
> >
> > Its heuristic doesn't account for some scenarios where it is, such as
> > when the queue pair count matches the core but not hyperthread count.
> >
> > Allow userspace to override the heuristic. This is an alternative
> > solution to that in the linked patch. That added more logic in the
> > kernel for these cases, but the agreement was that this was better left
> > to user control.
> >
> > Do not expand the existing napi_tx variable to a ternary value,
> > because doing so can break user applications that expect
> > boolean ('Y'/'N') instead of integer output. Add a new param instead.
> >
> > Link: https://patchwork.ozlabs.org/patch/725249/
> > Acked-by: Willem de Bruijn <willemb@google.com>
> > Acked-by: Jon Olson <jonolson@google.com>
> > Signed-off-by: Caleb Raitto <caraitto@google.com>
>
> So I looked into the history surrounding these issues.
>
> First of all, it's always ends up turning out crummy when drivers start
> to set affinities themselves. The worst possible case is to do it
> _conditionally_, and that is exactly what virtio_net is doing.
>
> From the user's perspective, this provides a really bad experience.
>
> So if I have a 32-queue device and there are 32 cpus, you'll do all
> the affinity settings, stopping Irqbalanced from doing anything
> right?
>
> So if I add one more cpu, you'll say "oops, no idea what to do in
> this situation" and not touch the affinities at all?
>
> That makes no sense at all.
>
> If the driver is going to set affinities at all, OWN that decision
> and set it all the time to something reasonable.
>
> Or accept that you shouldn't be touching this stuff in the first place
> and leave the affinities alone.
>
> Right now we're kinda in a situation where the driver has been setting
> affinities in the ncpus==nqueues cases for some time, so we can't stop
> doing it.
>
> Which means we have to set them in all cases to make the user
> experience sane again.
>
> I looked at the linked to patch again:
>
> https://patchwork.ozlabs.org/patch/725249/
>
> And I think the strategy should be made more generic, to get rid of
> the hyperthreading assumptions. I also agree that the "assign
> to first N cpus" logic doesn't make much sense either.
>
> Just distribute across the available cpus evenly, and be done with it.
Sounds good to me.
> If you have 64 cpus and 32 queues, this assigns queues to every other
> cpu.
Striping half the number of queues as cores on a hyperthreaded system
with two logical cores per physical core will allocate all queues on
only half the physical cores.
But it is probably not safe to make any assumptions on virtual to
physical core mapping, anyway, which makes the simplest strategy is
preferable.
^ permalink raw reply
* Trade Inquiry
From: Invictus Group @ 2018-07-29 17:18 UTC (permalink / raw)
To: netdev
Hello,
This is Ms Julian Smith and i am from Invictus Group Co.,LTD in United Kingdom.
We are glad to know about your company from the web and we are interested in your products.
Could you kindly send us your Latest catalog and price list for our trial order.
Best Regards,
Julian Smith
Purchasing Manager
^ permalink raw reply
* Re: [net-next 10/16] net/mlx5: Support PCIe buffer congestion handling via Devlink
From: Alexander Duyck @ 2018-07-29 22:00 UTC (permalink / raw)
To: Moshe Shemesh
Cc: Bjorn Helgaas, Jiri Pirko, Jakub Kicinski, Eran Ben Elisha,
Saeed Mahameed, David S. Miller, netdev@vger.kernel.org,
linux-pci
In-Reply-To: <CALBF4T81ArTWR_i6SogC4qe8xPf3WYvdrSkypXt65wOf_upopg@mail.gmail.com>
On Sun, Jul 29, 2018 at 2:23 AM, Moshe Shemesh <moshes20.il@gmail.com> wrote:
>
>
> On Sat, Jul 28, 2018 at 7:06 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>
>> On Thu, Jul 26, 2018 at 07:00:20AM -0700, Alexander Duyck wrote:
>> > On Thu, Jul 26, 2018 at 12:14 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> > > Thu, Jul 26, 2018 at 02:43:59AM CEST, jakub.kicinski@netronome.com
>> > > wrote:
>> > >>On Wed, 25 Jul 2018 08:23:26 -0700, Alexander Duyck wrote:
>> > >>> On Wed, Jul 25, 2018 at 5:31 AM, Eran Ben Elisha wrote:
>> > >>> > On 7/24/2018 10:51 PM, Jakub Kicinski wrote:
>> > >>> >>>> The devlink params haven't been upstream even for a full cycle
>> > >>> >>>> and
>> > >>> >>>> already you guys are starting to use them to configure standard
>> > >>> >>>> features like queuing.
>> > >>> >>>
>> > >>> >>> We developed the devlink params in order to support non-standard
>> > >>> >>> configuration only. And for non-standard, there are generic and
>> > >>> >>> vendor
>> > >>> >>> specific options.
>> > >>> >>
>> > >>> >> I thought it was developed for performing non-standard and
>> > >>> >> possibly
>> > >>> >> vendor specific configuration. Look at DEVLINK_PARAM_GENERIC_*
>> > >>> >> for
>> > >>> >> examples of well justified generic options for which we have no
>> > >>> >> other API. The vendor mlx4 options look fairly vendor specific
>> > >>> >> if you
>> > >>> >> ask me, too.
>> > >>> >>
>> > >>> >> Configuring queuing has an API. The question is it acceptable to
>> > >>> >> enter
>> > >>> >> into the risky territory of controlling offloads via devlink
>> > >>> >> parameters
>> > >>> >> or would we rather make vendors take the time and effort to model
>> > >>> >> things to (a subset) of existing APIs. The HW never fits the
>> > >>> >> APIs
>> > >>> >> perfectly.
>> > >>> >
>> > >>> > I understand what you meant here, I would like to highlight that
>> > >>> > this
>> > >>> > mechanism was not meant to handle SRIOV, Representors, etc.
>> > >>> > The vendor specific configuration suggested here is to handle a
>> > >>> > congestion
>> > >>> > state in Multi Host environment (which includes PF and multiple
>> > >>> > VFs per
>> > >>> > host), where one host is not aware to the other hosts, and each is
>> > >>> > running
>> > >>> > on its own pci/driver. It is a device working mode configuration.
>> > >>> >
>> > >>> > This couldn't fit into any existing API, thus creating this
>> > >>> > vendor specific
>> > >>> > unique API is needed.
>> > >>>
>> > >>> If we are just going to start creating devlink interfaces in for
>> > >>> every
>> > >>> one-off option a device wants to add why did we even bother with
>> > >>> trying to prevent drivers from using sysfs? This just feels like we
>> > >>> are back to the same arguments we had back in the day with it.
>> > >>>
>> > >>> I feel like the bigger question here is if devlink is how we are
>> > >>> going
>> > >>> to deal with all PCIe related features going forward, or should we
>> > >>> start looking at creating a new interface/tool for PCI/PCIe related
>> > >>> features? My concern is that we have already had features such as
>> > >>> DMA
>> > >>> Coalescing that didn't really fit into anything and now we are
>> > >>> starting to see other things related to DMA and PCIe bus credits.
>> > >>> I'm
>> > >>> wondering if we shouldn't start looking at a tool/interface to
>> > >>> configure all the PCIe related features such as interrupts, error
>> > >>> reporting, DMA configuration, power management, etc. Maybe we could
>> > >>> even look at sharing it across subsystems and include things like
>> > >>> storage, graphics, and other subsystems in the conversation.
>> > >>
>> > >>Agreed, for actual PCIe configuration (i.e. not ECN marking) we do
>> > >> need
>> > >>to build up an API. Sharing it across subsystems would be very cool!
>>
>> I read the thread (starting at [1], for anybody else coming in late)
>> and I see this has something to do with "configuring outbound PCIe
>> buffers", but I haven't seen the connection to PCIe protocol or
>> features, i.e., I can't connect this to anything in the PCIe spec.
>>
>> Can somebody help me understand how the PCI core is relevant? If
>> there's some connection with a feature defined by PCIe, or if it
>> affects the PCIe transaction protocol somehow, I'm definitely
>> interested in this. But if this only affects the data transferred
>> over PCIe, i.e., the data payloads of PCIe TLP packets, then I'm not
>> sure why the PCI core should care.
>>
>
>
> As you wrote, this is not a PCIe feature or affects the PCIe transaction
> protocol.
>
> Actually, due to hardware limitation in current device, we have enabled a
> workaround in hardware.
>
> This mode is proprietary and not relevant to other PCIe devices, thus is set
> using driver-specific parameter in devlink
Essentially what this feature is doing is communicating the need for
PCIe back-pressure to the network fabric. So as the buffers on the
device start to fill because the device isn't able to get back PCIe
credits fast enough it will then start to send congestion
notifications to the network stack itself if I understand this
correctly.
For now there are no major conflicts, but when we start getting into
stuff like PCIe DMA coalescing, and on a more general basis just PCIe
active state power management that is going to start making things
more complicated going forward.
I assume the devices we are talking about supporting this new feature
on either don't deal with ASPM or assume a quick turnaround to get out
of the lower power states? Otherwise that would definitely cause some
back-pressure buildups that would hurt performance.
- Alex
^ permalink raw reply
* Re: [PATCH] 9p: fix Use-After-Free in p9_write_work()
From: Dominique Martinet @ 2018-07-29 23:33 UTC (permalink / raw)
To: Tomas Bortoli; +Cc: davem, v9fs-developer, netdev, linux-kernel, syzkaller
In-Reply-To: <20180729130248.29612-1-tomasbortoli@gmail.com>
Tomas Bortoli wrote on Sun, Jul 29, 2018:
> There is a race condition between p9_free_req() and p9_write_work().
> A request might still need to be processed while p9_free_req() is called.
>
> To fix it, flush the read/write work before freeing any request.
>
> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com
It looks like I have not received this report, I found it through google
in the lkml archives but Dmitry do you have a convenient-ish way of
finding the report on the syzkaller website with that reported-by tag?
> ---
>
> To be able to flush the r/w work from client.c we need the p9_conn and
> p9_trans_fd definitions. Therefore this commit moves most of the declarations in
> trans_fd.c to trans_fd.h and import such file in client.c
This cannot work as it is, because you're not just intorudcing the
trans_fd types but you're really depending on the transport used being
fd.
'conn.wq' won't even be valid memory in other transports so I don't want
to know what trying to flush_work on this will do... :)
Other transports also have the same issue see discussion in
https://lkml.org/lkml/2018/7/19/727
(that is another syzbot report, slightly different but I believe it
points to the same issue)
Basically, a more global view of the problem is a race between
p9_tag_lookup returning a p9_req_t and another thread freeing it.
Matthew wrote the problem himself in a comment in p9_tag_lookup in his new
version that used to be in linux-next at the time (I took the commit out
temporarily until I've had time to benchmark it, but it will come back in,
just you're working on thin air right now because the bug was only found
thanks to this commit):
+ /* There's no refcount on the req; a malicious server could
cause
+ * us to dereference a NULL pointer
+ */
So a more proper solution would be to had a refcount to req, have
p9_tag_lookup increment the refcount within rcu_read_lock, and have a
deref function free the req when the count hits 0.
Now we're here though I'm not sure what to suggest, I had promised to
get some performance benchmark out by this past weekend and I obviously
failed, so this patch might be delayed to 4.20 and the refcount approach
would not work with the current req cache/reuse system we have right
now.
If you want to finish this anyway you can work on my 9p-test branch
(I've kept the commit there), and I'll take that patch in at the same
time as the other.
> Moreover, a couple of identifiers were altered to avoid name conflicts with the
> new import.
If we were to stick to this approach, two suggestions:
- headers aren't all-or-nothing, I think it's better to only expose
what you need (and e.g. keep the Opt_* enum in the .c)
- instead of exposing trans_fd specific stuff, it's cleaner to add a
new op vector '.flush' to the p9_trans_module struct and call that if it
exists.
Thanks,
--
Dominique
^ permalink raw reply
* Re: [V9fs-developer] [PATCH] 9p: fix Use-After-Free in p9_write_work()
From: Dominique Martinet @ 2018-07-30 0:18 UTC (permalink / raw)
To: Tomas Bortoli; +Cc: v9fs-developer, syzkaller, davem, linux-kernel, netdev
In-Reply-To: <20180729233336.GB28684@nautica>
Dominique Martinet wrote on Mon, Jul 30, 2018:
> Basically, a more global view of the problem is a race between
> p9_tag_lookup returning a p9_req_t and another thread freeing it.
(just correcting myself here, p9_tag_lookup won't be enough for the
write side, but similarily you'd just need to increment the refcount
when you schedule work with it and decrement when the worker is done)
--
Dominique
^ permalink raw reply
* Re: [PATCH net-next 1/2] tcp: call tcp_drop() in tcp collapse
From: Yafang Shao @ 2018-07-30 2:06 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, LKML
In-Reply-To: <CANn89i+12HvAsvk9crDcqGh=v-DvLjuGQCOzVTPVDTUhHgeDiw@mail.gmail.com>
On Sun, Jul 29, 2018 at 12:28 AM, Eric Dumazet <edumazet@google.com> wrote:
> On Sat, Jul 28, 2018 at 12:43 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>>
>> On Sat, Jul 28, 2018 at 11:38 AM, Eric Dumazet <edumazet@google.com> wrote:
>> > On Fri, Jul 27, 2018 at 8:35 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>> >
>> >> So what about LINUX_MIB_TCPOFOMERGE ?
>> >> Regarding LINUX_MIB_TCPOFOMERGE, a skb is already covered by another
>> >> skb, is that dropping the packet or simply lowering the memory
>> >> overhead ?
>> >
>> > What do you think ?
>> >
>> > If you receive two times the same payload, don't you have to drop one
>> > of the duplicate ?
>> >
>> > There is a a big difference between the two cases.
>>
>> If the drop caused some data lost (which may then cause retransmition
>> or something), then this is a really DROP.
>> While if the drop won't cause any data lost, meaning it is a
>> non-harmful behavior, I think it should not be defined as DROP.
>> This is my suggestion anyway.
>
> Sigh.
>
> We count drops, not because they are ' bad or something went wrong'.
>
> If TCP stack receives twice the same sequence (same payload), we
> _drop_ one of the duplicate, so we account for this event.
>
> When ' collapsing' we reorganize our own storage, not because we have
> to drop a payload,
> but for some memory pressure reason.
Thanks for you clarification.
So what about LINUX_MIB_TCPOFODROP ?
if (unlikely(tcp_try_rmem_schedule(sk, skb, skb->truesize))) {
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPOFODROP);
tcp_drop(sk, skb);
return;
}
It is also because of our own memory pressure, but we call tcp_drop() here.
I am not mean to disagree with you. I am just confused and want to
make it clear.
> We have specific SNMP counters to account for these, we do not want to
> pretend a packet was ' dropped' since it was not.
>
> If we have to _drop_ some packets, it is called Pruning, and we do
> properly account for these drops.
Agreed.
Thanks
Yafang
^ permalink raw reply
* Re: [PATCH net-next 1/2] tcp: call tcp_drop() in tcp collapse
From: Eric Dumazet @ 2018-07-30 2:27 UTC (permalink / raw)
To: Yafang Shao; +Cc: David Miller, netdev, LKML
In-Reply-To: <CALOAHbBFuASgsQK+iiAE+ebPOg5zjtzauYaN_Xtmfc9_wjTFgw@mail.gmail.com>
On Sun, Jul 29, 2018 at 7:06 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Sun, Jul 29, 2018 at 12:28 AM, Eric Dumazet <edumazet@google.com> wrote:
> > On Sat, Jul 28, 2018 at 12:43 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >>
> >> On Sat, Jul 28, 2018 at 11:38 AM, Eric Dumazet <edumazet@google.com> wrote:
> >> > On Fri, Jul 27, 2018 at 8:35 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >> >
> >> >> So what about LINUX_MIB_TCPOFOMERGE ?
> >> >> Regarding LINUX_MIB_TCPOFOMERGE, a skb is already covered by another
> >> >> skb, is that dropping the packet or simply lowering the memory
> >> >> overhead ?
> >> >
> >> > What do you think ?
> >> >
> >> > If you receive two times the same payload, don't you have to drop one
> >> > of the duplicate ?
> >> >
> >> > There is a a big difference between the two cases.
> >>
> >> If the drop caused some data lost (which may then cause retransmition
> >> or something), then this is a really DROP.
> >> While if the drop won't cause any data lost, meaning it is a
> >> non-harmful behavior, I think it should not be defined as DROP.
> >> This is my suggestion anyway.
> >
> > Sigh.
> >
> > We count drops, not because they are ' bad or something went wrong'.
> >
> > If TCP stack receives twice the same sequence (same payload), we
> > _drop_ one of the duplicate, so we account for this event.
> >
> > When ' collapsing' we reorganize our own storage, not because we have
> > to drop a payload,
> > but for some memory pressure reason.
>
> Thanks for you clarification.
> So what about LINUX_MIB_TCPOFODROP ?
>
> if (unlikely(tcp_try_rmem_schedule(sk, skb, skb->truesize))) {
> NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPOFODROP);
> tcp_drop(sk, skb);
> return;
> }
>
>
> It is also because of our own memory pressure, but we call tcp_drop() here.
Yes, we _drop_ a packet.
That is pretty clear that the payload is dropped, and that the sender
will have to _retransmit_.
>
> I am not mean to disagree with you. I am just confused and want to
> make it clear.
Collapsing is :
For (a bunch of packets)
Try (to compress them in order to reduce memory overhead)
No drop of payload happens here. Sender wont have to retransmit.
^ permalink raw reply
* Re: [PATCH net-next v6 1/4] net: vhost: lock the vqs one by one
From: Jason Wang @ 2018-07-30 2:54 UTC (permalink / raw)
To: Tonghao Zhang, mst; +Cc: Linux Kernel Network Developers, virtualization
In-Reply-To: <CAMDZJNX41vtdNNEAxHYwC+WcrJFkON70hVumVE9rbFDBC5QUOQ@mail.gmail.com>
On 2018年07月25日 20:05, Tonghao Zhang wrote:
> On Sun, Jul 22, 2018 at 11:26 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Sat, Jul 21, 2018 at 11:03:59AM -0700, xiangxia.m.yue@gmail.com wrote:
>>> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>>
>>> This patch changes the way that lock all vqs
>>> at the same, to lock them one by one. It will
>>> be used for next patch to avoid the deadlock.
>>>
>>> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>> drivers/vhost/vhost.c | 24 +++++++-----------------
>>> 1 file changed, 7 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>> index a502f1a..a1c06e7 100644
>>> --- a/drivers/vhost/vhost.c
>>> +++ b/drivers/vhost/vhost.c
>>> @@ -294,8 +294,11 @@ static void vhost_vq_meta_reset(struct vhost_dev *d)
>>> {
>>> int i;
>>>
>>> - for (i = 0; i < d->nvqs; ++i)
>>> + for (i = 0; i < d->nvqs; ++i) {
>>> + mutex_lock(&d->vqs[i]->mutex);
>>> __vhost_vq_meta_reset(d->vqs[i]);
>>> + mutex_unlock(&d->vqs[i]->mutex);
>>> + }
>>> }
>>>
>>> static void vhost_vq_reset(struct vhost_dev *dev,
>>> @@ -890,20 +893,6 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
>>> #define vhost_get_used(vq, x, ptr) \
>>> vhost_get_user(vq, x, ptr, VHOST_ADDR_USED)
>>>
>>> -static void vhost_dev_lock_vqs(struct vhost_dev *d)
>>> -{
>>> - int i = 0;
>>> - for (i = 0; i < d->nvqs; ++i)
>>> - mutex_lock_nested(&d->vqs[i]->mutex, i);
>>> -}
>>> -
>>> -static void vhost_dev_unlock_vqs(struct vhost_dev *d)
>>> -{
>>> - int i = 0;
>>> - for (i = 0; i < d->nvqs; ++i)
>>> - mutex_unlock(&d->vqs[i]->mutex);
>>> -}
>>> -
>>> static int vhost_new_umem_range(struct vhost_umem *umem,
>>> u64 start, u64 size, u64 end,
>>> u64 userspace_addr, int perm)
>>> @@ -953,7 +942,10 @@ static void vhost_iotlb_notify_vq(struct vhost_dev *d,
>>> if (msg->iova <= vq_msg->iova &&
>>> msg->iova + msg->size - 1 > vq_msg->iova &&
>>> vq_msg->type == VHOST_IOTLB_MISS) {
>>> + mutex_lock(&node->vq->mutex);
>>> vhost_poll_queue(&node->vq->poll);
>>> + mutex_unlock(&node->vq->mutex);
>>> +
>>> list_del(&node->node);
>>> kfree(node);
>>> }
>>> @@ -985,7 +977,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
>>> int ret = 0;
>>>
>>> mutex_lock(&dev->mutex);
>>> - vhost_dev_lock_vqs(dev);
>>> switch (msg->type) {
>>> case VHOST_IOTLB_UPDATE:
>>> if (!dev->iotlb) {
>>> @@ -1019,7 +1010,6 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
>>> break;
>>> }
>>>
>>> - vhost_dev_unlock_vqs(dev);
>>> mutex_unlock(&dev->mutex);
>>>
>>> return ret;
>> I do prefer the finer-grained locking but I remember we
>> discussed something like this in the past and Jason saw issues
>> with such a locking.
> This change is suggested by Jason. Should I send new version because
> the patch 3 is changed.
>
>> Jason?
Actually, the code was a little bit tricky here. Since it assumes
handle_tx() and handle_rx() run on a single thread. Though the lock
ordering is different, it was still safe.
Maybe we can add some comments to explain this.
Thanks
>>
>>> --
>>> 1.8.3.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* linux-next: manual merge of the crypto tree with the net-next tree
From: Stephen Rothwell @ 2018-07-30 3:22 UTC (permalink / raw)
To: Herbert Xu, David Miller, Networking
Cc: Linux-Next Mailing List, Linux Kernel Mailing List,
Krzysztof Kozlowski
[-- Attachment #1: Type: text/plain, Size: 992 bytes --]
Hi Herbert,
Today's linux-next merge of the crypto tree got conflicts in:
drivers/net/ethernet/freescale/fec_main.c
drivers/net/ethernet/freescale/fs_enet/mac-fec.c
between commits:
16f6e9835bcd ("net: ethernet: freescale: Use generic CRC32 implementation")
d805f6a86829 ("net: ethernet: fs-enet: Use generic CRC32 implementation")
from the net-next tree and commit:
5d258b48efbd ("net: ethernet: Use existing define with polynomial")
from the crypto tree.
I fixed it up (I used the net-next tree versions (but kept the rmeoval
of the CRC32_POLY and FEC_CRC_POLY defines) and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging. You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* linux-next: manual merge of the devicetree tree with the net-next tree
From: Stephen Rothwell @ 2018-07-30 4:37 UTC (permalink / raw)
To: Rob Herring, David Miller, Networking
Cc: Linux-Next Mailing List, Linux Kernel Mailing List, Anssi Hannula
[-- Attachment #1: Type: text/plain, Size: 2647 bytes --]
Hi Rob,
Today's linux-next merge of the devicetree tree got a conflict in:
Documentation/devicetree/bindings/net/can/xilinx_can.txt
between commit:
7cb0f17f5252 ("dt-bindings: can: xilinx_can: add Xilinx CAN FD bindings")
from the net-next tree and commit:
791d3ef2e111 ("dt-bindings: remove 'interrupt-parent' from bindings")
from the devicetree tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc Documentation/devicetree/bindings/net/can/xilinx_can.txt
index ae5c07e96ad5,9264d2f6a89d..000000000000
--- a/Documentation/devicetree/bindings/net/can/xilinx_can.txt
+++ b/Documentation/devicetree/bindings/net/can/xilinx_can.txt
@@@ -2,26 -2,19 +2,25 @@@ Xilinx Axi CAN/Zynq CANPS controller De
---------------------------------------------------------
Required properties:
-- compatible : Should be "xlnx,zynq-can-1.0" for Zynq CAN
- controllers and "xlnx,axi-can-1.00.a" for Axi CAN
- controllers.
-- reg : Physical base address and size of the Axi CAN/Zynq
- CANPS registers map.
+- compatible : Should be:
+ - "xlnx,zynq-can-1.0" for Zynq CAN controllers
+ - "xlnx,axi-can-1.00.a" for Axi CAN controllers
+ - "xlnx,canfd-1.0" for CAN FD controllers
+- reg : Physical base address and size of the controller
+ registers map.
- interrupts : Property with a value describing the interrupt
number.
- - interrupt-parent : Must be core interrupt controller
-- clock-names : List of input clock names - "can_clk", "pclk"
- (For CANPS), "can_clk" , "s_axi_aclk"(For AXI CAN)
+- clock-names : List of input clock names
+ - "can_clk", "pclk" (For CANPS),
+ - "can_clk", "s_axi_aclk" (For AXI CAN and CAN FD).
(See clock bindings for details).
- clocks : Clock phandles (see clock bindings for details).
-- tx-fifo-depth : Can Tx fifo depth.
-- rx-fifo-depth : Can Rx fifo depth.
+- tx-fifo-depth : Can Tx fifo depth (Zynq, Axi CAN).
+- rx-fifo-depth : Can Rx fifo depth (Zynq, Axi CAN, CAN FD in
+ sequential Rx mode).
+- tx-mailbox-count : Can Tx mailbox buffer count (CAN FD).
+- rx-mailbox-count : Can Rx mailbox buffer count (CAN FD in mailbox Rx
+ mode).
Example:
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [Question] bluetooth/{bnep,cmtp,hidp}: memory barriers
From: Andrea Parri @ 2018-07-30 3:10 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, David S. Miller
Cc: linux-bluetooth, netdev, linux-kernel, Jeffy Chen, Brian Norris,
AL Yu-Chen Cho
Hi,
I'm currently puzzled by the the three calls to smp_mb__before_atomic()
in bnep_session(), cmtp_session() and hidp_session_run() respectively:
On the one hand, these barriers provide no guarantee on the subsequent
atomic_read(s->terminate) (as the comments preceding the barriers seem
to suggest), because atomic_read() is not a read-modify-write.
On the other hand, I'm currently unable to say *why such an "mb" would
be required: not being too familiar with this code, I figured I should
ask before sending a patch. ;-)
Andrea
^ permalink raw reply
* Re: [PATCH net-next v6 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Jason Wang @ 2018-07-30 3:16 UTC (permalink / raw)
To: Tonghao Zhang, makita.toshiaki
Cc: toshiaki.makita1, mst, virtualization,
Linux Kernel Network Developers
In-Reply-To: <CAMDZJNVVJs35kuvktTxn+mmDz7db+1K-kfuOoMUn9Z=WoayUVw@mail.gmail.com>
On 2018年07月24日 11:28, Tonghao Zhang wrote:
> On Tue, Jul 24, 2018 at 10:53 AM Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp> wrote:
>> On 2018/07/24 2:31, Tonghao Zhang wrote:
>>> On Mon, Jul 23, 2018 at 10:20 PM Toshiaki Makita
>>> <toshiaki.makita1@gmail.com> wrote:
>>>> On 18/07/23 (月) 21:43, Tonghao Zhang wrote:
>>>>> On Mon, Jul 23, 2018 at 5:58 PM Toshiaki Makita
>>>>> <makita.toshiaki@lab.ntt.co.jp> wrote:
>>>>>> On 2018/07/22 3:04,xiangxia.m.yue@gmail.com wrote:
>>>>>>> From: Tonghao Zhang<xiangxia.m.yue@gmail.com>
>>>>>>>
>>>>>>> Factor out generic busy polling logic and will be
>>>>>>> used for in tx path in the next patch. And with the patch,
>>>>>>> qemu can set differently the busyloop_timeout for rx queue.
>>>>>>>
>>>>>>> Signed-off-by: Tonghao Zhang<xiangxia.m.yue@gmail.com>
>>>>>>> ---
>>>>>> ...
>>>>>>> +static void vhost_net_busy_poll_vq_check(struct vhost_net *net,
>>>>>>> + struct vhost_virtqueue *rvq,
>>>>>>> + struct vhost_virtqueue *tvq,
>>>>>>> + bool rx)
>>>>>>> +{
>>>>>>> + struct socket *sock = rvq->private_data;
>>>>>>> +
>>>>>>> + if (rx) {
>>>>>>> + if (!vhost_vq_avail_empty(&net->dev, tvq)) {
>>>>>>> + vhost_poll_queue(&tvq->poll);
>>>>>>> + } else if (unlikely(vhost_enable_notify(&net->dev, tvq))) {
>>>>>>> + vhost_disable_notify(&net->dev, tvq);
>>>>>>> + vhost_poll_queue(&tvq->poll);
>>>>>>> + }
>>>>>>> + } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>>>>> + !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>>> + vhost_poll_queue(&rvq->poll);
>>>>>> Now we wait for vq_avail for rx as well, I think you cannot skip
>>>>>> vhost_enable_notify() on tx. Probably you might want to do:
>>>>> I think vhost_enable_notify is needed.
>>>>>
>>>>>> } else if (sock && sk_has_rx_data(sock->sk)) {
>>>>>> if (!vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>>> vhost_poll_queue(&rvq->poll);
>>>>>> } else if (unlikely(vhost_enable_notify(&net->dev, rvq))) {
>>>>>> vhost_disable_notify(&net->dev, rvq);
>>>>>> vhost_poll_queue(&rvq->poll);
>>>>>> }
>>>>>> }
>>>>> As Jason review as before, we only want rx kick when packet is pending at
>>>>> socket but we're out of available buffers. So we just enable notify,
>>>>> but not poll it ?
>>>>>
>>>>> } else if ((sock && sk_has_rx_data(sock->sk)) &&
>>>>> !vhost_vq_avail_empty(&net->dev, rvq)) {
>>>>> vhost_poll_queue(&rvq->poll);
>>>>> else {
>>>>> vhost_enable_notify(&net->dev, rvq);
>>>>> }
>>>> When vhost_enable_notify() returns true the avail becomes non-empty
>>>> while we are enabling notify. We may delay the rx process if we don't
>>>> check the return value of vhost_enable_notify().
>>> I got it thanks.
>>>>>> Also it's better to care vhost_net_disable_vq()/vhost_net_enable_vq() on tx?
>>>>> I cant find why it is better, if necessary, we can do it.
>>>> The reason is pretty simple... we are busypolling the socket so we don't
>>>> need rx wakeups during it?
>>> OK, but one question, how about rx? do we use the
>>> vhost_net_disable_vq/vhost_net_ensable_vq on rx ?
>> If we are busypolling the sock tx buf? I'm not sure if polling it
>> improves the performance.
> Not the sock tx buff, when we are busypolling in handle_rx, we will
> check the tx vring via vhost_vq_avail_empty.
> So, should we the disable tvq, e.g. vhost_net_disable_vq(net, tvq)?> --
This could be done on top since tx wakeups only happnes when we run out
of sndbuf.
Thanks
^ permalink raw reply
* Re: [PATCH PATCH net-next 10/18] 9p: fix whitespace issues
From: Dominique Martinet @ 2018-07-30 3:54 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: davem, netdev, v9fs-developer, Stephen Hemminger
In-Reply-To: <20180724192918.31165-11-sthemmin@microsoft.com>
(removed tons of Cc to make v9fs-developer@ happy - please do include
linux-kernel@vger.kernel.org next time though, I only saw this patch by
chance snooping at netdev)
Stephen Hemminger wrote on Tue, Jul 24, 2018:
> Remove trailing whitespace and blank lines at EOF
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
LGTM, I'm not sure if someone would pick the whole series but as other
maintainers seem to have taken individual patches I've taken this one as
there's potential conflicts with what I have (planning to remove the
whole net/9p/util.c file)
> ---
> net/9p/client.c | 4 ++--
> net/9p/trans_virtio.c | 2 +-
> net/9p/util.c | 1 -
> 3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/net/9p/client.c b/net/9p/client.c
> index 5c1343195292..ff02826e0407 100644
> --- a/net/9p/client.c
> +++ b/net/9p/client.c
> @@ -341,7 +341,7 @@ struct p9_req_t *p9_tag_lookup(struct p9_client *c, u16 tag)
> * buffer to read the data into */
> tag++;
>
> - if(tag >= c->max_tag)
> + if (tag >= c->max_tag)
> return NULL;
>
> row = tag / P9_ROW_MAXTAG;
> @@ -1576,7 +1576,7 @@ p9_client_read(struct p9_fid *fid, u64 offset, struct iov_iter *to, int *err)
> int count = iov_iter_count(to);
> int rsize, non_zc = 0;
> char *dataptr;
> -
> +
> rsize = fid->iounit;
> if (!rsize || rsize > clnt->msize-P9_IOHDRSZ)
> rsize = clnt->msize - P9_IOHDRSZ;
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 05006cbb3361..279b24488d79 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -446,7 +446,7 @@ p9_virtio_zc_request(struct p9_client *client, struct p9_req_t *req,
> out += pack_sg_list_p(chan->sg, out, VIRTQUEUE_NUM,
> out_pages, out_nr_pages, offs, outlen);
> }
> -
> +
> /*
> * Take care of in data
> * For example TREAD have 11.
> diff --git a/net/9p/util.c b/net/9p/util.c
> index 59f278e64f58..55ad98277e85 100644
> --- a/net/9p/util.c
> +++ b/net/9p/util.c
> @@ -138,4 +138,3 @@ int p9_idpool_check(int id, struct p9_idpool *p)
> return idr_find(&p->pool, id) != NULL;
> }
> EXPORT_SYMBOL(p9_idpool_check);
> -
--
Dominique Martinet
^ permalink raw reply
* Re: [PATCH v3 0/3] net: extend eth_platform_get_mac_address()
From: Dan Carpenter @ 2018-07-30 5:54 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Grygorii Strashko, David Lechner, Ivan Khoronzhuk,
Kevin Hilman, Greg Kroah-Hartman, Sekhar Nori, Russell King,
linux-kernel, Andrew Lunn, Lukas Wunner, Srinivas Kandagatla,
netdev, Florian Fainelli, linux-omap, David S . Miller,
linux-arm-kernel
In-Reply-To: <20180719153243.20944-1-brgl@bgdev.pl>
Thanks for that.
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] 9p: fix Use-After-Free in p9_write_work()
From: Dominique Martinet @ 2018-07-30 6:00 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Tomas Bortoli, David Miller, v9fs-developer, netdev, LKML,
syzkaller
In-Reply-To: <CACT4Y+bf1NfCV9aTRCR_qwA0jWxhYDkR3CS8jQcJyvkYXCming@mail.gmail.com>
Dmitry Vyukov wrote on Mon, Jul 30, 2018:
> On Mon, Jul 30, 2018 at 1:33 AM, Dominique Martinet
> <asmadeus@codewreck.org> wrote:
> > Tomas Bortoli wrote on Sun, Jul 29, 2018:
> >> There is a race condition between p9_free_req() and p9_write_work().
> >> A request might still need to be processed while p9_free_req() is called.
> >>
> >> To fix it, flush the read/write work before freeing any request.
> >>
> >> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
> >> Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com
> >
> > It looks like I have not received this report, I found it through google
> > in the lkml archives
>
> But you should have been received it? Or not?
> We had some complaints that syzbot emails were not delivered, but in
> these cases they were not delivered to lkml, and only to explicitly
> CCed people.
I'm not on lkml and the archives I found do not list who were Cc'd that
I can see - it might have tried to send a copy to v9fs-developer that is
held up in the moderation queue for all I know :/
I'm not complaining I didn't get a copy (if I ever find time to work on
these, I can work through the list on the website) - I just need to know
how to find the report corresponding to patchs being sent.
> > but Dmitry do you have a convenient-ish way of
> > finding the report on the syzkaller website with that reported-by tag?
>
> Well, you can do:
> http://syzkaller.appspot.com/bug?extid=467050c1ce275af2a5b8
Thanks, for some reason I only thought of bug?id= which didn't work,
this is perfect.
Cheers,
--
Dominique
^ permalink raw reply
* [PATCH net-next v7] net/tls: Use socket data_ready callback on record availability
From: Vakul Garg @ 2018-07-30 10:38 UTC (permalink / raw)
To: netdev; +Cc: borisp, aviadye, davejwatson, davem, Vakul Garg
On receipt of a complete tls record, use socket's saved data_ready
callback instead of state_change callback. In function tls_queue(),
the TLS record is queued in encrypted state. But the decryption
happen inline when tls_sw_recvmsg() or tls_sw_splice_read() get invoked.
So it should be ok to notify the waiting context about the availability
of data as soon as we could collect a full TLS record. For new data
availability notification, sk_data_ready callback is more appropriate.
It points to sock_def_readable() which wakes up specifically for EPOLLIN
event. This is in contrast to the socket callback sk_state_change which
points to sock_def_wakeup() which issues a wakeup unconditionally
(without event mask).
Signed-off-by: Vakul Garg <vakul.garg@nxp.com>
---
v6->v7: Improved the commit message to contain the detailed reasoning.
(The same analysis was shared on the mail list.)
net/tls/tls_sw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 6deceb7c56ba..33838f11fafa 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1028,7 +1028,7 @@ static void tls_queue(struct strparser *strp, struct sk_buff *skb)
ctx->recv_pkt = skb;
strp_pause(strp);
- strp->sk->sk_state_change(strp->sk);
+ ctx->saved_data_ready(strp->sk);
}
static void tls_data_ready(struct sock *sk)
--
2.13.6
^ permalink raw reply related
* Re: [PATCH net-next 1/2] tcp: call tcp_drop() in tcp collapse
From: Yafang Shao @ 2018-07-30 5:40 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, LKML
In-Reply-To: <CANn89i+0peAq_BS3Ge-h2_MENHQAHtf8D_M0gzgchK6CxU8D+Q@mail.gmail.com>
On Mon, Jul 30, 2018 at 10:27 AM, Eric Dumazet <edumazet@google.com> wrote:
> On Sun, Jul 29, 2018 at 7:06 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>>
>> On Sun, Jul 29, 2018 at 12:28 AM, Eric Dumazet <edumazet@google.com> wrote:
>> > On Sat, Jul 28, 2018 at 12:43 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>> >>
>> >> On Sat, Jul 28, 2018 at 11:38 AM, Eric Dumazet <edumazet@google.com> wrote:
>> >> > On Fri, Jul 27, 2018 at 8:35 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>> >> >
>> >> >> So what about LINUX_MIB_TCPOFOMERGE ?
>> >> >> Regarding LINUX_MIB_TCPOFOMERGE, a skb is already covered by another
>> >> >> skb, is that dropping the packet or simply lowering the memory
>> >> >> overhead ?
>> >> >
>> >> > What do you think ?
>> >> >
>> >> > If you receive two times the same payload, don't you have to drop one
>> >> > of the duplicate ?
>> >> >
>> >> > There is a a big difference between the two cases.
>> >>
>> >> If the drop caused some data lost (which may then cause retransmition
>> >> or something), then this is a really DROP.
>> >> While if the drop won't cause any data lost, meaning it is a
>> >> non-harmful behavior, I think it should not be defined as DROP.
>> >> This is my suggestion anyway.
>> >
>> > Sigh.
>> >
>> > We count drops, not because they are ' bad or something went wrong'.
>> >
>> > If TCP stack receives twice the same sequence (same payload), we
>> > _drop_ one of the duplicate, so we account for this event.
>> >
>> > When ' collapsing' we reorganize our own storage, not because we have
>> > to drop a payload,
>> > but for some memory pressure reason.
>>
>> Thanks for you clarification.
>> So what about LINUX_MIB_TCPOFODROP ?
>>
>> if (unlikely(tcp_try_rmem_schedule(sk, skb, skb->truesize))) {
>> NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPOFODROP);
>> tcp_drop(sk, skb);
>> return;
>> }
>>
>>
>> It is also because of our own memory pressure, but we call tcp_drop() here.
>
> Yes, we _drop_ a packet.
>
> That is pretty clear that the payload is dropped, and that the sender
> will have to _retransmit_.
>
>>
>> I am not mean to disagree with you. I am just confused and want to
>> make it clear.
>
>
> Collapsing is :
>
> For (a bunch of packets)
> Try (to compress them in order to reduce memory overhead)
>
> No drop of payload happens here. Sender wont have to retransmit.
OK.
Thanks for your patient.
Should we put NET_INC_STATS(sock_net(sk), mib_idx) into the funtion
tcp_drop() ?
Then we could easily relate the sk_drops with the SNMP counters.
Something like that,
static void tcp_drop(struct sock *sk, struct sk_buff *skb, int mib_idx)
{
int segs = max_t(u16, 1, skb_shinfo(skb)->gso_segs);
atomic_add(segs, &sk->sk_drops);
NET_ADD_STATS(sock_net(sk), mib_idx, segs);
__kfree_skb(skb);
}
Thanks
Yafang
^ permalink raw reply
* Re: [PATCH] 9p: fix Use-After-Free in p9_write_work()
From: Dmitry Vyukov @ 2018-07-30 5:54 UTC (permalink / raw)
To: Dominique Martinet
Cc: Tomas Bortoli, David Miller, v9fs-developer, netdev, LKML,
syzkaller
In-Reply-To: <20180729233336.GB28684@nautica>
On Mon, Jul 30, 2018 at 1:33 AM, Dominique Martinet
<asmadeus@codewreck.org> wrote:
> Tomas Bortoli wrote on Sun, Jul 29, 2018:
>> There is a race condition between p9_free_req() and p9_write_work().
>> A request might still need to be processed while p9_free_req() is called.
>>
>> To fix it, flush the read/write work before freeing any request.
>>
>> Signed-off-by: Tomas Bortoli <tomasbortoli@gmail.com>
>> Reported-by: syzbot+467050c1ce275af2a5b8@syzkaller.appspotmail.com
>
> It looks like I have not received this report, I found it through google
> in the lkml archives
But you should have been received it? Or not?
We had some complaints that syzbot emails were not delivered, but in
these cases they were not delivered to lkml, and only to explicitly
CCed people.
> but Dmitry do you have a convenient-ish way of
> finding the report on the syzkaller website with that reported-by tag?
Well, you can do:
http://syzkaller.appspot.com/bug?extid=467050c1ce275af2a5b8
^ permalink raw reply
* Re: [PATCH net-next] virtio_net: force_napi_tx module param.
From: Jason Wang @ 2018-07-30 6:06 UTC (permalink / raw)
To: Jon Olson, mst
Cc: willemdebruijn.kernel, caleb.raitto, davem, netdev, Caleb Raitto
In-Reply-To: <CAJoqh4UDJvFvfmFrzgHyk5pNXaQUqnLk7HtJzuxGoUzfZoy64Q@mail.gmail.com>
On 2018年07月25日 08:17, Jon Olson wrote:
> On Tue, Jul 24, 2018 at 3:46 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Tue, Jul 24, 2018 at 06:31:54PM -0400, Willem de Bruijn wrote:
>>> On Tue, Jul 24, 2018 at 6:23 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Tue, Jul 24, 2018 at 04:52:53PM -0400, Willem de Bruijn wrote:
>>>>> >From the above linked patch, I understand that there are yet
>>>>> other special cases in production, such as a hard cap on #tx queues to
>>>>> 32 regardless of number of vcpus.
>>>> I don't think upstream kernels have this limit - we can
>>>> now use vmalloc for higher number of queues.
>>> Yes. that patch* mentioned it as a google compute engine imposed
>>> limit. It is exactly such cloud provider imposed rules that I'm
>>> concerned about working around in upstream drivers.
>>>
>>> * for reference, I mean https://patchwork.ozlabs.org/patch/725249/
>> Yea. Why does GCE do it btw?
> There are a few reasons for the limit, some historical, some current.
>
> Historically we did this because of a kernel limit on the number of
> TAP queues (in Montreal I thought this limit was 32). To my chagrin,
> the limit upstream at the time we did it was actually eight. We had
> increased the limit from eight to 32 internally, and it appears in
> upstream it has subsequently increased upstream to 256. We no longer
> use TAP for networking, so that constraint no longer applies for us,
> but when looking at removing/raising the limit we discovered no
> workloads that clearly benefited from lifting it, and it also placed
> more pressure on our virtual networking stack particularly on the Tx
> side. We left it as-is.
>
> In terms of current reasons there are really two. One is memory usage.
> As you know, virtio-net uses rx/tx pairs, so there's an expectation
> that the guest will have an Rx queue for every Tx queue. We run our
> individual virtqueues fairly deep (4096 entries) to give guests a wide
> time window for re-posting Rx buffers and avoiding starvation on
> packet delivery. Filling an Rx vring with max-sized mergeable buffers
> (4096 bytes) is 16MB of GFP_ATOMIC allocations. At 32 queues this can
> be up to 512MB of memory posted for network buffers. Scaling this to
> the largest VM GCE offers today (160 VCPUs -- n1-ultramem-160) keeping
> all of the Rx rings full would (in the large average Rx packet size
> case) consume up to 2.5 GB(!) of guest RAM. Now, those VMs have 3.8T
> of RAM available, but I don't believe we've observed a situation where
> they would have benefited from having 2.5 gigs of buffers posted for
> incoming network traffic :)
We can work to have async txq and rxq instead of paris if there's a
strong requirement.
>
> The second reason is interrupt related -- as I mentioned above, we
> have found no workloads that clearly benefit from so many queues, but
> we have found workloads that degrade. In particular workloads that do
> a lot of small packet processing but which aren't extremely latency
> sensitive can achieve higher PPS by taking fewer interrupt across
> fewer VCPUs due to better batching (this also incurs higher latency,
> but at the limit the "busy" cores end up suppressing most interrupts
> and spending most of their cycles farming out work). Memcache is a
> good example here, particularly if the latency targets for request
> completion are in the ~milliseconds range (rather than the
> microseconds we typically strive for with TCP_RR-style workloads).
>
> All of that said, we haven't been forthcoming with data (and
> unfortunately I don't have it handy in a useful form, otherwise I'd
> simply post it here), so I understand the hesitation to simply run
> with napi_tx across the board. As Willem said, this patch seemed like
> the least disruptive way to allow us to continue down the road of
> "universal" NAPI Tx and to hopefully get data across enough workloads
> (with VMs small, large, and absurdly large :) to present a compelling
> argument in one direction or another. As far as I know there aren't
> currently any NAPI related ethtool commands (based on a quick perusal
> of ethtool.h)
As I suggest before, maybe we can (ab)use tx-frames-irq.
Thanks
> -- it seems like it would be fairly involved/heavyweight
> to plumb one solely for this unless NAPI Tx is something many users
> will want to tune (and for which other drivers would support tuning).
>
> --
> Jon Olson
^ permalink raw reply
* Re: [net-next v1] net/ipv6: allow any source address for sendmsg pktinfo with ip_nonlocal_bind
From: Vincent Bernat @ 2018-07-30 6:08 UTC (permalink / raw)
To: David Miller; +Cc: kuznet, yoshfuji, netdev, tom
In-Reply-To: <20180729.122828.2192078281117324633.davem@davemloft.net>
❦ 29 juillet 2018 12:28 -0700, David Miller <davem@davemloft.net> :
>> When freebind feature is set of an IPv6 socket, any source address can
>> be used when sending UDP datagrams using IPv6 PKTINFO ancillary
>> message. Global non-local bind feature was added in commit
>> 35a256fee52c ("ipv6: Nonlocal bind") for IPv6. This commit also allows
>> IPv6 source address spoofing when non-local bind feature is enabled.
>>
>> Signed-off-by: Vincent Bernat <vincent@bernat.im>
>
> This definitely seems to make sense. And is consistent with the other
> tests involving freebind and transparent.
>
> This test involving ip_nonlocal_bind, freeebind, and transparent happens
> in several locations. Perhaps we should add a helper function for
> this?
Yes, I can do that. Should I also include one for SCTP?
--
"Elves and Dragons!" I says to him. "Cabbages and potatoes are better
for you and me."
-- J. R. R. Tolkien
^ 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