* Re: [PATCH v2] bpf: fix uapi bpf_prog_info fields alignment
From: Alexei Starovoitov @ 2019-06-25 14:16 UTC (permalink / raw)
To: Baruch Siach
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Network Development, bpf, Dmitry V . Levin,
Arnd Bergmann, linux-arch, Jiri Olsa, Geert Uytterhoeven,
Linus Torvalds
In-Reply-To: <a5fb2545a0cf151bc443efa10c16c5a4de6f2670.1561460681.git.baruch@tkos.co.il>
On Tue, Jun 25, 2019 at 4:07 AM Baruch Siach <baruch@tkos.co.il> wrote:
>
> Merge commit 1c8c5a9d38f60 ("Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next") undid the
> fix from commit 36f9814a494 ("bpf: fix uapi hole for 32 bit compat
> applications") by taking the gpl_compatible 1-bit field definition from
> commit b85fab0e67b162 ("bpf: Add gpl_compatible flag to struct
> bpf_prog_info") as is. That breaks architectures with 16-bit alignment
> like m68k. Embed gpl_compatible into an anonymous union with 32-bit pad
> member to restore alignment of following fields.
>
> Thanks to Dmitry V. Levin his analysis of this bug history.
>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> v2:
> Use anonymous union with pad to make it less likely to break again in
> the future.
> ---
> include/uapi/linux/bpf.h | 5 ++++-
> tools/include/uapi/linux/bpf.h | 5 ++++-
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a8b823c30b43..766eae02d7ae 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3142,7 +3142,10 @@ struct bpf_prog_info {
> __aligned_u64 map_ids;
> char name[BPF_OBJ_NAME_LEN];
> __u32 ifindex;
> - __u32 gpl_compatible:1;
> + union {
> + __u32 gpl_compatible:1;
> + __u32 pad;
> + };
Nack for the reasons explained in the previous thread
on the same subject.
Why cannot you go with earlier suggestion of _u32 :31; ?
^ permalink raw reply
* Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver
From: Johannes Berg @ 2019-06-25 14:14 UTC (permalink / raw)
To: Alex Elder, Dan Williams, Arnd Bergmann
Cc: Subash Abhinov Kasiviswanathan, abhishek.esse, Ben Chan,
Bjorn Andersson, cpratapa, David Miller, DTML, Eric Caruso,
evgreen, Ilias Apalodimas, Linux ARM, linux-arm-msm,
Linux Kernel Mailing List, linux-soc, Networking, syadagir
In-Reply-To: <7de004be-27b6-ac63-389d-8ea9d23d0361@linaro.org>
Hi Alex,
I'll just pick a few or your messages and reply there - some other
subthreads seem to have pretty much completed.
> Sorry for the delay. There's a lot here to go through, and with
> each message the picture is (slowly) getting a bit clearer for me.
> Still, there are some broad tradeoffs to consider and I think we
> need to get a little more specific again. I'm going to start a
> new thread (or rather re-subject a response to the very first one)
> that tries to do a fresh start that takes into account the
> discussion so far.
>
> I will also be talking with some people inside Qualcomm (including
> Subash) soon to make sure we don't miss any requirements or insights
> they know of that I don't realize are important.
That's much appreciated.
> > Linux usually tries to keep drivers generic and focused; each driver is
> > written for a specific function. For example, a USB device usually
> > provides multiple USB interfaces which will be bound to different Linux
> > drivers like a TTY, cdc-ether, QMI (via qmi_wwan), cdc-acm, etc.
>
> So USB has some attributes similar to what we're talking about
> here. But if I'm not mistaken we want some sort of an overall
> management scheme as well.
Yes. For the record, I think the part about "keep drivers generic and
focused" really only works for USB devices that expose different pieces
that look like any other network device or a TTY device on the USB
level, just the combination of these things (and knowing about that)
really makes them a modem.
For things like IPA or the (hypothetical) Intel driver we're talking
about, it's still all managed by a single (PCIe) driver. For the Intel
device in particular, even all the control channels are over exactly the
same transport mechanism as the data channels.
> > These drivers are often generic and we may not have enough information
> > in one driver to know that the parent of this interface is a WWAN
> > device. But another driver might. Since probing is asynchronous we may
> > have cdc-acm bind to a device and provide a TTY before cdc-ether (which
> > does know it's a WWAN) binds and provides the netdevice.
>
> Is this why Johannes wanted to have a "maybe attach" method?
Yes.
> I don't like the "maybe" API unless there's no other way to do it.
>
> Instead I think it would be better for the probing driver to register
> with a whatever the WWAN core is, and then have the WWAN core be
> responsible for pulling things all together when it receives a
> request to do so. I.e., something in user space should request
> that a registered data interface be brought up, and at that
> time everything "knows" it's implemented as part of a WWAN
> device.
Right, but then we just punt to userspace. Mostly we *do* (eventually!)
know that it's a WWAN device, just not every component can detect it.
Some components typically can.
So for example, you might have a USB multi-function device with a
network function (looks just like ethernet pretty much) but another TTY
control channel that actually has some specific WWAN IDs, so that part
can know it's a WWAN.
Here, the ethernet function would need "maybe" attach, and the control
channel would "definitively" attach, pulling it together as a WWAN
device without requiring any more action or information.
> So maybe:
> - Hardware probe detects a WWAN device
> - The drivers that detect the WWAN device register it with the
> WWAN core code.
> - A control channel is instantiated at/before the time the WWAN
> device is registered
> - Something in user space should manage the bring-up of any
> other things on the WWAN device thereafter
But those things need to actually get connected first :-)
In IPA/Intel case this is easy since it's a single driver. But if
there's multi-function device with ethernet being a completely separate
driver, the control channel cannot even reach that to tell it to create
a new data channel.
> > userspace should probably always create the netdevices (since they are
> > always useless until userspace coordinates with the firmware about
> > them) but that's not how things are yet.
>
> That's too bad. How hard would that be to change?
Depends, but as I said above it's probably orthogonal to the question.
The data channel driver would still need to attach to the WWAN device
somehow so it becomes reachable by the control plane (note this isn't
the same as "control channel" since the latter talks to the modem, the
control plane talks to the kernel drivers).
> > > - What causes a created channel to be removed?
> >
> > Driver removal, userspace WWAN daemon terminating the packet data
> > connection which the channel represents, the modem terminating the
> > packet data connection (eg network initiated disconnect), etc.
>
> OK this is as I expected. Driver (or device) removal is somewhat
> obvious, but you're confirming user space might request it as well.
If userspace actually had the ability to create (data) channels, then it
would have the ability to also remove them. Right now, this may or may
not be supported by the drivers that act together to form the interfaces
to a WWAN device.
> > > - You distinguish between attaching a netdevice and (what
> > > I'll call) activating it. What causes activation?
> >
> > Can you describe what you mean by "activating"? Do you mean
> > successfully TX/RX packets via the netdev and the outside world?
>
> Johannes mentioned an API to "maybe attach" a device. That begs
> the question of what happens if this request does *not* attach.
> Does the attach request have to be made again, or is it done
> automatically with a notification, or something else?
>
> So by "activation" I was trying to refer to the notion of this
> subsequent successful attach.
Oh. Well, what I was thinking that "maybe attach" would just be a sort
of "in-limbo" WWAN device that doesn't get visible to userspace or the
control plane until something did a "definitively attach" to it so it
was known to be a WWAN device.
The case of "maybe attach but never get to definitive attach" would be
the case where the USB driver bound a real ethernet device, for example,
not something that looks like an ethernet device but really is part of a
modem.
OTOH, "activating" a data channel is also needed somehow through the
control channel by talking to the modem, i.e. making a connection. In
the ideal case we'd not even *have* a netdev until it makes sense to
create a data channel, but in reality a lot of devices have one around
all the time (or even only support one), AFAICT.
> > I read "attach" here as simply associating an existing netdev with the
> > "parent" WWAN device. A purely Linux operation that is only book-
> > keeping and may not have any interaction with the modem.
>
> If that's the case I would want the "activation" to be a separate
> step. The attach would do the bookkeeping, and generally shouldn't
> fail. An attached interface would be brought up ("activated")
> separately and might fail if things aren't quite ready yet.
Right, but netdevs need to be brought up anyway, and that can fail?
> > > - How are the attributes of a WWAN device or channel set,
> > > or communicated?
> >
> > Via netlink attributes when userspace asks the WWAN device to create a
> > new channel. In the control methods I've seen, only userspace really
> > knows the channel identifier that it and the modem have agreed on (eg
> > what the MUX ID in the QMAP header would be, or the MBIM Session ID).
>
> Yes, that's the way it's worked for rmnet and IPA. Previously it
> was IOCTL requests but it's currently hard-wired.
Right. We're just trying to lift it out of the Qualcomm sphere into
something more generically useful.
johannes
^ permalink raw reply
* [PATCH net-next] tools: lib: bpf: libbpf: fix max() type mistmatch for 32bit
From: Ivan Khoronzhuk @ 2019-06-25 14:11 UTC (permalink / raw)
To: ast, netdev; +Cc: daniel, bpf, linux-kernel, Ivan Khoronzhuk
It fixes build error for 32bit coused by type mistmatch
size_t/unsigned long.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
Based on net-next/master
tools/lib/bpf/libbpf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 4259c9f0cfe7..d03016a559e2 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -778,7 +778,7 @@ static struct bpf_map *bpf_object__add_map(struct bpf_object *obj)
if (obj->nr_maps < obj->maps_cap)
return &obj->maps[obj->nr_maps++];
- new_cap = max(4ul, obj->maps_cap * 3 / 2);
+ new_cap = max((size_t)4, obj->maps_cap * 3 / 2);
new_maps = realloc(obj->maps, new_cap * sizeof(*obj->maps));
if (!new_maps) {
pr_warning("alloc maps for object failed\n");
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] can: mcp251x: add error check when wq alloc failed
From: Willem de Bruijn @ 2019-06-25 14:03 UTC (permalink / raw)
To: Weitao Hou
Cc: Wolfgang Grandegger, Marc Kleine-Budde, David Miller,
Greg Kroah-Hartman, allison, tglx, sean, linux-can,
Network Development, linux-kernel
In-Reply-To: <20190625125048.28849-1-houweitaoo@gmail.com>
On Tue, Jun 25, 2019 at 8:51 AM Weitao Hou <houweitaoo@gmail.com> wrote:
>
> add error check when workqueue alloc failed, and remove
> redundant code to make it clear
>
> Signed-off-by: Weitao Hou <houweitaoo@gmail.com>
Acked-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply
* Re: [PATCH v3 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Willem de Bruijn @ 2019-06-25 13:37 UTC (permalink / raw)
To: Neil Horman; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <20190625110247.GA29902@hmswarspite.think-freely.org>
On Tue, Jun 25, 2019 at 7:03 AM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Mon, Jun 24, 2019 at 06:15:29PM -0400, Willem de Bruijn wrote:
> > > > > + if (need_wait && !packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST)) {
> > > > > + po->wait_on_complete = 1;
> > > > > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > > >
> > > > This resets timeout on every loop. should only set above the loop once.
> > > >
> > > I explained exactly why I did that in the change log. Its because I reuse the
> > > timeout variable to get the return value of the wait_for_complete call.
> > > Otherwise I need to add additional data to the stack, which I don't want to do.
> > > Sock_sndtimeo is an inline function and really doesn't add any overhead to this
> > > path, so I see no reason not to reuse the variable.
> >
> > The issue isn't the reuse. It is that timeo is reset to sk_sndtimeo
> > each time. Whereas wait_for_common and its variants return the
> > number of jiffies left in case the loop needs to sleep again later.
> >
> > Reading sock_sndtimeo once and passing it to wait_.. repeatedly is a
> > common pattern across the stack.
> >
> But those patterns are unique to those situations. For instance, in
> tcp_sendmsg_locked, we aquire the value of the socket timeout, and use that to
> wait for the entire message send operation to complete, which consists of
> potentially several blocking operations (waiting for the tcp connection to be
> established, waiting for socket memory, etc). In that situation we want to wait
> for all of those operations to complete to send a single message, and fail if
> they exceed the timeout in aggregate. The semantics are different with
> AF_PACKET. In this use case, the message is in effect empty, and just used to
> pass some control information. tpacket_snd, sends as many frames from the
> memory mapped buffer as possible, and on each iteration we want to wait for the
> specified timeout for those frames to complete sending. I think resetting the
> timeout on each wait instance is the right way to go here.
I disagree. If a SO_SNDTIMEO of a given time is set, the thread should
not wait beyond that. Else what is the point of passing a specific
duration in the syscall?
Btw, we can always drop the timeo and go back to unconditional (bar
signals) waiting.
>
> > > > > @@ -2728,6 +2755,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > > err = net_xmit_errno(err);
> > > > > if (err && __packet_get_status(po, ph) ==
> > > > > TP_STATUS_AVAILABLE) {
> > > > > + /* re-init completion queue to avoid subsequent fallthrough
> > > > > + * on a future thread calling wait_on_complete_interruptible_timeout
> > > > > + */
> > > > > + po->wait_on_complete = 0;
> > > >
> > > > If setting where sleeping, no need for resetting if a failure happens
> > > > between those blocks.
> > > >
> > > > > + init_completion(&po->skb_completion);
> > > >
> > > > no need to reinit between each use?
> > > >
> > > I explained exactly why I did this in the comment above. We have to set
> > > wait_for_complete prior to calling transmit, so as to ensure that we call
> > > wait_for_completion before we exit the loop. However, in this error case, we
> > > exit the loop prior to calling wait_for_complete, so we need to reset the
> > > completion variable and the wait_for_complete flag. Otherwise we will be in a
> > > case where, on the next entrace to this loop we will have a completion variable
> > > with completion->done > 0, meaning the next wait will be a fall through case,
> > > which we don't want.
> >
> > By moving back to the point where schedule() is called, hopefully this
> > complexity automatically goes away. Same as my comment to the line
> > immediately above.
> >
> Its going to change what the complexity is, actually. I was looking at this
> last night, and I realized that your assertion that we could remove
> packet_next_frame came at a cost. This is because we need to determine if we
> have to wait before we call po->xmit, but we need to actually do the wait after
> po->xmit
Why? The existing method using schedule() doesn't.
Can we not just loop while sending and sleep immediately when
__packet_get_status returns !TP_STATUS_AVAILABLE?
I don't understand the need to probe the next packet to send instead
of the current.
This seems to be the crux of the disagreement. My guess is that it has
to do with setting wait_on_complete, but I don't see the problem. It
can be set immediately before going to sleep.
I don't meant to draw this out, btw, or add to your workload. If you
prefer, I can take a stab at my (admittedly hand-wavy) suggestion.
> (to ensure that wait_on_complete is set properly when the desructor is
> called). By moving the wait to the top of the loop and getting rid of
> packet_next_frame we now have a race condition in which we might call
> tpacket_destruct_skb with wait_on_complete set to false, causing us to wait for
> the maximum timeout erroneously. So I'm going to have to find a new way to
> signal the need to call complete, which I think will introduce a different level
> of complexity.
^ permalink raw reply
* Re: [PATCH v2] samples: bpf: make the use of xdp samples consistent
From: Song Liu @ 2019-06-25 13:31 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Toke Høiland-Jørgensen, Daniel T. Lee, Daniel Borkmann,
Alexei Starovoitov, bpf, Networking
In-Reply-To: <20190625120543.12b25184@carbon>
On Tue, Jun 25, 2019 at 5:09 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> On Tue, 25 Jun 2019 11:08:22 +0200
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> > "Daniel T. Lee" <danieltimlee@gmail.com> writes:
> >
> > > Currently, each xdp samples are inconsistent in the use.
> > > Most of the samples fetch the interface with it's name.
> > > (ex. xdp1, xdp2skb, xdp_redirect_cpu, xdp_sample_pkts, etc.)
> > >
> > > But some of the xdp samples are fetching the interface with
> > > ifindex by command argument.
> > >
> > > This commit enables xdp samples to fetch interface with it's name
> > > without changing the original index interface fetching.
> > > (<ifname|ifindex> fetching in the same way as xdp_sample_pkts_user.c does.)
> > >
> > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
Acked-by: Song Liu <songliubraving@fb.com>
> > > ---
> > > Changes in v2:
> > > - added xdp_redirect_user.c, xdp_redirect_map_user.c
> >
> > Great, thanks!
> >
> > Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Great you basically solved one of our TODOs:
> - TODO Change sample programs to accept ifnames as well as indexes
>
> https://github.com/xdp-project/xdp-project/blob/master/xdp-project.org#next-change-sample-programs-to-accept-ifnames-as-well-as-indexes
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next 0/2] Track recursive calls in TC act_mirred
From: John Hurley @ 2019-06-25 13:26 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Linux Netdev List, David Miller, Florian Westphal, Simon Horman,
Jakub Kicinski, oss-drivers
In-Reply-To: <108a6e4b-379c-ba43-5de0-27e31ade3470@mojatatu.com>
On Tue, Jun 25, 2019 at 12:18 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2019-06-24 6:13 p.m., John Hurley wrote:
> > These patches aim to prevent act_mirred causing stack overflow events from
> > recursively calling packet xmit or receive functions. Such events can
> > occur with poor TC configuration that causes packets to travel in loops
> > within the system.
> >
> > Florian Westphal advises that a recursion crash and packets looping are
> > separate issues and should be treated as such. David Miller futher points
> > out that pcpu counters cannot track the precise skb context required to
> > detect loops. Hence these patches are not aimed at detecting packet loops,
> > rather, preventing stack flows arising from such loops.
>
> Sigh. So we are still trying to save 2 bits?
> John, you said ovs has introduced a similar loop handling code;
> Is their approach similar to this? Bigger question: Is this approach
> "good enough"?
>
Hi Jamal.
Yes, OvS implements a similar approach to prevent recursion:
https://elixir.bootlin.com/linux/v5.2-rc6/source/net/openvswitch/actions.c#L1530
It was discussed on a previous thread that there are really 2 issues
here (even if it is the same thing that triggers them).
Firstly, the infinite looping of packets caused by poor config and,
secondly, the kernel panic caused by a stack overflow due to the
recursion in use.
These patches target the latter.
I think this approach is good enough to deal with the crashes as it
tracks a packets recursive calls (through act_mirred) on the network
stack.
If the packet is scheduled and releases the CPU then the counter is reset.
The packet may still loop but it will not cause stack overflows.
> Not to put more work for you, but one suggestion is to use skb metadata
> approach which is performance unfriendly (could be argued to more
> correct).
>
> Also: Please consider submitting a test case or two for tdc.
>
> cheers,
> jamal
>
^ permalink raw reply
* Re: [PATCH 03/11] xdp: add offset param to zero_copy_allocator
From: Laatz, Kevin @ 2019-06-25 13:14 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, ast, daniel, bjorn.topel, magnus.karlsson, bpf,
intel-wired-lan, bruce.richardson, ciara.loftus
In-Reply-To: <20190624122342.26c6a9b4@cakuba.netronome.com>
On 24/06/2019 20:23, Jakub Kicinski wrote:
> On Thu, 20 Jun 2019 09:09:50 +0000, Kevin Laatz wrote:
>> diff --git a/include/net/xdp.h b/include/net/xdp.h
>> index 0f25b3675c5c..ea801fd2bf98 100644
>> --- a/include/net/xdp.h
>> +++ b/include/net/xdp.h
>> @@ -53,7 +53,8 @@ struct xdp_mem_info {
>> struct page_pool;
>>
>> struct zero_copy_allocator {
>> - void (*free)(struct zero_copy_allocator *zca, unsigned long
>> handle);
>> + void (*free)(struct zero_copy_allocator *zca, unsigned long handle,
>> + off_t off);
>> };
>
> Please run checkpatch --strict on all your changes. The code
> formatting is incorrect in many ways in this series.
>
Thanks, will fix in the v2.
> Please include performance measurements proving the slow down
> is negligible in the cover letter.
>
Good suggestion. Will add to the cover letter in the v2!
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Laatz, Kevin @ 2019-06-25 13:12 UTC (permalink / raw)
To: Björn Töpel
Cc: Netdev, Alexei Starovoitov, Daniel Borkmann,
Björn Töpel, Karlsson, Magnus, bpf, intel-wired-lan,
Bruce Richardson, ciara.loftus
In-Reply-To: <CAJ+HfNijp8BgMgiOuohiuqDPz+spAutdG34gUqKzepYo2noE-w@mail.gmail.com>
On 24/06/2019 16:38, Björn Töpel wrote:
> On Thu, 20 Jun 2019 at 18:55, Kevin Laatz <kevin.laatz@intel.com> wrote:
>>
>> This patchset adds the ability to use unaligned chunks in the XDP umem.
>>
>> Currently, all chunk addresses passed to the umem are masked to be chunk
>> size aligned (default is 2k, max is PAGE_SIZE). This limits where we can
>> place chunks within the umem as well as limiting the packet sizes
>> that are
>> supported.
>>
>> The changes in this patchset removes these restrictions, allowing XDP
>> to be
>> more flexible in where it can place a chunk within a umem. By
>> relaxing where
>> the chunks can be placed, it allows us to use an arbitrary buffer
>> size and
>> place that wherever we have a free address in the umem. These changes
>> add the
>> ability to support jumboframes and make it easy to integrate with other
>> existing frameworks that have their own memory management systems,
>> such as
>> DPDK.
>>
>
> Thanks for working on this, Kevin and Ciara!
>
> I have some minor comments on the series, but in general I think it's
> in good shape!
>
> For some reason the series was submitted twice (at least on my side)?
Apologies for the confusion... The first set had a typo in the bpf
mailing list address (.com vs .org). Will fix for the v2.
>
>
> Thanks,
> Björn
Thanks for reviewing. Will address your comments in the v2.
>
>> Structure of the patchset:
>> Patch 1:
>> - Remove unnecessary masking and headroom addition during zero-copy Rx
>> buffer recycling in i40e. This change is required in order for the
>> buffer recycling to work in the unaligned chunk mode.
>>
>> Patch 2:
>> - Remove unnecessary masking and headroom addition during
>> zero-copy Rx buffer recycling in ixgbe. This change is required in
>> order for the buffer recycling to work in the unaligned chunk mode.
>>
>> Patch 3:
>> - Adds an offset parameter to zero_copy_allocator. This change will
>> enable us to calculate the original handle in zca_free. This will be
>> required for unaligned chunk mode since we can't easily mask back to
>> the original handle.
>>
>> Patch 4:
>> - Adds the offset parameter to i40e_zca_free. This change is needed
>> for
>> calculating the handle since we can't easily mask back to the
>> original
>> handle like we can in the aligned case.
>>
>> Patch 5:
>> - Adds the offset parameter to ixgbe_zca_free. This change is
>> needed for
>> calculating the handle since we can't easily mask back to the
>> original
>> handle like we can in the aligned case.
>>
>>
>> Patch 6:
>> - Add infrastructure for unaligned chunks. Since we are dealing
>> with unaligned chunks that could potentially cross a physical page
>> boundary, we add checks to keep track of that information. We can
>> later use this information to correctly handle buffers that are
>> placed at an address where they cross a page boundary.
>>
>> Patch 7:
>> - Add flags for umem configuration to libbpf
>>
>> Patch 8:
>> - Modify xdpsock application to add a command line option for
>> unaligned chunks
>>
>> Patch 9:
>> - Addition of command line argument to pass in a desired buffer size
>> and buffer recycling for unaligned mode. Passing in a buffer size
>> will
>> allow the application to use unaligned chunks with the unaligned
>> chunk
>> mode. Since we are now using unaligned chunks, we need to recycle
>> our
>> buffers in a slightly different way.
>>
>> Patch 10:
>> - Adds hugepage support to the xdpsock application
>>
>> Patch 11:
>> - Documentation update to include the unaligned chunk scenario. We
>> need
>> to explicitly state that the incoming addresses are only masked
>> in the
>> aligned chunk mode and not the unaligned chunk mode.
>>
>> Kevin Laatz (11):
>> i40e: simplify Rx buffer recycle
>> ixgbe: simplify Rx buffer recycle
>> xdp: add offset param to zero_copy_allocator
>> i40e: add offset to zca_free
>> ixgbe: add offset to zca_free
>> xsk: add support to allow unaligned chunk placement
>> libbpf: add flags to umem config
>> samples/bpf: add unaligned chunks mode support to xdpsock
>> samples/bpf: add buffer recycling for unaligned chunks to xdpsock
>> samples/bpf: use hugepages in xdpsock app
>> doc/af_xdp: include unaligned chunk case
>>
>> Documentation/networking/af_xdp.rst | 10 +-
>> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 21 ++--
>> drivers/net/ethernet/intel/i40e/i40e_xsk.h | 3 +-
>> .../ethernet/intel/ixgbe/ixgbe_txrx_common.h | 3 +-
>> drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 21 ++--
>> include/net/xdp.h | 3 +-
>> include/net/xdp_sock.h | 2 +
>> include/uapi/linux/if_xdp.h | 4 +
>> net/core/xdp.c | 11 ++-
>> net/xdp/xdp_umem.c | 17 ++--
>> net/xdp/xsk.c | 60 +++++++++--
>> net/xdp/xsk_queue.h | 60 +++++++++--
>> samples/bpf/xdpsock_user.c | 99 ++++++++++++++-----
>> tools/include/uapi/linux/if_xdp.h | 4 +
>> tools/lib/bpf/xsk.c | 7 ++
>> tools/lib/bpf/xsk.h | 2 +
>> 16 files changed, 241 insertions(+), 86 deletions(-)
>>
>> --
>> 2.17.1
>>
>
^ permalink raw reply
* RE: [PATCH net-next] net: stmmac: Fix the case when PHY handle is not present
From: Jose Abreu @ 2019-06-25 13:11 UTC (permalink / raw)
To: Jose Abreu, linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
Katsuhiro Suzuki
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue
In-Reply-To: <351cce38d1c572d8b171044f2856c7fae9f89cbc.1561450696.git.joabreu@synopsys.com>
++ Katsuhiro
From: Jose Abreu <joabreu@synopsys.com>
> Some DT bindings do not have the PHY handle. Let's fallback to manually
> discovery in case phylink_of_phy_connect() fails.
>
> Reported-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
> Fixes: 74371272f97f ("net: stmmac: Convert to phylink and remove phylib logic")
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
> Cc: Joao Pinto <jpinto@synopsys.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> ---
> Hello Katsuhiro,
>
> Can you please test this patch ?
> ---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index a48751989fa6..f4593d2d9d20 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -950,9 +950,12 @@ static int stmmac_init_phy(struct net_device *dev)
>
> node = priv->plat->phylink_node;
>
> - if (node) {
> + if (node)
> ret = phylink_of_phy_connect(priv->phylink, node, 0);
> - } else {
> +
> + /* Some DT bindings do not set-up the PHY handle. Let's try to
> + * manually parse it */
> + if (!node || ret) {
> int addr = priv->plat->phy_addr;
> struct phy_device *phydev;
>
> --
> 2.7.4
^ permalink raw reply
* Warnings generated from tcp_sacktag_write_queue.
From: Chinmay Agarwal @ 2019-06-25 13:07 UTC (permalink / raw)
To: netdev; +Cc: sharathv, kapandey
Dear All,
We are hitting the following WARN_ON condition:
WARN_ON((int)tcp_packets_in_flight(tp) < 0);
tcp_packets_in_flight = packets_out –( lost_out +
sacked_out ) + retrans_out (This value is coming -ve)
The tcp socket being used is in fin_wait_1 state.
The values for variables just before the crash:
packets_out = 0,
retrans_out = 28,
lost_out = 38,
sacked_out = 8
The only place I can find the packets_out value being set as 0 is:
void tcp_write_queue_purge(struct sock *sk)
{
...
tcp_sk(sk)->packets_out = 0;
inet_csk(sk)->icsk_backoff = 0;
}
Is there some code flow where packets_out can be set to 0 and other
values can remain unchanged?
If not, is there some scenario which may lead to "tcp_write_queue_purge"
called and not followed up by "tcp_clear_retrans"?
According to my understanding we should call "tcp_clear_retrans" after
setting packets_out to 0.
[ 1950.556150] Call trace:
[ 1950.558689] tcp_sacktag_write_queue+0x704/0x72c
[ 1950.561313] init: Untracked pid 10745 exited with status 0
[ 1950.563441] tcp_ack+0x3a4/0xd40
[ 1950.563447] tcp_rcv_state_process+0x1e8/0xbbc
[ 1950.563457] tcp_v4_do_rcv+0x18c/0x1cc
[ 1950.563461] tcp_v4_rcv+0x84c/0x8a8
[ 1950.563471] ip_protocol_deliver_rcu+0xdc/0x190
[ 1950.563474] ip_local_deliver_finish+0x64/0x80
[ 1950.563479] ip_local_deliver+0xc4/0xf8
[ 1950.563482] ip_rcv_finish+0x214/0x2e0
[ 1950.563486] ip_rcv+0x2fc/0x39c
[ 1950.563496] __netif_receive_skb_core+0x698/0x84c
[ 1950.563499] __netif_receive_skb+0x3c/0x7c
[ 1950.563503] process_backlog+0x98/0x148
[ 1950.563506] net_rx_action+0x128/0x388
[ 1950.563519] __do_softirq+0x20c/0x3f0
[ 1950.563528] irq_exit+0x9c/0xa8
[ 1950.563536] handle_IPI+0x174/0x278
[ 1950.563540] gic_handle_irq+0x124/0x1c0
[ 1950.563544] el1_irq+0xb4/0x12c
[ 1950.563556] lpm_cpuidle_enter+0x3f4/0x430
[ 1950.563561] cpuidle_enter_state+0x124/0x25c
[ 1950.563565] cpuidle_enter+0x30/0x40
[ 1950.563575] call_cpuidle+0x3c/0x60
[ 1950.563579] do_idle+0x190/0x228
[ 1950.563583] cpu_startup_entry+0x24/0x28
[ 1950.563588] secondary_start_kernel+0x12c/0x138
Thanks and Regards
Chinmay Agarwal
^ permalink raw reply
* Re: iwl_mvm_add_new_dqa_stream_wk BUG in lib/list_debug.c:56
From: Marc Haber @ 2019-06-25 13:03 UTC (permalink / raw)
To: linux-kernel, linux-wireless, netdev
In-Reply-To: <20190602134842.GC3249@torres.zugschlus.de>
On Sun, Jun 02, 2019 at 03:48:42PM +0200, Marc Haber wrote:
> On Thu, May 30, 2019 at 10:12:57AM +0200, Marc Haber wrote:
> > on my primary notebook, a Lenovo X260, with an Intel Wireless 8260
> > (8086:24f3), running Debian unstable, I have started to see network
> > hangs since upgrading to kernel 5.1. In this situation, I cannot
> > restart Network-Manager (the call just hangs), I can log out of X, but
> > the system does not cleanly shut down and I need to Magic SysRq myself
> > out of the running system. This happens about once every two days.
>
> The issue is also present in 5.1.5 and 5.1.6.
Almost a month later, 5.1.15 still crashes about twice a day on my
Notebook. The error message seems pretty clear to me, how can I go on
from there and may be identify a line number outside of a library?
Greetings
Marc
--
-----------------------------------------------------------------------------
Marc Haber | "I don't trust Computers. They | Mailadresse im Header
Leimen, Germany | lose things." Winona Ryder | Fon: *49 6224 1600402
Nordisch by Nature | How to make an American Quilt | Fax: *49 6224 1600421
^ permalink raw reply
* Re: [EXT] [PATCH V2] bnx2x: Prevent ptp_task to be rescheduled indefinitely
From: Guilherme Piccoli @ 2019-06-25 12:55 UTC (permalink / raw)
To: Sudarsana Reddy Kalluru
Cc: GR-everest-linux-l2, netdev@vger.kernel.org, Ariel Elior,
jay.vosburgh@canonical.com
In-Reply-To: <MN2PR18MB2528BCB89AC93EB791446BABD3E30@MN2PR18MB2528.namprd18.prod.outlook.com>
On Tue, Jun 25, 2019 at 1:02 AM Sudarsana Reddy Kalluru
<skalluru@marvell.com> wrote:
>
> Thanks for your changes and time on this. In general time-latching happens in couple or more milliseconds (even in some 100s of usec) under the normal traffic conditions. With this approach, there's a possibility that every packet has to wait for atleast 50ms for the timestamping. This in turn affects the wait-queue (of packets to be timestamped) at hardware as next TS recording happens only after the register is freed/read. And also, it incurs some latency for the ptp applications.
>
> PTP thread is consuming time may be due to the debug messages in this error path, which you are planning address already (thanks!!).
> "Also, I've dropped the PTP "outstanding, etc" messages to debug-level, they're quite flooding my log.
> Do you see cpu hog even after removing this message? In such case we may need to think of other alternatives such as sleep for 1 ms.
> Just for the info, the approach continuous-poll-for-timestamp() is used ixgbe driver (ixgbe_ptp_tx_hwtstamp_work()) as well.
>
Thanks again for the good insights Sudarsana! I'll do some experiments
dropping all messages and checking
if the ptp thread is still consuming a lot of CPU (I believe so). In
this case, I'll rework the approach by starting
the delays in 1ms to avoid impacting the HW wait-queue and causing
delays in ptp applications.
Cheers,
Guilherme
^ permalink raw reply
* [PATCH] can: mcp251x: add error check when wq alloc failed
From: Weitao Hou @ 2019-06-25 12:50 UTC (permalink / raw)
To: wg, mkl, davem, gregkh, allison, houweitaoo, tglx, sean
Cc: linux-can, netdev, linux-kernel
add error check when workqueue alloc failed, and remove
redundant code to make it clear
Signed-off-by: Weitao Hou <houweitaoo@gmail.com>
---
drivers/net/can/spi/mcp251x.c | 49 ++++++++++++++++-------------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 44e99e3d7134..2aec934fab0c 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -664,17 +664,6 @@ static int mcp251x_power_enable(struct regulator *reg, int enable)
return regulator_disable(reg);
}
-static void mcp251x_open_clean(struct net_device *net)
-{
- struct mcp251x_priv *priv = netdev_priv(net);
- struct spi_device *spi = priv->spi;
-
- free_irq(spi->irq, priv);
- mcp251x_hw_sleep(spi);
- mcp251x_power_enable(priv->transceiver, 0);
- close_candev(net);
-}
-
static int mcp251x_stop(struct net_device *net)
{
struct mcp251x_priv *priv = netdev_priv(net);
@@ -940,37 +929,43 @@ static int mcp251x_open(struct net_device *net)
flags | IRQF_ONESHOT, DEVICE_NAME, priv);
if (ret) {
dev_err(&spi->dev, "failed to acquire irq %d\n", spi->irq);
- mcp251x_power_enable(priv->transceiver, 0);
- close_candev(net);
- goto open_unlock;
+ goto out_close;
}
priv->wq = alloc_workqueue("mcp251x_wq", WQ_FREEZABLE | WQ_MEM_RECLAIM,
0);
+ if (!priv->wq) {
+ ret = -ENOMEM;
+ goto out_clean;
+ }
INIT_WORK(&priv->tx_work, mcp251x_tx_work_handler);
INIT_WORK(&priv->restart_work, mcp251x_restart_work_handler);
ret = mcp251x_hw_reset(spi);
- if (ret) {
- mcp251x_open_clean(net);
- goto open_unlock;
- }
+ if (ret)
+ goto out_free_wq;
ret = mcp251x_setup(net, spi);
- if (ret) {
- mcp251x_open_clean(net);
- goto open_unlock;
- }
+ if (ret)
+ goto out_free_wq;
ret = mcp251x_set_normal_mode(spi);
- if (ret) {
- mcp251x_open_clean(net);
- goto open_unlock;
- }
+ if (ret)
+ goto out_free_wq;
can_led_event(net, CAN_LED_EVENT_OPEN);
netif_wake_queue(net);
+ mutex_unlock(&priv->mcp_lock);
-open_unlock:
+ return 0;
+
+out_free_wq:
+ destroy_workqueue(priv->wq);
+out_clean:
+ free_irq(spi->irq, priv);
+ mcp251x_hw_sleep(spi);
+out_close:
+ mcp251x_power_enable(priv->transceiver, 0);
+ close_candev(net);
mutex_unlock(&priv->mcp_lock);
return ret;
}
--
2.18.0
^ permalink raw reply related
* Re: memory leak in sctp_get_port_local
From: Xin Long @ 2019-06-25 12:49 UTC (permalink / raw)
To: syzbot
Cc: davem, LKML, linux-sctp, Marcelo Ricardo Leitner, network dev,
Neil Horman, syzkaller-bugs, Vlad Yasevich
In-Reply-To: <00000000000069c3140589f6d3b7@google.com>
On Wed, May 29, 2019 at 2:28 AM syzbot
<syzbot+079bf326b38072f849d9@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: cd6c84d8 Linux 5.2-rc2
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=101a184aa00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=64479170dcaf0e11
> dashboard link: https://syzkaller.appspot.com/bug?extid=079bf326b38072f849d9
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=13b5dbbca00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1038444aa00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+079bf326b38072f849d9@syzkaller.appspotmail.com
>
> : Permanently added '10.128.0.127' (ECDSA) to the list of known hosts.
> executing program
> executing program
> BUG: memory leak
> unreferenced object 0xffff8881288ca380 (size 64):
> comm "softirq", pid 0, jiffies 4294944468 (age 13.410s)
> hex dump (first 32 bytes):
> 21 4e 00 00 00 00 00 00 00 00 00 00 00 00 00 00 !N..............
> 28 ae 85 23 81 88 ff ff 00 00 00 00 00 00 00 00 (..#............
> backtrace:
> [<0000000054ece54d>] kmemleak_alloc_recursive
> include/linux/kmemleak.h:55 [inline]
> [<0000000054ece54d>] slab_post_alloc_hook mm/slab.h:439 [inline]
> [<0000000054ece54d>] slab_alloc mm/slab.c:3326 [inline]
> [<0000000054ece54d>] kmem_cache_alloc+0x134/0x270 mm/slab.c:3488
> [<00000000d992ea84>] sctp_bucket_create net/sctp/socket.c:8395 [inline]
> [<00000000d992ea84>] sctp_get_port_local+0x189/0x5b0
> net/sctp/socket.c:8142
> [<0000000099206d90>] sctp_do_bind+0xcc/0x1e0 net/sctp/socket.c:402
> [<00000000b8795757>] sctp_bind+0x44/0x70 net/sctp/socket.c:302
> [<00000000672a44aa>] inet6_bind+0x40/0xb7 net/ipv6/af_inet6.c:445
> [<0000000001400e1c>] __sys_bind+0x11c/0x140 net/socket.c:1659
> [<00000000e69e8036>] __do_sys_bind net/socket.c:1670 [inline]
> [<00000000e69e8036>] __se_sys_bind net/socket.c:1668 [inline]
> [<00000000e69e8036>] __x64_sys_bind+0x1e/0x30 net/socket.c:1668
> [<000000001644bb1f>] do_syscall_64+0x76/0x1a0
> arch/x86/entry/common.c:301
> [<00000000199a1ea2>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
will post a fix for this:
@@ -4816,6 +4816,7 @@ static int sctp_setsockopt(struct sock *sk, int
level, int optname,
static int sctp_connect(struct sock *sk, struct sockaddr *addr,
int addr_len, int flags)
{
+ struct sctp_bind_addr *bp = &sctp_sk(sk)->ep->base.bind_addr;
struct inet_sock *inet = inet_sk(sk);
struct sctp_af *af;
int err = 0;
@@ -4826,12 +4827,13 @@ static int sctp_connect(struct sock *sk,
struct sockaddr *addr,
addr, addr_len);
/* We may need to bind the socket. */
- if (!inet->inet_num) {
+ if (!bp->port) {
if (sk->sk_prot->get_port(sk, 0)) {
release_sock(sk);
return -EAGAIN;
}
inet->inet_sport = htons(inet->inet_num);
+ bp->port = inet->inet_sport;
}
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> syzbot can test patches for this bug, for details see:
> https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [PATCH V3 07/10] net: dsa: microchip: Initial SPI regmap support
From: Vladimir Oltean @ 2019-06-25 12:40 UTC (permalink / raw)
To: Marek Vasut
Cc: netdev, Andrew Lunn, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <f1dfe749-4bfa-17f7-ede7-f9bc38afa0d4@denx.de>
On Tue, 25 Jun 2019 at 15:06, Marek Vasut <marex@denx.de> wrote:
>
> On 6/25/19 1:59 AM, Vladimir Oltean wrote:
> > On Tue, 25 Jun 2019 at 01:17, Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 6/24/19 12:35 AM, Marek Vasut wrote:
> >>> Add basic SPI regmap support into the driver.
> >>>
> >>> Previous patches unconver that ksz_spi_write() is always ever called
> >>> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN)
> >>> check and we can also drop the allocation of the txbuf which is part
> >>> of the driver data and wastes 256 bytes for no reason. Regmap covers
> >>> the whole thing now.
> >>>
> >>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>> Cc: Andrew Lunn <andrew@lunn.ch>
> >>> Cc: Florian Fainelli <f.fainelli@gmail.com>
> >>> Cc: Tristram Ha <Tristram.Ha@microchip.com>
> >>> Cc: Woojung Huh <Woojung.Huh@microchip.com>
> >>
> >> [...]
> >>
> >>> +#define KS_SPIOP_FLAG_MASK(opcode) \
> >>> + cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
> >>
> >> So the robot is complaining about this. I believe this is correct, as
> >> the mask should be in native endianness on the register and NOT the
> >> native endianness of the CPU.
> >>
> >> I think a cast would help here, e.g.:
> >> - cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
> >> - (__force unsigned long)cpu_to_be32((opcode) << (SPI_ADDR_SHIFT +
> >> SPI_TURNAROUND_SHIFT))
> >>
> >> Does this make sense ?
> >>
> >>> +#define KSZ_REGMAP_COMMON(width) \
> >>> + { \
> >>> + .val_bits = (width), \
> >>> + .reg_stride = (width) / 8, \
> >>> + .reg_bits = SPI_ADDR_SHIFT + SPI_ADDR_ALIGN, \
> >>> + .pad_bits = SPI_TURNAROUND_SHIFT, \
> >>> + .max_register = BIT(SPI_ADDR_SHIFT) - 1, \
> >>> + .cache_type = REGCACHE_NONE, \
> >>> + .read_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_RD), \
> >>> + .write_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_WR), \
> >>
> >> [...]
> >>
> >> --
> >> Best regards,
> >> Marek Vasut
> >
> > Hi Marek,
> >
> > I saw SPI buffers and endianness and got triggered :)
> > Would it make sense to take a look at CONFIG_PACKING for the KSZ9477 driver?
> > I don't know how bad the field alignment issue is on that hardware,
> > but on SJA1105 it was such a disaster that I couldn't have managed it
> > any other way.
>
> How does that help me here ? All I really need is a static constant mask
> for the register/flags , 32bit for KSZ9xxx and 16bit for KSZ87xx. I
> don't need any dynamic stuff, luckily.
>
Ok. I was thinking *instead of* regmap.
On the SJA1105 I couldn't use a spi regmap because:
* the enum regmap_endian wasn't enough to describe the hardware's bit
layout (it is big endian, but high-order and low-order 32 bit words
are swapped)
* it doesn't really expose a well-defined register map, but rather,
you're supposed to dynamically construct a TLV-type buffer and write
it starting with a fixed address.
I just thought you were facing some of these problems as well with
regmap. If not, that's perfectly fine!
> But I'm glad to see TJA1105 driver mainline :)
>
> --
> Best regards,
> Marek Vasut
Regards,
-Vladimir
^ permalink raw reply
* Re: [PATCH v3 2/2] bpf: Add selftests for bpf_perf_event_output
From: Daniel Borkmann @ 2019-06-25 12:40 UTC (permalink / raw)
To: allanzhang, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
In-Reply-To: <20190624235720.167067-3-allanzhang@google.com>
On 06/25/2019 01:57 AM, allanzhang wrote:
> Software event output is only enabled by a few prog types.
> This test is to ensure that all supported types are enbled for
Nit, typo: enbled
> bpf_perf_event_output sucessfully.
Nit, typo: sucessfully
> Signed-off-by: allanzhang <allanzhang@google.com>
For SOB, could you add proper formatted name before the email?
> ---
> tools/testing/selftests/bpf/test_verifier.c | 33 ++++++-
> .../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
> 2 files changed, 126 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c5514daf8865..901a188e1eea 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -50,7 +50,7 @@
> #define MAX_INSNS BPF_MAXINSNS
> #define MAX_TEST_INSNS 1000000
> #define MAX_FIXUPS 8
> -#define MAX_NR_MAPS 18
> +#define MAX_NR_MAPS 19
> #define MAX_TEST_RUNS 8
> #define POINTER_VALUE 0xcafe4all
> #define TEST_DATA_LEN 64
> @@ -84,6 +84,7 @@ struct bpf_test {
> int fixup_map_array_wo[MAX_FIXUPS];
> int fixup_map_array_small[MAX_FIXUPS];
> int fixup_sk_storage_map[MAX_FIXUPS];
> + int fixup_map_event_output[MAX_FIXUPS];
> const char *errstr;
> const char *errstr_unpriv;
> uint32_t retval, retval_unpriv, insn_processed;
> @@ -604,6 +605,28 @@ static int create_sk_storage_map(void)
> return fd;
> }
>
> +static int create_event_output_map(void)
> +{
> + struct bpf_create_map_attr attr = {
> + .name = "test_map",
> + .map_type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> + .key_size = 4,
> + .value_size = 4,
> + .max_entries = 1,
> + };
> + int fd, btf_fd;
> +
> + btf_fd = load_btf();
> + if (btf_fd < 0)
> + return -1;
> + attr.btf_fd = btf_fd;
> + fd = bpf_create_map_xattr(&attr);
This does not look correct, BTF for spinlock does not belong to perf event array.
> + close(attr.btf_fd);
> + if (fd < 0)
> + printf("Failed to create event_output\n");
> + return fd;
> +}
> +
> static char bpf_vlog[UINT_MAX >> 8];
>
> static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> @@ -627,6 +650,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> int *fixup_map_array_wo = test->fixup_map_array_wo;
> int *fixup_map_array_small = test->fixup_map_array_small;
> int *fixup_sk_storage_map = test->fixup_sk_storage_map;
> + int *fixup_map_event_output = test->fixup_map_event_output;
>
> if (test->fill_helper) {
> test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
> @@ -788,6 +812,13 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> fixup_sk_storage_map++;
> } while (*fixup_sk_storage_map);
> }
> + if (*fixup_map_event_output) {
> + map_fds[18] = create_event_output_map();
> + do {
> + prog[*fixup_map_event_output].imm = map_fds[18];
> + fixup_map_event_output++;
> + } while (*fixup_map_event_output);
> + }
> }
>
> static int set_admin(bool admin)
> diff --git a/tools/testing/selftests/bpf/verifier/event_output.c b/tools/testing/selftests/bpf/verifier/event_output.c
> new file mode 100644
> index 000000000000..b25eabcfaa56
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/event_output.c
> @@ -0,0 +1,94 @@
> +/* instructions used to output a skb based software event, produced
> + * from code snippet:
> +struct TMP {
> + uint64_t tmp;
> +} tt;
> +tt.tmp = 5;
> +bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> + &tt, sizeof(tt));
> +return 1;
[...]
^ permalink raw reply
* Re: [PATCH] net: phylink: further documentation clarifications
From: Andrew Lunn @ 2019-06-25 12:38 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
In-Reply-To: <E1hfi09-0007Zs-Vb@rmk-PC.armlinux.org.uk>
On Tue, Jun 25, 2019 at 10:44:33AM +0100, Russell King wrote:
> Clarify the validate() behaviour in a few cases which weren't mentioned
> in the documentation, but which are necessary for users to get the
> correct behaviour.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* DMA API usage in au1000_eth.c
From: Christoph Hellwig @ 2019-06-25 12:34 UTC (permalink / raw)
To: BjoernRiemerriemer, Matt Porter, Herbert Valerio Riedel
Cc: linux-mips, netdev, linux-kernel
Hi all,
you are the persons that have their names listed in the driver,
hope you all remember what you did 15 years ago :)
The au1000_eth driver uses the DMA API somewhat oddly. For one
it uses the DMA_ATTR_NON_CONSISTENT flag to allocate memory that
is not DMA coherent, accompanied by a comment say:
/* Allocate the data buffers
* Snooping works fine with eth on all au1xxx
*/
which suggests that it actually is DMA coherent. As far as I can
tell many amd mips platforms are DMA coherent, in which case
DMA_ATTR_NON_CONSISTENT is no-op and everything is fine here. But
it seems some are not, in which case DMA_ATTR_NON_CONSISTENT will
give us not coherent memory, in which case something would be
broken? Removing DMA_ATTR_NON_CONSISTENT would be no-op on
coherent platforms, but return an address in the cache segement
on those that are non-coherent. Do you know if the hardware
event cares?
The next issue is that it calls virt_to_phys on the return value
from dma_alloc_attrs. Why would the hardware care about the physical
address and not the DMA address (in general those are the same in
mips)?
Last but not least it stores the kernel address return from
dma_alloc_attrs in a u32 instead of a pointer, which is a little
odd and not type safe, but not otherwise dramatic.
I can prepare a patch to fix these up, but I'd like to confirm my
theory about coherent of the platform and device first.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 0/2] veth: Bulk XDP_TX
From: Daniel Borkmann @ 2019-06-25 12:29 UTC (permalink / raw)
To: Toshiaki Makita, Alexei Starovoitov, David S. Miller,
Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend
Cc: netdev, xdp-newbies, bpf, Toke Høiland-Jørgensen,
Jason Wang
In-Reply-To: <20190613093959.2796-1-toshiaki.makita1@gmail.com>
On 06/13/2019 11:39 AM, Toshiaki Makita wrote:
> This introduces bulk XDP_TX in veth.
> Improves XDP_TX performance by approximately 9%. The detailed
> explanation and performance numbers are shown in patch 2.
>
> v2:
> - Use stack for bulk queue instead of a global variable.
>
> v3:
> - Add act field to xdp_bulk_tx tracepoint to be in line with other XDP
> tracepoints.
>
> Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com>
>
> Toshiaki Makita (2):
> xdp: Add tracepoint for bulk XDP_TX
> veth: Support bulk XDP_TX
>
> drivers/net/veth.c | 60 ++++++++++++++++++++++++++++++++++++----------
> include/trace/events/xdp.h | 29 ++++++++++++++++++++++
> kernel/bpf/core.c | 1 +
> 3 files changed, 78 insertions(+), 12 deletions(-)
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net-next v2 08/12] xdp: tracking page_pool resources and safe removal
From: Ivan Khoronzhuk @ 2019-06-25 12:28 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, Toke Høiland-Jørgensen,
Tariq Toukan, toshiaki.makita1, grygorii.strashko, mcroce
In-Reply-To: <20190625115107.GB6485@khorivan>
On Tue, Jun 25, 2019 at 02:51:08PM +0300, Ivan Khoronzhuk wrote:
>On Tue, Jun 25, 2019 at 01:27:50PM +0200, Jesper Dangaard Brouer wrote:
>>On Tue, 25 Jun 2019 13:50:14 +0300
>>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>>
>>>Hi Jesper,
>>>
>>>Could you please clarify one question.
>>>
>>>On Tue, Jun 18, 2019 at 03:05:47PM +0200, Jesper Dangaard Brouer wrote:
>>>>This patch is needed before we can allow drivers to use page_pool for
>>>>DMA-mappings. Today with page_pool and XDP return API, it is possible to
>>>>remove the page_pool object (from rhashtable), while there are still
>>>>in-flight packet-pages. This is safely handled via RCU and failed lookups in
>>>>__xdp_return() fallback to call put_page(), when page_pool object is gone.
>>>>In-case page is still DMA mapped, this will result in page note getting
>>>>correctly DMA unmapped.
>>>>
>>>>To solve this, the page_pool is extended with tracking in-flight pages. And
>>>>XDP disconnect system queries page_pool and waits, via workqueue, for all
>>>>in-flight pages to be returned.
>>>>
>>>>To avoid killing performance when tracking in-flight pages, the implement
>>>>use two (unsigned) counters, that in placed on different cache-lines, and
>>>>can be used to deduct in-flight packets. This is done by mapping the
>>>>unsigned "sequence" counters onto signed Two's complement arithmetic
>>>>operations. This is e.g. used by kernel's time_after macros, described in
>>>>kernel commit 1ba3aab3033b and 5a581b367b5, and also explained in RFC1982.
>>>>
>>>>The trick is these two incrementing counters only need to be read and
>>>>compared, when checking if it's safe to free the page_pool structure. Which
>>>>will only happen when driver have disconnected RX/alloc side. Thus, on a
>>>>non-fast-path.
>>>>
>>>>It is chosen that page_pool tracking is also enabled for the non-DMA
>>>>use-case, as this can be used for statistics later.
>>>>
>>>>After this patch, using page_pool requires more strict resource "release",
>>>>e.g. via page_pool_release_page() that was introduced in this patchset, and
>>>>previous patches implement/fix this more strict requirement.
>>>>
>>>>Drivers no-longer call page_pool_destroy(). Drivers already call
>>>>xdp_rxq_info_unreg() which call xdp_rxq_info_unreg_mem_model(), which will
>>>>attempt to disconnect the mem id, and if attempt fails schedule the
>>>>disconnect for later via delayed workqueue.
>>>>
>>>>Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>>>Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>---
>>>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 3 -
>>>> include/net/page_pool.h | 41 ++++++++++---
>>>> net/core/page_pool.c | 62 +++++++++++++++-----
>>>> net/core/xdp.c | 65 +++++++++++++++++++--
>>>> 4 files changed, 136 insertions(+), 35 deletions(-)
>>>>
>>>>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>index 2f647be292b6..6c9d4d7defbc 100644
>>>>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>>>>@@ -643,9 +643,6 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
>>>> }
>>>>
>>>> xdp_rxq_info_unreg(&rq->xdp_rxq);
>>>>- if (rq->page_pool)
>>>>- page_pool_destroy(rq->page_pool);
>>>>-
>>>> mlx5_wq_destroy(&rq->wq_ctrl);
>>>> }
>>>>
>>>>diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>>>>index 754d980700df..f09b3f1994e6 100644
>>>>--- a/include/net/page_pool.h
>>>>+++ b/include/net/page_pool.h
>>>>@@ -16,14 +16,16 @@
>>>> * page_pool_alloc_pages() call. Drivers should likely use
>>>> * page_pool_dev_alloc_pages() replacing dev_alloc_pages().
>>>> *
>>>>- * If page_pool handles DMA mapping (use page->private), then API user
>>>>- * is responsible for invoking page_pool_put_page() once. In-case of
>>>>- * elevated refcnt, the DMA state is released, assuming other users of
>>>>- * the page will eventually call put_page().
>>>>+ * API keeps track of in-flight pages, in-order to let API user know
>>>>+ * when it is safe to dealloactor page_pool object. Thus, API users
>>>>+ * must make sure to call page_pool_release_page() when a page is
>>>>+ * "leaving" the page_pool. Or call page_pool_put_page() where
>>>>+ * appropiate. For maintaining correct accounting.
>>>> *
>>>>- * If no DMA mapping is done, then it can act as shim-layer that
>>>>- * fall-through to alloc_page. As no state is kept on the page, the
>>>>- * regular put_page() call is sufficient.
>>>>+ * API user must only call page_pool_put_page() once on a page, as it
>>>>+ * will either recycle the page, or in case of elevated refcnt, it
>>>>+ * will release the DMA mapping and in-flight state accounting. We
>>>>+ * hope to lift this requirement in the future.
>>>> */
>>>> #ifndef _NET_PAGE_POOL_H
>>>> #define _NET_PAGE_POOL_H
>>>>@@ -66,9 +68,10 @@ struct page_pool_params {
>>>> };
>>>>
>>>> struct page_pool {
>>>>- struct rcu_head rcu;
>>>> struct page_pool_params p;
>>>>
>>>>+ u32 pages_state_hold_cnt;
>>>>+
>>>> /*
>>>> * Data structure for allocation side
>>>> *
>>>>@@ -96,6 +99,8 @@ struct page_pool {
>>>> * TODO: Implement bulk return pages into this structure.
>>>> */
>>>> struct ptr_ring ring;
>>>>+
>>>>+ atomic_t pages_state_release_cnt;
>>>> };
>>>>
>>>> struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
>>>>@@ -109,8 +114,6 @@ static inline struct page *page_pool_dev_alloc_pages(struct page_pool *pool)
>>>>
>>>> struct page_pool *page_pool_create(const struct page_pool_params *params);
>>>>
>>>>-void page_pool_destroy(struct page_pool *pool);
>>>>-
>>>> void __page_pool_free(struct page_pool *pool);
>>>> static inline void page_pool_free(struct page_pool *pool)
>>>> {
>>>>@@ -143,6 +146,24 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
>>>> __page_pool_put_page(pool, page, true);
>>>> }
>>>>
>>>>+/* API user MUST have disconnected alloc-side (not allowed to call
>>>>+ * page_pool_alloc_pages()) before calling this. The free-side can
>>>>+ * still run concurrently, to handle in-flight packet-pages.
>>>>+ *
>>>>+ * A request to shutdown can fail (with false) if there are still
>>>>+ * in-flight packet-pages.
>>>>+ */
>>>>+bool __page_pool_request_shutdown(struct page_pool *pool);
>>>>+static inline bool page_pool_request_shutdown(struct page_pool *pool)
>>>>+{
>>>>+ /* When page_pool isn't compiled-in, net/core/xdp.c doesn't
>>>>+ * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
>>>>+ */
>>>>+#ifdef CONFIG_PAGE_POOL
>>>>+ return __page_pool_request_shutdown(pool);
>>>>+#endif
>>>>+}
>>>
>>>The free side can ran in softirq, that means fast cache recycle is accessed.
>>>And it increments not atomic pool->alloc.count.
>>>
>>>For instance While redirect, for remote interface, while .ndo_xdp_xmit the
>>>xdp_return_frame_rx_napi(xdpf) is called everywhere in error path ....
>^
>|
>
>>>
>>>In the same time, simultaneously, the work queue can try one more
>>>time to clear cash, calling __page_pool_request_shutdown()....
>>>
>>>Question, what prevents pool->alloc.count to be corrupted by race,
>>>causing to wrong array num and as result wrong page to be unmapped/put ....or
>>>even page leak. alloc.count usage is not protected,
>>>__page_pool_request_shutdown() is called not from same rx NAPI, even not from
>>>NAPI.
>>>
>>>Here, while alloc cache empty procedure in __page_pool_request_shutdown():
>>
>>You forgot to copy this comment, which explains:
>>
>> /* Empty alloc cache, assume caller made sure this is
>> * no-longer in use, and page_pool_alloc_pages() cannot be
>> * call concurrently.
>> */
>No I didn't. I'm talking about recycle, not allocation.
>To be more specific about this:
>__page_pool_recycle_direct() while remote ndev .ndo_xdp_xmit
>
>About this:
>"
>/* API user MUST have disconnected alloc-side (not allowed to call
>* page_pool_alloc_pages()) before calling this. The free-side can
>* still run concurrently, to handle in-flight packet-pages.
>"
For me it's important to know only if it means that alloc.count is
freed at first call of __mem_id_disconnect() while shutdown.
The workqueue for the rest is connected only with ring cache protected
by ring lock and not supposed that alloc.count can be changed while
workqueue tries to shutdonwn the pool.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* [PATCH net] vrf: reset rt_iif for recirculated mcast out pkts
From: Stephen Suryaputra @ 2019-06-25 10:33 UTC (permalink / raw)
To: netdev; +Cc: dsahern, Stephen Suryaputra
Multicast egress packets has skb_rtable(skb)->rt_iif set to the oif.
Depending on the socket, these packets might be recirculated back as
input and raw sockets that are opened for them are bound to the VRF. But
since skb_rtable(skb) is set and its rt_iif is non-zero, inet_iif()
function returns rt_iif instead of skb_iif (the VRF netdev). Hence, the
socket lookup fails.
Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
---
include/net/route.h | 1 +
net/ipv4/ip_output.c | 25 ++++++++++++++++++++++++-
net/ipv4/route.c | 33 +++++++++++++++++++++++++++++++++
3 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/include/net/route.h b/include/net/route.h
index 065b47754f05..55ff71ffb796 100644
--- a/include/net/route.h
+++ b/include/net/route.h
@@ -221,6 +221,7 @@ void ip_rt_get_source(u8 *src, struct sk_buff *skb, struct rtable *rt);
struct rtable *rt_dst_alloc(struct net_device *dev,
unsigned int flags, u16 type,
bool nopolicy, bool noxfrm, bool will_cache);
+struct rtable *rt_dst_clone(struct net_device *dev, struct rtable *rt);
struct in_ifaddr;
void fib_add_ifaddr(struct in_ifaddr *);
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 16f9159234a2..a5e240bad3ce 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -329,6 +329,19 @@ static int ip_mc_finish_output(struct net *net, struct sock *sk,
return dev_loopback_xmit(net, sk, skb);
}
+static void ip_mc_reset_rt_iif(struct net *net, struct rtable *rt,
+ struct sk_buff *newskb)
+{
+ struct rtable *new_rt;
+
+ new_rt = rt_dst_clone(net->loopback_dev, rt);
+ if (new_rt) {
+ new_rt->rt_iif = 0;
+ skb_dst_drop(newskb);
+ skb_dst_set(newskb, &new_rt->dst);
+ }
+}
+
int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
{
struct rtable *rt = skb_rtable(skb);
@@ -363,10 +376,20 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
#endif
) {
struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
- if (newskb)
+ if (newskb) {
+ /* Reset rt_iif so that inet_iif() will return
+ * skb->dev->ifIndex which is the VRF device for
+ * socket lookup. Setting this to VRF ifindex
+ * causes ipi_ifindex in in_pktinfo to be
+ * overwritten, see ipv4_pktinfo_prepare().
+ */
+ if (netif_is_l3_slave(dev))
+ ip_mc_reset_rt_iif(net, rt, newskb);
+
NF_HOOK(NFPROTO_IPV4, NF_INET_POST_ROUTING,
net, sk, newskb, NULL, newskb->dev,
ip_mc_finish_output);
+ }
}
/* Multicasts with ttl 0 must not go beyond the host */
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 6cb7cff22db9..8ea0735a6754 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1647,6 +1647,39 @@ struct rtable *rt_dst_alloc(struct net_device *dev,
}
EXPORT_SYMBOL(rt_dst_alloc);
+struct rtable *rt_dst_clone(struct net_device *dev, struct rtable *rt)
+{
+ struct rtable *new_rt;
+
+ new_rt = dst_alloc(&ipv4_dst_ops, dev, 1, DST_OBSOLETE_FORCE_CHK,
+ rt->dst.flags);
+
+ if (new_rt) {
+ new_rt->rt_genid = rt_genid_ipv4(dev_net(dev));
+ new_rt->rt_flags = rt->rt_flags;
+ new_rt->rt_type = rt->rt_type;
+ new_rt->rt_is_input = rt->rt_is_input;
+ new_rt->rt_iif = rt->rt_iif;
+ new_rt->rt_pmtu = rt->rt_pmtu;
+ new_rt->rt_mtu_locked = rt->rt_mtu_locked;
+ new_rt->rt_gw_family = rt->rt_gw_family;
+ if (rt->rt_gw_family == AF_INET)
+ new_rt->rt_gw4 = rt->rt_gw4;
+ else if (rt->rt_gw_family == AF_INET6)
+ new_rt->rt_gw6 = rt->rt_gw6;
+ INIT_LIST_HEAD(&new_rt->rt_uncached);
+
+ new_rt->dst.flags |= DST_HOST;
+ new_rt->dst.input = rt->dst.input;
+ new_rt->dst.output = rt->dst.output;
+ new_rt->dst.error = rt->dst.error;
+ new_rt->dst.lastuse = jiffies;
+ new_rt->dst.lwtstate = lwtstate_get(rt->dst.lwtstate);
+ }
+ return new_rt;
+}
+EXPORT_SYMBOL(rt_dst_clone);
+
/* called in rcu_read_lock() section */
int ip_mc_validate_source(struct sk_buff *skb, __be32 daddr, __be32 saddr,
u8 tos, struct net_device *dev,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Russell King - ARM Linux admin @ 2019-06-25 12:10 UTC (permalink / raw)
To: René van Dorst
Cc: sean.wang, f.fainelli, davem, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190625113158.Horde.pCaJOVUsgyhYLd5Diz5EZKI@www.vdorst.com>
On Tue, Jun 25, 2019 at 11:31:58AM +0000, René van Dorst wrote:
> > > + if (state->link || mode == MLO_AN_FIXED)
> > > + mcr |= PMCR_FORCE_LNK;
> >
> > This should be removed - state->link is not for use in mac_config.
> > Even in fixed mode, the link can be brought up/down by means of a
> > gpio, and this should be dealt with via the mac_link_* functions.
>
> Maybe I understand it wrong, but is it the intention that in
> phylink_mac_config with modes MLO_AN_FIXED and MLO_AN_PHY the MAC is always
> forces into a certain speed/mode/interface. So it never auto-negotiate because
> phylink select the best configuration for you?
You are not the only one who has recently tried to make use of
state->link in mac_config(), so I'm now preparing a set of patches
to split the current mac_config() method into two separate methods:
void (*mac_config_fixed)(struct net_device *ndev,
phy_interface_t iface, int speed, int duplex,
int pause);
void (*mac_config_inband)(struct net_device *ndev,
phy_interface_t iface, bool an_enabled,
unsigned long *advertising, int pause);
so that it's not possible to use members that should not be accessed
in various modes.
> Also the PMCR_FORCE_LNK is only set in phylink_link_up() or can I do it here
> and do nothing phylink_link_up()?
When the link comes up/down, mac_link_up() and mac_link_down() will be
called appropriately. In PHY mode, this is equivalent to phylink doing
this:
if (link_changed) {
if (phydev->link)
mac_link_up();
else
mac_link_down();
}
So the actions you would've done depending on phydev->link should be in
the mac_link_*() methods.
> Other question:
> Where does the MAC enable/disable TX and RX fits best? port_{enable,disable}?
> Or only mac_config() and port_disable?
mac_link_*().
> > > + if (state->pause || phylink_test(state->advertising, Pause))
> > > + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
> > > + if (state->pause & MLO_PAUSE_TX)
> > > + mcr |= PMCR_TX_FC_EN;
> > > + if (state->pause & MLO_PAUSE_RX)
> > > + mcr |= PMCR_RX_FC_EN;
> >
> > This is clearly wrong - if any bit in state->pause is set, then we
> > end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause
> > Pause set in the advertising mask, then both are set. This doesn't
> > seem right - are these bits setting the advertisement, or are they
> > telling the MAC to use flow control?
>
> Last one, tell the MAC to use flow control.
So the first if() statement is incorrect, and should be removed
entirely. You only want to enable the MAC to use flow control as a
result of the negotiation results.
> On the current driver both bits are set in a forced-link situation.
>
> If we always forces the MAC mode I think I always set these bits and don't
> anything with the Pause modes? Is that the right way to do it?
So what happens if your link partner (e.g. switch) does not support
flow control? What if your link partner floods such frames to all
ports? You end up transmitting flow control frames, which could be
sent to all stations on the network... seems not a good idea.
Implementing stuff properly and not taking short-cuts is always a
good idea for inter-operability.
> > > +
> > > +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> > > + unsigned long *supported,
> > > + struct phylink_link_state *state)
> > > +{
> > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > > +
> > > + switch (port) {
> > > + case 0: /* Internal phy */
> > > + case 1:
> > > + case 2:
> > > + case 3:
> > > + case 4:
> > > + if (state->interface != PHY_INTERFACE_MODE_NA &&
> > > + state->interface != PHY_INTERFACE_MODE_GMII)
> > > + goto unsupported;
> > > + break;
> > > + /* case 5: Port 5 not supported! */
> > > + case 6: /* 1st cpu port */
> > > + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> > > + state->interface != PHY_INTERFACE_MODE_TRGMII)
> >
> > PHY_INTERFACE_MODE_NA ?
>
> You mean PHY_INTERFACE_MODE_NA is missing?
Yes. Please see the updated documentation in the patch I sent this
morning "net: phylink: further documentation clarifications".
--
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] bpf: Allow bpf_skb_event_output for a few prog types
From: Daniel Borkmann @ 2019-06-25 12:06 UTC (permalink / raw)
To: allanzhang, Alexei Starovoitov, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, netdev, bpf, linux-kernel
In-Reply-To: <20190625001326.172280-3-allanzhang@google.com>
Hello,
On 06/25/2019 02:13 AM, allanzhang wrote:
> Software event output is only enabled by a few prog types right now (TC,
> LWT out, XDP, sockops). Many other skb based prog types need
> bpf_skb_event_output to produce software event.
>
> Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
>
> Test bpf code is generated from code snippet:
>
> struct TMP {
> uint64_t tmp;
> } tt;
> tt.tmp = 5;
> bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> &tt, sizeof(tt));
> return 1;
>
> the bpf assembly from llvm is:
> 0: b7 02 00 00 05 00 00 00 r2 = 5
> 1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
> 2: bf a4 00 00 00 00 00 00 r4 = r10
> 3: 07 04 00 00 f8 ff ff ff r4 += -8
> 4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
> 6: b7 03 00 00 00 00 00 00 r3 = 0
> 7: b7 05 00 00 08 00 00 00 r5 = 8
> 8: 85 00 00 00 19 00 00 00 call 25
> 9: b7 00 00 00 01 00 00 00 r0 = 1
> 10: 95 00 00 00 00 00 00 00 exit
>
> Patch 1 is enabling code.
> Patch 2 is fullly covered selftest code.
>
> Signed-off-by: allanzhang <allanzhang@google.com>
Thanks for the contribution! I'm a bit confused given the many submissions,
some are versioned in the subject (which is the correct way), but this patch
here was sent after v3 (?) but without a version. Which is the right one to
consider for review, I presume v3?
> ---
> tools/testing/selftests/bpf/test_verifier.c | 38 +++++++-
> .../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
> 2 files changed, 130 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c5514daf8865..45ce9dd4323f 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -1,10 +1,13 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> /*
> * Testsuite for eBPF verifier
> *
> * Copyright (c) 2014 PLUMgrid, http://plumgrid.com
> * Copyright (c) 2017 Facebook
> * Copyright (c) 2018 Covalent IO, Inc. http://covalent.io
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
This removal of SPDX probably slipped in as well here ..
> */
>
> #include <endian.h>
> @@ -50,7 +53,7 @@
> #define MAX_INSNS BPF_MAXINSNS
> #define MAX_TEST_INSNS 1000000
> #define MAX_FIXUPS 8
> -#define MAX_NR_MAPS 18
> +#define MAX_NR_MAPS 19
> #define MAX_TEST_RUNS 8
> #define POINTER_VALUE 0xcafe4all
> #define TEST_DATA_LEN 64
> @@ -84,6 +87,7 @@ struct bpf_test {
> int fixup_map_array_wo[MAX_FIXUPS];
> int fixup_map_array_small[MAX_FIXUPS];
> int fixup_sk_storage_map[MAX_FIXUPS];
> + int fixup_map_event_output[MAX_FIXUPS];
> const char *errstr;
> const char *errstr_unpriv;
> uint32_t retval, retval_unpriv, insn_processed;
> @@ -604,6 +608,28 @@ static int create_sk_storage_map(void)
> return fd;
> }
>
> +static int create_event_output_map(void)
> +{
> + struct bpf_create_map_attr attr = {
> + .name = "test_map",
> + .map_type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> + .key_size = 4,
> + .value_size = 4,
> + .max_entries = 1,
> + };
> + int fd, btf_fd;
> +
> + btf_fd = load_btf();
> + if (btf_fd < 0)
> + return -1;
> + attr.btf_fd = btf_fd;
> + fd = bpf_create_map_xattr(&attr);
> + close(attr.btf_fd);
> + if (fd < 0)
> + printf("Failed to create event_output\n");
> + return fd;
> +}
> +
> static char bpf_vlog[UINT_MAX >> 8];
>
> static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> @@ -627,6 +653,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> int *fixup_map_array_wo = test->fixup_map_array_wo;
> int *fixup_map_array_small = test->fixup_map_array_small;
> int *fixup_sk_storage_map = test->fixup_sk_storage_map;
> + int *fixup_map_event_output = test->fixup_map_event_output;
>
> if (test->fill_helper) {
> test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
> @@ -788,6 +815,13 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> fixup_sk_storage_map++;
> } while (*fixup_sk_storage_map);
> }
> + if (*fixup_map_event_output) {
> + map_fds[18] = create_event_output_map();
> + do {
> + prog[*fixup_map_event_output].imm = map_fds[18];
> + fixup_map_event_output++;
> + } while (*fixup_map_event_output);
> + }
> }
>
> static int set_admin(bool admin)
> diff --git a/tools/testing/selftests/bpf/verifier/event_output.c b/tools/testing/selftests/bpf/verifier/event_output.c
> new file mode 100644
> index 000000000000..b25eabcfaa56
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/event_output.c
> @@ -0,0 +1,94 @@
> +/* instructions used to output a skb based software event, produced
> + * from code snippet:
> +struct TMP {
> + uint64_t tmp;
> +} tt;
> +tt.tmp = 5;
> +bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> + &tt, sizeof(tt));
> +return 1;
> +
> +the bpf assembly from llvm is:
> + 0: b7 02 00 00 05 00 00 00 r2 = 5
> + 1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
> + 2: bf a4 00 00 00 00 00 00 r4 = r10
> + 3: 07 04 00 00 f8 ff ff ff r4 += -8
> + 4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
> + 6: b7 03 00 00 00 00 00 00 r3 = 0
> + 7: b7 05 00 00 08 00 00 00 r5 = 8
> + 8: 85 00 00 00 19 00 00 00 call 25
> + 9: b7 00 00 00 01 00 00 00 r0 = 1
> + 10: 95 00 00 00 00 00 00 00 exit
> +
> + The reason I put the code here instead of fill_helpers is that map fixup is
> + against the insns, instead of filled prog.
> +*/
> +
> +#define __PERF_EVENT_INSNS__ \
> + BPF_MOV64_IMM(BPF_REG_2, 5), \
> + BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -8), \
> + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), \
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8), \
> + BPF_LD_MAP_FD(BPF_REG_2, 0), \
> + BPF_MOV64_IMM(BPF_REG_3, 0), \
> + BPF_MOV64_IMM(BPF_REG_5, 8), \
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, \
> + BPF_FUNC_perf_event_output), \
> + BPF_MOV64_IMM(BPF_REG_0, 1), \
> + BPF_EXIT_INSN(),
> +{
> + "perfevent for sockops",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SOCK_OPS,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for tc",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for lwt out",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_LWT_OUT,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for xdp",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_XDP,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for socket filter",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SOCKET_FILTER,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for sk_skb",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SK_SKB,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for cgroup skb",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
>
^ permalink raw reply
* Re: [PATCH V3 07/10] net: dsa: microchip: Initial SPI regmap support
From: Marek Vasut @ 2019-06-25 12:06 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Andrew Lunn, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <CA+h21hpgXxnnRS7Upc1R82ajLum4k-3O9EQDROonO6jtAD+NZw@mail.gmail.com>
On 6/25/19 1:59 AM, Vladimir Oltean wrote:
> On Tue, 25 Jun 2019 at 01:17, Marek Vasut <marex@denx.de> wrote:
>>
>> On 6/24/19 12:35 AM, Marek Vasut wrote:
>>> Add basic SPI regmap support into the driver.
>>>
>>> Previous patches unconver that ksz_spi_write() is always ever called
>>> with len = 1, 2 or 4. We can thus drop the if (len > SPI_TX_BUF_LEN)
>>> check and we can also drop the allocation of the txbuf which is part
>>> of the driver data and wastes 256 bytes for no reason. Regmap covers
>>> the whole thing now.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Tristram Ha <Tristram.Ha@microchip.com>
>>> Cc: Woojung Huh <Woojung.Huh@microchip.com>
>>
>> [...]
>>
>>> +#define KS_SPIOP_FLAG_MASK(opcode) \
>>> + cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
>>
>> So the robot is complaining about this. I believe this is correct, as
>> the mask should be in native endianness on the register and NOT the
>> native endianness of the CPU.
>>
>> I think a cast would help here, e.g.:
>> - cpu_to_be32((opcode) << (SPI_ADDR_SHIFT + SPI_TURNAROUND_SHIFT))
>> - (__force unsigned long)cpu_to_be32((opcode) << (SPI_ADDR_SHIFT +
>> SPI_TURNAROUND_SHIFT))
>>
>> Does this make sense ?
>>
>>> +#define KSZ_REGMAP_COMMON(width) \
>>> + { \
>>> + .val_bits = (width), \
>>> + .reg_stride = (width) / 8, \
>>> + .reg_bits = SPI_ADDR_SHIFT + SPI_ADDR_ALIGN, \
>>> + .pad_bits = SPI_TURNAROUND_SHIFT, \
>>> + .max_register = BIT(SPI_ADDR_SHIFT) - 1, \
>>> + .cache_type = REGCACHE_NONE, \
>>> + .read_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_RD), \
>>> + .write_flag_mask = KS_SPIOP_FLAG_MASK(KS_SPIOP_WR), \
>>
>> [...]
>>
>> --
>> Best regards,
>> Marek Vasut
>
> Hi Marek,
>
> I saw SPI buffers and endianness and got triggered :)
> Would it make sense to take a look at CONFIG_PACKING for the KSZ9477 driver?
> I don't know how bad the field alignment issue is on that hardware,
> but on SJA1105 it was such a disaster that I couldn't have managed it
> any other way.
How does that help me here ? All I really need is a static constant mask
for the register/flags , 32bit for KSZ9xxx and 16bit for KSZ87xx. I
don't need any dynamic stuff, luckily.
But I'm glad to see TJA1105 driver mainline :)
--
Best regards,
Marek Vasut
^ 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