* Re: [PATCH v3 bpf-next 3/4] selftests/bpf: make PT_REGS_* work in userspace
From: Ilya Leoshkevich @ 2019-07-10 11:47 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, Stanislav Fomichev, Y Song, David S. Miller,
Alexei Starovoitov, Daniel Borkmann, Adrian Ratiu, david.daney
In-Reply-To: <CAEf4BzY0kO2si_ajouYNfsauaWdHkj042++bLaHe1W_G885i=g@mail.gmail.com>
> Am 09.07.2019 um 19:48 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>:
>
> On Tue, Jul 9, 2019 at 8:19 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>
>> Right now, on certain architectures, these macros are usable only with
>> kernel headers. This patch makes it possible to use them with userspace
>> headers and, as a consequence, not only in BPF samples, but also in BPF
>> selftests.
>>
>> On s390, provide the forward declaration of struct pt_regs and cast it
>> to user_pt_regs in PT_REGS_* macros. This is necessary, because instead
>> of the full struct pt_regs, s390 exposes only its first member
>> user_pt_regs to userspace, and bpf_helpers.h is used with both userspace
>> (in selftests) and kernel (in samples) headers. It was added in commit
>> 466698e654e8 ("s390/bpf: correct broken uapi for
>> BPF_PROG_TYPE_PERF_EVENT program type").
>>
>> Ditto on arm64.
>>
>> On x86, provide userspace versions of PT_REGS_* macros. Unlike s390 and
>> arm64, x86 provides struct pt_regs to both userspace and kernel, however,
>> with different member names.
>>
>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>> ---
>
> Just curious, what did you use as a reference for which register
> corresponds to which PARM, RET, etc for different archs? I've tried to
> look it up the other day, and it wasn't as straightforward to find as
> I hoped for, so maybe I'm missing something obvious.
For this particular change I did not have to look it up, because it all
was already in the code, I just needed to adapt it to userspace headers.
Normally I would google for „abi supplement“ to find this information.
A lazy way would be to simply ask the (cross-)compiler:
cat <<HERE | aarch64-linux-gnu-gcc -x c -O3 -S - -o -
int f(int a, int b, int c, int d, int e, int f, int g, int h, int i, int j);
int g() { return -f(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); }
HERE
I’ve just double checked the supported arches, and noticed that:
#define PT_REGS_PARM5(x) ((x)->uregs[4])
for bpf_target_arm (arm-linux-gnueabihf) looks wrong:
the 5th parameter should be passed on stack. This observation matches
[1].
#define PT_REGS_RC(x) ((x)->regs[1])
for bpf_target_mips (mips64el-linux-gnuabi64) also looks wrong:
the return value should be in register 2. This observation matches [2].
Since I’m not an expert on those architectures, my conclusions could be
incorrect (e.g. becase a different ABI is normally used in practice).
Adrian and David, could you please correct me if I’m wrong?
[1] http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf
[2] ftp://www.linux-mips.org/pub/linux/mips/doc/ABI/psABI_mips3.0.pdf
>> tools/testing/selftests/bpf/bpf_helpers.h | 61 +++++++++++++++--------
>> 1 file changed, 41 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
>> index 73071a94769a..212ec564e5c3 100644
>> --- a/tools/testing/selftests/bpf/bpf_helpers.h
>> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
>> @@ -358,6 +358,7 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
>>
>> #if defined(bpf_target_x86)
>>
>> +#ifdef __KERNEL__
>> #define PT_REGS_PARM1(x) ((x)->di)
>> #define PT_REGS_PARM2(x) ((x)->si)
>> #define PT_REGS_PARM3(x) ((x)->dx)
>> @@ -368,19 +369,35 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
>> #define PT_REGS_RC(x) ((x)->ax)
>> #define PT_REGS_SP(x) ((x)->sp)
>> #define PT_REGS_IP(x) ((x)->ip)
>> +#else
>> +#define PT_REGS_PARM1(x) ((x)->rdi)
>> +#define PT_REGS_PARM2(x) ((x)->rsi)
>> +#define PT_REGS_PARM3(x) ((x)->rdx)
>> +#define PT_REGS_PARM4(x) ((x)->rcx)
>> +#define PT_REGS_PARM5(x) ((x)->r8)
>> +#define PT_REGS_RET(x) ((x)->rsp)
>> +#define PT_REGS_FP(x) ((x)->rbp)
>> +#define PT_REGS_RC(x) ((x)->rax)
>> +#define PT_REGS_SP(x) ((x)->rsp)
>> +#define PT_REGS_IP(x) ((x)->rip)
>
> Will this also work for 32-bit x86?
Thanks, this is a good catch: this builds, but makes 64-bit accesses, as if it used the 64-bit
variant of pt_regs. I will fix this.
^ permalink raw reply
* Re: [PATCH net-next 00/11] Add drop monitor for offloaded data paths
From: Toke Høiland-Jørgensen @ 2019-07-10 11:39 UTC (permalink / raw)
To: Ido Schimmel, Jakub Kicinski
Cc: David Miller, netdev, jiri, mlxsw, dsahern, roopa, nikolay, andy,
pablo, pieter.jansenvanvuuren, andrew, f.fainelli, vivien.didelot,
idosch, Alexei Starovoitov
In-Reply-To: <20190710112011.GA552@splinter>
Ido Schimmel <idosch@idosch.org> writes:
> On Tue, Jul 09, 2019 at 03:34:30PM -0700, Jakub Kicinski wrote:
>> On Tue, 9 Jul 2019 15:38:44 +0300, Ido Schimmel wrote:
>> > On Mon, Jul 08, 2019 at 03:51:58PM -0700, Jakub Kicinski wrote:
>> > > On Mon, 8 Jul 2019 16:19:08 +0300, Ido Schimmel wrote:
>> > > > On Sun, Jul 07, 2019 at 12:45:41PM -0700, David Miller wrote:
>> > > > > From: Ido Schimmel <idosch@idosch.org>
>> > > > > Date: Sun, 7 Jul 2019 10:58:17 +0300
>> > > > >
>> > > > > > Users have several ways to debug the kernel and understand why a packet
>> > > > > > was dropped. For example, using "drop monitor" and "perf". Both
>> > > > > > utilities trace kfree_skb(), which is the function called when a packet
>> > > > > > is freed as part of a failure. The information provided by these tools
>> > > > > > is invaluable when trying to understand the cause of a packet loss.
>> > > > > >
>> > > > > > In recent years, large portions of the kernel data path were offloaded
>> > > > > > to capable devices. Today, it is possible to perform L2 and L3
>> > > > > > forwarding in hardware, as well as tunneling (IP-in-IP and VXLAN).
>> > > > > > Different TC classifiers and actions are also offloaded to capable
>> > > > > > devices, at both ingress and egress.
>> > > > > >
>> > > > > > However, when the data path is offloaded it is not possible to achieve
>> > > > > > the same level of introspection as tools such "perf" and "drop monitor"
>> > > > > > become irrelevant.
>> > > > > >
>> > > > > > This patchset aims to solve this by allowing users to monitor packets
>> > > > > > that the underlying device decided to drop along with relevant metadata
>> > > > > > such as the drop reason and ingress port.
>> > > > >
>> > > > > We are now going to have 5 or so ways to capture packets passing through
>> > > > > the system, this is nonsense.
>> > > > >
>> > > > > AF_PACKET, kfree_skb drop monitor, perf, XDP perf events, and now this
>> > > > > devlink thing.
>> > > > >
>> > > > > This is insanity, too many ways to do the same thing and therefore the
>> > > > > worst possible user experience.
>> > > > >
>> > > > > Pick _ONE_ method to trap packets and forward normal kfree_skb events,
>> > > > > XDP perf events, and these taps there too.
>> > > > >
>> > > > > I mean really, think about it from the average user's perspective. To
>> > > > > see all drops/pkts I have to attach a kfree_skb tracepoint, and not just
>> > > > > listen on devlink but configure a special tap thing beforehand and then
>> > > > > if someone is using XDP I gotta setup another perf event buffer capture
>> > > > > thing too.
>> > > >
>> > > > Let me try to explain again because I probably wasn't clear enough. The
>> > > > devlink-trap mechanism is not doing the same thing as other solutions.
>> > > >
>> > > > The packets we are capturing in this patchset are packets that the
>> > > > kernel (the CPU) never saw up until now - they were silently dropped by
>> > > > the underlying device performing the packet forwarding instead of the
>> > > > CPU.
>> >
>> > Jakub,
>> >
>> > It seems to me that most of the criticism is about consolidation of
>> > interfaces because you believe I'm doing something you can already do
>> > today, but this is not the case.
>>
>> To be clear I'm not opposed to the patches, I'm just trying to
>> facilitate a discussion.
>
> Sure, sorry if it came out the wrong way. I appreciate your feedback and
> the time you have spent on this subject.
>
>>
>> > Switch ASICs have dedicated traps for specific packets. Usually, these
>> > packets are control packets (e.g., ARP, BGP) which are required for the
>> > correct functioning of the control plane. You can see this in the SAI
>> > interface, which is an abstraction layer over vendors' SDKs:
>> >
>> > https://github.com/opencomputeproject/SAI/blob/master/inc/saihostif.h#L157
>> >
>> > We need to be able to configure the hardware policers of these traps and
>> > read their statistics to understand how many packets they dropped. We
>> > currently do not have a way to do any of that and we rely on hardcoded
>> > defaults in the driver which do not fit every use case (from
>> > experience):
>> >
>> > https://elixir.bootlin.com/linux/v5.2/source/drivers/net/ethernet/mellanox/mlxsw/spectrum.c#L4103
>> >
>> > We plan to extend devlink-trap mechanism to cover all these use cases. I
>> > hope you agree that this functionality belongs in devlink given it is a
>> > device-specific configuration and not a netdev-specific one.
>>
>> No disagreement on providing knobs for traps.
>>
>> > That being said, in its current form, this mechanism is focused on traps
>> > that correlate to packets the device decided to drop as this is very
>> > useful for debugging.
>>
>> That'd be mixing two things - trap configuration and tracing exceptions
>> in one API. That's a little suboptimal but not too terrible, especially
>> if there is a higher level APIs users can default to.
>
> TBH, initially I was only focused on the drops, but then it occurred to
> me that this is a too narrow scope. These traps are only a subset of the
> complete list of traps we have and we have similar requirements for both
> (statistics, setting policers etc.). Therefore, I decided to design this
> interface in a more generic way, so that it could support the different
> use cases.
>
>>
>> > Given that the entire configuration is done via devlink and that devlink
>> > stores all the information about these traps, it seems logical to also
>> > report these packets and their metadata to user space as devlink events.
>> >
>> > If this is not desirable, we can try to call into drop_monitor from
>> > devlink and add a new command (e.g., NET_DM_CMD_HW_ALERT), which will
>> > encode all the information we currently have in DEVLINK_CMD_TRAP_REPORT.
>> >
>> > IMO, this is less desirable, as instead of having one tool (devlink) to
>> > interact with this mechanism we will need two (devlink & dropwatch).
>> >
>> > Below I tried to answer all your questions and refer to all the points
>> > you brought up.
>> >
>> > > When you say silently dropped do you mean that mlxsw as of today
>> > > doesn't have any counters exposed for those events?
>> >
>> > Some of these packets are counted, but not all of them.
>> >
>> > > If we wanted to consolidate this into something existing we can either
>> > > (a) add similar traps in the kernel data path;
>> > > (b) make these traps extension of statistics.
>> > >
>> > > My knee jerk reaction to seeing the patches was that it adds a new
>> > > place where device statistics are reported.
>> >
>> > Not at all. This would be a step back. We can already count discards due
>> > to VLAN membership on ingress on a per-port basis. A software maintained
>> > global counter does not buy us anything.
>> >
>> > By also getting the dropped packet - coupled with the drop reason and
>> > ingress port - you can understand exactly why and on which VLAN the
>> > packet was dropped. I wrote a Wireshark dissector for these netlink
>> > packets to make our life easier. You can see the details in my comment
>> > to the cover letter:
>> >
>> > https://marc.info/?l=linux-netdev&m=156248736710238&w=2
>> >
>> > In case you do not care about individual packets, but still want more
>> > fine-grained statistics for your monitoring application, you can use
>> > eBPF. For example, one thing we did is attaching a kprobe to
>> > devlink_trap_report() with an eBPF program that dissects the incoming
>> > skbs and maintains a counter per-{5 tuple, drop reason}. With
>> > ebpf_exporter you can export these statistics to Prometheus on which you
>> > can run queries and visualize the results with Grafana. This is
>> > especially useful for tail and early drops since it allows you to
>> > understand which flows contribute to most of the drops.
>>
>> No question that the mechanism is useful.
>>
>> > > Users who want to know why things are dropped will not get detailed
>> > > breakdown from ethtool -S which for better or worse is the one stop
>> > > shop for device stats today.
>> >
>> > I hope I managed to explain why counters are not enough, but I also want
>> > to point out that ethtool statistics are not properly documented and
>> > this hinders their effectiveness. I did my best to document the exposed
>> > traps in order to avoid the same fate:
>> >
>> > https://patchwork.ozlabs.org/patch/1128585/
>> >
>> > In addition, there are selftests to show how each trap can be triggered
>> > to reduce the ambiguity even further:
>> >
>> > https://patchwork.ozlabs.org/patch/1128610/
>> >
>> > And a note in the documentation to make sure future functionality is
>> > tested as well:
>> >
>> > https://patchwork.ozlabs.org/patch/1128608/
>> >
>> > > Having thought about it some more, however, I think that having a
>> > > forwarding "exception" object and hanging statistics off of it is a
>> > > better design, even if we need to deal with some duplication to get
>> > > there.
>> > >
>> > > IOW having an way to "trap all packets which would increment a
>> > > statistic" (option (b) above) is probably a bad design.
>> > >
>> > > As for (a) I wonder how many of those events have a corresponding event
>> > > in the kernel stack?
>> >
>> > Generic packet drops all have a corresponding kfree_skb() calls in the
>> > kernel, but that does not mean that every packet dropped by the hardware
>> > would also be dropped by the kernel if it were to be injected to its Rx
>> > path.
>>
>> The notion that all SW events get captured by kfree_skb() would not be
>> correct.
>
> I meant that the generic drop reasons I'm exposing with this patchset
> all correspond to reasons for which the kernel would drop packets.
>
>> We have the kfree_skb(), and xdp_exception(), and drivers can
>> drop packets if various allocations fail.. the situation is already not
>> great.
>>
>> I think that having a single useful place where users can look to see
>> all traffic exception events would go a long way.
>
> I believe this was Dave's point as well. We have one tool to monitor
> kfree_skb() drops and with this patchset we will have another to monitor
> HW drops. As I mentioned in my previous reply, I will look into sending
> the events via drop_monitor by calling into it from devlink.
>
> I'm not involved with XDP (as you might have noticed), but I assume
> drop_monitor could be extended for this use case as well by doing
> register_trace_xdp_exception(). Then you could monitor SW, HW and XDP
> events using a single netlink channel, potentially split into different
> multicast groups to allow user space programs to receive only the events
> they care about.
Yes, having XDP hook into this would be good! This ties in nicely with
the need for better statistics for XDP (see
https://github.com/xdp-project/xdp-project/blob/master/xdp-project.org#consistency-for-statistics-with-xdp).
Unfortunately, it's not enough to just hook into the xdp_exception trace
point, as that is generally only triggered on XDP_ABORTED, not if the
XDP program just decides to drop the packet with XDP_DROP. So we may
need another mechanism to hook this if it's going to comprehensive; and
we'd probably want a way to do this that doesn't impose a performance
penalty when the drop monitor is not enabled...
-Toke
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: update BPF JIT S390 maintainers
From: Vasily Gorbik @ 2019-07-10 11:34 UTC (permalink / raw)
To: David S. Miller, Alexei Starovoitov, Daniel Borkmann
Cc: Heiko Carstens, Christian Borntraeger, Ilya Leoshkevich, netdev,
bpf, linux-s390
In-Reply-To: <patch.git-d365382dfc69.your-ad-here.call-01562755343-ext-3127@work.hours>
On Wed, Jul 10, 2019 at 12:43:45PM +0200, Vasily Gorbik wrote:
> Ilya Leoshkevich is joining as s390 bpf maintainer. With his background
> as gcc developer he would be valuable for the team and community as a
> whole. Ilya, have fun!
>
> Since there is now enough eyes on s390 bpf, relieve Christian Borntraeger,
> so that he could focus on his maintainer tasks for other components.
>
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
> MAINTAINERS | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 558acf24ea1e..98e7411dfe56 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3066,9 +3066,9 @@ S: Maintained
> F: arch/riscv/net/
>
> BPF JIT for S390
> +M: Ilya Leoshkevich <iii@linux.ibm.com>
> M: Heiko Carstens <heiko.carstens@de.ibm.com>
> M: Vasily Gorbik <gor@linux.ibm.com>
> -M: Christian Borntraeger <borntraeger@de.ibm.com>
> L: netdev@vger.kernel.org
> L: bpf@vger.kernel.org
> S: Maintained
> --
> 2.21.0
Dave, Alexei, Daniel,
would you take it via one of your trees? Or should I take it via s390?
Thanks,
Vasily
^ permalink raw reply
* Re: [PATCH net-next 00/11] Add drop monitor for offloaded data paths
From: Ido Schimmel @ 2019-07-10 11:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: David Miller, netdev, jiri, mlxsw, dsahern, roopa, nikolay, andy,
pablo, pieter.jansenvanvuuren, andrew, f.fainelli, vivien.didelot,
idosch, Alexei Starovoitov
In-Reply-To: <20190709153430.5f0f5295@cakuba.netronome.com>
On Tue, Jul 09, 2019 at 03:34:30PM -0700, Jakub Kicinski wrote:
> On Tue, 9 Jul 2019 15:38:44 +0300, Ido Schimmel wrote:
> > On Mon, Jul 08, 2019 at 03:51:58PM -0700, Jakub Kicinski wrote:
> > > On Mon, 8 Jul 2019 16:19:08 +0300, Ido Schimmel wrote:
> > > > On Sun, Jul 07, 2019 at 12:45:41PM -0700, David Miller wrote:
> > > > > From: Ido Schimmel <idosch@idosch.org>
> > > > > Date: Sun, 7 Jul 2019 10:58:17 +0300
> > > > >
> > > > > > Users have several ways to debug the kernel and understand why a packet
> > > > > > was dropped. For example, using "drop monitor" and "perf". Both
> > > > > > utilities trace kfree_skb(), which is the function called when a packet
> > > > > > is freed as part of a failure. The information provided by these tools
> > > > > > is invaluable when trying to understand the cause of a packet loss.
> > > > > >
> > > > > > In recent years, large portions of the kernel data path were offloaded
> > > > > > to capable devices. Today, it is possible to perform L2 and L3
> > > > > > forwarding in hardware, as well as tunneling (IP-in-IP and VXLAN).
> > > > > > Different TC classifiers and actions are also offloaded to capable
> > > > > > devices, at both ingress and egress.
> > > > > >
> > > > > > However, when the data path is offloaded it is not possible to achieve
> > > > > > the same level of introspection as tools such "perf" and "drop monitor"
> > > > > > become irrelevant.
> > > > > >
> > > > > > This patchset aims to solve this by allowing users to monitor packets
> > > > > > that the underlying device decided to drop along with relevant metadata
> > > > > > such as the drop reason and ingress port.
> > > > >
> > > > > We are now going to have 5 or so ways to capture packets passing through
> > > > > the system, this is nonsense.
> > > > >
> > > > > AF_PACKET, kfree_skb drop monitor, perf, XDP perf events, and now this
> > > > > devlink thing.
> > > > >
> > > > > This is insanity, too many ways to do the same thing and therefore the
> > > > > worst possible user experience.
> > > > >
> > > > > Pick _ONE_ method to trap packets and forward normal kfree_skb events,
> > > > > XDP perf events, and these taps there too.
> > > > >
> > > > > I mean really, think about it from the average user's perspective. To
> > > > > see all drops/pkts I have to attach a kfree_skb tracepoint, and not just
> > > > > listen on devlink but configure a special tap thing beforehand and then
> > > > > if someone is using XDP I gotta setup another perf event buffer capture
> > > > > thing too.
> > > >
> > > > Let me try to explain again because I probably wasn't clear enough. The
> > > > devlink-trap mechanism is not doing the same thing as other solutions.
> > > >
> > > > The packets we are capturing in this patchset are packets that the
> > > > kernel (the CPU) never saw up until now - they were silently dropped by
> > > > the underlying device performing the packet forwarding instead of the
> > > > CPU.
> >
> > Jakub,
> >
> > It seems to me that most of the criticism is about consolidation of
> > interfaces because you believe I'm doing something you can already do
> > today, but this is not the case.
>
> To be clear I'm not opposed to the patches, I'm just trying to
> facilitate a discussion.
Sure, sorry if it came out the wrong way. I appreciate your feedback and
the time you have spent on this subject.
>
> > Switch ASICs have dedicated traps for specific packets. Usually, these
> > packets are control packets (e.g., ARP, BGP) which are required for the
> > correct functioning of the control plane. You can see this in the SAI
> > interface, which is an abstraction layer over vendors' SDKs:
> >
> > https://github.com/opencomputeproject/SAI/blob/master/inc/saihostif.h#L157
> >
> > We need to be able to configure the hardware policers of these traps and
> > read their statistics to understand how many packets they dropped. We
> > currently do not have a way to do any of that and we rely on hardcoded
> > defaults in the driver which do not fit every use case (from
> > experience):
> >
> > https://elixir.bootlin.com/linux/v5.2/source/drivers/net/ethernet/mellanox/mlxsw/spectrum.c#L4103
> >
> > We plan to extend devlink-trap mechanism to cover all these use cases. I
> > hope you agree that this functionality belongs in devlink given it is a
> > device-specific configuration and not a netdev-specific one.
>
> No disagreement on providing knobs for traps.
>
> > That being said, in its current form, this mechanism is focused on traps
> > that correlate to packets the device decided to drop as this is very
> > useful for debugging.
>
> That'd be mixing two things - trap configuration and tracing exceptions
> in one API. That's a little suboptimal but not too terrible, especially
> if there is a higher level APIs users can default to.
TBH, initially I was only focused on the drops, but then it occurred to
me that this is a too narrow scope. These traps are only a subset of the
complete list of traps we have and we have similar requirements for both
(statistics, setting policers etc.). Therefore, I decided to design this
interface in a more generic way, so that it could support the different
use cases.
>
> > Given that the entire configuration is done via devlink and that devlink
> > stores all the information about these traps, it seems logical to also
> > report these packets and their metadata to user space as devlink events.
> >
> > If this is not desirable, we can try to call into drop_monitor from
> > devlink and add a new command (e.g., NET_DM_CMD_HW_ALERT), which will
> > encode all the information we currently have in DEVLINK_CMD_TRAP_REPORT.
> >
> > IMO, this is less desirable, as instead of having one tool (devlink) to
> > interact with this mechanism we will need two (devlink & dropwatch).
> >
> > Below I tried to answer all your questions and refer to all the points
> > you brought up.
> >
> > > When you say silently dropped do you mean that mlxsw as of today
> > > doesn't have any counters exposed for those events?
> >
> > Some of these packets are counted, but not all of them.
> >
> > > If we wanted to consolidate this into something existing we can either
> > > (a) add similar traps in the kernel data path;
> > > (b) make these traps extension of statistics.
> > >
> > > My knee jerk reaction to seeing the patches was that it adds a new
> > > place where device statistics are reported.
> >
> > Not at all. This would be a step back. We can already count discards due
> > to VLAN membership on ingress on a per-port basis. A software maintained
> > global counter does not buy us anything.
> >
> > By also getting the dropped packet - coupled with the drop reason and
> > ingress port - you can understand exactly why and on which VLAN the
> > packet was dropped. I wrote a Wireshark dissector for these netlink
> > packets to make our life easier. You can see the details in my comment
> > to the cover letter:
> >
> > https://marc.info/?l=linux-netdev&m=156248736710238&w=2
> >
> > In case you do not care about individual packets, but still want more
> > fine-grained statistics for your monitoring application, you can use
> > eBPF. For example, one thing we did is attaching a kprobe to
> > devlink_trap_report() with an eBPF program that dissects the incoming
> > skbs and maintains a counter per-{5 tuple, drop reason}. With
> > ebpf_exporter you can export these statistics to Prometheus on which you
> > can run queries and visualize the results with Grafana. This is
> > especially useful for tail and early drops since it allows you to
> > understand which flows contribute to most of the drops.
>
> No question that the mechanism is useful.
>
> > > Users who want to know why things are dropped will not get detailed
> > > breakdown from ethtool -S which for better or worse is the one stop
> > > shop for device stats today.
> >
> > I hope I managed to explain why counters are not enough, but I also want
> > to point out that ethtool statistics are not properly documented and
> > this hinders their effectiveness. I did my best to document the exposed
> > traps in order to avoid the same fate:
> >
> > https://patchwork.ozlabs.org/patch/1128585/
> >
> > In addition, there are selftests to show how each trap can be triggered
> > to reduce the ambiguity even further:
> >
> > https://patchwork.ozlabs.org/patch/1128610/
> >
> > And a note in the documentation to make sure future functionality is
> > tested as well:
> >
> > https://patchwork.ozlabs.org/patch/1128608/
> >
> > > Having thought about it some more, however, I think that having a
> > > forwarding "exception" object and hanging statistics off of it is a
> > > better design, even if we need to deal with some duplication to get
> > > there.
> > >
> > > IOW having an way to "trap all packets which would increment a
> > > statistic" (option (b) above) is probably a bad design.
> > >
> > > As for (a) I wonder how many of those events have a corresponding event
> > > in the kernel stack?
> >
> > Generic packet drops all have a corresponding kfree_skb() calls in the
> > kernel, but that does not mean that every packet dropped by the hardware
> > would also be dropped by the kernel if it were to be injected to its Rx
> > path.
>
> The notion that all SW events get captured by kfree_skb() would not be
> correct.
I meant that the generic drop reasons I'm exposing with this patchset
all correspond to reasons for which the kernel would drop packets.
> We have the kfree_skb(), and xdp_exception(), and drivers can
> drop packets if various allocations fail.. the situation is already not
> great.
>
> I think that having a single useful place where users can look to see
> all traffic exception events would go a long way.
I believe this was Dave's point as well. We have one tool to monitor
kfree_skb() drops and with this patchset we will have another to monitor
HW drops. As I mentioned in my previous reply, I will look into sending
the events via drop_monitor by calling into it from devlink.
I'm not involved with XDP (as you might have noticed), but I assume
drop_monitor could be extended for this use case as well by doing
register_trace_xdp_exception(). Then you could monitor SW, HW and XDP
events using a single netlink channel, potentially split into different
multicast groups to allow user space programs to receive only the events
they care about.
> Software side as I mentioned is pretty brutal, IDK how many users are
> actually willing to decode stack traces to figure out why their system
> is dropping packets :/
>
> > In my reply to Dave I gave buffer drops as an example.
>
> The example of buffer drops is also probably the case where having the
> packet is least useful, but yes, I definitely agree devices need a way
> of reporting events that can't happen in SW.
>
> > There are also situations in which packets can be dropped due to
> > device-specific exceptions and these do not have a corresponding drop
> > reason in the kernel. See example here:
> >
> > https://patchwork.ozlabs.org/patch/1128587/
> >
> > > If we could add corresponding trace points and just feed those from
> > > the device driver, that'd obviously be a holy grail.
> >
> > Unlike tracepoints, netlink gives you a structured and extensible
> > interface. For example, in Spectrum-1 we cannot provide the Tx port for
> > early/tail drops, whereas for Spectrum-2 and later we can. With netlink,
> > we can just omit the DEVLINK_ATTR_TRAP_OUT_PORT attribute for
> > Spectrum-1. You also get a programmatic interface that you can query for
> > this information:
> >
> > # devlink -v trap show netdevsim/netdevsim10 trap ingress_vlan_filter
> > netdevsim/netdevsim10:
> > name ingress_vlan_filter type drop generic true report false action drop group l2_drops
> > metadata:
> > input_port
>
> Right, you can set or not set skb fields to some extent but its
> definitely not as flexible as netlink.
^ permalink raw reply
* Re: [PATCH bpf-next] net: Don't uninstall an XDP program when none is installed
From: Maxim Mikityanskiy @ 2019-07-10 11:16 UTC (permalink / raw)
To: Alexei Starovoitov, David S. Miller
Cc: Daniel Borkmann, Björn Töpel, bpf@vger.kernel.org,
netdev@vger.kernel.org, Saeed Mahameed, Jakub Kicinski,
Jesper Dangaard Brouer, John Fastabend, Martin KaFai Lau,
Song Liu, Yonghong Song
In-Reply-To: <20190612161405.24064-1-maximmi@mellanox.com>
On 2019-06-12 19:14, Maxim Mikityanskiy wrote:
> dev_change_xdp_fd doesn't perform any checks in case it uninstalls an
> XDP program. It means that the driver's ndo_bpf can be called with
> XDP_SETUP_PROG asking to set it to NULL even if it's already NULL. This
> case happens if the user runs `ip link set eth0 xdp off` when there is
> no XDP program attached.
>
> The drivers typically perform some heavy operations on XDP_SETUP_PROG,
> so they all have to handle this case internally to return early if it
> happens. This patch puts this check into the kernel code, so that all
> drivers will benefit from it.
>
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> ---
> Björn, please take a look at this, Saeed told me you were doing
> something related, but I couldn't find it. If this fix is already
> covered by your work, please tell about that.
>
> net/core/dev.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 66f7508825bd..68b3e3320ceb 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8089,6 +8089,9 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack,
> bpf_prog_put(prog);
> return -EINVAL;
> }
> + } else {
> + if (!__dev_xdp_query(dev, bpf_op, query))
> + return 0;
> }
>
> err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
>
Alexei, so what about this patch? It's marked as "Changed Requested" in
patchwork, but Jakub's point looks resolved - I don't see any changes
required from my side.
^ permalink raw reply
* [PATCH iproute2 master 1/3] devlink: Change devlink health dump show command to dumpit
From: Tariq Toukan @ 2019-07-10 11:03 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, moshe, ayal, Tariq Toukan
In-Reply-To: <1562756601-19171-1-git-send-email-tariqt@mellanox.com>
From: Aya Levin <ayal@mellanox.com>
Although devlink health dump show command is given per reporter, it
returns large amounts of data. Trying to use the doit cb results in
OUT-OF-BUFFER error. This complementary patch raises the DUMP flag in
order to invoke the dumpit cb. We're safe as no existing drivers
implement the dump health reporter option yet.
Fixes: 041e6e651a8e ("devlink: Add devlink health dump show command")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
devlink/devlink.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index ac8c0fb149b6..e3e1e27ab312 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -6078,13 +6078,13 @@ static int cmd_fmsg_object_cb(const struct nlmsghdr *nlh, void *data)
return MNL_CB_OK;
}
-static int cmd_health_object_common(struct dl *dl, uint8_t cmd)
+static int cmd_health_object_common(struct dl *dl, uint8_t cmd, uint16_t flags)
{
struct fmsg_cb_data data;
struct nlmsghdr *nlh;
int err;
- nlh = mnlg_msg_prepare(dl->nlg, cmd, NLM_F_REQUEST | NLM_F_ACK);
+ nlh = mnlg_msg_prepare(dl->nlg, cmd, flags | NLM_F_REQUEST | NLM_F_ACK);
err = dl_argv_parse_put(nlh, dl,
DL_OPT_HANDLE | DL_OPT_HEALTH_REPORTER_NAME, 0);
@@ -6099,12 +6099,16 @@ static int cmd_health_object_common(struct dl *dl, uint8_t cmd)
static int cmd_health_dump_show(struct dl *dl)
{
- return cmd_health_object_common(dl, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET);
+ return cmd_health_object_common(dl,
+ DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
+ NLM_F_DUMP);
}
static int cmd_health_diagnose(struct dl *dl)
{
- return cmd_health_object_common(dl, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE);
+ return cmd_health_object_common(dl,
+ DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
+ 0);
}
static int cmd_health_recover(struct dl *dl)
--
1.8.3.1
^ permalink raw reply related
* [PATCH iproute2 master 2/3] devlink: Fix binary values print
From: Tariq Toukan @ 2019-07-10 11:03 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, moshe, ayal, Tariq Toukan
In-Reply-To: <1562756601-19171-1-git-send-email-tariqt@mellanox.com>
From: Aya Levin <ayal@mellanox.com>
Fix function pr_out_binary_value() to start printing the binary buffer
from offset 0 instead of offset 1. Remove redundant new line at the
beginning of the output
Example:
With patch:
mlx5e_txqsq:
05 00 00 00 05 00 00 00 01 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 8e 6e 3a 13 07 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0
Without patch
mlx5e_txqsq:
00 00 00 05 00 00 00 01 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 8e 6e 3a 13 07 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c0
Fixes: 844a61764c6f ("devlink: Add helper functions for name and value separately")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
devlink/devlink.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index e3e1e27ab312..4bced4e60ae8 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -1779,29 +1779,31 @@ static void pr_out_uint64_value(struct dl *dl, uint64_t value)
pr_out(" %"PRIu64, value);
}
+static bool is_binary_eol(int i)
+{
+ return !(i%16);
+}
+
static void pr_out_binary_value(struct dl *dl, uint8_t *data, uint32_t len)
{
- int i = 1;
+ int i = 0;
if (dl->json_output)
jsonw_start_array(dl->jw);
- else
- pr_out("\n");
while (i < len) {
- if (dl->json_output) {
+ if (dl->json_output)
jsonw_printf(dl->jw, "%d", data[i]);
- } else {
- pr_out(" %02x", data[i]);
- if (!(i % 16))
- pr_out("\n");
- }
+ else
+ pr_out("%02x ", data[i]);
i++;
+ if (!dl->json_output && is_binary_eol(i))
+ __pr_out_newline();
}
if (dl->json_output)
jsonw_end_array(dl->jw);
- else if ((i - 1) % 16)
- pr_out("\n");
+ else if (!is_binary_eol(i))
+ __pr_out_newline();
}
static void pr_out_str_value(struct dl *dl, const char *value)
--
1.8.3.1
^ permalink raw reply related
* [PATCH iproute2 master 0/3] devlink dumpit fixes
From: Tariq Toukan @ 2019-07-10 11:03 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, moshe, ayal, Tariq Toukan
Hi,
This series from Aya contains several fixes for devlink health
dump show command with binary data.
In patch 1 we replace the usage of doit with a dumpit, which
is non-blocking and allows transferring larger amount of data.
Patches 2 and 3 fix the output for binary data prints, for both
json and non-json.
Series generated against master commit:
2eb23f3e7aaf devlink: Show devlink port number
Regards,
Tariq
Aya Levin (3):
devlink: Change devlink health dump show command to dumpit
devlink: Fix binary values print
devlink: Remove enclosing array brackets binary print with json format
devlink/devlink.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH iproute2 master 3/3] devlink: Remove enclosing array brackets binary print with json format
From: Tariq Toukan @ 2019-07-10 11:03 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, moshe, ayal, Tariq Toukan
In-Reply-To: <1562756601-19171-1-git-send-email-tariqt@mellanox.com>
From: Aya Levin <ayal@mellanox.com>
Keep pr_out_binary_value function only for printing. Inner relations
like array grouping should be done outside the function.
Fixes: 844a61764c6f ("devlink: Add helper functions for name and value separately")
Signed-off-by: Aya Levin <ayal@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
devlink/devlink.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/devlink/devlink.c b/devlink/devlink.c
index 4bced4e60ae8..7532c3f888f9 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -1788,9 +1788,6 @@ static void pr_out_binary_value(struct dl *dl, uint8_t *data, uint32_t len)
{
int i = 0;
- if (dl->json_output)
- jsonw_start_array(dl->jw);
-
while (i < len) {
if (dl->json_output)
jsonw_printf(dl->jw, "%d", data[i]);
@@ -1800,9 +1797,7 @@ static void pr_out_binary_value(struct dl *dl, uint8_t *data, uint32_t len)
if (!dl->json_output && is_binary_eol(i))
__pr_out_newline();
}
- if (dl->json_output)
- jsonw_end_array(dl->jw);
- else if (!is_binary_eol(i))
+ if (!dl->json_output && !is_binary_eol(i))
__pr_out_newline();
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] MAINTAINERS: update BPF JIT S390 maintainers
From: Ilya Leoshkevich @ 2019-07-10 10:50 UTC (permalink / raw)
To: Vasily Gorbik
Cc: Alexei Starovoitov, Daniel Borkmann, Heiko Carstens,
Christian Borntraeger, David S. Miller, netdev, bpf, linux-s390
In-Reply-To: <patch.git-d365382dfc69.your-ad-here.call-01562755343-ext-3127@work.hours>
> Am 10.07.2019 um 12:43 schrieb Vasily Gorbik <gor@linux.ibm.com>:
>
> Ilya Leoshkevich is joining as s390 bpf maintainer. With his background
> as gcc developer he would be valuable for the team and community as a
> whole. Ilya, have fun!
>
> Since there is now enough eyes on s390 bpf, relieve Christian Borntraeger,
> so that he could focus on his maintainer tasks for other components.
>
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
> ---
> MAINTAINERS | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 558acf24ea1e..98e7411dfe56 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3066,9 +3066,9 @@ S: Maintained
> F: arch/riscv/net/
>
> BPF JIT for S390
> +M: Ilya Leoshkevich <iii@linux.ibm.com>
> M: Heiko Carstens <heiko.carstens@de.ibm.com>
> M: Vasily Gorbik <gor@linux.ibm.com>
> -M: Christian Borntraeger <borntraeger@de.ibm.com>
> L: netdev@vger.kernel.org
> L: bpf@vger.kernel.org
> S: Maintained
> --
> 2.21.0
Thanks, Vasily!
Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH] MAINTAINERS: update BPF JIT S390 maintainers
From: Christian Borntraeger @ 2019-07-10 10:44 UTC (permalink / raw)
To: Vasily Gorbik, Alexei Starovoitov, Daniel Borkmann
Cc: Heiko Carstens, Ilya Leoshkevich, David S. Miller, netdev, bpf,
linux-s390
In-Reply-To: <patch.git-d365382dfc69.your-ad-here.call-01562755343-ext-3127@work.hours>
On 10.07.19 12:43, Vasily Gorbik wrote:
> Ilya Leoshkevich is joining as s390 bpf maintainer. With his background
> as gcc developer he would be valuable for the team and community as a
> whole. Ilya, have fun!
>
> Since there is now enough eyes on s390 bpf, relieve Christian Borntraeger,
> so that he could focus on his maintainer tasks for other components.
>
> Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@e.ibm.com>
> ---
> MAINTAINERS | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 558acf24ea1e..98e7411dfe56 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3066,9 +3066,9 @@ S: Maintained
> F: arch/riscv/net/
>
> BPF JIT for S390
> +M: Ilya Leoshkevich <iii@linux.ibm.com>
> M: Heiko Carstens <heiko.carstens@de.ibm.com>
> M: Vasily Gorbik <gor@linux.ibm.com>
> -M: Christian Borntraeger <borntraeger@de.ibm.com>
> L: netdev@vger.kernel.org
> L: bpf@vger.kernel.org
> S: Maintained
>
^ permalink raw reply
* [PATCH] MAINTAINERS: update BPF JIT S390 maintainers
From: Vasily Gorbik @ 2019-07-10 10:43 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann
Cc: Heiko Carstens, Christian Borntraeger, Ilya Leoshkevich,
David S. Miller, netdev, bpf, linux-s390
Ilya Leoshkevich is joining as s390 bpf maintainer. With his background
as gcc developer he would be valuable for the team and community as a
whole. Ilya, have fun!
Since there is now enough eyes on s390 bpf, relieve Christian Borntraeger,
so that he could focus on his maintainer tasks for other components.
Signed-off-by: Vasily Gorbik <gor@linux.ibm.com>
---
MAINTAINERS | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 558acf24ea1e..98e7411dfe56 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3066,9 +3066,9 @@ S: Maintained
F: arch/riscv/net/
BPF JIT for S390
+M: Ilya Leoshkevich <iii@linux.ibm.com>
M: Heiko Carstens <heiko.carstens@de.ibm.com>
M: Vasily Gorbik <gor@linux.ibm.com>
-M: Christian Borntraeger <borntraeger@de.ibm.com>
L: netdev@vger.kernel.org
L: bpf@vger.kernel.org
S: Maintained
--
2.21.0
^ permalink raw reply related
* RE: [PATCH 08/12] net: stmmac: Fix misuses of GENMASK macro
From: Jose Abreu @ 2019-07-10 10:33 UTC (permalink / raw)
To: Joe Perches, Andrew Morton, Giuseppe Cavallaro, Alexandre Torgue,
Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai
Cc: David S. Miller, netdev@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <b38b0b67e724cd026709194b68c2be5ee1058c57.1562734889.git.joe@perches.com>
From: Joe Perches <joe@perches.com>
Date: Jul/10/2019, 06:04:21 (UTC+00:00)
> Arguments are supposed to be ordered high then low.
>
> Signed-off-by: Joe Perches <joe@perches.com>
If you submit another version please add:
Fixes: 293e4365a1ad ("stmmac: change descriptor layout")
Fixes: 9f93ac8d4085 ("net-next: stmmac: Add dwmac-sun8i")
---
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH iproute2 1/2] Revert "ip6tunnel: fix 'ip -6 {show|change} dev <name>' cmds"
From: Andrea Claudi @ 2019-07-10 10:11 UTC (permalink / raw)
To: Mahesh Bandewar (महेश बंडेवार)
Cc: linux-netdev, Stephen Hemminger, David Ahern
In-Reply-To: <CAF2d9jiGnR-A-A-mEv-84Mu6xfwFNvWt5jp+iiBhMGNPMkaDZg@mail.gmail.com>
On Tue, Jul 9, 2019 at 7:31 PM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> wrote:
>
> On Tue, Jul 9, 2019 at 6:16 AM Andrea Claudi <aclaudi@redhat.com> wrote:
> >
> > This reverts commit ba126dcad20e6d0e472586541d78bdd1ac4f1123.
> > It breaks tunnel creation when using 'dev' parameter:
> >
> > $ ip link add type dummy
> > $ ip -6 tunnel add ip6tnl1 mode ip6ip6 remote 2001:db8:ffff:100::2 local 2001:db8:ffff:100::1 hoplimit 1 tclass 0x0 dev dummy0
> > add tunnel "ip6tnl0" failed: File exists
> >
> > dev parameter must be used to specify the device to which
> > the tunnel is binded, and not the tunnel itself.
> >
> > Reported-by: Jianwen Ji <jiji@redhat.com>
> > Reviewed-by: Matteo Croce <mcroce@redhat.com>
> > Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> > ---
> > ip/ip6tunnel.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
> > index 56fd3466ed062..999408ed801b1 100644
> > --- a/ip/ip6tunnel.c
> > +++ b/ip/ip6tunnel.c
> > @@ -298,8 +298,6 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
> > p->link = ll_name_to_index(medium);
> > if (!p->link)
> > return nodev(medium);
> > - else
> > - strlcpy(p->name, medium, sizeof(p->name));
> NACK
>
> I see that ba126dcad20e6d0e472586541d78bdd1ac4f1123 has broke
> something but that doesn't mean revert of the original fix is correct
> way of fixing it. The original fix is fixing the show and change
> command. Shouldn't you try fixing the add command so that all (show,
> change, and add) work correctly?
>
Hi Mahesh,
Thanks for sharing your opinion. I think I already answered this
replying to your review of patch 2/2 of this series.
To summarize that up I think there is a misunderstanding on the
meaning of "dev" param, and "name" param (which is the default) must
be used instead in the cases highlighted in your commit.
Regards,
Andrea
> > }
> > return 0;
> > }
> > --
> > 2.20.1
> >
^ permalink raw reply
* Re: [RESEND] test_verifier #13 fails on arm64: "retval 65507 != -29"
From: Paolo Pisati @ 2019-07-10 10:02 UTC (permalink / raw)
To: Yonghong Song
Cc: Paolo Pisati, Daniel Borkmann, Alexei Starovoitov, Martin Lau,
Song Liu, bpf@vger.kernel.org, netdev
In-Reply-To: <68248069-bcf6-69dd-b0a9-f4ec11e50092@fb.com>
On Mon, Jul 01, 2019 at 09:51:25PM +0000, Yonghong Song wrote:
>
> Below is the test case.
> {
> "valid read map access into a read-only array 2",
> .insns = {
> BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
> BPF_LD_MAP_FD(BPF_REG_1, 0),
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> BPF_FUNC_map_lookup_elem),
> BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
>
> BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
> BPF_MOV64_IMM(BPF_REG_2, 4),
> BPF_MOV64_IMM(BPF_REG_3, 0),
> BPF_MOV64_IMM(BPF_REG_4, 0),
> BPF_MOV64_IMM(BPF_REG_5, 0),
> BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0,
> BPF_FUNC_csum_diff),
> BPF_EXIT_INSN(),
> },
> .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> .fixup_map_array_ro = { 3 },
> .result = ACCEPT,
> .retval = -29,
> },
>
> The issue may be with helper bpf_csum_diff().
> Maybe you can check bpf_csum_diff() helper return value
> to confirm and take a further look at bpf_csum_diff implementations
> between x64 and amd64.
Indeed, the different result comes from csum_partial() or, more precisely,
do_csum().
x86-64 uses an asm optimized version residing in arch/x86/lib/csum-partial_64.c,
while the generic version is in lib/checksum.c.
I replaced the x86-64 csum_partial() / do_csum() code, with the one in
lib/checksum.c and by doing so i reproduced the same error on x86-64 (thus, it's
not an arch dependent issue).
I added some debugging to bpf_csum_diff(), and here are the results with different
checksum implementation code:
http://paste.debian.net/1091037/
lib/checksum.c:
...
[ 206.084537] ____bpf_csum_diff from_size: 1 to_size: 0
[ 206.085274] ____bpf_csum_diff from[0]: 28
[ 206.085276] ____bpf_csum_diff diff[0]: 4294967267
[ 206.085277] ____bpf_csum_diff diff_size: 4 seed: 0
After csum_partial() call:
[ 206.086059] ____bpf_csum_diff csum: 65507 - 0xffe3
arch/x86/lib/csum-partial_64.c
...
[ 40.467308] ____bpf_csum_diff from_size: 1 to_size: 0
[ 40.468141] ____bpf_csum_diff from[0]: 28
[ 40.468143] ____bpf_csum_diff diff[0]: 4294967267
[ 40.468144] ____bpf_csum_diff diff_size: 4 seed: 0
After csum_partial() call:
[ 40.468937] ____bpf_csum_diff csum: -29 - 0xffffffe3
One thing that i noticed, x86-64 csum-partial_64.c::do_csum() doesn't reduce the
calculated checksum to 16bit before returning it (unless the input value is
odd - *):
arch/x86/lib/csum-partial_64.c::do_csum()
...
if (unlikely(odd)) {
result = from32to16(result);
result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
}
return result;
}
contrary to all the other do_csum() implementations (that i could understand):
lib/checksum.c::do_csum()
arch/alpha/lib/checksum.c::do_csum()
arch/parisc/lib/checksum.c::do_csum()
Apparently even ia64 does the folding (arch/ia64/lib/do_csum.S see a comment right
before .do_csum_exit:), and arch/c6x/lib/csum_64plus.S too (see
arch/c6x/lib/csum_64plus.S).
Funnily enough, if i change do_csum() for x86-64, folding the
checksum to 16 bit (following all the other implementations):
--- a/arch/x86/lib/csum-partial_64.c
+++ b/arch/x86/lib/csum-partial_64.c
@@ -112,8 +112,8 @@ static unsigned do_csum(const unsigned char *buff, unsigned
len)
if (len & 1)
result += *buff;
result = add32_with_carry(result>>32, result & 0xffffffff);
+ result = from32to16(result);
if (unlikely(odd)) {
- result = from32to16(result);
result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
}
return result;
then, the x86-64 result match the others: 65507 or 0xffe3.
As a last attempt, i tried running the bpf test_verifier on an armhf platform,
and i got a completely different number:
[ 57.667999] ____bpf_csum_diff from_size: 1 to_size: 0
[ 57.668016] ____bpf_csum_diff from[0]: 28
[ 57.668028] ____bpf_csum_diff diff[0]: 4294967267
[ 57.668039] ____bpf_csum_diff diff_size: 4 seed: 0
After csum_partial() call:
[ 57.668052] ____bpf_csum_diff::2002 csum: 131042 - 0x0001ffe2
Not sure what to make of these number, but i have a question: whats is the
correct checksum of the memory chunk passed to csum_partial()? Is it really -29?
Because, at least 2 other implementations i tested (the arm assembly code and
the c implementation in lib/checksum.c) computes a different value, so either
there's a bug in checksum calcution (2 out of 3???), or we are interpreting the
returned value from csum_partial() somehow wrongly.
*: originally, the x86-64 did the 16bit folding, but the logic was changed to
what we have today during a big rewrite - search for:
commit 3ef076bb685a461bbaff37a1f06010fc4d7ce733
Author: Andi Kleen <ak@suse.de>
Date: Fri Jun 13 04:27:34 2003 -0700
[PATCH] x86-64 merge
in this historic repo:
https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git
--
bye,
p.
^ permalink raw reply
* Re: [PATCH 00/12] treewide: Fix GENMASK misuses
From: Russell King - ARM Linux admin @ 2019-07-10 9:43 UTC (permalink / raw)
To: Johannes Berg
Cc: Joe Perches, Andrew Morton, Patrick Venture, Nancy Yuen,
Benjamin Fair, Andrew Jeffery, openbmc, linux-kernel,
linux-aspeed, linux-arm-kernel, linux-amlogic, netdev,
linux-mediatek, linux-stm32, linux-wireless, linux-media,
linux-iio, devel, alsa-devel, linux-mmc, dri-devel
In-Reply-To: <5fa1fa6998332642c49e2d5209193ffe2713f333.camel@sipsolutions.net>
On Wed, Jul 10, 2019 at 11:17:31AM +0200, Johannes Berg wrote:
> On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote:
> > These GENMASK uses are inverted argument order and the
> > actual masks produced are incorrect. Fix them.
> >
> > Add checkpatch tests to help avoid more misuses too.
> >
> > Joe Perches (12):
> > checkpatch: Add GENMASK tests
>
> IMHO this doesn't make a lot of sense as a checkpatch test - just throw
> in a BUILD_BUG_ON()?
My personal take on this is that GENMASK() is really not useful, it's
just pure obfuscation and leads to exactly these kinds of mistakes.
Yes, I fully understand the argument that you can just specify the
start and end bits, and it _in theory_ makes the code more readable.
However, the problem is when writing code. GENMASK(a, b). Is a the
starting bit or ending bit? Is b the number of bits? It's confusing
and causes mistakes resulting in incorrect code. A BUILD_BUG_ON()
can catch some of the cases, but not all of them.
For example:
GENMASK(6, 2)
would satisify the requirement that a > b, so a BUILD_BUG_ON() will
not trigger, but was the author meaning 0x3c or 0xc0?
Personally, I've decided I am _not_ going to use GENMASK() in my code
because I struggle to get the macro arguments correct - I'm _much_
happier, and it is way more reliable for me to write the mask in hex
notation.
I think this is where use of a ternary operator would come in use. The
normal way of writing a number of bits tends to be "a:b", so if GENMASK
took something like GENMASK(6:2), then I'd have less issue with it,
because it's argument is then in a familiar notation.
Yes, I'm sure that someone will point out that the GENMASK arguments
are just in the same order, but that doesn't prevent _me_ frequently
getting it wrong - and that's the point. The macro seems to me to
cause more problems than it solves.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net v2] Validate required parameters in inet6_validate_link_af
From: Maxim Mikityanskiy @ 2019-07-10 9:36 UTC (permalink / raw)
To: Stefan Lippers-Hollmann
Cc: David Miller, jakub.kicinski@netronome.com, kuznet@ms2.inr.ac.ru,
yoshfuji@linux-ipv6.org, netdev@vger.kernel.org, Leon Romanovsky
In-Reply-To: <20190709031135.123a5b19@mir>
On 2019-07-09 04:11, Stefan Lippers-Hollmann wrote:
> Hi
>
> On 2019-05-22, David Miller wrote:
>> From: Maxim Mikityanskiy <maximmi@mellanox.com>
>> Date: Tue, 21 May 2019 06:40:04 +0000
>>
>>> inet6_set_link_af requires that at least one of IFLA_INET6_TOKEN or
>>> IFLA_INET6_ADDR_GET_MODE is passed. If none of them is passed, it
>>> returns -EINVAL, which may cause do_setlink() to fail in the middle of
>>> processing other commands and give the following warning message:
>>>
>>> A link change request failed with some changes committed already.
>>> Interface eth0 may have been left with an inconsistent configuration,
>>> please check.
>>>
>>> Check the presence of at least one of them in inet6_validate_link_af to
>>> detect invalid parameters at an early stage, before do_setlink does
>>> anything. Also validate the address generation mode at an early stage.
>>>
>>> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
>>
>> Applied, thank you.
>
> After updating from kernel 5.1.16 to 5.2, I noticed that my
> systemd-networkd (241-5, Debian/unstable) managed bridges didn't
> come up and needed a manual "ip link set dev br-lan up" to get
> configured. Bisecting between v5.1 and v5.2 pointed to this
> patch and reverting just this change from v5.2 fixes the issue
> for me again.
This patch changes behavior only in case of invalid input. If the
userspace sends a valid message over netlink, nothing changes after my
patch. However, for some subset of invalid inputs, it used to be
undefined behavior, and the kernel used to apply partial configuration
before it noticed that the input was invalid and failed. After my patch,
this subset of invalid inputs is handled properly, resulting in an
immediate error returned to the userspace, and no configuration is
affected. So, my patch is actually a bug fix.
Unfortunately, commit [1] introduced a regression in systemd, and it
started sending invalid input to the kernel, apparently didn't pay
attention to the error returned and relied on the undefined behavior
(the partial configuration update that took place before my patch).
Later on, commit [2] was introduced, and it should fix that regression
in systemd.
What you experience may be explained by this bug in systemd:
1. systemd broke, but the issue remained unnoticed, because some part of
the configuration was still applied.
2. The bug in systemd was eventually fixed, but apparently you haven't
updated to the version that has this fix yet.
3. My fix for the kernel was merged.
4. As you are using a systemd without fix, the issue led to more severe
consequences, because now no configuration is applied after an invalid
request from systemd.
I haven't tried to reproduce your configuration though, but I guess the
things above are what has happened. I suggest you to update systemd to a
version that has commit [2] (or to build it from master if no newer
version has been released since then) - I hope it solves your issue.
Otherwise, let me know.
[1]:
https://github.com/systemd/systemd/commit/0e2fdb83bb5e22047e0c7cc058b415d0e93f02cf
[2]:
https://github.com/systemd/systemd/commit/4d48747c43922250a62cf6e0ad9ee364665ef82d
> $ git bisect start
> # good: [e93c9c99a629c61837d5a7fc2120cd2b6c70dbdd] Linux 5.1
> git bisect good e93c9c99a629c61837d5a7fc2120cd2b6c70dbdd
> # bad: [46713c3d2f8da5e3d8ddd2249bcb1d9974fb5d28] Merge tag 'for-linus-20190706' of git://git.kernel.dk/linux-block
> git bisect bad 46713c3d2f8da5e3d8ddd2249bcb1d9974fb5d28
> # good: [a2d635decbfa9c1e4ae15cb05b68b2559f7f827c] Merge tag 'drm-next-2019-05-09' of git://anongit.freedesktop.org/drm/drm
> git bisect good a2d635decbfa9c1e4ae15cb05b68b2559f7f827c
> # good: [22c58fd70ca48a29505922b1563826593b08cc00] Merge tag 'armsoc-soc' of git://git.kernel.org/pub/scm/linux/kernel/git/soc/soc
> git bisect good 22c58fd70ca48a29505922b1563826593b08cc00
> # good: [61939b12dc24d0ac958020f261046c35a16e0c48] block: print offending values when cloned rq limits are exceeded
> git bisect good 61939b12dc24d0ac958020f261046c35a16e0c48
> # bad: [3510955b327176fd4cbab5baa75b449f077722a2] mm/list_lru.c: fix memory leak in __memcg_init_list_lru_node
> git bisect bad 3510955b327176fd4cbab5baa75b449f077722a2
> # bad: [30d1d92a888d03681b927c76a35181b4eed7071f] Merge tag 'nds32-for-linux-5.2-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/greentime/linux
> git bisect bad 30d1d92a888d03681b927c76a35181b4eed7071f
> # bad: [dbde71df810c62e72e2aa6d88a0686a6092956cd] Merge tag 'tty-5.2-rc3' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty
> git bisect bad dbde71df810c62e72e2aa6d88a0686a6092956cd
> # bad: [100f6d8e09905c59be45b6316f8f369c0be1b2d8] net: correct zerocopy refcnt with udp MSG_MORE
> git bisect bad 100f6d8e09905c59be45b6316f8f369c0be1b2d8
> # bad: [4ca6dee5220fe2377bf12b354ef85978425c9ec7] dpaa2-eth: Make constant 64-bit long
> git bisect bad 4ca6dee5220fe2377bf12b354ef85978425c9ec7
> # bad: [b5730061d1056abf317caea823b94d6e12b5b4f6] cxgb4: offload VLAN flows regardless of VLAN ethtype
> git bisect bad b5730061d1056abf317caea823b94d6e12b5b4f6
> # bad: [c1e85c6ce57ef1eb73966152993a341c8123a8ea] net: macb: save/restore the remaining registers and features
> git bisect bad c1e85c6ce57ef1eb73966152993a341c8123a8ea
> # bad: [f42c104f2ec94a9255a835cd4cd1bd76279d4d06] Documentation: add TLS offload documentation
> git bisect bad f42c104f2ec94a9255a835cd4cd1bd76279d4d06
> # bad: [d008b3d2be4b00267e7af5c21269e7af4f65c6e2] mISDN: Fix indenting in dsp_cmx.c
> git bisect bad d008b3d2be4b00267e7af5c21269e7af4f65c6e2
> # bad: [40a1578d631a8ac1cf0ef797c435114107747859] ocelot: Dont allocate another multicast list, use __dev_mc_sync
> git bisect bad 40a1578d631a8ac1cf0ef797c435114107747859
> # bad: [7dc2bccab0ee37ac28096b8fcdc390a679a15841] Validate required parameters in inet6_validate_link_af
> git bisect bad 7dc2bccab0ee37ac28096b8fcdc390a679a15841
> # first bad commit: [7dc2bccab0ee37ac28096b8fcdc390a679a15841] Validate required parameters in inet6_validate_link_af
>
> While I originally noticed this issue on real hardware (r8169, e1000,
> e1000e, e100, alx) and multiple systems with a slightly complex bridge
> setup, I can reproduce it with a very basic configuration under kvm
> (upon which all the tests below are based):
>
> $ cat /etc/systemd/network/20-wired.network
> [Match]
> Name=ens4
>
> [Network]
> DHCP=yes
>
> (same results with just DHCP=ipv4)
>
> With the above systemd-networkd configuration, the system comes up
> without network access:
>
> # ip a
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
> link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> inet 127.0.0.1/8 scope host lo
> valid_lft forever preferred_lft forever
> inet6 ::1/128 scope host
> valid_lft forever preferred_lft forever
> 2: ens4: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN group default qlen 1000
> link/ether 00:16:3e:00:00:00 brd ff:ff:ff:ff:ff:ff
>
> # networkctl | cat -
> IDX LINK TYPE OPERATIONAL SETUP
> 1 lo loopback carrier unmanaged
> 2 ens4 ether off configuring
>
> 2 links listed.
>
> Manually enabling the interface does help:
>
> # ip link set dev ens4 up
>
> # ip a
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default qlen 1000
> link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> inet 127.0.0.1/8 scope host lo
> valid_lft forever preferred_lft forever
> inet6 ::1/128 scope host
> valid_lft forever preferred_lft forever
> 2: ens4: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP group default qlen 1000
> link/ether 00:16:3e:00:00:00 brd ff:ff:ff:ff:ff:ff
> inet 172.23.6.0/14 brd 172.23.255.255 scope global dynamic ens4
> valid_lft 43199sec preferred_lft 43199sec
> inet6 2003:xxxx:xxxx:xxxx::197/128 scope global tentative dynamic noprefixroute
> valid_lft 13809sec preferred_lft 1209sec
> inet6 fdxx:xxxx:xxxx::197/128 scope global tentative noprefixroute
> valid_lft forever preferred_lft forever
> inet6 fdxx:xxxx:xxxx:0:216:3eff:fe00:0/64 scope global tentative mngtmpaddr noprefixroute
> valid_lft forever preferred_lft forever
> inet6 2003:xxxx:xxxx:xxxx:216:3eff:fe00:0/64 scope global tentative dynamic mngtmpaddr noprefixroute
> valid_lft 13809sec preferred_lft 1209sec
> inet6 fe80::216:3eff:fe00:0/64 scope link
> valid_lft forever preferred_lft forever
>
> # networkctl | cat -
> IDX LINK TYPE OPERATIONAL SETUP
> 1 lo loopback carrier unmanaged
> 2 ens4 ether routable configured
>
> 2 links listed.
>
> A quick test of upgrading all systemd packages to 242-2 from
> Debian/experimental shows the same issue; Debian 10/ buster (stable)
> is shipping with systemd 241-5.
>
> DHCPv4 is served by a recent OpenWrt/ master snapshot on ipq8065/ nbg6817
> (ARMv7), using dnsmasq 2.80-13 and odhcpd-ipv6only 2019-05-17-41a74cba-3
> covering DHCPv6 and prefix delegation.
>
> Attached are xz compressed versions of the kernel configuration (amd64),
> dmesg and journalctl output.
>
> The Debian/unstable VM was started with qemu-kvm 1:3.1+dfsg-8 on a
> Debian/unstable host running kernel 5.2 with this patch reverted:
>
> $ QEMU_AUDIO_DRV=pa qemu-system-x86_64 \
> -machine accel=kvm:tcg \
> -monitor stdio \
> -rtc base=localtime \
> -cpu qemu64,+vmx \
> -smp 3 \
> -m 4096 \
> -device virtio-gpu-pci \
> -device virtio-net-pci,mac=00:16:3E:00:00:00,netdev=tap-br-lan0 \
> -netdev tap,ifname=tap-br-lan0,script=no,id=tap-br-lan0 \
> -device AC97 \
> -drive file=/srv/storage/vm/linux.qcow2.img,if=none,discard=unmap,index=0,media=disk,id=hd0 \
> -device virtio-scsi-pci,id=scsi -device scsi-hd,drive=hd0 \
> -usb \
> -device usb-tablet \
> -device usb-ehci,id=ehci \
> -device nec-usb-xhci,id=xhci \
> -device virtio-rng-pci \
> -boot menu=on
>
> Regards
> Stefan Lippers-Hollmann
>
^ permalink raw reply
* [PATCH net-next] net: mlx5: Fix compiling error in tls.c
From: Mao Wenan @ 2019-07-10 9:38 UTC (permalink / raw)
To: davem, saeedm; +Cc: netdev, linux-kernel, Mao Wenan
There are some errors while compiling tls.c if
CONFIG_MLX5_FPGA_TLS is not obvious on.
drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls.c: In function mlx5e_tls_set_ipv4_flow:
./include/linux/mlx5/device.h:61:39: error: invalid application of sizeof to incomplete type struct mlx5_ifc_tls_flow_bits
#define __mlx5_st_sz_bits(typ) sizeof(struct mlx5_ifc_##typ##_bits)
^
./include/linux/compiler.h:330:9: note: in definition of macro __compiletime_assert
if (!(condition)) \
^~~~~~~~~
...
drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls.c: In function mlx5e_tls_build_netdev:
drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls.c:202:13: error: MLX5_ACCEL_TLS_TX undeclared (first use in this function); did you mean __MLX5_ACCEL_TLS_H__?
if (caps & MLX5_ACCEL_TLS_TX) {
^~~~~~~~~~~~~~~~~
__MLX5_ACCEL_TLS_H__
drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls.c:207:13: error: MLX5_ACCEL_TLS_RX undeclared (first use in this function); did you mean MLX5_ACCEL_TLS_TX?
if (caps & MLX5_ACCEL_TLS_RX) {
^~~~~~~~~~~~~~~~~
MLX5_ACCEL_TLS_TX
drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls.c:212:15: error: MLX5_ACCEL_TLS_LRO undeclared (first use in this function); did you mean MLX5_ACCEL_TLS_RX?
if (!(caps & MLX5_ACCEL_TLS_LRO)) {
^~~~~~~~~~~~~~~~~~
MLX5_ACCEL_TLS_RX
make[5]: *** [drivers/net/ethernet/mellanox/mlx5/core/en_accel/tls.o] Error 1
make[5]: *** Waiting for unfinished jobs....
make[4]: *** [drivers/net/ethernet/mellanox/mlx5/core] Error 2
make[3]: *** [drivers/net/ethernet/mellanox] Error 2
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [drivers/net/ethernet] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [drivers/net] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2
make: *** Waiting for unfinished jobs....
this patch is to fix this error using 'depends on MLX5_FPGA_TLS' when MLX5_TLS is set.
Fixes: e2869fb2068b ("net/mlx5: Kconfig, Better organize compilation flags")
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
drivers/net/ethernet/mellanox/mlx5/core/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
index 37fef8c..1da2770 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Kconfig
@@ -139,6 +139,7 @@ config MLX5_TLS
depends on MLX5_CORE_EN
depends on TLS_DEVICE
depends on TLS=y || MLX5_CORE=m
+ depends on MLX5_FPGA_TLS
select MLX5_ACCEL
default n
help
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next v3] net/mlx5e: Convert single case statement switch statements into if statements
From: Leon Romanovsky @ 2019-07-10 9:31 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Saeed Mahameed, David S. Miller, Boris Pismenny, netdev,
linux-rdma, linux-kernel, clang-built-linux, Nick Desaulniers
In-Reply-To: <20190710060614.6155-1-natechancellor@gmail.com>
On Tue, Jul 09, 2019 at 11:06:15PM -0700, Nathan Chancellor wrote:
> During the review of commit 1ff2f0fa450e ("net/mlx5e: Return in default
> case statement in tx_post_resync_params"), Leon and Nick pointed out
> that the switch statements can be converted to single if statements
> that return early so that the code is easier to follow.
>
> Suggested-by: Leon Romanovsky <leon@kernel.org>
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
Thanks again,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
^ permalink raw reply
* Re: [PATCH 00/12] treewide: Fix GENMASK misuses
From: Johannes Berg @ 2019-07-10 9:17 UTC (permalink / raw)
To: Joe Perches, Andrew Morton, Patrick Venture, Nancy Yuen,
Benjamin Fair, Andrew Jeffery, openbmc, linux-kernel,
linux-aspeed, linux-arm-kernel, linux-amlogic, netdev,
linux-mediatek, linux-stm32, linux-wireless, linux-media
Cc: dri-devel, linux-iio, linux-mmc, devel, alsa-devel
In-Reply-To: <cover.1562734889.git.joe@perches.com>
On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote:
> These GENMASK uses are inverted argument order and the
> actual masks produced are incorrect. Fix them.
>
> Add checkpatch tests to help avoid more misuses too.
>
> Joe Perches (12):
> checkpatch: Add GENMASK tests
IMHO this doesn't make a lot of sense as a checkpatch test - just throw
in a BUILD_BUG_ON()?
johannes
^ permalink raw reply
* Re: [PATCH net-next,v4 05/12] net: flow_offload: add list handling functions
From: Jiri Pirko @ 2019-07-10 9:06 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ogerlitz,
Manish.Chopra, marcelo.leitner, mkubecek, venkatkumar.duvvuru,
maxime.chevallier, cphealy, phil, netfilter-devel
In-Reply-To: <20190710073652.GD2282@nanopsycho>
Wed, Jul 10, 2019 at 09:36:52AM CEST, jiri@resnulli.us wrote:
>Tue, Jul 09, 2019 at 10:55:43PM CEST, pablo@netfilter.org wrote:
>
>[...]
>
>
>>@@ -176,6 +176,7 @@ struct flow_block_cb *flow_block_cb_alloc(struct net *net, tc_setup_cb_t *cb,
>> if (!block_cb)
>> return ERR_PTR(-ENOMEM);
>>
>>+ block_cb->net = net;
>> block_cb->cb = cb;
>> block_cb->cb_ident = cb_ident;
>> block_cb->cb_priv = cb_priv;
>>@@ -194,6 +195,22 @@ void flow_block_cb_free(struct flow_block_cb *block_cb)
>> }
>> EXPORT_SYMBOL(flow_block_cb_free);
>>
>>+struct flow_block_cb *flow_block_cb_lookup(struct flow_block_offload *f,
>>+ tc_setup_cb_t *cb, void *cb_ident)
>>+{
>>+ struct flow_block_cb *block_cb;
>>+
>>+ list_for_each_entry(block_cb, f->driver_block_list, driver_list) {
>>+ if (block_cb->net == f->net &&
Looking at this a bit more, this is wrong. This breaks block sharing
concept. The original lookup look up the block_cb in certain block - the
block to be shared. With this, you broke the block sharing feature for
mlxsw.
We need to maintain the existing block concept (changed to flow_block).
>
>I don't understand why you need net for this. You should have a list of
>cbs per subsystem (tc/nft) go over it here.
>
>The clash of 2 suybsytems is prevented later on by
>flow_block_cb_is_busy().
>
>Am I missing something?
>If not, could you please remove use of net from flow_block_cb_alloc()
>and from here and replace it by some shared flow structure holding the
>cb list that would be used by both tc and nft?
>
>
>
>>+ block_cb->cb == cb &&
>>+ block_cb->cb_ident == cb_ident)
>>+ return block_cb;
>>+ }
>>+
>>+ return NULL;
>>+}
>>+EXPORT_SYMBOL(flow_block_cb_lookup);
>>+
>
>[...]
^ permalink raw reply
* Re: [PATCH bpf] selftests/bpf: fix bpf_target_sparc check
From: Ilya Leoshkevich @ 2019-07-10 8:59 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, Networking
In-Reply-To: <CAEf4BzZswDkvPbhNnovLjWWmmhR2VBWtrCJkpMXM8M_5Ztn4-w@mail.gmail.com>
> Am 09.07.2019 um 19:56 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>:
>
> On Tue, Jul 9, 2019 at 8:22 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>
>> bpf_helpers.h fails to compile on sparc: the code should be checking
>> for defined(bpf_target_sparc), but checks simply for bpf_target_sparc.
>>
>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>> ---
>> tools/testing/selftests/bpf/bpf_helpers.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
>> index 5f6f9e7aba2a..a8fea087aa90 100644
>> --- a/tools/testing/selftests/bpf/bpf_helpers.h
>> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
>> @@ -443,7 +443,7 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
>> #ifdef bpf_target_powerpc
>
> While at it, can you please also fix this one?
Do you mean #ifdef bpf_target_powerpc? I think it’s correct, because it’s #ifdef, not #if.
But I could change it to #if defined() for consistency.
Best regards,
Ilya
^ permalink raw reply
* RE: [PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR
From: David Laight @ 2019-07-10 8:57 UTC (permalink / raw)
To: 'Jian-Hong Pan', Yan-Hsuan Chuang, Kalle Valo,
David S . Miller, Larry Finger, Christoph Hellwig
Cc: linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux@endlessm.com, Daniel Drake,
stable@vger.kernel.org
In-Reply-To: <20190710083825.7115-1-jian-hong@endlessm.com>
From: Jian-Hong Pan
> Sent: 10 July 2019 09:38
>
> Testing with RTL8822BE hardware, when available memory is low, we
> frequently see a kernel panic and system freeze.
>
> First, rtw_pci_rx_isr encounters a memory allocation failure (trimmed):
>
> rx routine starvation
> WARNING: CPU: 7 PID: 9871 at drivers/net/wireless/realtek/rtw88/pci.c:822
> rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
> [ 2356.580313] RIP: 0010:rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
>
> Then we see a variety of different error conditions and kernel panics,
> such as this one (trimmed):
>
> rtw_pci 0000:02:00.0: pci bus timeout, check dma status
> skbuff: skb_over_panic: text:00000000091b6e66 len:415 put:415 head:00000000d2880c6f
> data:000000007a02b1ea tail:0x1df end:0xc0 dev:<NULL>
> ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:105!
> invalid opcode: 0000 [#1] SMP NOPTI
> RIP: 0010:skb_panic+0x43/0x45
>
> When skb allocation fails and the "rx routine starvation" is hit, the
> function returns immediately without updating the RX ring. At this
> point, the RX ring may continue referencing an old skb which was already
> handed off to ieee80211_rx_irqsafe(). When it comes to be used again,
> bad things happen.
>
> This patch allocates a new, data-sized skb first in RX ISR. After
> copying the data in, we pass it to the upper layers. However, if skb
> allocation fails, we effectively drop the frame. In both cases, the
> original, full size ring skb is reused.
>
> In addition, by fixing the kernel crash, the RX routine should now
> generally behave better under low memory conditions.
A couple of minor nits (see below).
You may want to do a followup patch that changes the rx buffers
(used by the hardware) to by just memory buffers.
Nothing (probably) relies on them being skb with all the accociated
baggage.
David
>
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053
> Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> Cc: <stable@vger.kernel.org>
> ---
> v2:
> - Allocate new data-sized skb and put data into it, then pass it to
> mac80211. Reuse the original skb in RX ring by DMA sync.
> - Modify the commit message.
> - Introduce following [PATCH v3 2/2] rtw88: pci: Use DMA sync instead
> of remapping in RX ISR.
>
> v3:
> - Same as v2.
>
> drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++-------------
> 1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index cfe05ba7280d..e9fe3ad896c8 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> u32 pkt_offset;
> u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> u32 buf_desc_sz = chip->rx_buf_desc_sz;
> + u32 new_len;
> u8 *rx_desc;
> dma_addr_t dma;
>
> @@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> pkt_stat.shift;
>
> - if (pkt_stat.is_c2h) {
> - /* keep rx_desc, halmac needs it */
> - skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> + /* discard current skb if the new skb cannot be allocated as a
> + * new one in rx ring later
> + */
That comment isn't quite right.
maybe: "Allocate a new skb for this frame, discard if none available"
> + new_len = pkt_stat.pkt_len + pkt_offset;
> + new = dev_alloc_skb(new_len);
> + if (WARN_ONCE(!new, "rx routine starvation\n"))
I think you should count these??
> + goto next_rp;
> +
> + /* put the DMA data including rx_desc from phy to new skb */
> + skb_put_data(new, skb->data, new_len);
>
> - /* pass offset for further operation */
> - *((u32 *)skb->cb) = pkt_offset;
> - skb_queue_tail(&rtwdev->c2h_queue, skb);
> + if (pkt_stat.is_c2h) {
> + /* pass rx_desc & offset for further operation */
> + *((u32 *)new->cb) = pkt_offset;
> + skb_queue_tail(&rtwdev->c2h_queue, new);
> ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work);
> } else {
> - /* remove rx_desc, maybe use skb_pull? */
> - skb_put(skb, pkt_stat.pkt_len);
> - skb_reserve(skb, pkt_offset);
> -
> - /* alloc a smaller skb to mac80211 */
> - new = dev_alloc_skb(pkt_stat.pkt_len);
> - if (!new) {
> - new = skb;
> - } else {
> - skb_put_data(new, skb->data, skb->len);
> - dev_kfree_skb_any(skb);
> - }
> - /* TODO: merge into rx.c */
> - rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> + /* remove rx_desc */
> + skb_pull(new, pkt_offset);
> +
> + rtw_rx_stats(rtwdev, pkt_stat.vif, new);
> memcpy(new->cb, &rx_status, sizeof(rx_status));
> ieee80211_rx_irqsafe(rtwdev->hw, new);
> }
>
> - /* skb delivered to mac80211, alloc a new one in rx ring */
> - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE);
> - if (WARN(!new, "rx routine starvation\n"))
> - return;
> -
> - ring->buf[cur_rp] = new;
> - rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);
> +next_rp:
> + /* new skb delivered to mac80211, re-enable original skb DMA */
> + rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
>
> /* host read next element in ring */
> if (++cur_rp >= ring->r.len)
> --
> 2.22.0
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR
From: Jian-Hong Pan @ 2019-07-10 8:54 UTC (permalink / raw)
To: Tony Chuang
Cc: Kalle Valo, David S . Miller, Larry Finger, David Laight,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux@endlessm.com, Daniel Drake,
stable@vger.kernel.org
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D1864779@RTITMBSVM04.realtek.com.tw>
Tony Chuang <yhchuang@realtek.com> 於 2019年7月10日 週三 下午4:37寫道：
>
> > Subject: [PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in
> > RX ISR
> >
> > Testing with RTL8822BE hardware, when available memory is low, we
> > frequently see a kernel panic and system freeze.
> >
> > First, rtw_pci_rx_isr encounters a memory allocation failure (trimmed):
> >
> > rx routine starvation
> > WARNING: CPU: 7 PID: 9871 at drivers/net/wireless/realtek/rtw88/pci.c:822
> > rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
> > [ 2356.580313] RIP: 0010:rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
> >
> > Then we see a variety of different error conditions and kernel panics,
> > such as this one (trimmed):
> >
> > rtw_pci 0000:02:00.0: pci bus timeout, check dma status
> > skbuff: skb_over_panic: text:00000000091b6e66 len:415 put:415
> > head:00000000d2880c6f data:000000007a02b1ea tail:0x1df end:0xc0
> > dev:<NULL>
> > ------------[ cut here ]------------
> > kernel BUG at net/core/skbuff.c:105!
> > invalid opcode: 0000 [#1] SMP NOPTI
> > RIP: 0010:skb_panic+0x43/0x45
> >
> > When skb allocation fails and the "rx routine starvation" is hit, the
> > function returns immediately without updating the RX ring. At this
> > point, the RX ring may continue referencing an old skb which was already
> > handed off to ieee80211_rx_irqsafe(). When it comes to be used again,
> > bad things happen.
> >
> > This patch allocates a new, data-sized skb first in RX ISR. After
> > copying the data in, we pass it to the upper layers. However, if skb
> > allocation fails, we effectively drop the frame. In both cases, the
> > original, full size ring skb is reused.
> >
> > In addition, to fixing the kernel crash, the RX routine should now
> > generally behave better under low memory conditions.
> >
> > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053
> > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> > drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++-------------
> > 1 file changed, 22 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> > b/drivers/net/wireless/realtek/rtw88/pci.c
> > index cfe05ba7280d..e9fe3ad896c8 100644
> > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > @@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev,
> > struct rtw_pci *rtwpci,
> > u32 pkt_offset;
> > u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> > u32 buf_desc_sz = chip->rx_buf_desc_sz;
> > + u32 new_len;
> > u8 *rx_desc;
> > dma_addr_t dma;
> >
> > @@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev,
> > struct rtw_pci *rtwpci,
> > pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> > pkt_stat.shift;
> >
> > - if (pkt_stat.is_c2h) {
> > - /* keep rx_desc, halmac needs it */
> > - skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> > + /* discard current skb if the new skb cannot be allocated as a
> > + * new one in rx ring later
> > + */
> > + new_len = pkt_stat.pkt_len + pkt_offset;
> > + new = dev_alloc_skb(new_len);
> > + if (WARN_ONCE(!new, "rx routine starvation\n"))
> > + goto next_rp;
> > +
> > + /* put the DMA data including rx_desc from phy to new skb */
> > + skb_put_data(new, skb->data, new_len);
> >
> > - /* pass offset for further operation */
> > - *((u32 *)skb->cb) = pkt_offset;
> > - skb_queue_tail(&rtwdev->c2h_queue, skb);
> > + if (pkt_stat.is_c2h) {
> > + /* pass rx_desc & offset for further operation */
> > + *((u32 *)new->cb) = pkt_offset;
> > + skb_queue_tail(&rtwdev->c2h_queue, new);
> > ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work);
> > } else {
> > - /* remove rx_desc, maybe use skb_pull? */
> > - skb_put(skb, pkt_stat.pkt_len);
> > - skb_reserve(skb, pkt_offset);
> > -
> > - /* alloc a smaller skb to mac80211 */
> > - new = dev_alloc_skb(pkt_stat.pkt_len);
> > - if (!new) {
> > - new = skb;
> > - } else {
> > - skb_put_data(new, skb->data, skb->len);
> > - dev_kfree_skb_any(skb);
> > - }
> > - /* TODO: merge into rx.c */
> > - rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> > + /* remove rx_desc */
> > + skb_pull(new, pkt_offset);
> > +
> > + rtw_rx_stats(rtwdev, pkt_stat.vif, new);
> > memcpy(new->cb, &rx_status, sizeof(rx_status));
> > ieee80211_rx_irqsafe(rtwdev->hw, new);
> > }
> >
> > - /* skb delivered to mac80211, alloc a new one in rx ring */
> > - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE);
> > - if (WARN(!new, "rx routine starvation\n"))
> > - return;
> > -
> > - ring->buf[cur_rp] = new;
> > - rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);
> > +next_rp:
> > + /* new skb delivered to mac80211, re-enable original skb DMA */
> > + rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
> >
> > /* host read next element in ring */
> > if (++cur_rp >= ring->r.len)
> > --
> > 2.22.0
>
> Now it looks good to me. Thanks.
>
> Acked-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Yan-Hsuan
Uh! Thanks for your ack.
But I just sent version 3 patches (including [PATCH v3 2/2] rtw88:
pci: Use DMA sync instead of remapping in RX ISR) by following
Christoph's comment. [1]
Could you please also review the 2 patches of version 3? Thank you.
[1]: https://lkml.org/lkml/2019/7/9/507
Jian-Hong Pan
^ permalink raw reply
* [PATCH v3 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR
From: Jian-Hong Pan @ 2019-07-10 8:38 UTC (permalink / raw)
To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller, Larry Finger,
David Laight, Christoph Hellwig
Cc: linux-wireless, netdev, linux-kernel, linux, Daniel Drake,
Jian-Hong Pan, stable
In-Reply-To: <20190709161550.GA8703@infradead.org>
Since each skb in RX ring is reused instead of new allocation, we can
treat the DMA in a more efficient way by DMA synchronization.
Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
Cc: <stable@vger.kernel.org>
---
v2:
- New patch by following [PATCH v3 1/2] rtw88: pci: Rearrange the
memory usage for skb in RX ISR.
v3:
- Remove rtw_pci_sync_rx_desc_cpu and call dma_sync_single_for_cpu in
rtw_pci_rx_isr directly.
- Remove the return value of rtw_pci_sync_rx_desc_device.
- Use DMA_FROM_DEVICE instead of PCI_DMA_FROMDEVICE.
drivers/net/wireless/realtek/rtw88/pci.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index e9fe3ad896c8..485d30c06935 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -206,6 +206,23 @@ static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
return 0;
}
+static void rtw_pci_sync_rx_desc_device(struct rtw_dev *rtwdev, dma_addr_t dma,
+ struct rtw_pci_rx_ring *rx_ring,
+ u32 idx, u32 desc_sz)
+{
+ struct device *dev = rtwdev->dev;
+ struct rtw_pci_rx_buffer_desc *buf_desc;
+ int buf_sz = RTK_PCI_RX_BUF_SIZE;
+
+ dma_sync_single_for_device(dev, dma, buf_sz, DMA_FROM_DEVICE);
+
+ buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
+ idx * desc_sz);
+ memset(buf_desc, 0, sizeof(*buf_desc));
+ buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
+ buf_desc->dma = cpu_to_le32(dma);
+}
+
static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
struct rtw_pci_rx_ring *rx_ring,
u8 desc_size, u32 len)
@@ -782,8 +799,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
rtw_pci_dma_check(rtwdev, ring, cur_rp);
skb = ring->buf[cur_rp];
dma = *((dma_addr_t *)skb->cb);
- pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
- PCI_DMA_FROMDEVICE);
+ dma_sync_single_for_cpu(rtwdev->dev, dma, RTK_PCI_RX_BUF_SIZE,
+ DMA_FROM_DEVICE);
rx_desc = skb->data;
chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status);
@@ -818,7 +835,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
next_rp:
/* new skb delivered to mac80211, re-enable original skb DMA */
- rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
+ rtw_pci_sync_rx_desc_device(rtwdev, dma, ring, cur_rp,
+ buf_desc_sz);
/* host read next element in ring */
if (++cur_rp >= ring->r.len)
--
2.22.0
^ 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