* 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
* [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
* [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
* 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
* 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 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: 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
* [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
* [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 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
* 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
* 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 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 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: 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: 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: [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: [PATCH v2] bpf: fix uapi bpf_prog_info fields alignment
From: Alexei Starovoitov @ 2019-06-25 14:16 UTC (permalink / raw)
To: Baruch Siach
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Network Development, bpf, Dmitry V . Levin,
Arnd Bergmann, linux-arch, Jiri Olsa, Geert Uytterhoeven,
Linus Torvalds
In-Reply-To: <a5fb2545a0cf151bc443efa10c16c5a4de6f2670.1561460681.git.baruch@tkos.co.il>
On Tue, Jun 25, 2019 at 4:07 AM Baruch Siach <baruch@tkos.co.il> wrote:
>
> Merge commit 1c8c5a9d38f60 ("Merge
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next") undid the
> fix from commit 36f9814a494 ("bpf: fix uapi hole for 32 bit compat
> applications") by taking the gpl_compatible 1-bit field definition from
> commit b85fab0e67b162 ("bpf: Add gpl_compatible flag to struct
> bpf_prog_info") as is. That breaks architectures with 16-bit alignment
> like m68k. Embed gpl_compatible into an anonymous union with 32-bit pad
> member to restore alignment of following fields.
>
> Thanks to Dmitry V. Levin his analysis of this bug history.
>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> v2:
> Use anonymous union with pad to make it less likely to break again in
> the future.
> ---
> include/uapi/linux/bpf.h | 5 ++++-
> tools/include/uapi/linux/bpf.h | 5 ++++-
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a8b823c30b43..766eae02d7ae 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3142,7 +3142,10 @@ struct bpf_prog_info {
> __aligned_u64 map_ids;
> char name[BPF_OBJ_NAME_LEN];
> __u32 ifindex;
> - __u32 gpl_compatible:1;
> + union {
> + __u32 gpl_compatible:1;
> + __u32 pad;
> + };
Nack for the reasons explained in the previous thread
on the same subject.
Why cannot you go with earlier suggestion of _u32 :31; ?
^ permalink raw reply
* Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver
From: Johannes Berg @ 2019-06-25 14:14 UTC (permalink / raw)
To: Alex Elder, Dan Williams, Arnd Bergmann
Cc: Subash Abhinov Kasiviswanathan, abhishek.esse, Ben Chan,
Bjorn Andersson, cpratapa, David Miller, DTML, Eric Caruso,
evgreen, Ilias Apalodimas, Linux ARM, linux-arm-msm,
Linux Kernel Mailing List, linux-soc, Networking, syadagir
In-Reply-To: <7de004be-27b6-ac63-389d-8ea9d23d0361@linaro.org>
Hi Alex,
I'll just pick a few or your messages and reply there - some other
subthreads seem to have pretty much completed.
> Sorry for the delay. There's a lot here to go through, and with
> each message the picture is (slowly) getting a bit clearer for me.
> Still, there are some broad tradeoffs to consider and I think we
> need to get a little more specific again. I'm going to start a
> new thread (or rather re-subject a response to the very first one)
> that tries to do a fresh start that takes into account the
> discussion so far.
>
> I will also be talking with some people inside Qualcomm (including
> Subash) soon to make sure we don't miss any requirements or insights
> they know of that I don't realize are important.
That's much appreciated.
> > Linux usually tries to keep drivers generic and focused; each driver is
> > written for a specific function. For example, a USB device usually
> > provides multiple USB interfaces which will be bound to different Linux
> > drivers like a TTY, cdc-ether, QMI (via qmi_wwan), cdc-acm, etc.
>
> So USB has some attributes similar to what we're talking about
> here. But if I'm not mistaken we want some sort of an overall
> management scheme as well.
Yes. For the record, I think the part about "keep drivers generic and
focused" really only works for USB devices that expose different pieces
that look like any other network device or a TTY device on the USB
level, just the combination of these things (and knowing about that)
really makes them a modem.
For things like IPA or the (hypothetical) Intel driver we're talking
about, it's still all managed by a single (PCIe) driver. For the Intel
device in particular, even all the control channels are over exactly the
same transport mechanism as the data channels.
> > These drivers are often generic and we may not have enough information
> > in one driver to know that the parent of this interface is a WWAN
> > device. But another driver might. Since probing is asynchronous we may
> > have cdc-acm bind to a device and provide a TTY before cdc-ether (which
> > does know it's a WWAN) binds and provides the netdevice.
>
> Is this why Johannes wanted to have a "maybe attach" method?
Yes.
> I don't like the "maybe" API unless there's no other way to do it.
>
> Instead I think it would be better for the probing driver to register
> with a whatever the WWAN core is, and then have the WWAN core be
> responsible for pulling things all together when it receives a
> request to do so. I.e., something in user space should request
> that a registered data interface be brought up, and at that
> time everything "knows" it's implemented as part of a WWAN
> device.
Right, but then we just punt to userspace. Mostly we *do* (eventually!)
know that it's a WWAN device, just not every component can detect it.
Some components typically can.
So for example, you might have a USB multi-function device with a
network function (looks just like ethernet pretty much) but another TTY
control channel that actually has some specific WWAN IDs, so that part
can know it's a WWAN.
Here, the ethernet function would need "maybe" attach, and the control
channel would "definitively" attach, pulling it together as a WWAN
device without requiring any more action or information.
> So maybe:
> - Hardware probe detects a WWAN device
> - The drivers that detect the WWAN device register it with the
> WWAN core code.
> - A control channel is instantiated at/before the time the WWAN
> device is registered
> - Something in user space should manage the bring-up of any
> other things on the WWAN device thereafter
But those things need to actually get connected first :-)
In IPA/Intel case this is easy since it's a single driver. But if
there's multi-function device with ethernet being a completely separate
driver, the control channel cannot even reach that to tell it to create
a new data channel.
> > userspace should probably always create the netdevices (since they are
> > always useless until userspace coordinates with the firmware about
> > them) but that's not how things are yet.
>
> That's too bad. How hard would that be to change?
Depends, but as I said above it's probably orthogonal to the question.
The data channel driver would still need to attach to the WWAN device
somehow so it becomes reachable by the control plane (note this isn't
the same as "control channel" since the latter talks to the modem, the
control plane talks to the kernel drivers).
> > > - What causes a created channel to be removed?
> >
> > Driver removal, userspace WWAN daemon terminating the packet data
> > connection which the channel represents, the modem terminating the
> > packet data connection (eg network initiated disconnect), etc.
>
> OK this is as I expected. Driver (or device) removal is somewhat
> obvious, but you're confirming user space might request it as well.
If userspace actually had the ability to create (data) channels, then it
would have the ability to also remove them. Right now, this may or may
not be supported by the drivers that act together to form the interfaces
to a WWAN device.
> > > - You distinguish between attaching a netdevice and (what
> > > I'll call) activating it. What causes activation?
> >
> > Can you describe what you mean by "activating"? Do you mean
> > successfully TX/RX packets via the netdev and the outside world?
>
> Johannes mentioned an API to "maybe attach" a device. That begs
> the question of what happens if this request does *not* attach.
> Does the attach request have to be made again, or is it done
> automatically with a notification, or something else?
>
> So by "activation" I was trying to refer to the notion of this
> subsequent successful attach.
Oh. Well, what I was thinking that "maybe attach" would just be a sort
of "in-limbo" WWAN device that doesn't get visible to userspace or the
control plane until something did a "definitively attach" to it so it
was known to be a WWAN device.
The case of "maybe attach but never get to definitive attach" would be
the case where the USB driver bound a real ethernet device, for example,
not something that looks like an ethernet device but really is part of a
modem.
OTOH, "activating" a data channel is also needed somehow through the
control channel by talking to the modem, i.e. making a connection. In
the ideal case we'd not even *have* a netdev until it makes sense to
create a data channel, but in reality a lot of devices have one around
all the time (or even only support one), AFAICT.
> > I read "attach" here as simply associating an existing netdev with the
> > "parent" WWAN device. A purely Linux operation that is only book-
> > keeping and may not have any interaction with the modem.
>
> If that's the case I would want the "activation" to be a separate
> step. The attach would do the bookkeeping, and generally shouldn't
> fail. An attached interface would be brought up ("activated")
> separately and might fail if things aren't quite ready yet.
Right, but netdevs need to be brought up anyway, and that can fail?
> > > - How are the attributes of a WWAN device or channel set,
> > > or communicated?
> >
> > Via netlink attributes when userspace asks the WWAN device to create a
> > new channel. In the control methods I've seen, only userspace really
> > knows the channel identifier that it and the modem have agreed on (eg
> > what the MUX ID in the QMAP header would be, or the MBIM Session ID).
>
> Yes, that's the way it's worked for rmnet and IPA. Previously it
> was IOCTL requests but it's currently hard-wired.
Right. We're just trying to lift it out of the Qualcomm sphere into
something more generically useful.
johannes
^ permalink raw reply
* [PATCH net-next] tools: lib: bpf: libbpf: fix max() type mistmatch for 32bit
From: Ivan Khoronzhuk @ 2019-06-25 14:11 UTC (permalink / raw)
To: ast, netdev; +Cc: daniel, bpf, linux-kernel, Ivan Khoronzhuk
It fixes build error for 32bit coused by type mistmatch
size_t/unsigned long.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
Based on net-next/master
tools/lib/bpf/libbpf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 4259c9f0cfe7..d03016a559e2 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -778,7 +778,7 @@ static struct bpf_map *bpf_object__add_map(struct bpf_object *obj)
if (obj->nr_maps < obj->maps_cap)
return &obj->maps[obj->nr_maps++];
- new_cap = max(4ul, obj->maps_cap * 3 / 2);
+ new_cap = max((size_t)4, obj->maps_cap * 3 / 2);
new_maps = realloc(obj->maps, new_cap * sizeof(*obj->maps));
if (!new_maps) {
pr_warning("alloc maps for object failed\n");
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] can: mcp251x: add error check when wq alloc failed
From: Willem de Bruijn @ 2019-06-25 14:03 UTC (permalink / raw)
To: Weitao Hou
Cc: Wolfgang Grandegger, Marc Kleine-Budde, David Miller,
Greg Kroah-Hartman, allison, tglx, sean, linux-can,
Network Development, linux-kernel
In-Reply-To: <20190625125048.28849-1-houweitaoo@gmail.com>
On Tue, Jun 25, 2019 at 8:51 AM Weitao Hou <houweitaoo@gmail.com> wrote:
>
> add error check when workqueue alloc failed, and remove
> redundant code to make it clear
>
> Signed-off-by: Weitao Hou <houweitaoo@gmail.com>
Acked-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply
* Re: [PATCH v3 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Willem de Bruijn @ 2019-06-25 13:37 UTC (permalink / raw)
To: Neil Horman; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <20190625110247.GA29902@hmswarspite.think-freely.org>
On Tue, Jun 25, 2019 at 7:03 AM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Mon, Jun 24, 2019 at 06:15:29PM -0400, Willem de Bruijn wrote:
> > > > > + if (need_wait && !packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST)) {
> > > > > + po->wait_on_complete = 1;
> > > > > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > > >
> > > > This resets timeout on every loop. should only set above the loop once.
> > > >
> > > I explained exactly why I did that in the change log. Its because I reuse the
> > > timeout variable to get the return value of the wait_for_complete call.
> > > Otherwise I need to add additional data to the stack, which I don't want to do.
> > > Sock_sndtimeo is an inline function and really doesn't add any overhead to this
> > > path, so I see no reason not to reuse the variable.
> >
> > The issue isn't the reuse. It is that timeo is reset to sk_sndtimeo
> > each time. Whereas wait_for_common and its variants return the
> > number of jiffies left in case the loop needs to sleep again later.
> >
> > Reading sock_sndtimeo once and passing it to wait_.. repeatedly is a
> > common pattern across the stack.
> >
> But those patterns are unique to those situations. For instance, in
> tcp_sendmsg_locked, we aquire the value of the socket timeout, and use that to
> wait for the entire message send operation to complete, which consists of
> potentially several blocking operations (waiting for the tcp connection to be
> established, waiting for socket memory, etc). In that situation we want to wait
> for all of those operations to complete to send a single message, and fail if
> they exceed the timeout in aggregate. The semantics are different with
> AF_PACKET. In this use case, the message is in effect empty, and just used to
> pass some control information. tpacket_snd, sends as many frames from the
> memory mapped buffer as possible, and on each iteration we want to wait for the
> specified timeout for those frames to complete sending. I think resetting the
> timeout on each wait instance is the right way to go here.
I disagree. If a SO_SNDTIMEO of a given time is set, the thread should
not wait beyond that. Else what is the point of passing a specific
duration in the syscall?
Btw, we can always drop the timeo and go back to unconditional (bar
signals) waiting.
>
> > > > > @@ -2728,6 +2755,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > > > > err = net_xmit_errno(err);
> > > > > if (err && __packet_get_status(po, ph) ==
> > > > > TP_STATUS_AVAILABLE) {
> > > > > + /* re-init completion queue to avoid subsequent fallthrough
> > > > > + * on a future thread calling wait_on_complete_interruptible_timeout
> > > > > + */
> > > > > + po->wait_on_complete = 0;
> > > >
> > > > If setting where sleeping, no need for resetting if a failure happens
> > > > between those blocks.
> > > >
> > > > > + init_completion(&po->skb_completion);
> > > >
> > > > no need to reinit between each use?
> > > >
> > > I explained exactly why I did this in the comment above. We have to set
> > > wait_for_complete prior to calling transmit, so as to ensure that we call
> > > wait_for_completion before we exit the loop. However, in this error case, we
> > > exit the loop prior to calling wait_for_complete, so we need to reset the
> > > completion variable and the wait_for_complete flag. Otherwise we will be in a
> > > case where, on the next entrace to this loop we will have a completion variable
> > > with completion->done > 0, meaning the next wait will be a fall through case,
> > > which we don't want.
> >
> > By moving back to the point where schedule() is called, hopefully this
> > complexity automatically goes away. Same as my comment to the line
> > immediately above.
> >
> Its going to change what the complexity is, actually. I was looking at this
> last night, and I realized that your assertion that we could remove
> packet_next_frame came at a cost. This is because we need to determine if we
> have to wait before we call po->xmit, but we need to actually do the wait after
> po->xmit
Why? The existing method using schedule() doesn't.
Can we not just loop while sending and sleep immediately when
__packet_get_status returns !TP_STATUS_AVAILABLE?
I don't understand the need to probe the next packet to send instead
of the current.
This seems to be the crux of the disagreement. My guess is that it has
to do with setting wait_on_complete, but I don't see the problem. It
can be set immediately before going to sleep.
I don't meant to draw this out, btw, or add to your workload. If you
prefer, I can take a stab at my (admittedly hand-wavy) suggestion.
> (to ensure that wait_on_complete is set properly when the desructor is
> called). By moving the wait to the top of the loop and getting rid of
> packet_next_frame we now have a race condition in which we might call
> tpacket_destruct_skb with wait_on_complete set to false, causing us to wait for
> the maximum timeout erroneously. So I'm going to have to find a new way to
> signal the need to call complete, which I think will introduce a different level
> of complexity.
^ permalink raw reply
* Re: [PATCH v2] samples: bpf: make the use of xdp samples consistent
From: Song Liu @ 2019-06-25 13:31 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Toke Høiland-Jørgensen, Daniel T. Lee, Daniel Borkmann,
Alexei Starovoitov, bpf, Networking
In-Reply-To: <20190625120543.12b25184@carbon>
On Tue, Jun 25, 2019 at 5:09 AM Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
>
> On Tue, 25 Jun 2019 11:08:22 +0200
> Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> > "Daniel T. Lee" <danieltimlee@gmail.com> writes:
> >
> > > Currently, each xdp samples are inconsistent in the use.
> > > Most of the samples fetch the interface with it's name.
> > > (ex. xdp1, xdp2skb, xdp_redirect_cpu, xdp_sample_pkts, etc.)
> > >
> > > But some of the xdp samples are fetching the interface with
> > > ifindex by command argument.
> > >
> > > This commit enables xdp samples to fetch interface with it's name
> > > without changing the original index interface fetching.
> > > (<ifname|ifindex> fetching in the same way as xdp_sample_pkts_user.c does.)
> > >
> > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
Acked-by: Song Liu <songliubraving@fb.com>
> > > ---
> > > Changes in v2:
> > > - added xdp_redirect_user.c, xdp_redirect_map_user.c
> >
> > Great, thanks!
> >
> > Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
> Great you basically solved one of our TODOs:
> - TODO Change sample programs to accept ifnames as well as indexes
>
> https://github.com/xdp-project/xdp-project/blob/master/xdp-project.org#next-change-sample-programs-to-accept-ifnames-as-well-as-indexes
>
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next 0/2] Track recursive calls in TC act_mirred
From: John Hurley @ 2019-06-25 13:26 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Linux Netdev List, David Miller, Florian Westphal, Simon Horman,
Jakub Kicinski, oss-drivers
In-Reply-To: <108a6e4b-379c-ba43-5de0-27e31ade3470@mojatatu.com>
On Tue, Jun 25, 2019 at 12:18 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2019-06-24 6:13 p.m., John Hurley wrote:
> > These patches aim to prevent act_mirred causing stack overflow events from
> > recursively calling packet xmit or receive functions. Such events can
> > occur with poor TC configuration that causes packets to travel in loops
> > within the system.
> >
> > Florian Westphal advises that a recursion crash and packets looping are
> > separate issues and should be treated as such. David Miller futher points
> > out that pcpu counters cannot track the precise skb context required to
> > detect loops. Hence these patches are not aimed at detecting packet loops,
> > rather, preventing stack flows arising from such loops.
>
> Sigh. So we are still trying to save 2 bits?
> John, you said ovs has introduced a similar loop handling code;
> Is their approach similar to this? Bigger question: Is this approach
> "good enough"?
>
Hi Jamal.
Yes, OvS implements a similar approach to prevent recursion:
https://elixir.bootlin.com/linux/v5.2-rc6/source/net/openvswitch/actions.c#L1530
It was discussed on a previous thread that there are really 2 issues
here (even if it is the same thing that triggers them).
Firstly, the infinite looping of packets caused by poor config and,
secondly, the kernel panic caused by a stack overflow due to the
recursion in use.
These patches target the latter.
I think this approach is good enough to deal with the crashes as it
tracks a packets recursive calls (through act_mirred) on the network
stack.
If the packet is scheduled and releases the CPU then the counter is reset.
The packet may still loop but it will not cause stack overflows.
> Not to put more work for you, but one suggestion is to use skb metadata
> approach which is performance unfriendly (could be argued to more
> correct).
>
> Also: Please consider submitting a test case or two for tdc.
>
> cheers,
> jamal
>
^ permalink raw reply
* Re: [PATCH 03/11] xdp: add offset param to zero_copy_allocator
From: Laatz, Kevin @ 2019-06-25 13:14 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, ast, daniel, bjorn.topel, magnus.karlsson, bpf,
intel-wired-lan, bruce.richardson, ciara.loftus
In-Reply-To: <20190624122342.26c6a9b4@cakuba.netronome.com>
On 24/06/2019 20:23, Jakub Kicinski wrote:
> On Thu, 20 Jun 2019 09:09:50 +0000, Kevin Laatz wrote:
>> diff --git a/include/net/xdp.h b/include/net/xdp.h
>> index 0f25b3675c5c..ea801fd2bf98 100644
>> --- a/include/net/xdp.h
>> +++ b/include/net/xdp.h
>> @@ -53,7 +53,8 @@ struct xdp_mem_info {
>> struct page_pool;
>>
>> struct zero_copy_allocator {
>> - void (*free)(struct zero_copy_allocator *zca, unsigned long
>> handle);
>> + void (*free)(struct zero_copy_allocator *zca, unsigned long handle,
>> + off_t off);
>> };
>
> Please run checkpatch --strict on all your changes. The code
> formatting is incorrect in many ways in this series.
>
Thanks, will fix in the v2.
> Please include performance measurements proving the slow down
> is negligible in the cover letter.
>
Good suggestion. Will add to the cover letter in the v2!
^ permalink raw reply
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