* 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:19 UTC (permalink / raw)
To: Arnd Bergmann, Alex Elder
Cc: Dan Williams, 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: <CAK8P3a1ixL9ZjYz=pWTxvMfeD89S6QxSeHt9ZCL9dkCNV5pMHQ@mail.gmail.com>
On Mon, 2019-06-24 at 18:40 +0200, Arnd Bergmann wrote:
> On Mon, Jun 24, 2019 at 6:21 PM Alex Elder <elder@linaro.org> wrote:
> > On 6/18/19 2:03 PM, Johannes Berg wrote:
> >
> > > Really there are two possible ways (and they intersect to some extent).
> > >
> > > One is the whole multi-function device, where a single WWAN device is
> > > composed of channels offered by actually different drivers, e.g. for a
> > > typical USB device you might have something like cdc_ether and the
> > > usb_wwan TTY driver. In this way, we need to "compose" the WWAN device
> > > similarly, e.g. by using the underlying USB device "struct device"
> > > pointer to tie it together.
> >
> > I *think* this model makes the most sense. But at this point
> > it would take very little to convince me otherwise... (And then
> > I saw Arnd's message advocating the other one, unfortunately...)
> >
> > > The other is something like IPA or the Intel modem driver, where the
> > > device is actually a single (e.g. PCIe) device and just has a single
> > > driver, but that single driver offers different channels.
> >
> > What I don't like about this is that it's more monolithic. It
> > seems better to have the low-level IPA or Intel modem driver (or
> > any other driver that can support communication between the AP
> > and WWAN device) present communication paths that other function-
> > specific drivers can attach to and use.
>
> I did not understand Johannes description as two competing models
> for the same code, but rather two kinds of existing hardware that
> a new driver system would have to deal with.
Right.
> I was trying to simplify it to just having the second model, by adding
> a hack to support the first, but my view was rather unpopular so
> far, so if everyone agrees on one way to do it, don't worry about me ;-)
:-)
However, to also reply to Alex: I don't know exactly how IPA works, but
for the Intel modem at least you can't fundamentally have two drivers
for different parts of the functionality, since it's just a single piece
of hardware and you need to allocate hardware resources from a common
pool etc. So you cannot split the driver into "Intel modem control
channel driver" and "Intel modem data channel driver". In fact, it's
just a single "struct device" on the PCIe bus that you can bind to, and
only one driver can bind at a time.
So, IOW, I'm not sure I see how you'd split that up. I guess you could
if you actually do something like the "rmnet" model, and I suppose
you're free to do that for IPA if you like, but I tend to think that's
actually a burden, not a win since you just get more complex code that
needs to interact with more pieces. A single driver for a single
hardware that knows about the few types of channels seems simpler to me.
> - to answer Johannes question, my understanding is that the interface
> between kernel and firmware/hardware for IPA has a single 'struct
> device' that is used for both the data and the control channels,
> rather than having a data channel and an independent control device,
> so this falls into the same category as the Intel one (please correct
> me on that)
That sounds about the same then, right.
Are the control channels to IPA are actually also tunnelled over the
rmnet protocol? And even if they are, perhaps they have a different
hardware queue or so? That'd be the case for Intel - different hardware
queue, same (or at least similar) protocol spoken for the DMA hardware
itself, but different contents of the messages obviously.
> - The user space being proprietary is exactly what we need to avoid
> with the wwan subsystem. We need to be able to use the same
> method for setting up Intel, Qualcomm, Samsung, Unisoc or
> Hisilicon modems or anything else that hooks into the subsystem,
> and support that in network manager as well as the Android
> equivalent.
> If Qualcomm wants to provide their own proprietary user space
> solution, we can't stop them, but then that should also work on
> all the others unless they intentionally break it. ;-)
:-)
I tend to think there's always going to be some level of specific
handling here, because e.g. the Intel modem can expose MBIM or AT
command control channels, but not much else (that'd be useful for us
anyway, since we don't know how to speak debug protocol etc.). Other
modems will expose *only* AT commands, or *only* MBIM, and yet others
may also offer QMI and then that might be preferable.
> > > and simply require that the channel is attached to the wwan device with
> > > the representation-specific call (wwan_attach_netdev, wwan_attach_tty,
> > > ...).
> >
> > Or maybe have the WWAN device present interfaces with attributes,
> > and have drivers that are appropriate for each interface attach
> > to only the ones they recognize they support.
>
> I think you both mean the same thing here, a structure with callback
> pointers that may or may not be filled by the driver depending on its
> capabilities.
:-)
> What we should try to avoid though is a way to add driver private
> interfaces that risk having multiple drivers create similar functionality
> in incompatible ways.
Right.
johannes
^ permalink raw reply
* Re: Warnings generated from tcp_sacktag_write_queue.
From: Eric Dumazet @ 2019-06-25 14:24 UTC (permalink / raw)
To: Chinmay Agarwal, netdev; +Cc: sharathv, kapandey
In-Reply-To: <20190625130706.GA6891@chinagar-linux.qualcomm.com>
On 6/25/19 6:07 AM, Chinmay Agarwal wrote:
> 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
>
You do not provide what exact kernel version you are using,
this is probably the most important information we need.
^ permalink raw reply
* Re: WWAN Controller Framework (was IPA [PATCH v2 00/17])
From: Johannes Berg @ 2019-06-25 14:34 UTC (permalink / raw)
To: Alex Elder, davem, arnd, bjorn.andersson, ilias.apalodimas,
Dan Williams
Cc: evgreen, benchan, ejcaruso, cpratapa, syadagir, subashab,
abhishek.esse, netdev, devicetree, linux-kernel, linux-soc,
linux-arm-kernel, linux-arm-msm
In-Reply-To: <6dae9d1c-ceae-7e88-fe61-f4cda82820ea@linaro.org>
On Mon, 2019-06-24 at 12:06 -0500, Alex Elder wrote:
> > OK I want to try to organize a little more concisely some of the
> > discussion on this, because there is a very large amount of volume
> > to date and I think we need to try to narrow the focus back down
> > again.
Sounds good to me!
> > I'm going to use a few terms here. Some of these I really don't
> > like, but I want to be unambiguous *and* (at least for now) I want
> > to avoid the very overloaded term "device".
> >
> > I have lots more to say, but let's start with a top-level picture,
> > to make sure we're all on the same page.
> >
> > WWAN Communication
> > Channel (Physical)
> > | ------------------------
> > ------------ v | :+ Control | \
> > > |-----------| :+ Data | |
> > > AP | | WWAN unit :+ Voice | > Functions
> > > |===========| :+ GPS | |
> >
> > ------------ ^ | :+ ... | /
> > | -------------------------
> > Multiplexed WWAN
> > Communication
> > Channel (Physical)
Sounds right to me. I'm not sure if you're distinguishing here between
the "Data function" and multiple data channels to the data function, but
at this point I guess it doesn't matter.
> > - The *AP* is the main CPU complex that's running Linux on one or
> > more CPU cores.
> > - A *WWAN unit* is an entity that shares one or more physical
> > *WWAN communication channels* with the AP.
> > - A *WWAN communication channel* is a bidirectional means of
> > carrying data between the AP and WWAN unit.
> > - A WWAN communication channel carries data using a *WWAN protocol*.
> > - A WWAN unit implements one or more *WWAN functions*, such as
> > 5G data, LTE voice, GPS, and so on.
> > - A WWAN unit shall implement a *WWAN control function*, used to
> > manage the use of other WWAN functions, as well as the WWAN unit
> > itself.
I think here we need to be more careful. I don't know how you want to
call it, but we actually have multiple levels of control here.
You have
* hardware control, to control how you actually use the (multiple or
not) physical communication channel(s) to the WWAN unit
* this is partially exposed to userspace via the WWAN netlink family or
something like that, so userspace can create new netdevs to tx/rx
with the "data function" and to the network; note that it could be
one or multiple
* WWAN control, which is typically userspace communicating with the
WWAN control function in the WWAN unit, but this can take different
forms (as I mentioned earlier, e.g. AT commands, MBIM, QMI)
> > - The AP communicates with a WWAN function using a WWAN protocol.
Right, that's just device specific (IPA vs. Intel vs. ...)
> > - A WWAN physical channel can be *multiplexed*, in which case it
> > carries the data for one or more *WWAN logical channels*.
This ... depends a bit on how you exactly define a physical channel
here. Is that, to you, the PCIe/USB link? In that case, yes, obviously
you have only one physical channel for each WWAN unit.
However, I'd probably see this slightly differently, because e.g. the
Intel modem has multiple DMA engines, and so you actually have multiple
DMA rings to talk to the WWAN unit, and I'd have called each DMA ring a
physical channel. And then, you just have a 1:1 from physical to logical
channel since it doesn't actually carry a multiplexing protocol.
> > - A multiplexed WWAN communication channel uses a *WWAN wultiplexing
> > protocol*, which is used to separate independent data streams
> > carrying other WWAN protocols.
Like just described, this isn't really needed and is a device-specific
property.
> > - A WWAN logical channel carries a bidirectional stream of WWAN
> > protocol data between an entity on the AP and a WWAN function.
> >
> > Does that adequately represent a very high-level picture of what
> > we're trying to manage?
Pretty much.
I only disagree slightly on the control planes (there are multiple, and
multiple options for the "Control function" one), and on the whole
notion of physical link/logical link/multiplexing which is device
specific.
> > And if I understand it right, the purpose of the generic framework
> > being discussed is to define a common mechanism for managing (i.e.,
> > discovering, creating, destroying, querying, configuring, enabling,
> > disabling, etc.) WWAN units and the functions they implement, along
> > with the communication and logical channels used to communicate with
> > them.
Well, some subset of that matrix, the framework won't actually destroy
WWAN units I hope ;-)
But yes. I'd probably captured it in layers, and say that we have a
WWAN framework layer
- discover, query, configure WWAN units
- enable, disable channels to the functions inside the WWAN units
WWAN device driver
- implement (partial) API offered by WWAN framework layer to allow
these things
(sometimes may not allow creating more control or data channels for
example, and fixed function channels are precreated, but then can
still discover data about the device and configure the channels
- implement the device-specific protocols etc. necessary to achieve
this
Userspace
- uses control function channel (e.g. TTY) to talk directly to the WWAN
unit's control function
- uses WWAN framework APIs to create/configure/... (other) function
channels
(it may be necessary to create a control channel even, before being
able to use it, since different options (AT/MBIM/QMI) may be there
- configures netdevs (data function channels) after their creation
johannes
^ permalink raw reply
* Re: [PATCH net-next] net: stmmac: Fix the case when PHY handle is not present
From: Katsuhiro Suzuki @ 2019-06-25 14:40 UTC (permalink / raw)
To: Jose Abreu, linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue
In-Reply-To: <78EB27739596EE489E55E81C33FEC33A0B9D78A2@DE02WEMBXB.internal.synopsys.com>
Hello Jose,
This patch works fine with my Tinker Board. Thanks a lot!
Tested-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
BTW, from network guys point of view, is it better to add a phy node
into device trees that have no phy node such as the Tinker Board?
Best Regards,
Katsuhiro Suzuki
On 2019/06/25 22:11, Jose Abreu wrote:
> ++ 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
* Re: [PATCH iproute2] testsuite: don't clobber /tmp
From: Matteo Croce @ 2019-06-25 14:39 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev, David Ahern
In-Reply-To: <CAGnkfhyS64WA+947iQFwA9+=yS6Zk856SWBR9Zy7w90xhBmC=Q@mail.gmail.com>
On Thu, Jun 13, 2019 at 7:15 PM Matteo Croce <mcroce@redhat.com> wrote:
>
> On Wed, Jun 12, 2019 at 8:20 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Wed, 12 Jun 2019 19:32:29 +0200
> > Matteo Croce <mcroce@redhat.com> wrote:
> >
> > > On Wed, Jun 12, 2019 at 6:04 PM Matteo Croce <mcroce@redhat.com> wrote:
> > > >
> > > > On Wed, Jun 12, 2019 at 5:55 PM Stephen Hemminger
> > > > <stephen@networkplumber.org> wrote:
> > > > >
> > > > > On Tue, 11 Jun 2019 20:03:26 +0200
> > > > > Matteo Croce <mcroce@redhat.com> wrote:
> > > > >
> > > > > > Even if not running the testsuite, every build will leave
> > > > > > a stale tc_testkenv.* file in the system temp directory.
> > > > > > Conditionally create the temp file only if we're running the testsuite.
> > > > > >
> > > > > > Signed-off-by: Matteo Croce <mcroce@redhat.com>
> > > > > > ---
> > > > > > testsuite/Makefile | 5 ++++-
> > > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/testsuite/Makefile b/testsuite/Makefile
> > > > > > index 7f247bbc..5353244b 100644
> > > > > > --- a/testsuite/Makefile
> > > > > > +++ b/testsuite/Makefile
> > > > > > @@ -14,7 +14,9 @@ TESTS_DIR := $(dir $(TESTS))
> > > > > >
> > > > > > IPVERS := $(filter-out iproute2/Makefile,$(wildcard iproute2/*))
> > > > > >
> > > > > > -KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > > > > +ifeq ($(MAKECMDGOALS),alltests)
> > > > > > + KENVFN := $(shell mktemp /tmp/tc_testkenv.XXXXXX)
> > > > > > +endif
> > > > > > ifneq (,$(wildcard /proc/config.gz))
> > > > > > KCPATH := /proc/config.gz
> > > > > > else
> > > > > > @@ -94,3 +96,4 @@ endif
> > > > > > rm "$$TMP_ERR" "$$TMP_OUT"; \
> > > > > > sudo dmesg > $(RESULTS_DIR)/$@.$$o.dmesg; \
> > > > > > done
> > > > > > + @$(RM) $(KENVFN)
> > > > >
> > > > > My concern is that there are several targets in this one Makefile.
> > > > >
> > > > > Why not use -u which gives name but does not create the file?
> > > >
> > > > As the manpage says, this is unsafe, as a file with the same name can
> > > > be created in the meantime.
> > > > Another option is to run the mktemp in the target shell, but this will
> > > > require to escape every single end of line to make it a single shell
> > > > command, e.g.:
> > > >
> > > > KENVFN=$$(mktemp /tmp/tc_testkenv.XXXXXX); \
> > > > if [ "$(KCPATH)" = "/proc/config.gz" ]; then \
> > > > gunzip -c $(KCPATH) >$$KENVFN; \
> > > > ...
> > > > done ; \
> > > > $(RM) $$KENVFN
> > > >
> > > > --
> > > > Matteo Croce
> > > > per aspera ad upstream
> > >
> > > Anyway, looking for "tc" instead of "alltests" is probably better, as
> > > it only runs mktemp when at least the tc test is selected, both
> > > manually or via make check from topdir, eg.g
> > >
> > > ifeq ($(MAKECMDGOALS),tc)
> > >
> > > Do you agree?
> >
> > Why use /tmp at all for this config file?
>
> To me any path could work, both /tmp or in the current dir, I have no
> preference.
> The important thing is to remove them wherever they are, as clobbering
> the build dir is bad as messing /tmp.
>
> Anyway, I double checked, and the only target which uses that
> temporary file is 'alltests' so, if the path is ok, I think that the
> condition "ifeq ($(MAKECMDGOALS),alltests)" is the only one which
> fixes the issue and keeps the behaviour unaltered.
> I did some quick tests and it works for me.
>
> Bye,
> --
> Matteo Croce
> per aspera ad upstream
Hi,
any more thoughts about this patch?
--
Matteo Croce
per aspera ad upstream
^ permalink raw reply
* Re: [PATCH rdma-next v1 11/12] IB/mlx5: Implement DEVX dispatching event
From: Yishai Hadas @ 2019-06-25 14:41 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <20190624180558.GL7418@mellanox.com>
On 6/24/2019 9:06 PM, Jason Gunthorpe wrote:
> On Mon, Jun 24, 2019 at 07:55:32PM +0300, Yishai Hadas wrote:
>
>>>> + /* Explicit filtering to kernel events which may occur frequently */
>>>> + if (event_type == MLX5_EVENT_TYPE_CMD ||
>>>> + event_type == MLX5_EVENT_TYPE_PAGE_REQUEST)
>>>> + return NOTIFY_OK;
>>>> +
>>>> + table = container_of(nb, struct mlx5_devx_event_table, devx_nb.nb);
>>>> + dev = container_of(table, struct mlx5_ib_dev, devx_event_table);
>>>> + is_unaffiliated = is_unaffiliated_event(dev->mdev, event_type);
>>>> +
>>>> + if (!is_unaffiliated)
>>>> + obj_type = get_event_obj_type(event_type, data);
>>>> + event = xa_load(&table->event_xa, event_type | (obj_type << 16));
>>>> + if (!event)
>>>> + return NOTIFY_DONE;
>>>
>>> event should be in the rcu as well
>>
>> Do we really need this ? I didn't see a flow that really requires
>> that.
>
> I think there are no frees left? Even so it makes much more sense to
> include the event in the rcu as if we ever did need to kfree it would
> have to be via rcu
>
OK
>>>> + while (list_empty(&ev_queue->event_list)) {
>>>> + spin_unlock_irq(&ev_queue->lock);
>>>> +
>>>> + if (filp->f_flags & O_NONBLOCK)
>>>> + return -EAGAIN;
>>>> +
>>>> + if (wait_event_interruptible(ev_queue->poll_wait,
>>>> + (!list_empty(&ev_queue->event_list) ||
>>>> + ev_queue->is_destroyed))) {
>>>> + return -ERESTARTSYS;
>>>> + }
>>>> +
>>>> + if (list_empty(&ev_queue->event_list) &&
>>>> + ev_queue->is_destroyed)
>>>> + return -EIO;
>>>
>>> All these tests should be under the lock.
>>
>> We can't call wait_event_interruptible() above which may sleep under the
>> lock, correct ? are you referring to the list_empty() and
>> is_destroyed ?
>
> yes
>
>> By the way looking in uverb code [1], similar code which is not done under
>> the lock as of here..
>>
>> [1] https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/uverbs_main.c#L244
>
> Also not a good idea
>
>>> Why don't we return EIO as soon as is-destroyed happens? What is the
>>> point of flushing out the accumulated events?
>>
>> It follows the above uverb code/logic that returns existing events even in
>> that case, also the async command events in this file follows that logic, I
>> suggest to stay consistent.
>
> Don't follow broken uverbs stuff...
May it be that there is some event that we still want to deliver post
unbind/hot-unplug ? for example IB_EVENT_DEVICE_FATAL in uverbs and
others from the driver code.
Not sure that we want to change this logic.
What do you think ?
>
>>> Maybe the event should be re-added on error? Tricky.
>>
>> What will happen if another copy_to_user may then fail again (loop ?) ...
>> not sure that we want to get into this tricky handling ...
>>
>> As of above, It follows the logic from uverbs at that area.
>> https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/uverbs_main.c#L267
>
> again it is wrong...
>
> There is no loop if you just stick the item back on the head of the
> list and exit, which is probably the right thing to do..
>
What if copy_to_user() will fail again just later on ? we might end-up
with loop of read(s) that always find an event as it was put back.
I suggest to leave this flow as it's now, at least for this series
submission.
Agree ?
>>>> @@ -2374,6 +2705,17 @@ static int devx_hot_unplug_async_cmd_event_file(struct ib_uobject *uobj,
>>>> static int devx_hot_unplug_async_event_file(struct ib_uobject *uobj,
>>>> enum rdma_remove_reason why)
>>>> {
>>>> + struct devx_async_event_file *ev_file =
>>>> + container_of(uobj, struct devx_async_event_file,
>>>> + uobj);
>>>> + struct devx_async_event_queue *ev_queue = &ev_file->ev_queue;
>>>> +
>>>> + spin_lock_irq(&ev_queue->lock);
>>>> + ev_queue->is_destroyed = 1;
>>>> + spin_unlock_irq(&ev_queue->lock);
>>>> +
>>>> + if (why == RDMA_REMOVE_DRIVER_REMOVE)
>>>> + wake_up_interruptible(&ev_queue->poll_wait);
>>>
>>> Why isn't this wakeup always done?
>>
>> Maybe you are right and this can be always done to wake up any readers as
>> the 'is_destroyed' was set.
>>
>> By the way, any idea why it was done as such in uverbs [1] for similar flow
>> ? also the command events follows that.
>
> I don't know, it is probably pointless too.
>
> If we don't need it here then we shouldn't have it.
>
> These random pointless ifs bother me as we have to spend time trying
> to figure out that they are pointless down the road.
>
OK, will drop this if.
^ permalink raw reply
* Re: [PATCH net-next] net: stmmac: Fix the case when PHY handle is not present
From: Andrew Lunn @ 2019-06-25 14:51 UTC (permalink / raw)
To: Katsuhiro Suzuki
Cc: Jose Abreu, linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue
In-Reply-To: <5859e2c5-112f-597c-3bd5-e30e96b86152@katsuster.net>
On Tue, Jun 25, 2019 at 11:40:00PM +0900, Katsuhiro Suzuki wrote:
> Hello Jose,
>
> This patch works fine with my Tinker Board. Thanks a lot!
>
> Tested-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
>
>
> BTW, from network guys point of view, is it better to add a phy node
> into device trees that have no phy node such as the Tinker Board?
Hi Katsuhiro
It makes it less ambiguous if there is a phy-handle. It is then very
clear which PHY should be used. For a development board, which people
can be tinkering around with, there is a chance they add a second PHY
to the MDIO bus, or an Ethernet switch, etc. Without explicitly
listing the PHY, it might get the wrong one. However this is generally
a problem if phy_find_first() is used. I think in this case, something
is setting priv->plat->phy_addr, so it is also clearly defined which
PHY to use.
Andrew
^ permalink raw reply
* [RFC, PATCH 0/2, net-next] net: netsec: Add XDP Support
From: Ilias Apalodimas @ 2019-06-25 15:06 UTC (permalink / raw)
To: netdev, jaswinder.singh
Cc: ard.biesheuvel, bjorn.topel, magnus.karlsson, brouer, daniel, ast,
makita.toshiaki, jakub.kicinski, john.fastabend, davem,
Ilias Apalodimas
This is a respin of https://www.spinics.net/lists/netdev/msg526066.html
Since page_pool API fixes are merged into net-next we can now safely use
it's DMA mapping capabilities.
The first patch changes the buffer allocation from netdev_alloc_frag()
to page_pool API. Although this will lead to slightly reduced performance
(on raw packet drops only) we can use the API for XDP buffer recycling.
Another side effect is a slight increase in memory usage, due to using a
single page per packet.
The second patch adds XDP support on the driver.
There's a bunch of interesting options that come up due to the single
Tx queue.
Use of locking (to avoid messing up the Tx queue since ndo_xdp_xmit
and the normal stack can co-exist) is one thing.
We also need to track down the 'buffer type' for TX and properly free or
recycle the packet depending on it's nature. Since we use page_pool API in
the XDP_TX case the buffers are already mapped for us and we only need to
sync them, while on the ndo_xdp_xmit we need to map and send them
Ilias Apalodimas (2):
net: netsec: Use page_pool API
net: netsec: add XDP support
drivers/net/ethernet/socionext/Kconfig | 1 +
drivers/net/ethernet/socionext/netsec.c | 459 ++++++++++++++++++++----
2 files changed, 394 insertions(+), 66 deletions(-)
--
2.20.1
^ permalink raw reply
* [RFC, PATCH 1/2, net-next] net: netsec: Use page_pool API
From: Ilias Apalodimas @ 2019-06-25 15:06 UTC (permalink / raw)
To: netdev, jaswinder.singh
Cc: ard.biesheuvel, bjorn.topel, magnus.karlsson, brouer, daniel, ast,
makita.toshiaki, jakub.kicinski, john.fastabend, davem,
Ilias Apalodimas
In-Reply-To: <1561475179-7686-1-git-send-email-ilias.apalodimas@linaro.org>
Use page_pool and it's DMA mapping capabilities for Rx buffers instead
of netdev/napi_alloc_frag()
Although this will result in a slight performance penalty on small sized
packets (~10%) the use of the API will allow to easily add XDP support.
The penalty won't be visible in network testing i.e ipef/netperf etc, it
only happens during raw packet drops.
Furthermore we intend to add recycling capabilities on the API
in the future. Once the recycling is added the performance penalty will
go away.
The only 'real' penalty is the slightly increased memory usage, since we
now allocate a page per packet instead of the amount of bytes we need +
skb metadata (difference is roughly 2kb per packet).
With a minimum of 4BG of RAM on the only SoC that has this NIC the
extra memory usage is negligible (a bit more on 64K pages)
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
drivers/net/ethernet/socionext/Kconfig | 1 +
drivers/net/ethernet/socionext/netsec.c | 121 +++++++++++++++---------
2 files changed, 75 insertions(+), 47 deletions(-)
diff --git a/drivers/net/ethernet/socionext/Kconfig b/drivers/net/ethernet/socionext/Kconfig
index 25f18be27423..95e99baf3f45 100644
--- a/drivers/net/ethernet/socionext/Kconfig
+++ b/drivers/net/ethernet/socionext/Kconfig
@@ -26,6 +26,7 @@ config SNI_NETSEC
tristate "Socionext NETSEC ethernet support"
depends on (ARCH_SYNQUACER || COMPILE_TEST) && OF
select PHYLIB
+ select PAGE_POOL
select MII
---help---
Enable to add support for the SocioNext NetSec Gigabit Ethernet
diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 48fd7448b513..e653b24d0534 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -11,6 +11,7 @@
#include <linux/io.h>
#include <net/tcp.h>
+#include <net/page_pool.h>
#include <net/ip6_checksum.h>
#define NETSEC_REG_SOFT_RST 0x104
@@ -235,7 +236,8 @@
#define DESC_NUM 256
#define NETSEC_SKB_PAD (NET_SKB_PAD + NET_IP_ALIGN)
-#define NETSEC_RX_BUF_SZ 1536
+#define NETSEC_RX_BUF_NON_DATA (NETSEC_SKB_PAD + \
+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
#define DESC_SZ sizeof(struct netsec_de)
@@ -258,6 +260,8 @@ struct netsec_desc_ring {
struct netsec_desc *desc;
void *vaddr;
u16 head, tail;
+ struct page_pool *page_pool;
+ struct xdp_rxq_info xdp_rxq;
};
struct netsec_priv {
@@ -673,33 +677,27 @@ static void netsec_process_tx(struct netsec_priv *priv)
}
static void *netsec_alloc_rx_data(struct netsec_priv *priv,
- dma_addr_t *dma_handle, u16 *desc_len,
- bool napi)
+ dma_addr_t *dma_handle, u16 *desc_len)
+
{
- size_t total_len = SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
- size_t payload_len = NETSEC_RX_BUF_SZ;
- dma_addr_t mapping;
- void *buf;
- total_len += SKB_DATA_ALIGN(payload_len + NETSEC_SKB_PAD);
+ struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_RX];
+ struct page *page;
- buf = napi ? napi_alloc_frag(total_len) : netdev_alloc_frag(total_len);
- if (!buf)
+ page = page_pool_dev_alloc_pages(dring->page_pool);
+ if (!page)
return NULL;
- mapping = dma_map_single(priv->dev, buf + NETSEC_SKB_PAD, payload_len,
- DMA_FROM_DEVICE);
- if (unlikely(dma_mapping_error(priv->dev, mapping)))
- goto err_out;
-
- *dma_handle = mapping;
- *desc_len = payload_len;
-
- return buf;
+ /* page_pool API will map the whole page, skip
+ * NET_SKB_PAD + NET_IP_ALIGN for the payload
+ */
+ *dma_handle = page_pool_get_dma_addr(page) + NETSEC_SKB_PAD;
+ /* make sure the incoming payload fits in the page with the needed
+ * NET_SKB_PAD + NET_IP_ALIGN + skb_shared_info
+ */
+ *desc_len = PAGE_SIZE - NETSEC_RX_BUF_NON_DATA;
-err_out:
- skb_free_frag(buf);
- return NULL;
+ return page_address(page);
}
static void netsec_rx_fill(struct netsec_priv *priv, u16 from, u16 num)
@@ -728,10 +726,10 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
u16 idx = dring->tail;
struct netsec_de *de = dring->vaddr + (DESC_SZ * idx);
struct netsec_desc *desc = &dring->desc[idx];
+ struct page *page = virt_to_page(desc->addr);
u16 pkt_len, desc_len;
dma_addr_t dma_handle;
void *buf_addr;
- u32 truesize;
if (de->attr & (1U << NETSEC_RX_PKT_OWN_FIELD)) {
/* reading the register clears the irq */
@@ -766,8 +764,8 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
/* allocate a fresh buffer and map it to the hardware.
* This will eventually replace the old buffer in the hardware
*/
- buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len,
- true);
+ buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len);
+
if (unlikely(!buf_addr))
break;
@@ -775,22 +773,19 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
DMA_FROM_DEVICE);
prefetch(desc->addr);
- truesize = SKB_DATA_ALIGN(desc->len + NETSEC_SKB_PAD) +
- SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
- skb = build_skb(desc->addr, truesize);
+ skb = build_skb(desc->addr, desc->len + NETSEC_RX_BUF_NON_DATA);
if (unlikely(!skb)) {
- /* free the newly allocated buffer, we are not going to
- * use it
+ /* If skb fails recycle_direct will either unmap and
+ * free the page or refill the cache depending on the
+ * cache state. Since we paid the allocation cost if
+ * building an skb fails try to put the page into cache
*/
- dma_unmap_single(priv->dev, dma_handle, desc_len,
- DMA_FROM_DEVICE);
- skb_free_frag(buf_addr);
+ page_pool_recycle_direct(dring->page_pool, page);
netif_err(priv, drv, priv->ndev,
"rx failed to build skb\n");
break;
}
- dma_unmap_single_attrs(priv->dev, desc->dma_addr, desc->len,
- DMA_FROM_DEVICE, DMA_ATTR_SKIP_CPU_SYNC);
+ page_pool_release_page(dring->page_pool, page);
/* Update the descriptor with the new buffer we allocated */
desc->len = desc_len;
@@ -980,21 +975,26 @@ static void netsec_uninit_pkt_dring(struct netsec_priv *priv, int id)
if (!dring->vaddr || !dring->desc)
return;
-
for (idx = 0; idx < DESC_NUM; idx++) {
desc = &dring->desc[idx];
if (!desc->addr)
continue;
- dma_unmap_single(priv->dev, desc->dma_addr, desc->len,
- id == NETSEC_RING_RX ? DMA_FROM_DEVICE :
- DMA_TO_DEVICE);
- if (id == NETSEC_RING_RX)
- skb_free_frag(desc->addr);
- else if (id == NETSEC_RING_TX)
+ if (id == NETSEC_RING_RX) {
+ struct page *page = virt_to_page(desc->addr);
+
+ page_pool_put_page(dring->page_pool, page, false);
+ } else if (id == NETSEC_RING_TX) {
+ dma_unmap_single(priv->dev, desc->dma_addr, desc->len,
+ DMA_TO_DEVICE);
dev_kfree_skb(desc->skb);
+ }
}
+ /* Rx is currently using page_pool */
+ if (xdp_rxq_info_is_reg(&dring->xdp_rxq))
+ xdp_rxq_info_unreg(&dring->xdp_rxq);
+
memset(dring->desc, 0, sizeof(struct netsec_desc) * DESC_NUM);
memset(dring->vaddr, 0, DESC_SZ * DESC_NUM);
@@ -1059,7 +1059,23 @@ static void netsec_setup_tx_dring(struct netsec_priv *priv)
static int netsec_setup_rx_dring(struct netsec_priv *priv)
{
struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_RX];
- int i;
+ struct page_pool_params pp_params = { 0 };
+ int i, err;
+
+ pp_params.order = 0;
+ /* internal DMA mapping in page_pool */
+ pp_params.flags = PP_FLAG_DMA_MAP;
+ pp_params.pool_size = DESC_NUM;
+ pp_params.nid = cpu_to_node(0);
+ pp_params.dev = priv->dev;
+ pp_params.dma_dir = DMA_FROM_DEVICE;
+
+ dring->page_pool = page_pool_create(&pp_params);
+ if (IS_ERR(dring->page_pool)) {
+ err = PTR_ERR(dring->page_pool);
+ dring->page_pool = NULL;
+ goto err_out;
+ }
for (i = 0; i < DESC_NUM; i++) {
struct netsec_desc *desc = &dring->desc[i];
@@ -1067,10 +1083,10 @@ static int netsec_setup_rx_dring(struct netsec_priv *priv)
void *buf;
u16 len;
- buf = netsec_alloc_rx_data(priv, &dma_handle, &len,
- false);
+ buf = netsec_alloc_rx_data(priv, &dma_handle, &len);
+
if (!buf) {
- netsec_uninit_pkt_dring(priv, NETSEC_RING_RX);
+ err = -ENOMEM;
goto err_out;
}
desc->dma_addr = dma_handle;
@@ -1079,11 +1095,22 @@ static int netsec_setup_rx_dring(struct netsec_priv *priv)
}
netsec_rx_fill(priv, 0, DESC_NUM);
+ err = xdp_rxq_info_reg(&dring->xdp_rxq, priv->ndev, 0);
+ if (err)
+ goto err_out;
+
+ err = xdp_rxq_info_reg_mem_model(&dring->xdp_rxq, MEM_TYPE_PAGE_POOL,
+ dring->page_pool);
+ if (err) {
+ page_pool_free(dring->page_pool);
+ goto err_out;
+ }
return 0;
err_out:
- return -ENOMEM;
+ netsec_uninit_pkt_dring(priv, NETSEC_RING_RX);
+ return err;
}
static int netsec_netdev_load_ucode_region(struct netsec_priv *priv, u32 reg,
--
2.20.1
^ permalink raw reply related
* [RFC, PATCH 2/2, net-next] net: netsec: add XDP support
From: Ilias Apalodimas @ 2019-06-25 15:06 UTC (permalink / raw)
To: netdev, jaswinder.singh
Cc: ard.biesheuvel, bjorn.topel, magnus.karlsson, brouer, daniel, ast,
makita.toshiaki, jakub.kicinski, john.fastabend, davem,
Ilias Apalodimas
In-Reply-To: <1561475179-7686-1-git-send-email-ilias.apalodimas@linaro.org>
The interface only supports 1 Tx queue so locking is introduced on
the Tx queue if XDP is enabled to make sure .ndo_start_xmit and
.ndo_xdp_xmit won't corrupt Tx ring
- Performance (SMMU off)
Benchmark XDP_SKB XDP_DRV
xdp1 291kpps 344kpps
rxdrop 282kpps 342kpps
- Performance (SMMU on)
Benchmark XDP_SKB XDP_DRV
xdp1 167kpps 324kpps
rxdrop 164kpps 323kpps
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
drivers/net/ethernet/socionext/netsec.c | 351 ++++++++++++++++++++++--
1 file changed, 325 insertions(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index e653b24d0534..c7c7e5119b46 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -9,6 +9,9 @@
#include <linux/etherdevice.h>
#include <linux/interrupt.h>
#include <linux/io.h>
+#include <linux/netlink.h>
+#include <linux/bpf.h>
+#include <linux/bpf_trace.h>
#include <net/tcp.h>
#include <net/page_pool.h>
@@ -236,23 +239,41 @@
#define DESC_NUM 256
#define NETSEC_SKB_PAD (NET_SKB_PAD + NET_IP_ALIGN)
-#define NETSEC_RX_BUF_NON_DATA (NETSEC_SKB_PAD + \
+#define NETSEC_RXBUF_HEADROOM (max(XDP_PACKET_HEADROOM, NET_SKB_PAD) + \
+ NET_IP_ALIGN)
+#define NETSEC_RX_BUF_NON_DATA (NETSEC_RXBUF_HEADROOM + \
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
#define DESC_SZ sizeof(struct netsec_de)
#define NETSEC_F_NETSEC_VER_MAJOR_NUM(x) ((x) & 0xffff0000)
+#define NETSEC_XDP_PASS 0
+#define NETSEC_XDP_CONSUMED BIT(0)
+#define NETSEC_XDP_TX BIT(1)
+#define NETSEC_XDP_REDIR BIT(2)
+#define NETSEC_XDP_RX_OK (NETSEC_XDP_PASS | NETSEC_XDP_TX | NETSEC_XDP_REDIR)
+
enum ring_id {
NETSEC_RING_TX = 0,
NETSEC_RING_RX
};
+enum buf_type {
+ TYPE_NETSEC_SKB = 0,
+ TYPE_NETSEC_XDP_TX,
+ TYPE_NETSEC_XDP_NDO,
+};
+
struct netsec_desc {
- struct sk_buff *skb;
+ union {
+ struct sk_buff *skb;
+ struct xdp_frame *xdpf;
+ };
dma_addr_t dma_addr;
void *addr;
u16 len;
+ u8 buf_type;
};
struct netsec_desc_ring {
@@ -260,13 +281,17 @@ struct netsec_desc_ring {
struct netsec_desc *desc;
void *vaddr;
u16 head, tail;
+ u16 xdp_xmit; /* netsec_xdp_xmit packets */
+ bool is_xdp;
struct page_pool *page_pool;
struct xdp_rxq_info xdp_rxq;
+ spinlock_t lock; /* XDP tx queue locking */
};
struct netsec_priv {
struct netsec_desc_ring desc_ring[NETSEC_RING_MAX];
struct ethtool_coalesce et_coalesce;
+ struct bpf_prog *xdp_prog;
spinlock_t reglock; /* protect reg access */
struct napi_struct napi;
phy_interface_t phy_interface;
@@ -303,6 +328,11 @@ struct netsec_rx_pkt_info {
bool err_flag;
};
+static void netsec_set_tx_de(struct netsec_priv *priv,
+ struct netsec_desc_ring *dring,
+ const struct netsec_tx_pkt_ctrl *tx_ctrl,
+ const struct netsec_desc *desc, void *buf);
+
static void netsec_write(struct netsec_priv *priv, u32 reg_addr, u32 val)
{
writel(val, priv->ioaddr + reg_addr);
@@ -609,6 +639,9 @@ static bool netsec_clean_tx_dring(struct netsec_priv *priv)
int tail = dring->tail;
int cnt = 0;
+ if (dring->is_xdp)
+ spin_lock(&dring->lock);
+
pkts = 0;
bytes = 0;
entry = dring->vaddr + DESC_SZ * tail;
@@ -622,16 +655,24 @@ static bool netsec_clean_tx_dring(struct netsec_priv *priv)
eop = (entry->attr >> NETSEC_TX_LAST) & 1;
dma_rmb();
- dma_unmap_single(priv->dev, desc->dma_addr, desc->len,
- DMA_TO_DEVICE);
- if (eop) {
- pkts++;
+ if (!eop)
+ goto next;
+
+ if (desc->buf_type == TYPE_NETSEC_SKB) {
+ dma_unmap_single(priv->dev, desc->dma_addr, desc->len,
+ DMA_TO_DEVICE);
bytes += desc->skb->len;
dev_kfree_skb(desc->skb);
+ } else {
+ if (desc->buf_type == TYPE_NETSEC_XDP_NDO)
+ dma_unmap_single(priv->dev, desc->dma_addr,
+ desc->len, DMA_TO_DEVICE);
+ xdp_return_frame(desc->xdpf);
}
/* clean up so netsec_uninit_pkt_dring() won't free the skb
* again
*/
+next:
*desc = (struct netsec_desc){};
/* entry->attr is not going to be accessed by the NIC until
@@ -645,6 +686,8 @@ static bool netsec_clean_tx_dring(struct netsec_priv *priv)
entry = dring->vaddr + DESC_SZ * tail;
cnt++;
}
+ if (dring->is_xdp)
+ spin_unlock(&dring->lock);
if (!cnt)
return false;
@@ -688,12 +731,13 @@ static void *netsec_alloc_rx_data(struct netsec_priv *priv,
if (!page)
return NULL;
- /* page_pool API will map the whole page, skip
- * NET_SKB_PAD + NET_IP_ALIGN for the payload
+ /* We allocate the same buffer length for XDP and non-XDP cases.
+ * page_pool API will map the whole page, skip what's needed for
+ * network payloads and/or XDP
*/
- *dma_handle = page_pool_get_dma_addr(page) + NETSEC_SKB_PAD;
- /* make sure the incoming payload fits in the page with the needed
- * NET_SKB_PAD + NET_IP_ALIGN + skb_shared_info
+ *dma_handle = page_pool_get_dma_addr(page) + NETSEC_RXBUF_HEADROOM;
+ /* Make sure the incoming payload fits in the page for XDP and non-XDP
+ * cases and reserve enough space for headroom + skb_shared_info
*/
*desc_len = PAGE_SIZE - NETSEC_RX_BUF_NON_DATA;
@@ -714,12 +758,144 @@ static void netsec_rx_fill(struct netsec_priv *priv, u16 from, u16 num)
}
}
+static void netsec_xdp_ring_tx_db(struct netsec_priv *priv, u16 pkts)
+{
+ if (likely(pkts))
+ netsec_write(priv, NETSEC_REG_NRM_TX_PKTCNT, pkts);
+}
+
+static void netsec_finalize_xdp_rx(struct netsec_priv *priv, u32 xdp_res,
+ u16 pkts)
+{
+ if (xdp_res & NETSEC_XDP_REDIR)
+ xdp_do_flush_map();
+
+ if (xdp_res & NETSEC_XDP_TX)
+ netsec_xdp_ring_tx_db(priv, pkts);
+}
+
+/* The current driver only supports 1 Txq, this should run under spin_lock() */
+static u32 netsec_xdp_queue_one(struct netsec_priv *priv,
+ struct xdp_frame *xdpf, bool is_ndo)
+
+{
+ struct netsec_desc_ring *tx_ring = &priv->desc_ring[NETSEC_RING_TX];
+ struct page *page = virt_to_page(xdpf->data);
+ struct netsec_tx_pkt_ctrl tx_ctrl = {};
+ struct netsec_desc tx_desc;
+ dma_addr_t dma_handle;
+ u16 filled;
+
+ if (tx_ring->head >= tx_ring->tail)
+ filled = tx_ring->head - tx_ring->tail;
+ else
+ filled = tx_ring->head + DESC_NUM - tx_ring->tail;
+
+ if (DESC_NUM - filled <= 1)
+ return NETSEC_XDP_CONSUMED;
+
+ if (is_ndo) {
+ /* this is for ndo_xdp_xmit, the buffer needs mapping before
+ * sending
+ */
+ dma_handle = dma_map_single(priv->dev, xdpf->data, xdpf->len,
+ DMA_TO_DEVICE);
+ if (dma_mapping_error(priv->dev, dma_handle))
+ return NETSEC_XDP_CONSUMED;
+ tx_desc.buf_type = TYPE_NETSEC_XDP_NDO;
+ } else {
+ /* This is the device Rx buffer from page_pool. No need to remap
+ * just sync and send it
+ */
+ dma_handle = page_pool_get_dma_addr(page) +
+ NETSEC_RXBUF_HEADROOM;
+ dma_sync_single_for_device(priv->dev, dma_handle, xdpf->len,
+ DMA_BIDIRECTIONAL);
+ tx_desc.buf_type = TYPE_NETSEC_XDP_TX;
+ }
+ tx_ctrl.cksum_offload_flag = false;
+ tx_ctrl.tcp_seg_offload_flag = false;
+ tx_ctrl.tcp_seg_len = 0;
+
+ tx_desc.dma_addr = dma_handle;
+ tx_desc.addr = xdpf->data;
+ tx_desc.len = xdpf->len;
+
+ netsec_set_tx_de(priv, tx_ring, &tx_ctrl, &tx_desc, xdpf);
+
+ return NETSEC_XDP_TX;
+}
+
+static u32 netsec_xdp_xmit_back(struct netsec_priv *priv, struct xdp_buff *xdp)
+{
+ struct netsec_desc_ring *tx_ring = &priv->desc_ring[NETSEC_RING_TX];
+ struct xdp_frame *xdpf = convert_to_xdp_frame(xdp);
+ u32 ret;
+
+ if (unlikely(!xdpf))
+ return NETSEC_XDP_CONSUMED;
+
+ spin_lock(&tx_ring->lock);
+ ret = netsec_xdp_queue_one(priv, xdpf, false);
+ spin_unlock(&tx_ring->lock);
+
+ return ret;
+}
+
+static u32 netsec_run_xdp(struct netsec_priv *priv, struct bpf_prog *prog,
+ struct xdp_buff *xdp)
+{
+ u32 ret = NETSEC_XDP_PASS;
+ int err;
+ u32 act;
+
+ rcu_read_lock();
+ act = bpf_prog_run_xdp(prog, xdp);
+
+ switch (act) {
+ case XDP_PASS:
+ ret = NETSEC_XDP_PASS;
+ break;
+ case XDP_TX:
+ ret = netsec_xdp_xmit_back(priv, xdp);
+ if (ret != NETSEC_XDP_TX)
+ xdp_return_buff(xdp);
+ break;
+ case XDP_REDIRECT:
+ err = xdp_do_redirect(priv->ndev, xdp, prog);
+ if (!err) {
+ ret = NETSEC_XDP_REDIR;
+ } else {
+ ret = NETSEC_XDP_CONSUMED;
+ xdp_return_buff(xdp);
+ }
+ break;
+ default:
+ bpf_warn_invalid_xdp_action(act);
+ /* fall through */
+ case XDP_ABORTED:
+ trace_xdp_exception(priv->ndev, prog, act);
+ /* fall through -- handle aborts by dropping packet */
+ case XDP_DROP:
+ ret = NETSEC_XDP_CONSUMED;
+ xdp_return_buff(xdp);
+ break;
+ }
+
+ rcu_read_unlock();
+
+ return ret;
+}
+
static int netsec_process_rx(struct netsec_priv *priv, int budget)
{
struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_RX];
+ struct bpf_prog *xdp_prog = READ_ONCE(priv->xdp_prog);
struct net_device *ndev = priv->ndev;
struct netsec_rx_pkt_info rx_info;
- struct sk_buff *skb;
+ struct sk_buff *skb = NULL;
+ u16 xdp_xmit = 0;
+ u32 xdp_act = 0;
int done = 0;
while (done < budget) {
@@ -727,8 +903,10 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
struct netsec_de *de = dring->vaddr + (DESC_SZ * idx);
struct netsec_desc *desc = &dring->desc[idx];
struct page *page = virt_to_page(desc->addr);
+ u32 xdp_result = XDP_PASS;
u16 pkt_len, desc_len;
dma_addr_t dma_handle;
+ struct xdp_buff xdp;
void *buf_addr;
if (de->attr & (1U << NETSEC_RX_PKT_OWN_FIELD)) {
@@ -773,7 +951,23 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
DMA_FROM_DEVICE);
prefetch(desc->addr);
+ xdp.data_hard_start = desc->addr;
+ xdp.data = desc->addr + NETSEC_RXBUF_HEADROOM;
+ xdp_set_data_meta_invalid(&xdp);
+ xdp.data_end = xdp.data + pkt_len;
+ xdp.rxq = &dring->xdp_rxq;
+
+ if (xdp_prog) {
+ xdp_result = netsec_run_xdp(priv, xdp_prog, &xdp);
+ if (xdp_result != NETSEC_XDP_PASS) {
+ xdp_act |= xdp_result;
+ if (xdp_result == NETSEC_XDP_TX)
+ xdp_xmit++;
+ goto next;
+ }
+ }
skb = build_skb(desc->addr, desc->len + NETSEC_RX_BUF_NON_DATA);
+
if (unlikely(!skb)) {
/* If skb fails recycle_direct will either unmap and
* free the page or refill the cache depending on the
@@ -787,27 +981,30 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget)
}
page_pool_release_page(dring->page_pool, page);
- /* Update the descriptor with the new buffer we allocated */
- desc->len = desc_len;
- desc->dma_addr = dma_handle;
- desc->addr = buf_addr;
-
- skb_reserve(skb, NETSEC_SKB_PAD);
- skb_put(skb, pkt_len);
+ skb_reserve(skb, xdp.data - xdp.data_hard_start);
+ skb_put(skb, xdp.data_end - xdp.data);
skb->protocol = eth_type_trans(skb, priv->ndev);
if (priv->rx_cksum_offload_flag &&
rx_info.rx_cksum_result == NETSEC_RX_CKSUM_OK)
skb->ip_summed = CHECKSUM_UNNECESSARY;
- if (napi_gro_receive(&priv->napi, skb) != GRO_DROP) {
+next:
+ if ((skb && napi_gro_receive(&priv->napi, skb) != GRO_DROP) ||
+ xdp_result & NETSEC_XDP_RX_OK) {
ndev->stats.rx_packets++;
- ndev->stats.rx_bytes += pkt_len;
+ ndev->stats.rx_bytes += xdp.data_end - xdp.data;
}
+ /* Update the descriptor with fresh buffers */
+ desc->len = desc_len;
+ desc->dma_addr = dma_handle;
+ desc->addr = buf_addr;
+
netsec_rx_fill(priv, idx, 1);
dring->tail = (dring->tail + 1) % DESC_NUM;
}
+ netsec_finalize_xdp_rx(priv, xdp_act, xdp_xmit);
return done;
}
@@ -837,8 +1034,7 @@ static int netsec_napi_poll(struct napi_struct *napi, int budget)
static void netsec_set_tx_de(struct netsec_priv *priv,
struct netsec_desc_ring *dring,
const struct netsec_tx_pkt_ctrl *tx_ctrl,
- const struct netsec_desc *desc,
- struct sk_buff *skb)
+ const struct netsec_desc *desc, void *buf)
{
int idx = dring->head;
struct netsec_de *de;
@@ -861,10 +1057,16 @@ static void netsec_set_tx_de(struct netsec_priv *priv,
de->data_buf_addr_lw = lower_32_bits(desc->dma_addr);
de->buf_len_info = (tx_ctrl->tcp_seg_len << 16) | desc->len;
de->attr = attr;
- dma_wmb();
+ /* under spin_lock if using XDP */
+ if (!dring->is_xdp)
+ dma_wmb();
dring->desc[idx] = *desc;
- dring->desc[idx].skb = skb;
+ if (desc->buf_type == TYPE_NETSEC_SKB)
+ dring->desc[idx].skb = buf;
+ else if (desc->buf_type == TYPE_NETSEC_XDP_TX ||
+ desc->buf_type == TYPE_NETSEC_XDP_NDO)
+ dring->desc[idx].xdpf = buf;
/* move head ahead */
dring->head = (dring->head + 1) % DESC_NUM;
@@ -915,8 +1117,12 @@ static netdev_tx_t netsec_netdev_start_xmit(struct sk_buff *skb,
u16 tso_seg_len = 0;
int filled;
+ if (dring->is_xdp)
+ spin_lock_bh(&dring->lock);
filled = netsec_desc_used(dring);
if (netsec_check_stop_tx(priv, filled)) {
+ if (dring->is_xdp)
+ spin_unlock_bh(&dring->lock);
net_warn_ratelimited("%s %s Tx queue full\n",
dev_name(priv->dev), ndev->name);
return NETDEV_TX_BUSY;
@@ -949,6 +1155,8 @@ static netdev_tx_t netsec_netdev_start_xmit(struct sk_buff *skb,
tx_desc.dma_addr = dma_map_single(priv->dev, skb->data,
skb_headlen(skb), DMA_TO_DEVICE);
if (dma_mapping_error(priv->dev, tx_desc.dma_addr)) {
+ if (dring->is_xdp)
+ spin_unlock_bh(&dring->lock);
netif_err(priv, drv, priv->ndev,
"%s: DMA mapping failed\n", __func__);
ndev->stats.tx_dropped++;
@@ -957,11 +1165,14 @@ static netdev_tx_t netsec_netdev_start_xmit(struct sk_buff *skb,
}
tx_desc.addr = skb->data;
tx_desc.len = skb_headlen(skb);
+ tx_desc.buf_type = TYPE_NETSEC_SKB;
skb_tx_timestamp(skb);
netdev_sent_queue(priv->ndev, skb->len);
netsec_set_tx_de(priv, dring, &tx_ctrl, &tx_desc, skb);
+ if (dring->is_xdp)
+ spin_unlock_bh(&dring->lock);
netsec_write(priv, NETSEC_REG_NRM_TX_PKTCNT, 1); /* submit another tx */
return NETDEV_TX_OK;
@@ -1042,6 +1253,7 @@ static int netsec_alloc_dring(struct netsec_priv *priv, enum ring_id id)
static void netsec_setup_tx_dring(struct netsec_priv *priv)
{
struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_TX];
+ struct bpf_prog *xdp_prog = READ_ONCE(priv->xdp_prog);
int i;
for (i = 0; i < DESC_NUM; i++) {
@@ -1054,11 +1266,18 @@ static void netsec_setup_tx_dring(struct netsec_priv *priv)
*/
de->attr = 1U << NETSEC_TX_SHIFT_OWN_FIELD;
}
+
+ if (xdp_prog)
+ dring->is_xdp = true;
+ else
+ dring->is_xdp = false;
+
}
static int netsec_setup_rx_dring(struct netsec_priv *priv)
{
struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_RX];
+ struct bpf_prog *xdp_prog = READ_ONCE(priv->xdp_prog);
struct page_pool_params pp_params = { 0 };
int i, err;
@@ -1068,7 +1287,7 @@ static int netsec_setup_rx_dring(struct netsec_priv *priv)
pp_params.pool_size = DESC_NUM;
pp_params.nid = cpu_to_node(0);
pp_params.dev = priv->dev;
- pp_params.dma_dir = DMA_FROM_DEVICE;
+ pp_params.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE;
dring->page_pool = page_pool_create(&pp_params);
if (IS_ERR(dring->page_pool)) {
@@ -1490,6 +1709,9 @@ static int netsec_netdev_init(struct net_device *ndev)
if (ret)
goto err2;
+ spin_lock_init(&priv->desc_ring[NETSEC_RING_TX].lock);
+ spin_lock_init(&priv->desc_ring[NETSEC_RING_RX].lock);
+
return 0;
err2:
netsec_free_dring(priv, NETSEC_RING_RX);
@@ -1522,6 +1744,81 @@ static int netsec_netdev_ioctl(struct net_device *ndev, struct ifreq *ifr,
return phy_mii_ioctl(ndev->phydev, ifr, cmd);
}
+static int netsec_xdp_xmit(struct net_device *ndev, int n,
+ struct xdp_frame **frames, u32 flags)
+{
+ struct netsec_priv *priv = netdev_priv(ndev);
+ struct netsec_desc_ring *tx_ring = &priv->desc_ring[NETSEC_RING_TX];
+ int drops = 0;
+ int i;
+
+ if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
+ return -EINVAL;
+
+ spin_lock(&tx_ring->lock);
+ for (i = 0; i < n; i++) {
+ struct xdp_frame *xdpf = frames[i];
+ int err;
+
+ err = netsec_xdp_queue_one(priv, xdpf, true);
+ if (err != NETSEC_XDP_TX) {
+ xdp_return_frame_rx_napi(xdpf);
+ drops++;
+ } else {
+ tx_ring->xdp_xmit++;
+ }
+ }
+ spin_unlock(&tx_ring->lock);
+
+ if (unlikely(flags & XDP_XMIT_FLUSH)) {
+ netsec_xdp_ring_tx_db(priv, tx_ring->xdp_xmit);
+ tx_ring->xdp_xmit = 0;
+ }
+
+ return n - drops;
+}
+
+static int netsec_xdp_setup(struct netsec_priv *priv, struct bpf_prog *prog,
+ struct netlink_ext_ack *extack)
+{
+ struct net_device *dev = priv->ndev;
+ struct bpf_prog *old_prog;
+
+ /* For now just support only the usual MTU sized frames */
+ if (prog && dev->mtu > 1500) {
+ NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported on XDP");
+ return -EOPNOTSUPP;
+ }
+
+ if (netif_running(dev))
+ netsec_netdev_stop(dev);
+
+ /* Detach old prog, if any */
+ old_prog = xchg(&priv->xdp_prog, prog);
+ if (old_prog)
+ bpf_prog_put(old_prog);
+
+ if (netif_running(dev))
+ netsec_netdev_open(dev);
+
+ return 0;
+}
+
+static int netsec_xdp(struct net_device *ndev, struct netdev_bpf *xdp)
+{
+ struct netsec_priv *priv = netdev_priv(ndev);
+
+ switch (xdp->command) {
+ case XDP_SETUP_PROG:
+ return netsec_xdp_setup(priv, xdp->prog, xdp->extack);
+ case XDP_QUERY_PROG:
+ xdp->prog_id = priv->xdp_prog ? priv->xdp_prog->aux->id : 0;
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
static const struct net_device_ops netsec_netdev_ops = {
.ndo_init = netsec_netdev_init,
.ndo_uninit = netsec_netdev_uninit,
@@ -1532,6 +1829,8 @@ static const struct net_device_ops netsec_netdev_ops = {
.ndo_set_mac_address = eth_mac_addr,
.ndo_validate_addr = eth_validate_addr,
.ndo_do_ioctl = netsec_netdev_ioctl,
+ .ndo_xdp_xmit = netsec_xdp_xmit,
+ .ndo_bpf = netsec_xdp,
};
static int netsec_of_probe(struct platform_device *pdev,
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2] bpf: fix uapi bpf_prog_info fields alignment
From: Dmitry V. Levin @ 2019-06-25 15:08 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Baruch Siach, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Network Development,
bpf, Arnd Bergmann, linux-arch, Jiri Olsa, Geert Uytterhoeven,
Linus Torvalds
In-Reply-To: <CAADnVQJ3MPVCL-0x2gDYbUQsrmu8WipnisqXoU8ja4vZ-5nTmA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2189 bytes --]
On Tue, Jun 25, 2019 at 07:16:55AM -0700, Alexei Starovoitov wrote:
> 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; ?
By the way, why not use aligned types as suggested by Geert?
They are already used for other members of struct bpf_prog_info anyway.
FWIW, we use aligned types for bpf in strace and that approach
proved to be more robust than manual padding.
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH V3 07/10] net: dsa: microchip: Initial SPI regmap support
From: Marek Vasut @ 2019-06-25 15:18 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Andrew Lunn, Florian Fainelli, Tristram Ha, Woojung Huh
In-Reply-To: <CA+h21hot0TMh7G_BZ15DNXKTaxVv88B-8O7XpXPuqpNiQD5bxA@mail.gmail.com>
On 6/25/19 2:40 PM, Vladimir Oltean wrote:
> 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)
Oooooof. Is the switch development some competition in register layout
braindeath ? What's it gonna be next ... I was gonna suggest something,
but then reconsidered, better not give them any wild ideas :-)
> * 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!
All right, that's not the case here, whew.
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH v2] bpf: fix uapi bpf_prog_info fields alignment
From: Alexei Starovoitov @ 2019-06-25 15:19 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Baruch Siach, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Network Development,
bpf, Arnd Bergmann, linux-arch, Jiri Olsa, Geert Uytterhoeven,
Linus Torvalds
In-Reply-To: <20190625150835.GA24947@altlinux.org>
On Tue, Jun 25, 2019 at 8:08 AM Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> On Tue, Jun 25, 2019 at 07:16:55AM -0700, Alexei Starovoitov wrote:
> > 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; ?
>
> By the way, why not use aligned types as suggested by Geert?
> They are already used for other members of struct bpf_prog_info anyway.
>
> FWIW, we use aligned types for bpf in strace and that approach
> proved to be more robust than manual padding.
because __aligned_u64 is used for pointers.
^ permalink raw reply
* Re: [PATCH net-next] net: stmmac: Fix the case when PHY handle is not present
From: Katsuhiro Suzuki @ 2019-06-25 15:26 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jose Abreu, linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue
In-Reply-To: <20190625145105.GA4722@lunn.ch>
Hello Andrew,
On 2019/06/25 23:51, Andrew Lunn wrote:
> On Tue, Jun 25, 2019 at 11:40:00PM +0900, Katsuhiro Suzuki wrote:
>> Hello Jose,
>>
>> This patch works fine with my Tinker Board. Thanks a lot!
>>
>> Tested-by: Katsuhiro Suzuki <katsuhiro@katsuster.net>
>>
>>
>> BTW, from network guys point of view, is it better to add a phy node
>> into device trees that have no phy node such as the Tinker Board?
>
> Hi Katsuhiro
>
> It makes it less ambiguous if there is a phy-handle. It is then very
> clear which PHY should be used. For a development board, which people
> can be tinkering around with, there is a chance they add a second PHY
> to the MDIO bus, or an Ethernet switch, etc. Without explicitly
> listing the PHY, it might get the wrong one. However this is generally
> a problem if phy_find_first() is used. I think in this case, something
> is setting priv->plat->phy_addr, so it is also clearly defined which
> PHY to use.
>
> Andrew
>
Hmm, I see. This stmmac driver can choose PHY by the kernel module
parameter 'phyaddr' (if no one set this parameter, priv->plat->phy_addr
goes to 0). So there is no ambiguous in this case and need no changes
for device tree.
Thank you for your comment.
Best Regards,
Katsuhiro Suzuki
^ permalink raw reply
* [PATCH 0/1] Fix broken build of mlx5
From: Jes Sorensen @ 2019-06-25 15:27 UTC (permalink / raw)
To: saeedm; +Cc: netdev, kernel-team, Jes Sorensen
From: Jes Sorensen <jsorensen@fb.com>
This fixes an obvious build error that could have been caught by
simply building the code before pushing out the patch.
Cheers,
Jes
Jes Sorensen (1):
mlx5: Fix build when CONFIG_MLX5_EN_RXNFC is disabled
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
--
2.21.0
^ permalink raw reply
* [PATCH 1/1] mlx5: Fix build when CONFIG_MLX5_EN_RXNFC is disabled
From: Jes Sorensen @ 2019-06-25 15:27 UTC (permalink / raw)
To: saeedm; +Cc: netdev, kernel-team, Jes Sorensen
In-Reply-To: <20190625152708.23729-1-Jes.Sorensen@gmail.com>
From: Jes Sorensen <jsorensen@fb.com>
The previous patch broke the build with a static declaration for
a public function.
Fixes: 8f0916c6dc5c (net/mlx5e: Fix ethtool rxfh commands when CONFIG_MLX5_EN_RXNFC is disabled)
Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index dd764e0471f2..776040d91bd4 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1905,7 +1905,8 @@ static int mlx5e_flash_device(struct net_device *dev,
/* When CONFIG_MLX5_EN_RXNFC=n we only support ETHTOOL_GRXRINGS
* otherwise this function will be defined from en_fs_ethtool.c
*/
-static int mlx5e_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info, u32 *rule_locs)
+int mlx5e_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info,
+ u32 *rule_locs)
{
struct mlx5e_priv *priv = netdev_priv(dev);
--
2.21.0
^ permalink raw reply related
* Re: selftests: bpf: test_libbpf.sh failed at file test_l4lb.o
From: Dan Rue @ 2019-06-25 15:31 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Andrii Nakryiko, Naresh Kamboju,
open list:KERNEL SELFTEST FRAMEWORK, bpf, Netdev, open list, Xdp,
David S. Miller, Daniel Borkmann, Alexei Starovoitov, Martin Lau,
Yonghong Song, john fastabend, Jesper Dangaard Brouer,
Jakub Kicinski, Shuah Khan
In-Reply-To: <CAADnVQKZycXgSw6C0qa7g0y=W3xRhM_4Rqcj7ZzL=rGh_n4mgA@mail.gmail.com>
On Mon, Jun 24, 2019 at 12:58:15PM -0700, Alexei Starovoitov wrote:
> On Mon, Jun 24, 2019 at 12:53 PM Dan Rue <dan.rue@linaro.org> wrote:
> >
> > I would say if it's not possible to check at runtime, and it requires
> > clang 9.0, that this test should not be enabled by default.
>
> The latest clang is the requirement.
> If environment has old clang or no clang at all these tests will be failing.
Hi Alexei!
I'm not certain if I'm interpreting you as you intended, but it sounds
like you're telling me that if the test build environment does not use
'latest clang' (i guess latest as of today?), that these tests will
fail, and that is how it is going to be. If I have that wrong, please
correct me and disregard the rest of my message.
Please understand where we are coming from. We (and many others) run
thousands of tests from a lot of test frameworks, and so our environment
often has mutually exclusive requirements when it comes to things like
toolchain selection.
We believe, strongly, that a test should not emit a "fail" for a missing
requirement. Fail is a serious thing, and should be reserved for an
actual issue that needs to be investigated, reported, and fixed.
This is how we treat test failures - we investigate, report, and fix
them when possible. When they're not real failures, we waste our time
(and yours, in this case).
By adding the tests to TEST_GEN_PROGS, you're adding them to the general
test set that those of us running test farms try to run continuously
across a wide range of hardware environments and kernel branches.
My suggestion is that if you do not want us running them, don't add them
to TEST_GEN_PROGS. I thought the suggestion of testing for adequate
clang support and adding them conditionally at build-time was an idea
worth consideration.
Thanks,
Dan
--
Linaro - Kernel Validation
^ permalink raw reply
* Re: [PATCH v2] bpf: fix uapi bpf_prog_info fields alignment
From: Dmitry V. Levin @ 2019-06-25 15:36 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Baruch Siach, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Network Development,
bpf, Arnd Bergmann, linux-arch, Jiri Olsa, Geert Uytterhoeven,
Linus Torvalds
In-Reply-To: <CAADnVQJNLk7tAHRHr7V7ugvCX9iCjaH4_vS9YuNWcMpwnA6ZyA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]
On Tue, Jun 25, 2019 at 08:19:35AM -0700, Alexei Starovoitov wrote:
> On Tue, Jun 25, 2019 at 8:08 AM Dmitry V. Levin <ldv@altlinux.org> wrote:
> > On Tue, Jun 25, 2019 at 07:16:55AM -0700, Alexei Starovoitov wrote:
> > > 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; ?
> >
> > By the way, why not use aligned types as suggested by Geert?
> > They are already used for other members of struct bpf_prog_info anyway.
> >
> > FWIW, we use aligned types for bpf in strace and that approach
> > proved to be more robust than manual padding.
>
> because __aligned_u64 is used for pointers.
Does the fact that __aligned_u64 is used for pointers mean that
__aligned_u64 should not be used for anything but pointers?
--
ldv
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH net] net/sched: flower: fix infinite loop in fl_walk()
From: Davide Caratti @ 2019-06-25 15:47 UTC (permalink / raw)
To: Cong Wang
Cc: Vlad Buslov, David S. Miller, Linux Kernel Network Developers,
Lucas Bates
In-Reply-To: <CAM_iQpVVMBUdhv3o=doLhpWxee91zUPKjAOtUwryUEj0pfowdg@mail.gmail.com>
On Thu, 2019-06-20 at 10:33 -0700, Cong Wang wrote:
> On Thu, Jun 20, 2019 at 5:52 AM Davide Caratti <dcaratti@redhat.com> wrote:
> > hello Cong, thanks for reading.
> >
> > On Wed, 2019-06-19 at 15:04 -0700, Cong Wang wrote:
> > > On Wed, Jun 19, 2019 at 2:10 PM Davide Caratti <dcaratti@redhat.com> wrote:
> > > > on some CPUs (e.g. i686), tcf_walker.cookie has the same size as the IDR.
> > > > In this situation, the following script:
> > > >
> > > > # tc filter add dev eth0 ingress handle 0xffffffff flower action ok
> > > > # tc filter show dev eth0 ingress
> > > >
> > > > results in an infinite loop.
[...]
> I am not sure it is better to handle this overflow inside idr_get_next_ul()
> or just let its callers to handle it. According to the comments above
> idr_get_next_ul() it sounds like it is not expected to overflow, so...
>
> diff --git a/lib/idr.c b/lib/idr.c
> index c34e256d2f01..a38f5e391cec 100644
> --- a/lib/idr.c
> +++ b/lib/idr.c
> @@ -267,6 +267,9 @@ void *idr_get_next_ul(struct idr *idr, unsigned
> long *nextid)
> if (!slot)
> return NULL;
>
> + /* overflow */
> + if (iter.index < id)
> + return NULL;
> *nextid = iter.index + base;
> return rcu_dereference_raw(*slot);
> }
hello Cong,
I tested the above patch, but I still see the infinite loop on kernel
5.2.0-0.rc5.git0.1.fc31.i686 .
idr_get_next_ul() returns the entry in the radix tree which is greater or
equal to '*nextid' (which has the same value as 'id' in the above hunk).
So, when the radix tree contains one slot with index equal to ULONG_MAX,
whatever can be the value of 'id', the condition in that if() will always
be false (and the function will keep returning non-NULL, hence the
infinite loop).
I also tried this:
if (iter.index == id && id == ULONG_MAX) {
return NULL;
}
it fixes the infinite loop, but it clearly breaks the function semantic
(and anyway, it's not sufficient to fix my test, at least with cls_flower
it still dumps the entry with id 0xffffffff several times). I'm for
fixing the callers of idr_get_next_ul(), and in details:
- apply this patch for cls_flower
- change tcf_dump_walker() in act_api.c as follows, and add a TDC testcase
for 'gact'.
index 4e5d2e9ace5d..f34888c8a952 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -228,8 +228,11 @@ static int tcf_dump_walker(struct tcf_idrinfo
*idrinfo, struct sk_buff *skb,
idr_for_each_entry_ul(idr, p, id) {
index++;
- if (index < s_i)
+ if (index < s_i) {
+ if (id == ULONG_MAX)
+ break;
continue;
+ }
if (jiffy_since &&
time_after(jiffy_since,
WDYT?
thanks a lot,
--
davide
^ permalink raw reply related
* [net-next 1/1] tipc: simplify stale link failure criteria
From: Jon Maloy @ 2019-06-25 15:36 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
In commit a4dc70d46cf1 ("tipc: extend link reset criteria for stale
packet retransmission") we made link retransmission failure events
dependent on the link tolerance, and not only of the number of failed
retransmission attempts, as we did earlier. This works well. However,
keeping the original, additional criteria of 99 failed retransmissions
is now redundant, and may in some cases lead to failure detection
times in the order of minutes instead of the expected 1.5 sec link
tolerance value.
We now remove this criteria altogether.
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/link.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/net/tipc/link.c b/net/tipc/link.c
index bcfb0a4..af50b53 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -107,7 +107,6 @@ struct tipc_stats {
* @backlogq: queue for messages waiting to be sent
* @snt_nxt: next sequence number to use for outbound messages
* @prev_from: sequence number of most previous retransmission request
- * @stale_cnt: counter for number of identical retransmit attempts
* @stale_limit: time when repeated identical retransmits must force link reset
* @ackers: # of peers that needs to ack each packet before it can be released
* @acked: # last packet acked by a certain peer. Used for broadcast.
@@ -167,7 +166,6 @@ struct tipc_link {
u16 snd_nxt;
u16 prev_from;
u16 window;
- u16 stale_cnt;
unsigned long stale_limit;
/* Reception */
@@ -910,7 +908,6 @@ void tipc_link_reset(struct tipc_link *l)
l->acked = 0;
l->silent_intv_cnt = 0;
l->rst_cnt = 0;
- l->stale_cnt = 0;
l->bc_peer_is_up = false;
memset(&l->mon_state, 0, sizeof(l->mon_state));
tipc_link_reset_stats(l);
@@ -1068,8 +1065,7 @@ static bool link_retransmit_failure(struct tipc_link *l, struct tipc_link *r,
if (r->prev_from != from) {
r->prev_from = from;
r->stale_limit = jiffies + msecs_to_jiffies(r->tolerance);
- r->stale_cnt = 0;
- } else if (++r->stale_cnt > 99 && time_after(jiffies, r->stale_limit)) {
+ } else if (time_after(jiffies, r->stale_limit)) {
pr_warn("Retransmission failure on link <%s>\n", l->name);
link_print(l, "State of link ");
pr_info("Failed msg: usr %u, typ %u, len %u, err %u\n",
@@ -1515,7 +1511,6 @@ int tipc_link_rcv(struct tipc_link *l, struct sk_buff *skb,
/* Forward queues and wake up waiting users */
if (likely(tipc_link_release_pkts(l, msg_ack(hdr)))) {
- l->stale_cnt = 0;
tipc_link_advance_backlog(l, xmitq);
if (unlikely(!skb_queue_empty(&l->wakeupq)))
link_prepare_wakeup(l);
@@ -2584,7 +2579,7 @@ int tipc_link_dump(struct tipc_link *l, u16 dqueues, char *buf)
i += scnprintf(buf + i, sz - i, " %u", l->silent_intv_cnt);
i += scnprintf(buf + i, sz - i, " %u", l->rst_cnt);
i += scnprintf(buf + i, sz - i, " %u", l->prev_from);
- i += scnprintf(buf + i, sz - i, " %u", l->stale_cnt);
+ i += scnprintf(buf + i, sz - i, " %u", 0);
i += scnprintf(buf + i, sz - i, " %u", l->acked);
list = &l->transmq;
--
2.1.4
^ permalink raw reply related
* Re: Warnings generated from tcp_sacktag_write_queue.
From: Chinmay Agarwal @ 2019-06-25 15:57 UTC (permalink / raw)
To: Eric Dumazet, netdev; +Cc: sharathv, kapandey
In-Reply-To: <ab6bb900-e9b7-f2b2-0a56-d1c9e14d2db6@gmail.com>
On Tue, Jun 25, 2019 at 04:24:14PM +0200, Eric Dumazet wrote:
>
>
> On 6/25/19 6:07 AM, Chinmay Agarwal wrote:
> > 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
> >
>
>
> You do not provide what exact kernel version you are using,
> this is probably the most important information we need.
>
The kernel version used is 4.14.
^ permalink raw reply
* 4.19: Traced deadlock during xfrm_user module load
From: Thomas Jarosch @ 2019-06-25 15:55 UTC (permalink / raw)
To: netdev; +Cc: netfilter-devel, Juliana Rodrigueiro
Hi all,
we're in the process of upgrading to kernel 4.19 and hit
a very rare lockup on boot during "xfrm_user" module load.
The tested kernel was 4.19.55.
When the strongswan IPsec service starts, it loads the xfrm_user module.
-> modprobe hangs forever.
Also network services like ssh or apache stop responding,
ICMP ping still works.
By chance we had magic sysRq enabled and were able to get some meaningful stack
traces. We've rebuilt the kernel with LOCKDEP + DEBUG_INFO + DEBUG_INFO_REDUCED,
but so far failed to reproduce the issue even when hammering the suspected
deadlock case. Though it's just hammering it for a few hours yet.
Preliminary analysis:
"modprobe xfrm_user":
xfrm_user_init()
register_pernet_subsys()
-> grab pernet_ops_rwsem
..
netlink_table_grab()
calls schedule() as "nl_table_users" is non-zero
conntrack netlink related program "info_iponline" does this in parallel:
netlink_bind()
netlink_lock_table() -> increases "nl_table_users"
nfnetlink_bind()
# does not unlock the table as it's locked by netlink_bind()
__request_module()
call_usermodehelper_exec()
"modprobe nf_conntrack_netlink" runs and inits nf_conntrack_netlink:
ctnetlink_init()
register_pernet_subsys()
-> blocks on "pernet_ops_rwsem" thanks to xfrm_user module
-> schedule()
-> deadlock forever
Full stack traces:
strongswan starts on boot and loads "xfrm_user":
kernel: modprobe D 0 3825 3762 0x80000080
kernel: Call Trace:
kernel: __schedule+0x188/0x4d0
kernel: ? add_wait_queue_exclusive+0x4d/0x60
kernel: schedule+0x21/0x70
kernel: netlink_table_grab+0x9a/0xf0
kernel: ? wake_up_q+0x60/0x60
kernel: __netlink_kernel_create+0x15f/0x1e0
kernel: xfrm_user_net_init+0x45/0x80 [xfrm_user]
kernel: ? xfrm_user_net_exit+0x60/0x60 [xfrm_user]
kernel: ops_init+0x68/0x100
kernel: ? vprintk_emit+0x9e/0x1a0
kernel: ? 0xf826c000
kernel: ? xfrm_dump_sa+0x120/0x120 [xfrm_user]
kernel: register_pernet_operations+0xef/0x1d0
kernel: ? 0xf826c000
kernel: register_pernet_subsys+0x1c/0x30
kernel: xfrm_user_init+0x18/0x1000 [xfrm_user]
kernel: do_one_initcall+0x44/0x190
kernel: ? free_unref_page_commit+0x6a/0xd0
kernel: do_init_module+0x46/0x1c0
kernel: load_module+0x1dc1/0x2400
kernel: sys_init_module+0xed/0x120
kernel: do_fast_syscall_32+0x7a/0x200
kernel: entry_SYSENTER_32+0x6b/0xbe
Another program triggers a conntrack netlink operation in parallel:
kernel: info_iponline D 0 3787 3684 0x00000084
kernel: Call Trace:
kernel: __schedule+0x188/0x4d0
kernel: schedule+0x21/0x70
kernel: schedule_timeout+0x195/0x260
kernel: ? wake_up_process+0xf/0x20
kernel: ? insert_work+0x86/0xa0
kernel: wait_for_common+0xe2/0x190
kernel: ? wake_up_q+0x60/0x60
kernel: wait_for_completion_killable+0x12/0x30
kernel: call_usermodehelper_exec+0xda/0x170
kernel: __request_module+0x115/0x2e0
kernel: ? __wake_up_common_lock+0x7a/0xa0
kernel: ? __wake_up+0xd/0x20
kernel: nfnetlink_bind+0x28/0x53 [nfnetlink]
kernel: netlink_bind+0x138/0x300
kernel: ? netlink_setsockopt+0x320/0x320
kernel: __sys_bind+0x65/0xb0
kernel: ? __audit_syscall_exit+0x1fb/0x270
kernel: ? __audit_syscall_entry+0xad/0xf0
kernel: sys_socketcall+0x2f0/0x350
kernel: ? _cond_resched+0x12/0x40
kernel: do_fast_syscall_32+0x7a/0x200
kernel: entry_SYSENTER_32+0x6b/0xbe
This triggers a module load by the kernel:
kernel: modprobe D 0 3827 1578 0x80000080
kernel: Call Trace:
kernel: __schedule+0x188/0x4d0
kernel: schedule+0x21/0x70
kernel: rwsem_down_write_failed+0xf5/0x210
kernel: ? 0xf8283000
kernel: call_rwsem_down_write_failed+0x9/0xc
kernel: down_write+0x18/0x30
kernel: register_pernet_subsys+0x10/0x30
kernel: ctnetlink_init+0x48/0x1000 [nf_conntrack_netlink]
kernel: do_one_initcall+0x44/0x190
kernel: ? free_unref_page_commit+0x6a/0xd0
kernel: do_init_module+0x46/0x1c0
kernel: load_module+0x1dc1/0x2400
kernel: sys_init_module+0xed/0x120
kernel: do_fast_syscall_32+0x7a/0x200
kernel: entry_SYSENTER_32+0x6b/0xbe
Other network related operations are deadlocked in netlink_table_grab():
kernel: ntpd D 0 3831 3830 0x00000080
kernel: Call Trace:
kernel: __schedule+0x188/0x4d0
kernel: ? add_wait_queue_exclusive+0x4d/0x60
kernel: schedule+0x21/0x70
kernel: netlink_table_grab+0x9a/0xf0
kernel: ? wake_up_q+0x60/0x60
kernel: netlink_release+0x14e/0x460
kernel: __sock_release+0x2d/0xb0
kernel: sock_close+0xd/0x20
kernel: __fput+0x93/0x1c0
kernel: ____fput+0x8/0x10
kernel: task_work_run+0x82/0xa0
kernel: exit_to_usermode_loop+0x8d/0x90
kernel: do_fast_syscall_32+0x1a7/0x200
kernel: entry_SYSENTER_32+0x6b/0xbe
The easy workaround for us is to load the conntrack netlink
modules before starting strongswan. Since we use classic init,
this works. In parallel booting systemd world, people might
still hit the issue, so it's worth fixing.
The related function netlink_create() unlocks the table
before requesting modules and locks it afterwards again.
We guess it's racy to call netlink_unlock_table()
before doing the
err = nlk->netlink_bind(net, group + 1);
call in netlink_bind() ?
What would be the best fix here?
Best regards,
Thomas Jarosch and Juliana Rodrigueiro
^ permalink raw reply
* [PATCH] ipvs: allow tunneling with gre encapsulation
From: Vadim Fedorenko @ 2019-06-25 15:59 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Konstantin Khlebnikov, Wensong Zhang, Julian Anastasov,
Jozsef Kadlecsik, Florian Westphal, David S. Miller, netdev,
lvs-devel, netfilter-devel
windows real servers can handle gre tunnels, this patch allows
gre encapsulation with the tunneling method, thereby letting ipvs
be load balancer for windows-based services
Signed-off-by: Vadim Fedorenko <vfedorenko@yandex-team.ru>
---
include/uapi/linux/ip_vs.h | 1 +
net/netfilter/ipvs/ip_vs_xmit.c | 76 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 77 insertions(+)
diff --git a/include/uapi/linux/ip_vs.h b/include/uapi/linux/ip_vs.h
index e4f1806..4102ddc 100644
--- a/include/uapi/linux/ip_vs.h
+++ b/include/uapi/linux/ip_vs.h
@@ -128,6 +128,7 @@
enum {
IP_VS_CONN_F_TUNNEL_TYPE_IPIP = 0, /* IPIP */
IP_VS_CONN_F_TUNNEL_TYPE_GUE, /* GUE */
+ IP_VS_CONN_F_TUNNEL_TYPE_GRE, /* GRE */
IP_VS_CONN_F_TUNNEL_TYPE_MAX,
};
diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 71fc6d6..fad3f33 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -29,6 +29,7 @@
#include <linux/tcp.h> /* for tcphdr */
#include <net/ip.h>
#include <net/gue.h>
+#include <net/gre.h>
#include <net/tcp.h> /* for csum_tcpudp_magic */
#include <net/udp.h>
#include <net/icmp.h> /* for icmp_send */
@@ -389,6 +390,12 @@ static inline bool decrement_ttl(struct netns_ipvs *ipvs,
skb->ip_summed == CHECKSUM_PARTIAL)
mtu -= GUE_PLEN_REMCSUM + GUE_LEN_PRIV;
}
+ if (dest->tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ __be16 tflags = 0;
+ if (dest->tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+ mtu -= gre_calc_hlen(tflags);
+ }
if (mtu < 68) {
IP_VS_DBG_RL("%s(): mtu less than 68\n", __func__);
goto err_put;
@@ -549,6 +556,12 @@ static inline bool decrement_ttl(struct netns_ipvs *ipvs,
skb->ip_summed == CHECKSUM_PARTIAL)
mtu -= GUE_PLEN_REMCSUM + GUE_LEN_PRIV;
}
+ if (dest->tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ __be16 tflags = 0;
+ if (dest->tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+ mtu -= gre_calc_hlen(tflags);
+ }
if (mtu < IPV6_MIN_MTU) {
IP_VS_DBG_RL("%s(): mtu less than %d\n", __func__,
IPV6_MIN_MTU);
@@ -1079,6 +1092,24 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
return 0;
}
+static void
+ipvs_gre_encap(struct net *net, struct sk_buff *skb,
+ struct ip_vs_conn *cp, __u8 *next_protocol)
+{
+ size_t hdrlen;
+ __be16 tflags = 0;
+ __be16 proto = *next_protocol == IPPROTO_IPIP ? htons(ETH_P_IP) :
htons(ETH_P_IPV6);
+
+ if (cp->dest->tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+
+ hdrlen = gre_calc_hlen(tflags);
+
+ gre_build_header(skb, hdrlen, tflags, proto, 0, 0);
+
+ *next_protocol = IPPROTO_GRE;
+}
+
/*
* IP Tunneling transmitter
*
@@ -1153,6 +1184,18 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
max_headroom += sizeof(struct udphdr) + gue_hdrlen;
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ size_t gre_hdrlen;
+ __be16 tflags = 0;
+
+ if (tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+
+ gre_hdrlen = gre_calc_hlen(tflags);
+
+ max_headroom += gre_hdrlen;
+ }
+
/* We only care about the df field if sysctl_pmtu_disc(ipvs) is set */
dfp = sysctl_pmtu_disc(ipvs) ? &df : NULL;
skb = ip_vs_prepare_tunneled_skb(skb, cp->af, max_headroom,
@@ -1174,6 +1217,13 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
}
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ if (tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ gso_type |= SKB_GSO_GRE_CSUM;
+ else
+ gso_type |= SKB_GSO_GRE;
+ }
+
if (iptunnel_handle_offloads(skb, gso_type))
goto tx_error;
@@ -1194,6 +1244,9 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
udp_set_csum(!check, skb, saddr, cp->daddr.ip, skb->len);
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ ipvs_gre_encap(net, skb, cp, &next_protocol);
+ }
skb_push(skb, sizeof(struct iphdr));
skb_reset_network_header(skb);
@@ -1289,6 +1342,18 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
max_headroom += sizeof(struct udphdr) + gue_hdrlen;
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ size_t gre_hdrlen;
+ __be16 tflags = 0;
+
+ if (tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ tflags |= TUNNEL_CSUM;
+
+ gre_hdrlen = gre_calc_hlen(tflags);
+
+ max_headroom += gre_hdrlen;
+ }
+
skb = ip_vs_prepare_tunneled_skb(skb, cp->af, max_headroom,
&next_protocol, &payload_len,
&dsfield, &ttl, NULL);
@@ -1308,6 +1373,13 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
}
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ if (tun_flags & IP_VS_TUNNEL_ENCAP_FLAG_CSUM)
+ gso_type |= SKB_GSO_GRE_CSUM;
+ else
+ gso_type |= SKB_GSO_GRE;
+ }
+
if (iptunnel_handle_offloads(skb, gso_type))
goto tx_error;
@@ -1328,6 +1400,10 @@ static inline int __tun_gso_type_mask(int encaps_af, int
orig_af)
udp6_set_csum(!check, skb, &saddr, &cp->daddr.in6, skb->len);
}
+ if (tun_type == IP_VS_CONN_F_TUNNEL_TYPE_GRE) {
+ ipvs_gre_encap(net, skb, cp, &next_protocol);
+ }
+
skb_push(skb, sizeof(struct ipv6hdr));
skb_reset_network_header(skb);
memset(&(IPCB(skb)->opt), 0, sizeof(IPCB(skb)->opt));
--
1.9.1
^ permalink raw reply related
* [net-next 1/1] tipc: eliminate unnecessary skb expansion during retransmission
From: Jon Maloy @ 2019-06-25 16:08 UTC (permalink / raw)
To: davem, netdev
Cc: gordan.mihaljevic, tung.q.nguyen, hoang.h.le, jon.maloy,
canh.d.luu, ying.xue, tipc-discussion
We increase the allocated headroom for the buffer copies to be
retransmitted. This eliminates the need for the lower stack levels
(UDP/IP/L2) to expand the headroom in order to add their own headers.
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
net/tipc/link.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tipc/link.c b/net/tipc/link.c
index af50b53..aa79bf8 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1125,7 +1125,7 @@ static int tipc_link_bc_retrans(struct tipc_link *l, struct tipc_link *r,
continue;
TIPC_SKB_CB(skb)->nxt_retr = jiffies + TIPC_BC_RETR_LIM;
}
- _skb = __pskb_copy(skb, MIN_H_SIZE, GFP_ATOMIC);
+ _skb = __pskb_copy(skb, LL_MAX_HEADER + MIN_H_SIZE, GFP_ATOMIC);
if (!_skb)
return 0;
hdr = buf_msg(_skb);
--
2.1.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox