* Re: [PATCH 1/2 nf-next v3] netfilter: nft_meta: Add NFT_META_BRI_IIFVPROTO support
From: Pablo Neira Ayuso @ 2019-07-03 12:23 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: wenxu, fw, netfilter-devel, netdev
In-Reply-To: <366e228f-7253-e388-4799-f0f9c18d1111@cumulusnetworks.com>
On Wed, Jul 03, 2019 at 03:08:01PM +0300, Nikolay Aleksandrov wrote:
> On 28/06/2019 03:49, wenxu@ucloud.cn wrote:
> > From: wenxu <wenxu@ucloud.cn>
> >
> > This patch provide a meta to get the bridge vlan proto
> >
> > nft add rule bridge firewall zones counter meta br_vlan_proto 0x8100
> >
> > Signed-off-by: wenxu <wenxu@ucloud.cn>
> > ---
> > include/uapi/linux/netfilter/nf_tables.h | 2 ++
> > net/netfilter/nft_meta.c | 9 +++++++++
> > 2 files changed, 11 insertions(+)
> >
>
> Hi,
> When using the internal bridge API outside of the bridge I'd advise you to CC bridge
> maintainers as well.
Will keep this mind, thanks.
> This patch is clearly wrong since you cannot access the vlan
> fields directly because bridge vlan support might be disabled from the kernel config
> as Pablo has noticed as well. In general I'd try to avoid using the internal API directly,
> but that is a different matter.
BROPT_VLAN_ENABLED is exposed through netlink and sysfs, and this only
consults the value. I guess you refer to the fact that...
> Please consult with include/linux/if_bridge.h for exported
> functions that are supposed to be visible outside of the bridge, if you need anything else
> make sure to add support for it there. The usage of br_opt_get directly for example must
> be changed to br_vlan_enabled().
Indeed... this patch should be using br_vlan_enabled() instead.
Thanks.
^ permalink raw reply
* Re: [RFC v2] vhost: introduce mdev based hardware vhost backend
From: Jason Wang @ 2019-07-03 12:16 UTC (permalink / raw)
To: Tiwei Bie
Cc: mst, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang
In-Reply-To: <20190703115245.GA22374@___>
On 2019/7/3 下午7:52, Tiwei Bie wrote:
> On Wed, Jul 03, 2019 at 06:09:51PM +0800, Jason Wang wrote:
>> On 2019/7/3 下午5:13, Tiwei Bie wrote:
>>> Details about this can be found here:
>>>
>>> https://lwn.net/Articles/750770/
>>>
>>> What's new in this version
>>> ==========================
>>>
>>> A new VFIO device type is introduced - vfio-vhost. This addressed
>>> some comments from here: https://patchwork.ozlabs.org/cover/984763/
>>>
>>> Below is the updated device interface:
>>>
>>> Currently, there are two regions of this device: 1) CONFIG_REGION
>>> (VFIO_VHOST_CONFIG_REGION_INDEX), which can be used to setup the
>>> device; 2) NOTIFY_REGION (VFIO_VHOST_NOTIFY_REGION_INDEX), which
>>> can be used to notify the device.
>>>
>>> 1. CONFIG_REGION
>>>
>>> The region described by CONFIG_REGION is the main control interface.
>>> Messages will be written to or read from this region.
>>>
>>> The message type is determined by the `request` field in message
>>> header. The message size is encoded in the message header too.
>>> The message format looks like this:
>>>
>>> struct vhost_vfio_op {
>>> __u64 request;
>>> __u32 flags;
>>> /* Flag values: */
>>> #define VHOST_VFIO_NEED_REPLY 0x1 /* Whether need reply */
>>> __u32 size;
>>> union {
>>> __u64 u64;
>>> struct vhost_vring_state state;
>>> struct vhost_vring_addr addr;
>>> } payload;
>>> };
>>>
>>> The existing vhost-kernel ioctl cmds are reused as the message
>>> requests in above structure.
>>
>> Still a comments like V1. What's the advantage of inventing a new protocol?
> I'm trying to make it work in VFIO's way..
>
>> I believe either of the following should be better:
>>
>> - using vhost ioctl, we can start from SET_VRING_KICK/SET_VRING_CALL and
>> extend it with e.g notify region. The advantages is that all exist userspace
>> program could be reused without modification (or minimal modification). And
>> vhost API hides lots of details that is not necessary to be understood by
>> application (e.g in the case of container).
> Do you mean reusing vhost's ioctl on VFIO device fd directly,
> or introducing another mdev driver (i.e. vhost_mdev instead of
> using the existing vfio_mdev) for mdev device?
Can we simply add them into ioctl of mdev_parent_ops?
>
>> - using PCI layout, then you don't even need to re-invent notifiy region at
>> all and we can pass-through them to guest.
> Like what you said previously, virtio has transports other than PCI.
> And it will look a bit odd when using transports other than PCI..
Yes.
>
>> Personally, I prefer vhost ioctl.
> +1
>
>>
> [...]
>>> 3. VFIO interrupt ioctl API
>>>
>>> VFIO interrupt ioctl API is used to setup device interrupts.
>>> IRQ-bypass can also be supported.
>>>
>>> Currently, the data path interrupt can be configured via the
>>> VFIO_VHOST_VQ_IRQ_INDEX with virtqueue's callfd.
>>
>> How about DMA API? Do you expect to use VFIO IOMMU API or using vhost
>> SET_MEM_TABLE? VFIO IOMMU API is more generic for sure but with
>> SET_MEM_TABLE DMA can be done at the level of parent device which means it
>> can work for e.g the card with on-chip IOMMU.
> Agree. In this RFC, it assumes userspace will use VFIO IOMMU API
> to do the DMA programming. But like what you said, there could be
> a problem when using cards with on-chip IOMMU.
Yes, another issue is SET_MEM_TABLE can not be used to update just a
part of the table. This seems less flexible than VFIO API but it could
be extended.
>
>> And what's the plan for vIOMMU?
> As this RFC assumes userspace will use VFIO IOMMU API, userspace
> just needs to follow the same way like what vfio-pci device does
> in QEMU to support vIOMMU.
Right, this is more a question for the qemu part. It means it needs to
go for ordinary VFIO path to get all notifiers/listeners support from
vIOMMU.
>
>>
>>> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
>>> ---
>>> drivers/vhost/Makefile | 2 +
>>> drivers/vhost/vdpa.c | 770 +++++++++++++++++++++++++++++++++++++
>>> include/linux/vdpa_mdev.h | 72 ++++
>>> include/uapi/linux/vfio.h | 19 +
>>> include/uapi/linux/vhost.h | 25 ++
>>> 5 files changed, 888 insertions(+)
>>> create mode 100644 drivers/vhost/vdpa.c
>>> create mode 100644 include/linux/vdpa_mdev.h
>>
>> We probably need some sample parent device implementation. It could be a
>> software datapath like e.g we can start from virtio-net device in guest or a
>> vhost/tap on host.
> Yeah, something like this would be interesting!
Plan to do something like that :) ?
Thanks
>
> Thanks,
> Tiwei
>
>> Thanks
>>
>>
^ permalink raw reply
* [PATCH bpf v2] xdp: fix race on generic receive path
From: Ilya Maximets @ 2019-07-03 12:09 UTC (permalink / raw)
To: netdev
Cc: linux-kernel, bpf, xdp-newbies, David S. Miller,
Björn Töpel, Magnus Karlsson, Jonathan Lemon,
Jakub Kicinski, Alexei Starovoitov, Daniel Borkmann,
Ilya Maximets
In-Reply-To: <CGME20190703120922eucas1p2d97e3b994425ecdd2dadd13744ac2a77@eucas1p2.samsung.com>
Unlike driver mode, generic xdp receive could be triggered
by different threads on different CPU cores at the same time
leading to the fill and rx queue breakage. For example, this
could happen while sending packets from two processes to the
first interface of veth pair while the second part of it is
open with AF_XDP socket.
Need to take a lock for each generic receive to avoid race.
Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
Version 2:
* spin_lock_irqsave --> spin_lock_bh.
include/net/xdp_sock.h | 2 ++
net/xdp/xsk.c | 31 ++++++++++++++++++++++---------
2 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index d074b6d60f8a..ac3c047d058c 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -67,6 +67,8 @@ struct xdp_sock {
* in the SKB destructor callback.
*/
spinlock_t tx_completion_lock;
+ /* Protects generic receive. */
+ spinlock_t rx_lock;
u64 rx_dropped;
};
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index a14e8864e4fa..5e0637db92ea 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -123,13 +123,17 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
u64 addr;
int err;
- if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index)
- return -EINVAL;
+ spin_lock_bh(&xs->rx_lock);
+
+ if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index) {
+ err = -EINVAL;
+ goto out_unlock;
+ }
if (!xskq_peek_addr(xs->umem->fq, &addr) ||
len > xs->umem->chunk_size_nohr - XDP_PACKET_HEADROOM) {
- xs->rx_dropped++;
- return -ENOSPC;
+ err = -ENOSPC;
+ goto out_drop;
}
addr += xs->umem->headroom;
@@ -138,13 +142,21 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
memcpy(buffer, xdp->data_meta, len + metalen);
addr += metalen;
err = xskq_produce_batch_desc(xs->rx, addr, len);
- if (!err) {
- xskq_discard_addr(xs->umem->fq);
- xsk_flush(xs);
- return 0;
- }
+ if (err)
+ goto out_drop;
+
+ xskq_discard_addr(xs->umem->fq);
+ xskq_produce_flush_desc(xs->rx);
+ spin_unlock_bh(&xs->rx_lock);
+
+ xs->sk.sk_data_ready(&xs->sk);
+ return 0;
+
+out_drop:
xs->rx_dropped++;
+out_unlock:
+ spin_unlock_bh(&xs->rx_lock);
return err;
}
@@ -765,6 +777,7 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
xs = xdp_sk(sk);
mutex_init(&xs->mutex);
+ spin_lock_init(&xs->rx_lock);
spin_lock_init(&xs->tx_completion_lock);
mutex_lock(&net->xdp.lock);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/2 nf-next v3] netfilter: nft_meta: Add NFT_META_BRI_IIFVPROTO support
From: Nikolay Aleksandrov @ 2019-07-03 12:08 UTC (permalink / raw)
To: wenxu, pablo, fw; +Cc: netfilter-devel, netdev
In-Reply-To: <1561682975-21790-1-git-send-email-wenxu@ucloud.cn>
On 28/06/2019 03:49, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> This patch provide a meta to get the bridge vlan proto
>
> nft add rule bridge firewall zones counter meta br_vlan_proto 0x8100
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
> ---
> include/uapi/linux/netfilter/nf_tables.h | 2 ++
> net/netfilter/nft_meta.c | 9 +++++++++
> 2 files changed, 11 insertions(+)
>
Hi,
When using the internal bridge API outside of the bridge I'd advise you to CC bridge
maintainers as well. This patch is clearly wrong since you cannot access the vlan
fields directly because bridge vlan support might be disabled from the kernel config
as Pablo has noticed as well. In general I'd try to avoid using the internal API directly,
but that is a different matter. Please consult with include/linux/if_bridge.h for exported
functions that are supposed to be visible outside of the bridge, if you need anything else
make sure to add support for it there. The usage of br_opt_get directly for example must
be changed to br_vlan_enabled().
Thanks,
Nik
^ permalink raw reply
* Re: [PATCH bpf] xdp: fix race on generic receive path
From: Ilya Maximets @ 2019-07-03 12:03 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, linux-kernel, bpf, xdp-newbies, David S. Miller,
Björn Töpel, Magnus Karlsson, Jonathan Lemon,
Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20190702174014.005a3166@cakuba.netronome.com>
On 03.07.2019 3:40, Jakub Kicinski wrote:
> On Tue, 2 Jul 2019 17:36:34 +0300, Ilya Maximets wrote:
>> Unlike driver mode, generic xdp receive could be triggered
>> by different threads on different CPU cores at the same time
>> leading to the fill and rx queue breakage. For example, this
>> could happen while sending packets from two processes to the
>> first interface of veth pair while the second part of it is
>> open with AF_XDP socket.
>>
>> Need to take a lock for each generic receive to avoid race.
>>
>> Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>
>> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
>> index a14e8864e4fa..19f41d2b670c 100644
>> --- a/net/xdp/xsk.c
>> +++ b/net/xdp/xsk.c
>> @@ -119,17 +119,22 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
>> {
>> u32 metalen = xdp->data - xdp->data_meta;
>> u32 len = xdp->data_end - xdp->data;
>> + unsigned long flags;
>> void *buffer;
>> u64 addr;
>> int err;
>>
>> - if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index)
>> - return -EINVAL;
>> + spin_lock_irqsave(&xs->rx_lock, flags);
>
> Why _irqsave, rather than _bh?
Yes, spin_lock_bh() is enough here. Will change in v2.
Thanks.
>
>> + if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index) {
>> + err = -EINVAL;
>> + goto out_unlock;
>> + }
>>
>> if (!xskq_peek_addr(xs->umem->fq, &addr) ||
>> len > xs->umem->chunk_size_nohr - XDP_PACKET_HEADROOM) {
>> - xs->rx_dropped++;
>> - return -ENOSPC;
>> + err = -ENOSPC;
>> + goto out_drop;
>> }
>>
>> addr += xs->umem->headroom;
>
>
>
^ permalink raw reply
* Re: [RFC v2] vhost: introduce mdev based hardware vhost backend
From: Tiwei Bie @ 2019-07-03 11:52 UTC (permalink / raw)
To: Jason Wang
Cc: mst, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang
In-Reply-To: <7b8279b2-aa7e-7adc-eeff-20dfaf4400d0@redhat.com>
On Wed, Jul 03, 2019 at 06:09:51PM +0800, Jason Wang wrote:
> On 2019/7/3 下午5:13, Tiwei Bie wrote:
> > Details about this can be found here:
> >
> > https://lwn.net/Articles/750770/
> >
> > What's new in this version
> > ==========================
> >
> > A new VFIO device type is introduced - vfio-vhost. This addressed
> > some comments from here: https://patchwork.ozlabs.org/cover/984763/
> >
> > Below is the updated device interface:
> >
> > Currently, there are two regions of this device: 1) CONFIG_REGION
> > (VFIO_VHOST_CONFIG_REGION_INDEX), which can be used to setup the
> > device; 2) NOTIFY_REGION (VFIO_VHOST_NOTIFY_REGION_INDEX), which
> > can be used to notify the device.
> >
> > 1. CONFIG_REGION
> >
> > The region described by CONFIG_REGION is the main control interface.
> > Messages will be written to or read from this region.
> >
> > The message type is determined by the `request` field in message
> > header. The message size is encoded in the message header too.
> > The message format looks like this:
> >
> > struct vhost_vfio_op {
> > __u64 request;
> > __u32 flags;
> > /* Flag values: */
> > #define VHOST_VFIO_NEED_REPLY 0x1 /* Whether need reply */
> > __u32 size;
> > union {
> > __u64 u64;
> > struct vhost_vring_state state;
> > struct vhost_vring_addr addr;
> > } payload;
> > };
> >
> > The existing vhost-kernel ioctl cmds are reused as the message
> > requests in above structure.
>
>
> Still a comments like V1. What's the advantage of inventing a new protocol?
I'm trying to make it work in VFIO's way..
> I believe either of the following should be better:
>
> - using vhost ioctl, we can start from SET_VRING_KICK/SET_VRING_CALL and
> extend it with e.g notify region. The advantages is that all exist userspace
> program could be reused without modification (or minimal modification). And
> vhost API hides lots of details that is not necessary to be understood by
> application (e.g in the case of container).
Do you mean reusing vhost's ioctl on VFIO device fd directly,
or introducing another mdev driver (i.e. vhost_mdev instead of
using the existing vfio_mdev) for mdev device?
>
> - using PCI layout, then you don't even need to re-invent notifiy region at
> all and we can pass-through them to guest.
Like what you said previously, virtio has transports other than PCI.
And it will look a bit odd when using transports other than PCI..
>
> Personally, I prefer vhost ioctl.
+1
>
>
> >
[...]
> >
> > 3. VFIO interrupt ioctl API
> >
> > VFIO interrupt ioctl API is used to setup device interrupts.
> > IRQ-bypass can also be supported.
> >
> > Currently, the data path interrupt can be configured via the
> > VFIO_VHOST_VQ_IRQ_INDEX with virtqueue's callfd.
>
>
> How about DMA API? Do you expect to use VFIO IOMMU API or using vhost
> SET_MEM_TABLE? VFIO IOMMU API is more generic for sure but with
> SET_MEM_TABLE DMA can be done at the level of parent device which means it
> can work for e.g the card with on-chip IOMMU.
Agree. In this RFC, it assumes userspace will use VFIO IOMMU API
to do the DMA programming. But like what you said, there could be
a problem when using cards with on-chip IOMMU.
>
> And what's the plan for vIOMMU?
As this RFC assumes userspace will use VFIO IOMMU API, userspace
just needs to follow the same way like what vfio-pci device does
in QEMU to support vIOMMU.
>
>
> >
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> > drivers/vhost/Makefile | 2 +
> > drivers/vhost/vdpa.c | 770 +++++++++++++++++++++++++++++++++++++
> > include/linux/vdpa_mdev.h | 72 ++++
> > include/uapi/linux/vfio.h | 19 +
> > include/uapi/linux/vhost.h | 25 ++
> > 5 files changed, 888 insertions(+)
> > create mode 100644 drivers/vhost/vdpa.c
> > create mode 100644 include/linux/vdpa_mdev.h
>
>
> We probably need some sample parent device implementation. It could be a
> software datapath like e.g we can start from virtio-net device in guest or a
> vhost/tap on host.
Yeah, something like this would be interesting!
Thanks,
Tiwei
>
> Thanks
>
>
> >
^ permalink raw reply
* [PATCH bpf-next] selftests/bpf: fix test_reuseport_array on s390
From: Ilya Leoshkevich @ 2019-07-03 11:50 UTC (permalink / raw)
To: bpf, netdev; +Cc: Ilya Leoshkevich
Fix endianness issue: passing a pointer to 64-bit fd as a 32-bit key
does not work on big-endian architectures. So cast fd to 32-bits when
necessary.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tools/testing/selftests/bpf/test_maps.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c
index a3fbc571280a..5443b9bd75ed 100644
--- a/tools/testing/selftests/bpf/test_maps.c
+++ b/tools/testing/selftests/bpf/test_maps.c
@@ -1418,7 +1418,7 @@ static void test_map_wronly(void)
assert(bpf_map_get_next_key(fd, &key, &value) == -1 && errno == EPERM);
}
-static void prepare_reuseport_grp(int type, int map_fd,
+static void prepare_reuseport_grp(int type, int map_fd, size_t map_elem_size,
__s64 *fds64, __u64 *sk_cookies,
unsigned int n)
{
@@ -1428,6 +1428,8 @@ static void prepare_reuseport_grp(int type, int map_fd,
const int optval = 1;
unsigned int i;
u64 sk_cookie;
+ void *value;
+ __s32 fd32;
__s64 fd64;
int err;
@@ -1449,8 +1451,14 @@ static void prepare_reuseport_grp(int type, int map_fd,
"err:%d errno:%d\n", err, errno);
/* reuseport_array does not allow unbound sk */
- err = bpf_map_update_elem(map_fd, &index0, &fd64,
- BPF_ANY);
+ if (map_elem_size == sizeof(__u64))
+ value = &fd64;
+ else {
+ assert(map_elem_size == sizeof(__u32));
+ fd32 = (__s32)fd64;
+ value = &fd32;
+ }
+ err = bpf_map_update_elem(map_fd, &index0, value, BPF_ANY);
CHECK(err != -1 || errno != EINVAL,
"reuseport array update unbound sk",
"sock_type:%d err:%d errno:%d\n",
@@ -1478,7 +1486,7 @@ static void prepare_reuseport_grp(int type, int map_fd,
* reuseport_array does not allow
* non-listening tcp sk.
*/
- err = bpf_map_update_elem(map_fd, &index0, &fd64,
+ err = bpf_map_update_elem(map_fd, &index0, value,
BPF_ANY);
CHECK(err != -1 || errno != EINVAL,
"reuseport array update non-listening sk",
@@ -1541,7 +1549,7 @@ static void test_reuseport_array(void)
for (t = 0; t < ARRAY_SIZE(types); t++) {
type = types[t];
- prepare_reuseport_grp(type, map_fd, grpa_fds64,
+ prepare_reuseport_grp(type, map_fd, sizeof(__u64), grpa_fds64,
grpa_cookies, ARRAY_SIZE(grpa_fds64));
/* Test BPF_* update flags */
@@ -1649,7 +1657,8 @@ static void test_reuseport_array(void)
sizeof(__u32), sizeof(__u32), array_size, 0);
CHECK(map_fd == -1, "reuseport array create",
"map_fd:%d, errno:%d\n", map_fd, errno);
- prepare_reuseport_grp(SOCK_STREAM, map_fd, &fd64, &sk_cookie, 1);
+ prepare_reuseport_grp(SOCK_STREAM, map_fd, sizeof(__u32), &fd64,
+ &sk_cookie, 1);
fd = fd64;
err = bpf_map_update_elem(map_fd, &index3, &fd, BPF_NOEXIST);
CHECK(err == -1, "reuseport array update 32 bit fd",
--
2.21.0
^ permalink raw reply related
* Re: [PATCH net-next v6 06/15] ethtool: netlink bitset handling
From: Jiri Pirko @ 2019-07-03 11:49 UTC (permalink / raw)
To: Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <cb614bebee1686293127194e8f7ced72955c7c7f.1562067622.git.mkubecek@suse.cz>
Tue, Jul 02, 2019 at 01:50:09PM CEST, mkubecek@suse.cz wrote:
>The ethtool netlink code uses common framework for passing arbitrary
>length bit sets to allow future extensions. A bitset can be a list (only
>one bitmap) or can consist of value and mask pair (used e.g. when client
>want to modify only some bits). A bitset can use one of two formats:
>verbose (bit by bit) or compact.
>
>Verbose format consists of bitset size (number of bits), list flag and
>an array of bit nests, telling which bits are part of the list or which
>bits are in the mask and which of them are to be set. In requests, bits
>can be identified by index (position) or by name. In replies, kernel
>provides both index and name. Verbose format is suitable for "one shot"
>applications like standard ethtool command as it avoids the need to
>either keep bit names (e.g. link modes) in sync with kernel or having to
>add an extra roundtrip for string set request (e.g. for private flags).
>
>Compact format uses one (list) or two (value/mask) arrays of 32-bit
>words to store the bitmap(s). It is more suitable for long running
>applications (ethtool in monitor mode or network management daemons)
>which can retrieve the names once and then pass only compact bitmaps to
>save space.
>
>Userspace requests can use either format and ETHTOOL_RF_COMPACT flag in
>request header tells kernel which format to use in reply. Notifications
>always use compact format.
>
>Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>---
> Documentation/networking/ethtool-netlink.txt | 61 ++
> include/uapi/linux/ethtool_netlink.h | 35 ++
> net/ethtool/Makefile | 2 +-
> net/ethtool/bitset.c | 606 +++++++++++++++++++
> net/ethtool/bitset.h | 40 ++
> net/ethtool/netlink.h | 9 +
> 6 files changed, 752 insertions(+), 1 deletion(-)
> create mode 100644 net/ethtool/bitset.c
> create mode 100644 net/ethtool/bitset.h
>
>diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
>index 97c369aa290b..4636682c551f 100644
>--- a/Documentation/networking/ethtool-netlink.txt
>+++ b/Documentation/networking/ethtool-netlink.txt
>@@ -73,6 +73,67 @@ set, the behaviour is the same as (or closer to) the behaviour before it was
> introduced.
>
>
>+Bit sets
>+--------
>+
>+For short bitmaps of (reasonably) fixed length, standard NLA_BITFIELD32 type
>+is used. For arbitrary length bitmaps, ethtool netlink uses a nested attribute
>+with contents of one of two forms: compact (two binary bitmaps representing
>+bit values and mask of affected bits) and bit-by-bit (list of bits identified
>+by either index or name).
>+
>+Compact form: nested (bitset) atrribute contents:
>+
>+ ETHTOOL_A_BITSET_LIST (flag) no mask, only a list
>+ ETHTOOL_A_BITSET_SIZE (u32) number of significant bits
>+ ETHTOOL_A_BITSET_VALUE (binary) bitmap of bit values
>+ ETHTOOL_A_BITSET_MASK (binary) bitmap of valid bits
>+
>+Value and mask must have length at least ETHTOOL_A_BITSET_SIZE bits rounded up
>+to a multiple of 32 bits. They consist of 32-bit words in host byte order,
Looks like the blocks are similar to NLA_BITFIELD32. Why don't you user
nested array of NLA_BITFIELD32 instead?
>+words ordered from least significant to most significant (i.e. the same way as
>+bitmaps are passed with ioctl interface).
>+
>+For compact form, ETHTOOL_A_BITSET_SIZE and ETHTOOL_A_BITSET_VALUE are
>+mandatory. Similar to BITFIELD32, a compact form bit set requests to set bits
Double space^^
>+in the mask to 1 (if the bit is set in value) or 0 (if not) and preserve the
>+rest. If ETHTOOL_A_BITSET_LIST is present, there is no mask and bitset
>+represents a simple list of bits.
Okay, that is a bit confusing. Why not to rename to something like:
ETHTOOL_A_BITSET_NO_MASK (flag)
?
>+
>+Kernel bit set length may differ from userspace length if older application is
>+used on newer kernel or vice versa. If userspace bitmap is longer, an error is
>+issued only if the request actually tries to set values of some bits not
>+recognized by kernel.
>+
>+Bit-by-bit form: nested (bitset) attribute contents:
>+
>+ ETHTOOL_A_BITSET_LIST (flag) no mask, only a list
>+ ETHTOOL_A_BITSET_SIZE (u32) number of significant bits
>+ ETHTOOL_A_BITSET_BIT (nested) array of bits
>+ ETHTOOL_A_BITSET_BIT+ (nested) one bit
>+ ETHTOOL_A_BIT_INDEX (u32) bit index (0 for LSB)
>+ ETHTOOL_A_BIT_NAME (string) bit name
>+ ETHTOOL_A_BIT_VALUE (flag) present if bit is set
>+
>+Bit size is optional for bit-by-bit form. ETHTOOL_A_BITSET_BITS nest can only
>+contain ETHTOOL_A_BITS_BIT attributes but there can be an arbitrary number of
>+them. A bit may be identified by its index or by its name. When used in
>+requests, listed bits are set to 0 or 1 according to ETHTOOL_A_BIT_VALUE, the
>+rest is preserved. A request fails if index exceeds kernel bit length or if
>+name is not recognized.
>+
>+When ETHTOOL_A_BITSET_LIST flag is present, bitset is interpreted as a simple
>+bit list. ETHTOOL_A_BIT_VALUE attributes are not used in such case. Bit list
>+represents a bitmap with listed bits set and the rest zero.
>+
>+In requests, application can use either form. Form used by kernel in reply is
>+determined by a flag in flags field of request header. Semantics of value and
>+mask depends on the attribute. General idea is that flags control request
>+processing, info_mask control which parts of the information are returned in
>+"get" request and index identifies a particular subcommand or an object to
>+which the request applies.
This is quite complex and confusing. Having the same API for 2 APIs is
odd. The API should be crystal clear, easy to use.
Why can't you have 2 commands, one working with bit arrays only, one
working with strings? Something like:
X_GET
ETHTOOL_A_BITS (nested)
ETHTOOL_A_BIT_ARRAY (BITFIELD32)
X_NAMES_GET
ETHTOOL_A_BIT_NAMES (nested)
ETHTOOL_A_BIT_INDEX
ETHTOOL_A_BIT_NAME
For set, you can also have multiple cmds:
X_SET - to set many at once, by bit index
ETHTOOL_A_BITS (nested)
ETHTOOL_A_BIT_ARRAY (BITFIELD32)
X_ONE_SET - to set one, by bit index
ETHTOOL_A_BIT_INDEX
ETHTOOL_A_BIT_VALUE
X_ONE_SET - to set one, by name
ETHTOOL_A_BIT_NAME
ETHTOOL_A_BIT_VALUE
[...]
^ permalink raw reply
* Re: [PATCH] bpf, libbpf: Smatch: Fix potential NULL pointer dereference
From: Leo Yan @ 2019-07-03 11:46 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
netdev, bpf, linux-kernel, Dan Carpenter
In-Reply-To: <b834fba1-5b2c-4406-8275-1cf8383655e3@iogearbox.net>
On Wed, Jul 03, 2019 at 12:23:05PM +0200, Daniel Borkmann wrote:
> On 07/02/2019 12:25 PM, Leo Yan wrote:
> > Based on the following report from Smatch, fix the potential
> > NULL pointer dereference check.
> >
> > tools/lib/bpf/libbpf.c:3493
> > bpf_prog_load_xattr() warn: variable dereferenced before check 'attr'
> > (see line 3483)
> >
> > 3479 int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> > 3480 struct bpf_object **pobj, int *prog_fd)
> > 3481 {
> > 3482 struct bpf_object_open_attr open_attr = {
> > 3483 .file = attr->file,
> > 3484 .prog_type = attr->prog_type,
> > ^^^^^^
> > 3485 };
> >
> > At the head of function, it directly access 'attr' without checking if
> > it's NULL pointer. This patch moves the values assignment after
> > validating 'attr' and 'attr->file'.
> >
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > tools/lib/bpf/libbpf.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 197b574406b3..809b633fa3d9 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -3479,10 +3479,7 @@ int bpf_prog_load(const char *file, enum bpf_prog_type type,
> > int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> > struct bpf_object **pobj, int *prog_fd)
> > {
> > - struct bpf_object_open_attr open_attr = {
> > - .file = attr->file,
> > - .prog_type = attr->prog_type,
> > - };
>
> Applied, thanks! Fyi, I retained the zeroing of open_attr as otherwise if we ever
> extend struct bpf_object_open_attr in future, we'll easily miss this and pass in
> garbage to bpf_object__open_xattr().
Thanks for the info, Daniel.
I checked the link [1] and thanks for the improvement when applied this
patch.
Thanks,
Leo Yan
[1] https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=33bae185f74d49a0d7b1bfaafb8e959efce0f243
^ permalink raw reply
* Re: [PATCH 1/2 nf-next v3] netfilter: nft_meta: Add NFT_META_BRI_IIFVPROTO support
From: Pablo Neira Ayuso @ 2019-07-03 11:41 UTC (permalink / raw)
To: wenxu; +Cc: fw, netfilter-devel, netdev
In-Reply-To: <20190702234629.zgjpu664dr6jgawc@salvia>
On Wed, Jul 03, 2019 at 01:46:29AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Jun 28, 2019 at 08:49:34AM +0800, wenxu@ucloud.cn wrote:
> > From: wenxu <wenxu@ucloud.cn>
> >
> > This patch provide a meta to get the bridge vlan proto
> >
> > nft add rule bridge firewall zones counter meta br_vlan_proto 0x8100
>
> Applied, thanks.
Tossing this:
net/netfilter/nft_meta.c: In function ‘nft_meta_get_eval’:
net/netfilter/nft_meta.c:255:31: error: ‘struct net_bridge’ has no
member named ‘vlan_proto’
nft_reg_store16(dest, p->br->vlan_proto);
^~
^ permalink raw reply
* [PATCH v2] rt2x00: no need to check return value of debugfs_create functions
From: Greg Kroah-Hartman @ 2019-07-03 11:39 UTC (permalink / raw)
To: Stanislaw Gruszka, Helmut Schaa, Kalle Valo
Cc: David S. Miller, linux-wireless, netdev
In-Reply-To: <20190703065631.GA28822@kroah.com>
When calling debugfs functions, there is no need to ever check the
return value. The function can work or not, but the code logic should
never do something different based on this.
Because we don't need to save the individual debugfs files and
directories, remove the local storage of them and just remove the entire
debugfs directory in a single call, making things a lot simpler.
Cc: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Helmut Schaa <helmut.schaa@googlemail.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: - rebase against wireless-drivers-next
- clean up the debugfs "blob" creation logic a bit more, no need to
care about that return value.
.../net/wireless/ralink/rt2x00/rt2x00debug.c | 136 +++++-------------
1 file changed, 35 insertions(+), 101 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
index ef5f51512212..4d4e3888ef20 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
@@ -65,26 +65,6 @@ struct rt2x00debug_intf {
* - crypto stats file
*/
struct dentry *driver_folder;
- struct dentry *driver_entry;
- struct dentry *chipset_entry;
- struct dentry *dev_flags;
- struct dentry *cap_flags;
- struct dentry *restart_hw;
- struct dentry *register_folder;
- struct dentry *csr_off_entry;
- struct dentry *csr_val_entry;
- struct dentry *eeprom_off_entry;
- struct dentry *eeprom_val_entry;
- struct dentry *bbp_off_entry;
- struct dentry *bbp_val_entry;
- struct dentry *rf_off_entry;
- struct dentry *rf_val_entry;
- struct dentry *rfcsr_off_entry;
- struct dentry *rfcsr_val_entry;
- struct dentry *queue_folder;
- struct dentry *queue_frame_dump_entry;
- struct dentry *queue_stats_entry;
- struct dentry *crypto_stats_entry;
/*
* The frame dump file only allows a single reader,
@@ -596,39 +576,34 @@ static const struct file_operations rt2x00debug_restart_hw = {
.llseek = generic_file_llseek,
};
-static struct dentry *rt2x00debug_create_file_driver(const char *name,
- struct rt2x00debug_intf
- *intf,
- struct debugfs_blob_wrapper
- *blob)
+static void rt2x00debug_create_file_driver(const char *name,
+ struct rt2x00debug_intf *intf,
+ struct debugfs_blob_wrapper *blob)
{
char *data;
data = kzalloc(3 * MAX_LINE_LENGTH, GFP_KERNEL);
if (!data)
- return NULL;
+ return;
blob->data = data;
data += sprintf(data, "driver:\t%s\n", intf->rt2x00dev->ops->name);
data += sprintf(data, "version:\t%s\n", DRV_VERSION);
blob->size = strlen(blob->data);
- return debugfs_create_blob(name, 0400, intf->driver_folder, blob);
+ debugfs_create_blob(name, 0400, intf->driver_folder, blob);
}
-static struct dentry *rt2x00debug_create_file_chipset(const char *name,
- struct rt2x00debug_intf
- *intf,
- struct
- debugfs_blob_wrapper
- *blob)
+static void rt2x00debug_create_file_chipset(const char *name,
+ struct rt2x00debug_intf *intf,
+ struct debugfs_blob_wrapper *blob)
{
const struct rt2x00debug *debug = intf->debug;
char *data;
data = kzalloc(9 * MAX_LINE_LENGTH, GFP_KERNEL);
if (!data)
- return NULL;
+ return;
blob->data = data;
data += sprintf(data, "rt chip:\t%04x\n", intf->rt2x00dev->chip.rt);
@@ -654,13 +629,15 @@ static struct dentry *rt2x00debug_create_file_chipset(const char *name,
blob->size = strlen(blob->data);
- return debugfs_create_blob(name, 0400, intf->driver_folder, blob);
+ debugfs_create_blob(name, 0400, intf->driver_folder, blob);
}
void rt2x00debug_register(struct rt2x00_dev *rt2x00dev)
{
const struct rt2x00debug *debug = rt2x00dev->ops->debugfs;
struct rt2x00debug_intf *intf;
+ struct dentry *queue_folder;
+ struct dentry *register_folder;
intf = kzalloc(sizeof(struct rt2x00debug_intf), GFP_KERNEL);
if (!intf) {
@@ -676,43 +653,27 @@ void rt2x00debug_register(struct rt2x00_dev *rt2x00dev)
debugfs_create_dir(intf->rt2x00dev->ops->name,
rt2x00dev->hw->wiphy->debugfsdir);
- intf->driver_entry =
- rt2x00debug_create_file_driver("driver", intf, &intf->driver_blob);
+ rt2x00debug_create_file_driver("driver", intf, &intf->driver_blob);
+ rt2x00debug_create_file_chipset("chipset", intf, &intf->chipset_blob);
+ debugfs_create_file("dev_flags", 0400, intf->driver_folder, intf,
+ &rt2x00debug_fop_dev_flags);
+ debugfs_create_file("cap_flags", 0400, intf->driver_folder, intf,
+ &rt2x00debug_fop_cap_flags);
+ debugfs_create_file("restart_hw", 0200, intf->driver_folder, intf,
+ &rt2x00debug_restart_hw);
- intf->chipset_entry =
- rt2x00debug_create_file_chipset("chipset",
- intf, &intf->chipset_blob);
-
- intf->dev_flags = debugfs_create_file("dev_flags", 0400,
- intf->driver_folder, intf,
- &rt2x00debug_fop_dev_flags);
-
- intf->cap_flags = debugfs_create_file("cap_flags", 0400,
- intf->driver_folder, intf,
- &rt2x00debug_fop_cap_flags);
-
- intf->restart_hw = debugfs_create_file("restart_hw", 0200,
- intf->driver_folder, intf,
- &rt2x00debug_restart_hw);
-
- intf->register_folder =
- debugfs_create_dir("register", intf->driver_folder);
+ register_folder = debugfs_create_dir("register", intf->driver_folder);
#define RT2X00DEBUGFS_CREATE_REGISTER_ENTRY(__intf, __name) \
({ \
if (debug->__name.read) { \
- (__intf)->__name##_off_entry = \
- debugfs_create_u32(__stringify(__name) "_offset", \
- 0600, \
- (__intf)->register_folder, \
- &(__intf)->offset_##__name); \
+ debugfs_create_u32(__stringify(__name) "_offset", 0600, \
+ register_folder, \
+ &(__intf)->offset_##__name); \
\
- (__intf)->__name##_val_entry = \
- debugfs_create_file(__stringify(__name) "_value", \
- 0600, \
- (__intf)->register_folder, \
- (__intf), \
- &rt2x00debug_fop_##__name); \
+ debugfs_create_file(__stringify(__name) "_value", 0600, \
+ register_folder, (__intf), \
+ &rt2x00debug_fop_##__name); \
} \
})
@@ -724,26 +685,21 @@ void rt2x00debug_register(struct rt2x00_dev *rt2x00dev)
#undef RT2X00DEBUGFS_CREATE_REGISTER_ENTRY
- intf->queue_folder =
- debugfs_create_dir("queue", intf->driver_folder);
+ queue_folder = debugfs_create_dir("queue", intf->driver_folder);
- intf->queue_frame_dump_entry =
- debugfs_create_file("dump", 0400, intf->queue_folder,
- intf, &rt2x00debug_fop_queue_dump);
+ debugfs_create_file("dump", 0400, queue_folder, intf,
+ &rt2x00debug_fop_queue_dump);
skb_queue_head_init(&intf->frame_dump_skbqueue);
init_waitqueue_head(&intf->frame_dump_waitqueue);
- intf->queue_stats_entry =
- debugfs_create_file("queue", 0400, intf->queue_folder,
- intf, &rt2x00debug_fop_queue_stats);
+ debugfs_create_file("queue", 0400, queue_folder, intf,
+ &rt2x00debug_fop_queue_stats);
#ifdef CONFIG_RT2X00_LIB_CRYPTO
if (rt2x00_has_cap_hw_crypto(rt2x00dev))
- intf->crypto_stats_entry =
- debugfs_create_file("crypto", 0444, intf->queue_folder,
- intf,
- &rt2x00debug_fop_crypto_stats);
+ debugfs_create_file("crypto", 0444, queue_folder, intf,
+ &rt2x00debug_fop_crypto_stats);
#endif
return;
@@ -758,29 +714,7 @@ void rt2x00debug_deregister(struct rt2x00_dev *rt2x00dev)
skb_queue_purge(&intf->frame_dump_skbqueue);
-#ifdef CONFIG_RT2X00_LIB_CRYPTO
- debugfs_remove(intf->crypto_stats_entry);
-#endif
- debugfs_remove(intf->queue_stats_entry);
- debugfs_remove(intf->queue_frame_dump_entry);
- debugfs_remove(intf->queue_folder);
- debugfs_remove(intf->rfcsr_val_entry);
- debugfs_remove(intf->rfcsr_off_entry);
- debugfs_remove(intf->rf_val_entry);
- debugfs_remove(intf->rf_off_entry);
- debugfs_remove(intf->bbp_val_entry);
- debugfs_remove(intf->bbp_off_entry);
- debugfs_remove(intf->eeprom_val_entry);
- debugfs_remove(intf->eeprom_off_entry);
- debugfs_remove(intf->csr_val_entry);
- debugfs_remove(intf->csr_off_entry);
- debugfs_remove(intf->register_folder);
- debugfs_remove(intf->dev_flags);
- debugfs_remove(intf->restart_hw);
- debugfs_remove(intf->cap_flags);
- debugfs_remove(intf->chipset_entry);
- debugfs_remove(intf->driver_entry);
- debugfs_remove(intf->driver_folder);
+ debugfs_remove_recursive(intf->driver_folder);
kfree(intf->chipset_blob.data);
kfree(intf->driver_blob.data);
kfree(intf);
--
2.22.0
^ permalink raw reply related
* Re: [PATCH] netfilter: nf_log: Replace a seq_printf() call by seq_puts() in seq_show()
From: Pablo Neira Ayuso @ 2019-07-03 11:38 UTC (permalink / raw)
To: Markus Elfring
Cc: netfilter-devel, coreteam, netdev, David S. Miller,
Florian Westphal, Jozsef Kadlecsik, LKML, kernel-janitors
In-Reply-To: <c7d397c8-4f41-1831-505f-b3fbcc3663fb@web.de>
On Tue, Jul 02, 2019 at 08:11:53PM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 2 Jul 2019 20:06:30 +0200
>
> A string which did not contain a data format specification should be put
> into a sequence. Thus use the corresponding function “seq_puts”.
>
> This issue was detected by using the Coccinelle software.
Applied, thanks.
^ permalink raw reply
* [PATCH net] net: hns: add support for vlan TSO
From: Yonglong Liu @ 2019-07-03 11:12 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-kernel, linuxarm, salil.mehta, yisen.zhuang,
shiju.jose
The hip07 chip support vlan TSO, this patch adds NETIF_F_TSO
and NETIF_F_TSO6 flags to vlan_features to improve the
performance after adding vlan to the net ports.
Signed-off-by: Yonglong Liu <liuyonglong@huawei.com>
---
drivers/net/ethernet/hisilicon/hns/hns_enet.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
index fe879c0..2235dd5 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -2370,6 +2370,7 @@ static int hns_nic_dev_probe(struct platform_device *pdev)
ndev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_GSO |
NETIF_F_GRO | NETIF_F_TSO | NETIF_F_TSO6;
+ ndev->vlan_features |= NETIF_F_TSO | NETIF_F_TSO6;
ndev->max_mtu = MAC_MAX_MTU_V2 -
(ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN);
break;
--
2.8.1
^ permalink raw reply related
* Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink interface
From: Michal Kubecek @ 2019-07-03 11:13 UTC (permalink / raw)
To: netdev
Cc: Jiri Pirko, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190703100435.GS2250@nanopsycho>
On Wed, Jul 03, 2019 at 12:04:35PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 06:34:37PM CEST, mkubecek@suse.cz wrote:
> >On Tue, Jul 02, 2019 at 03:05:15PM +0200, Jiri Pirko wrote:
> >> Tue, Jul 02, 2019 at 01:50:04PM CEST, mkubecek@suse.cz wrote:
> >> >+
> >> >+ req_info->dev = dev;
> >> >+ ethnl_update_u32(&req_info->req_mask, tb[ETHTOOL_A_HEADER_INFOMASK]);
> >> >+ ethnl_update_u32(&req_info->global_flags, tb[ETHTOOL_A_HEADER_GFLAGS]);
> >> >+ ethnl_update_u32(&req_info->req_flags, tb[ETHTOOL_A_HEADER_RFLAGS]);
> >>
> >> Just:
> >> req_info->req_mask = nla_get_u32(tb[ETHTOOL_A_HEADER_INFOMASK];
> >> ...
> >>
> >> Not sure what ethnl_update_u32() is good for, but it is not needed here.
> >
> >That would result in null pointer dereference if the attribute is
> >missing. So you would need at least
> >
> > if (tb[ETHTOOL_A_HEADER_INFOMASK])
> > req_info->req_mask = nla_get_u32(tb[ETHTOOL_A_HEADER_INFOMASK]);
> > if (tb[ETHTOOL_A_HEADER_GFLAGS])
> > req_info->global_flags =
> > nla_get_u32(tb[ETHTOOL_A_HEADER_GFLAGS]);
> > if (tb[ETHTOOL_A_HEADER_RFLAGS])
> > req_info->req_flags = nla_get_u32(tb[ETHTOOL_A_HEADER_RFLAGS]);
>
> Yeah, sure.
>
> >
> >I don't think it looks better.
>
> Better than hiding something inside a helper in my opinion - helper that
> is there for different reason moreover. Much easier to read the code
> and follow.
OK, I'll use nla_get_u32() directly here. With the change below, use of
ethnl_update_u32() would really look unnatural.
> >> >+/* The ethnl_update_* helpers set value pointed to by @dst to the value of
> >> >+ * netlink attribute @attr (if attr is not null). They return true if *dst
> >> >+ * value was changed, false if not.
> >> >+ */
> >> >+static inline bool ethnl_update_u32(u32 *dst, struct nlattr *attr)
> >>
> >> I'm still not sure I'm convinced about these "update helpers" :)
> >
> >Just imagine what would e.g.
> >
> > if (ethnl_update_u32(&data.rx_pending, tb[ETHTOOL_A_RING_RX_PENDING]))
> > mod = true;
> > if (ethnl_update_u32(&data.rx_mini_pending,
> > tb[ETHTOOL_A_RING_RX_MINI_PENDING]))
> > mod = true;
> > if (ethnl_update_u32(&data.rx_jumbo_pending,
> > tb[ETHTOOL_A_RING_RX_JUMBO_PENDING]))
> > mod = true;
> > if (ethnl_update_u32(&data.tx_pending, tb[ETHTOOL_A_RING_TX_PENDING]))
> > mod = true;
> > if (!mod)
> > return 0;
> >
> >look like without them. And coalescing parameters would be much worse
> >(22 attributes / struct members).
>
> No, I understand your motivation, don't get me wrong. I just wonder that
> no other netlink implementation need such mechanism. Maybe I'm not
> looking close enough. But if it does, should be rathe netlink helper.
I'll check some existing interfaces to see how they handle "set" type
requests.
> Regarding the example code you have here. It is prefered to store
> function result in a variable "if check" that variable. But in your,
> code, couldn't this be done without ifs?
>
> bool mod = false;
>
> ethnl_update_u32(&mod, &data.rx_pending, tb[ETHTOOL_A_RING_RX_PENDING]))
> ethnl_update_u32(&mod, &data.rx_mini_pending,
> tb[ETHTOOL_A_RING_RX_MINI_PENDING]))
> ethnl_update_u32(&mod, &data.rx_jumbo_pending,
> tb[ETHTOOL_A_RING_RX_JUMBO_PENDING]))
> ethnl_update_u32(&mod, &data.tx_pending, tb[ETHTOOL_A_RING_TX_PENDING]))
>
> if (!mod)
> return 0;
Ah, right. Somehow I completely missed the possibility that update
helper can use "set of leave as it is" logic instead of "set to true or
false". Thanks, I'll rewrite the update helpers to this style.
Michal
^ permalink raw reply
* bug: tpacket_snd can cause data corruption
From: Frank de Brabander @ 2019-07-03 11:07 UTC (permalink / raw)
To: David S . Miller, Willem de Bruijn; +Cc: netdev, Frank de Brabander
In commit 5cd8d46e a fix was applied for data corruption in
tpacket_snd. A selftest was added in commit 358be656 which
validates this fix.
Unfortunately this bug still persists, although since this fix less
likely to trigger. This bug was initially observed using a PACKET_MMAP
application, but can also be seen by tweaking the kernel selftest.
By tweaking the selftest txring_overwrite.c to run
as an infinite loop, the data corruption will still trigger. It
seems to occur faster by generating interrupts (e.g. by plugging
in USB devices). Tested with kernel version 5.2-RC7.
Cause for this bug is still unclear.
Signed-off-by: Frank de Brabander <debrabander@gmail.com>
---
tools/testing/selftests/net/txring_overwrite.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/net/txring_overwrite.c b/tools/testing/selftests/net/txring_overwrite.c
index fd8b1c6..3ee23e5 100644
--- a/tools/testing/selftests/net/txring_overwrite.c
+++ b/tools/testing/selftests/net/txring_overwrite.c
@@ -143,19 +143,22 @@ static int read_verify_pkt(int fdr, char payload_char)
int ret;
ret = read(fdr, buf, sizeof(buf));
- if (ret != sizeof(buf))
- error(1, errno, "read");
+ if (ret != sizeof(buf)) {
+ //error(1, errno, "read");
+ printf("read error\n");
+ return 1;
+ }
if (buf[60] != payload_char) {
printf("wrong pattern: 0x%x != 0x%x\n", buf[60], payload_char);
return 1;
}
- printf("read: %c (0x%x)\n", buf[60], buf[60]);
+ //printf("read: %c (0x%x)\n", buf[60], buf[60]);
return 0;
}
-int main(int argc, char **argv)
+void run_test(void)
{
const char payload_patterns[] = "ab";
char *ring;
@@ -177,3 +180,10 @@ int main(int argc, char **argv)
return ret;
}
+
+int main(int argc, char **argv)
+{
+ while (true) {
+ run_test();
+ }
+}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] rt2x00: no need to check return value of debugfs_create functions
From: Greg Kroah-Hartman @ 2019-07-03 11:08 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Helmut Schaa, Kalle Valo, David S. Miller, linux-wireless, netdev
In-Reply-To: <20190703105759.GB30509@redhat.com>
On Wed, Jul 03, 2019 at 12:58:00PM +0200, Stanislaw Gruszka wrote:
> On Wed, Jul 03, 2019 at 08:56:31AM +0200, Greg Kroah-Hartman wrote:
> > When calling debugfs functions, there is no need to ever check the
> > return value. The function can work or not, but the code logic should
> > never do something different based on this.
> >
> > Because we don't need to save the individual debugfs files and
> > directories, remove the local storage of them and just remove the entire
> > debugfs directory in a single call, making things a lot simpler.
> >
> > Cc: Stanislaw Gruszka <sgruszka@redhat.com>
> > Cc: Helmut Schaa <helmut.schaa@googlemail.com>
> > Cc: Kalle Valo <kvalo@codeaurora.org>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: linux-wireless@vger.kernel.org
> > Cc: netdev@vger.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > .../net/wireless/ralink/rt2x00/rt2x00debug.c | 100 ++++--------------
> > 1 file changed, 23 insertions(+), 77 deletions(-)
>
> This patch will not apply on wireless-drivers-next due to my recent
> change which add new debugfs file:
>
> commit e7f15d58dfe43f18199251f430d7713b0b8fad34
> Author: Stanislaw Gruszka <sgruszka@redhat.com>
> Date: Thu May 2 11:07:00 2019 +0200
>
> rt2x00: add restart hw
>
> Could you please rebase the patch ? (I can do this as well
> if you want me to).
No problem, I can rebase and redo it, I'll go suck down the
wireless-drivers-next tree and resend it, thanks for leting me know.
greg k-h
^ permalink raw reply
* Re: [PATCH] rt2x00: no need to check return value of debugfs_create functions
From: Stanislaw Gruszka @ 2019-07-03 10:58 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Helmut Schaa, Kalle Valo, David S. Miller, linux-wireless, netdev
In-Reply-To: <20190703065631.GA28822@kroah.com>
On Wed, Jul 03, 2019 at 08:56:31AM +0200, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value. The function can work or not, but the code logic should
> never do something different based on this.
>
> Because we don't need to save the individual debugfs files and
> directories, remove the local storage of them and just remove the entire
> debugfs directory in a single call, making things a lot simpler.
>
> Cc: Stanislaw Gruszka <sgruszka@redhat.com>
> Cc: Helmut Schaa <helmut.schaa@googlemail.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-wireless@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> .../net/wireless/ralink/rt2x00/rt2x00debug.c | 100 ++++--------------
> 1 file changed, 23 insertions(+), 77 deletions(-)
This patch will not apply on wireless-drivers-next due to my recent
change which add new debugfs file:
commit e7f15d58dfe43f18199251f430d7713b0b8fad34
Author: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Thu May 2 11:07:00 2019 +0200
rt2x00: add restart hw
Could you please rebase the patch ? (I can do this as well
if you want me to).
Stanislaw
^ permalink raw reply
* linux-next: manual merge of the akpm tree with the net-next tree
From: Stephen Rothwell @ 2019-07-03 10:49 UTC (permalink / raw)
To: Andrew Morton, David Miller, Networking
Cc: Linux Next Mailing List, Linux Kernel Mailing List,
Eiichi Tsukata
[-- Attachment #1: Type: text/plain, Size: 1254 bytes --]
Hi all,
Today's linux-next merge of the akpm tree got a conflict in:
net/ipv6/sysctl_net_ipv6.c
between commit:
00dc3307c0f7 ("net/ipv6: Fix misuse of proc_dointvec "flowlabel_reflect"")
from the net-next tree and patch:
"proc-sysctl-add-shared-variables-for-range-check-fix-2-fix"
from the akpm tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc net/ipv6/sysctl_net_ipv6.c
index 57f520d1bf45,e00cf070e542..000000000000
--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c
@@@ -112,8 -112,8 +112,8 @@@ static struct ctl_table ipv6_table_temp
.data = &init_net.ipv6.sysctl.flowlabel_reflect,
.maxlen = sizeof(int),
.mode = 0644,
- .proc_handler = proc_dointvec,
+ .proc_handler = proc_dointvec_minmax,
- .extra1 = &zero,
+ .extra1 = SYSCTL_ZERO,
.extra2 = &flowlabel_reflect_max,
},
{
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 04/12] net: sched: add tcf_block_setup()
From: Jiri Pirko @ 2019-07-03 10:43 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, netfilter-devel, davem, thomas.lendacky, f.fainelli,
ariel.elior, michael.chan, santosh, madalin.bucur, yisen.zhuang,
salil.mehta, jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch,
jakub.kicinski, peppe.cavallaro, grygorii.strashko, andrew,
vivien.didelot, alexandre.torgue, joabreu, linux-net-drivers,
ganeshgr, ogerlitz, Manish.Chopra, marcelo.leitner, mkubecek,
venkatkumar.duvvuru, cphealy
In-Reply-To: <20190626131626.ihkjqvs2iciski2o@salvia>
Wed, Jun 26, 2019 at 03:16:26PM CEST, pablo@netfilter.org wrote:
>On Wed, Jun 26, 2019 at 02:12:56PM +0200, Jiri Pirko wrote:
>> Tue, Jun 25, 2019 at 10:31:54AM CEST, pablo@netfilter.org wrote:
>> >On Fri, Jun 21, 2019 at 07:16:03PM +0200, Jiri Pirko wrote:
>> >> Thu, Jun 20, 2019 at 09:49:09PM CEST, pablo@netfilter.org wrote:
>> >>
>> >> [...]
>> >>
>> >> >
>> >> >+static LIST_HEAD(tcf_block_cb_list);
>> >>
>> >> I still don't like the global list. Have to go throught the code more
>> >> carefully, but why you can't pass the priv/ctx from tc/netfilter. From
>> >> tc it would be tcf_block as it is now, from netfilter something else.
>> >
>> >This tcf_block_cb_list should go away at some point, once drivers know
>> >how to deal with multiple subsystems using the setup block
>> >infrastructure. As I said in my previous email, only one can set up
>> >the block at this stage, the ones coming later will hit busy.
>>
>> The driver should know if it can bind or is busy. Also, the bind cmd
>> should contain type of binder (tc/nft/whatever) or perhaps rather binder
>> priority (according to the hook order in rx/tx).
>
>OK, so I see two possible paths then:
>
>#1 Add global list and allow one single subsystem to bind by now. Then
> later, in a follow up patchset. Add binder type and priority once
> there is a driver that can handle the three subsystems, remove
> this global list and each driver deals/knows what to do from the
> binder path.
>
>#2 Remove the global list now, each driver maintains a list of flow blocks
> internally, allow one single flow block by now. This will need a bit more
> code, since there will be code in the driver to maintain the list of
> existing flow blocks, per driver, instead of global. So it will be
> a per-driver global local to check if there is a flow block with
> this [ cb, cb_ident ] already in place.
>
>#1 is almost ready - it's this batch :-) - then #2 may need more code -
>this batch is slightly large.
>
>I understand though that path #2 may make it easier for the first
>driver client allowing for the three subsystems to bind.
>
>Let me know what path your prefer.
I definitelly prefer #2. Thanks!
>
>Thanks for reviewing.
^ permalink raw reply
* Re: [PATCH] bpf: Replace a seq_printf() call by seq_puts() in btf_enum_seq_show()
From: Greg KH @ 2019-07-03 10:43 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Markus Elfring, bpf, netdev, Alexei Starovoitov, Martin KaFai Lau,
Song Liu, Yonghong Song, LKML, kernel-janitors
In-Reply-To: <e0c9978f-7304-8a25-1bc9-b2be8a038382@iogearbox.net>
On Wed, Jul 03, 2019 at 12:09:51PM +0200, Daniel Borkmann wrote:
> On 07/02/2019 07:13 PM, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Tue, 2 Jul 2019 19:04:08 +0200
> >
> > A string which did not contain a data format specification should be put
> > into a sequence. Thus use the corresponding function “seq_puts”.
> >
> > This issue was detected by using the Coccinelle software.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>
> The code is fine as is, I'm not applying this.
Just a heads up, this person/bot is in my kill-file, making it easier to
ignore crazy things like this. I recommend it for other maintainers to
also do as well.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 1/3] vsock/virtio: use RCU to avoid use-after-free on the_virtio_vsock
From: Stefano Garzarella @ 2019-07-03 10:41 UTC (permalink / raw)
To: Jason Wang, Stefan Hajnoczi
Cc: netdev, kvm, virtualization, Michael S. Tsirkin, David S. Miller,
linux-kernel
In-Reply-To: <05311244-ed23-d061-a620-7b83d83c11f5@redhat.com>
On Wed, Jul 03, 2019 at 05:53:58PM +0800, Jason Wang wrote:
>
> On 2019/6/28 下午8:36, Stefano Garzarella wrote:
> > Some callbacks used by the upper layers can run while we are in the
> > .remove(). A potential use-after-free can happen, because we free
> > the_virtio_vsock without knowing if the callbacks are over or not.
> >
> > To solve this issue we move the assignment of the_virtio_vsock at the
> > end of .probe(), when we finished all the initialization, and at the
> > beginning of .remove(), before to release resources.
> > For the same reason, we do the same also for the vdev->priv.
> >
> > We use RCU to be sure that all callbacks that use the_virtio_vsock
> > ended before freeing it. This is not required for callbacks that
> > use vdev->priv, because after the vdev->config->del_vqs() we are sure
> > that they are ended and will no longer be invoked.
> >
> > We also take the mutex during the .remove() to avoid that .probe() can
> > run while we are resetting the device.
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> > net/vmw_vsock/virtio_transport.c | 67 +++++++++++++++++++++-----------
> > 1 file changed, 44 insertions(+), 23 deletions(-)
> >
> > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> > index 9c287e3e393c..7ad510ec12e0 100644
> > --- a/net/vmw_vsock/virtio_transport.c
> > +++ b/net/vmw_vsock/virtio_transport.c
> > @@ -65,19 +65,22 @@ struct virtio_vsock {
> > u32 guest_cid;
> > };
> > -static struct virtio_vsock *virtio_vsock_get(void)
> > -{
> > - return the_virtio_vsock;
> > -}
> > -
> > static u32 virtio_transport_get_local_cid(void)
> > {
> > - struct virtio_vsock *vsock = virtio_vsock_get();
> > + struct virtio_vsock *vsock;
> > + u32 ret;
> > - if (!vsock)
> > - return VMADDR_CID_ANY;
> > + rcu_read_lock();
> > + vsock = rcu_dereference(the_virtio_vsock);
> > + if (!vsock) {
> > + ret = VMADDR_CID_ANY;
> > + goto out_rcu;
> > + }
> > - return vsock->guest_cid;
> > + ret = vsock->guest_cid;
> > +out_rcu:
> > + rcu_read_unlock();
> > + return ret;
> > }
> > static void virtio_transport_loopback_work(struct work_struct *work)
> > @@ -197,14 +200,18 @@ virtio_transport_send_pkt(struct virtio_vsock_pkt *pkt)
> > struct virtio_vsock *vsock;
> > int len = pkt->len;
> > - vsock = virtio_vsock_get();
> > + rcu_read_lock();
> > + vsock = rcu_dereference(the_virtio_vsock);
> > if (!vsock) {
> > virtio_transport_free_pkt(pkt);
> > - return -ENODEV;
> > + len = -ENODEV;
> > + goto out_rcu;
> > }
> > - if (le64_to_cpu(pkt->hdr.dst_cid) == vsock->guest_cid)
> > - return virtio_transport_send_pkt_loopback(vsock, pkt);
> > + if (le64_to_cpu(pkt->hdr.dst_cid) == vsock->guest_cid) {
> > + len = virtio_transport_send_pkt_loopback(vsock, pkt);
> > + goto out_rcu;
> > + }
> > if (pkt->reply)
> > atomic_inc(&vsock->queued_replies);
> > @@ -214,6 +221,9 @@ virtio_transport_send_pkt(struct virtio_vsock_pkt *pkt)
> > spin_unlock_bh(&vsock->send_pkt_list_lock);
> > queue_work(virtio_vsock_workqueue, &vsock->send_pkt_work);
> > +
> > +out_rcu:
> > + rcu_read_unlock();
> > return len;
> > }
> > @@ -222,12 +232,14 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
> > {
> > struct virtio_vsock *vsock;
> > struct virtio_vsock_pkt *pkt, *n;
> > - int cnt = 0;
> > + int cnt = 0, ret;
> > LIST_HEAD(freeme);
> > - vsock = virtio_vsock_get();
> > + rcu_read_lock();
> > + vsock = rcu_dereference(the_virtio_vsock);
> > if (!vsock) {
> > - return -ENODEV;
> > + ret = -ENODEV;
> > + goto out_rcu;
> > }
> > spin_lock_bh(&vsock->send_pkt_list_lock);
> > @@ -255,7 +267,11 @@ virtio_transport_cancel_pkt(struct vsock_sock *vsk)
> > queue_work(virtio_vsock_workqueue, &vsock->rx_work);
> > }
> > - return 0;
> > + ret = 0;
> > +
> > +out_rcu:
> > + rcu_read_unlock();
> > + return ret;
> > }
> > static void virtio_vsock_rx_fill(struct virtio_vsock *vsock)
> > @@ -590,8 +606,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> > vsock->rx_buf_max_nr = 0;
> > atomic_set(&vsock->queued_replies, 0);
> > - vdev->priv = vsock;
> > - the_virtio_vsock = vsock;
> > mutex_init(&vsock->tx_lock);
> > mutex_init(&vsock->rx_lock);
> > mutex_init(&vsock->event_lock);
> > @@ -613,6 +627,9 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> > virtio_vsock_event_fill(vsock);
> > mutex_unlock(&vsock->event_lock);
> > + vdev->priv = vsock;
> > + rcu_assign_pointer(the_virtio_vsock, vsock);
>
>
> You probably need to use rcu_dereference_protected() to access
> the_virtio_vsock in the function in order to survive from sparse.
>
Ooo, thanks!
Do you mean when we check if the_virtio_vsock is not null at the beginning of
virtio_vsock_probe()?
>
> > +
> > mutex_unlock(&the_virtio_vsock_mutex);
> > return 0;
> > @@ -627,6 +644,12 @@ static void virtio_vsock_remove(struct virtio_device *vdev)
> > struct virtio_vsock *vsock = vdev->priv;
> > struct virtio_vsock_pkt *pkt;
> > + mutex_lock(&the_virtio_vsock_mutex);
> > +
> > + vdev->priv = NULL;
> > + rcu_assign_pointer(the_virtio_vsock, NULL);
>
>
> This is still suspicious, can we access the_virtio_vsock through vdev->priv?
> If yes, we may still get use-after-free since it was not protected by RCU.
We will free the object only after calling the del_vqs(), so we are sure
that the vq_callbacks ended and will no longer be invoked.
So, IIUC it shouldn't happen.
>
> Another more interesting question, I believe we will do singleton for
> virtio_vsock structure. Then what's the point of using vdev->priv to access
> the_virtio_vsock? It looks to me we can it brings extra troubles for doing
> synchronization.
I thought about it when I tried to use RCU to stop the worker and I
think make sense. Maybe can be another series after this will be merged.
@Stefan, what do you think about that?
Thanks,
Stefano
^ permalink raw reply
* RE: [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool
From: Jose Abreu @ 2019-07-03 10:40 UTC (permalink / raw)
To: Jose Abreu, linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org, Jesper Dangaard Brouer
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai
In-Reply-To: <1b254bb7fc6044c5e6e2fdd9e00088d1d13a808b.1562149883.git.joabreu@synopsys.com>
++ Jesper: Who is most active committer of page pool API (?) ... Can you
please help review this ?
From: Jose Abreu <joabreu@synopsys.com>
> Mapping and unmapping DMA region is an high bottleneck in stmmac driver,
> specially in the RX path.
>
> This commit introduces support for Page Pool API and uses it in all RX
> queues. With this change, we get more stable troughput and some increase
> of banwidth with iperf:
> - MAC1000 - 950 Mbps
> - XGMAC: 9.22 Gbps
>
> 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>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Maxime Ripard <maxime.ripard@bootlin.com>
> Cc: Chen-Yu Tsai <wens@csie.org>
> ---
> drivers/net/ethernet/stmicro/stmmac/Kconfig | 1 +
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 10 +-
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 196 ++++++----------------
> 3 files changed, 63 insertions(+), 144 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index 943189dcccb1..2325b40dff6e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -3,6 +3,7 @@ config STMMAC_ETH
> tristate "STMicroelectronics Multi-Gigabit Ethernet driver"
> depends on HAS_IOMEM && HAS_DMA
> select MII
> + select PAGE_POOL
> select PHYLINK
> select CRC32
> imply PTP_1588_CLOCK
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 513f4e2df5f6..5cd966c154f3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -20,6 +20,7 @@
> #include <linux/ptp_clock_kernel.h>
> #include <linux/net_tstamp.h>
> #include <linux/reset.h>
> +#include <net/page_pool.h>
>
> struct stmmac_resources {
> void __iomem *addr;
> @@ -54,14 +55,19 @@ struct stmmac_tx_queue {
> u32 mss;
> };
>
> +struct stmmac_rx_buffer {
> + struct page *page;
> + dma_addr_t addr;
> +};
> +
> struct stmmac_rx_queue {
> u32 rx_count_frames;
> u32 queue_index;
> + struct page_pool *page_pool;
> + struct stmmac_rx_buffer *buf_pool;
> struct stmmac_priv *priv_data;
> struct dma_extended_desc *dma_erx;
> struct dma_desc *dma_rx ____cacheline_aligned_in_smp;
> - struct sk_buff **rx_skbuff;
> - dma_addr_t *rx_skbuff_dma;
> unsigned int cur_rx;
> unsigned int dirty_rx;
> u32 rx_zeroc_thresh;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index c8fe85ef9a7e..9f44e8193208 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1197,26 +1197,14 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv, struct dma_desc *p,
> int i, gfp_t flags, u32 queue)
> {
> struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
> - struct sk_buff *skb;
> + struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
>
> - skb = __netdev_alloc_skb_ip_align(priv->dev, priv->dma_buf_sz, flags);
> - if (!skb) {
> - netdev_err(priv->dev,
> - "%s: Rx init fails; skb is NULL\n", __func__);
> + buf->page = page_pool_dev_alloc_pages(rx_q->page_pool);
> + if (!buf->page)
> return -ENOMEM;
> - }
> - rx_q->rx_skbuff[i] = skb;
> - rx_q->rx_skbuff_dma[i] = dma_map_single(priv->device, skb->data,
> - priv->dma_buf_sz,
> - DMA_FROM_DEVICE);
> - if (dma_mapping_error(priv->device, rx_q->rx_skbuff_dma[i])) {
> - netdev_err(priv->dev, "%s: DMA mapping error\n", __func__);
> - dev_kfree_skb_any(skb);
> - return -EINVAL;
> - }
> -
> - stmmac_set_desc_addr(priv, p, rx_q->rx_skbuff_dma[i]);
>
> + buf->addr = buf->page->dma_addr;
> + stmmac_set_desc_addr(priv, p, buf->addr);
> if (priv->dma_buf_sz == BUF_SIZE_16KiB)
> stmmac_init_desc3(priv, p);
>
> @@ -1232,13 +1220,10 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv, struct dma_desc *p,
> static void stmmac_free_rx_buffer(struct stmmac_priv *priv, u32 queue, int i)
> {
> struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
> + struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
>
> - if (rx_q->rx_skbuff[i]) {
> - dma_unmap_single(priv->device, rx_q->rx_skbuff_dma[i],
> - priv->dma_buf_sz, DMA_FROM_DEVICE);
> - dev_kfree_skb_any(rx_q->rx_skbuff[i]);
> - }
> - rx_q->rx_skbuff[i] = NULL;
> + page_pool_put_page(rx_q->page_pool, buf->page, false);
> + buf->page = NULL;
> }
>
> /**
> @@ -1321,10 +1306,6 @@ static int init_dma_rx_desc_rings(struct net_device *dev, gfp_t flags)
> queue);
> if (ret)
> goto err_init_rx_buffers;
> -
> - netif_dbg(priv, probe, priv->dev, "[%p]\t[%p]\t[%x]\n",
> - rx_q->rx_skbuff[i], rx_q->rx_skbuff[i]->data,
> - (unsigned int)rx_q->rx_skbuff_dma[i]);
> }
>
> rx_q->cur_rx = 0;
> @@ -1498,8 +1479,9 @@ static void free_dma_rx_desc_resources(struct stmmac_priv *priv)
> sizeof(struct dma_extended_desc),
> rx_q->dma_erx, rx_q->dma_rx_phy);
>
> - kfree(rx_q->rx_skbuff_dma);
> - kfree(rx_q->rx_skbuff);
> + kfree(rx_q->buf_pool);
> + if (rx_q->page_pool)
> + page_pool_request_shutdown(rx_q->page_pool);
> }
> }
>
> @@ -1551,20 +1533,28 @@ static int alloc_dma_rx_desc_resources(struct stmmac_priv *priv)
> /* RX queues buffers and DMA */
> for (queue = 0; queue < rx_count; queue++) {
> struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
> + struct page_pool_params pp_params = { 0 };
>
> rx_q->queue_index = queue;
> rx_q->priv_data = priv;
>
> - rx_q->rx_skbuff_dma = kmalloc_array(DMA_RX_SIZE,
> - sizeof(dma_addr_t),
> - GFP_KERNEL);
> - if (!rx_q->rx_skbuff_dma)
> + pp_params.flags = PP_FLAG_DMA_MAP;
> + pp_params.order = DIV_ROUND_UP(priv->dma_buf_sz, PAGE_SIZE);
> + pp_params.nid = dev_to_node(priv->device);
> + pp_params.dev = priv->device;
> + pp_params.dma_dir = DMA_FROM_DEVICE;
> +
> + rx_q->page_pool = page_pool_create(&pp_params);
> + if (IS_ERR(rx_q->page_pool)) {
> + ret = PTR_ERR(rx_q->page_pool);
> + rx_q->page_pool = NULL;
> goto err_dma;
> + }
>
> - rx_q->rx_skbuff = kmalloc_array(DMA_RX_SIZE,
> - sizeof(struct sk_buff *),
> - GFP_KERNEL);
> - if (!rx_q->rx_skbuff)
> + rx_q->buf_pool = kmalloc_array(DMA_RX_SIZE,
> + sizeof(*rx_q->buf_pool),
> + GFP_KERNEL);
> + if (!rx_q->buf_pool)
> goto err_dma;
>
> if (priv->extend_desc) {
> @@ -3295,9 +3285,8 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
> int dirty = stmmac_rx_dirty(priv, queue);
> unsigned int entry = rx_q->dirty_rx;
>
> - int bfsize = priv->dma_buf_sz;
> -
> while (dirty-- > 0) {
> + struct stmmac_rx_buffer *buf = &rx_q->buf_pool[entry];
> struct dma_desc *p;
> bool use_rx_wd;
>
> @@ -3306,49 +3295,22 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
> else
> p = rx_q->dma_rx + entry;
>
> - if (likely(!rx_q->rx_skbuff[entry])) {
> - struct sk_buff *skb;
> -
> - skb = netdev_alloc_skb_ip_align(priv->dev, bfsize);
> - if (unlikely(!skb)) {
> - /* so for a while no zero-copy! */
> - rx_q->rx_zeroc_thresh = STMMAC_RX_THRESH;
> - if (unlikely(net_ratelimit()))
> - dev_err(priv->device,
> - "fail to alloc skb entry %d\n",
> - entry);
> - break;
> - }
> -
> - rx_q->rx_skbuff[entry] = skb;
> - rx_q->rx_skbuff_dma[entry] =
> - dma_map_single(priv->device, skb->data, bfsize,
> - DMA_FROM_DEVICE);
> - if (dma_mapping_error(priv->device,
> - rx_q->rx_skbuff_dma[entry])) {
> - netdev_err(priv->dev, "Rx DMA map failed\n");
> - dev_kfree_skb(skb);
> + if (!buf->page) {
> + buf->page = page_pool_dev_alloc_pages(rx_q->page_pool);
> + if (!buf->page)
> break;
> - }
> -
> - stmmac_set_desc_addr(priv, p, rx_q->rx_skbuff_dma[entry]);
> - stmmac_refill_desc3(priv, rx_q, p);
> -
> - if (rx_q->rx_zeroc_thresh > 0)
> - rx_q->rx_zeroc_thresh--;
> -
> - netif_dbg(priv, rx_status, priv->dev,
> - "refill entry #%d\n", entry);
> }
> - dma_wmb();
> +
> + buf->addr = buf->page->dma_addr;
> + stmmac_set_desc_addr(priv, p, buf->addr);
> + stmmac_refill_desc3(priv, rx_q, p);
>
> rx_q->rx_count_frames++;
> rx_q->rx_count_frames %= priv->rx_coal_frames;
> use_rx_wd = priv->use_riwt && rx_q->rx_count_frames;
>
> - stmmac_set_rx_owner(priv, p, use_rx_wd);
> -
> dma_wmb();
> + stmmac_set_rx_owner(priv, p, use_rx_wd);
>
> entry = STMMAC_GET_ENTRY(entry, DMA_RX_SIZE);
> }
> @@ -3373,9 +3335,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> unsigned int next_entry = rx_q->cur_rx;
> int coe = priv->hw->rx_csum;
> unsigned int count = 0;
> - bool xmac;
> -
> - xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
>
> if (netif_msg_rx_status(priv)) {
> void *rx_head;
> @@ -3389,11 +3348,12 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> stmmac_display_ring(priv, rx_head, DMA_RX_SIZE, true);
> }
> while (count < limit) {
> + struct stmmac_rx_buffer *buf;
> + struct dma_desc *np, *p;
> int entry, status;
> - struct dma_desc *p;
> - struct dma_desc *np;
>
> entry = next_entry;
> + buf = &rx_q->buf_pool[entry];
>
> if (priv->extend_desc)
> p = (struct dma_desc *)(rx_q->dma_erx + entry);
> @@ -3423,20 +3383,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> stmmac_rx_extended_status(priv, &priv->dev->stats,
> &priv->xstats, rx_q->dma_erx + entry);
> if (unlikely(status == discard_frame)) {
> + page_pool_recycle_direct(rx_q->page_pool, buf->page);
> priv->dev->stats.rx_errors++;
> - if (priv->hwts_rx_en && !priv->extend_desc) {
> - /* DESC2 & DESC3 will be overwritten by device
> - * with timestamp value, hence reinitialize
> - * them in stmmac_rx_refill() function so that
> - * device can reuse it.
> - */
> - dev_kfree_skb_any(rx_q->rx_skbuff[entry]);
> - rx_q->rx_skbuff[entry] = NULL;
> - dma_unmap_single(priv->device,
> - rx_q->rx_skbuff_dma[entry],
> - priv->dma_buf_sz,
> - DMA_FROM_DEVICE);
> - }
> + buf->page = NULL;
> } else {
> struct sk_buff *skb;
> int frame_len;
> @@ -3476,58 +3425,18 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> frame_len, status);
> }
>
> - /* The zero-copy is always used for all the sizes
> - * in case of GMAC4 because it needs
> - * to refill the used descriptors, always.
> - */
> - if (unlikely(!xmac &&
> - ((frame_len < priv->rx_copybreak) ||
> - stmmac_rx_threshold_count(rx_q)))) {
> - skb = netdev_alloc_skb_ip_align(priv->dev,
> - frame_len);
> - if (unlikely(!skb)) {
> - if (net_ratelimit())
> - dev_warn(priv->device,
> - "packet dropped\n");
> - priv->dev->stats.rx_dropped++;
> - continue;
> - }
> -
> - dma_sync_single_for_cpu(priv->device,
> - rx_q->rx_skbuff_dma
> - [entry], frame_len,
> - DMA_FROM_DEVICE);
> - skb_copy_to_linear_data(skb,
> - rx_q->
> - rx_skbuff[entry]->data,
> - frame_len);
> -
> - skb_put(skb, frame_len);
> - dma_sync_single_for_device(priv->device,
> - rx_q->rx_skbuff_dma
> - [entry], frame_len,
> - DMA_FROM_DEVICE);
> - } else {
> - skb = rx_q->rx_skbuff[entry];
> - if (unlikely(!skb)) {
> - if (net_ratelimit())
> - netdev_err(priv->dev,
> - "%s: Inconsistent Rx chain\n",
> - priv->dev->name);
> - priv->dev->stats.rx_dropped++;
> - continue;
> - }
> - prefetch(skb->data - NET_IP_ALIGN);
> - rx_q->rx_skbuff[entry] = NULL;
> - rx_q->rx_zeroc_thresh++;
> -
> - skb_put(skb, frame_len);
> - dma_unmap_single(priv->device,
> - rx_q->rx_skbuff_dma[entry],
> - priv->dma_buf_sz,
> - DMA_FROM_DEVICE);
> + skb = netdev_alloc_skb_ip_align(priv->dev, frame_len);
> + if (unlikely(!skb)) {
> + priv->dev->stats.rx_dropped++;
> + continue;
> }
>
> + dma_sync_single_for_cpu(priv->device, buf->addr,
> + frame_len, DMA_FROM_DEVICE);
> + skb_copy_to_linear_data(skb, page_address(buf->page),
> + frame_len);
> + skb_put(skb, frame_len);
> +
> if (netif_msg_pktdata(priv)) {
> netdev_dbg(priv->dev, "frame received (%dbytes)",
> frame_len);
> @@ -3547,6 +3456,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>
> napi_gro_receive(&ch->rx_napi, skb);
>
> + page_pool_recycle_direct(rx_q->page_pool, buf->page);
> + buf->page = NULL;
> +
> priv->dev->stats.rx_packets++;
> priv->dev->stats.rx_bytes += frame_len;
> }
> --
> 2.7.4
^ permalink raw reply
* [PATCH net-next 1/3] net: stmmac: Implement RX Coalesce Frames setting
From: Jose Abreu @ 2019-07-03 10:37 UTC (permalink / raw)
To: linux-kernel, netdev, linux-stm32, linux-arm-kernel
Cc: Jose Abreu, Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai
In-Reply-To: <cover.1562149883.git.joabreu@synopsys.com>
Add support for coalescing RX path by specifying number of frames which
don't need to have interrupt on completion bit set.
This is only available when RX Watchdog is enabled.
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>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Chen-Yu Tsai <wens@csie.org>
---
drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 ++
drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 7 +++++--
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 18 ++++++++++++------
4 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 2403a65167b2..dfd47fdfa447 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -252,6 +252,7 @@ struct stmmac_safety_stats {
#define STMMAC_MAX_COAL_TX_TICK 100000
#define STMMAC_TX_MAX_FRAMES 256
#define STMMAC_TX_FRAMES 1
+#define STMMAC_RX_FRAMES 25
/* Packets types */
enum packets_types {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 123898235cb0..513f4e2df5f6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -55,6 +55,7 @@ struct stmmac_tx_queue {
};
struct stmmac_rx_queue {
+ u32 rx_count_frames;
u32 queue_index;
struct stmmac_priv *priv_data;
struct dma_extended_desc *dma_erx;
@@ -110,6 +111,7 @@ struct stmmac_priv {
/* Frequently used values are kept adjacent for cache effect */
u32 tx_coal_frames;
u32 tx_coal_timer;
+ u32 rx_coal_frames;
int tx_coalesce;
int hwts_tx_en;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index cfd93eefb50e..6efb66820d4c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -701,8 +701,10 @@ static int stmmac_get_coalesce(struct net_device *dev,
ec->tx_coalesce_usecs = priv->tx_coal_timer;
ec->tx_max_coalesced_frames = priv->tx_coal_frames;
- if (priv->use_riwt)
+ if (priv->use_riwt) {
+ ec->rx_max_coalesced_frames = priv->rx_coal_frames;
ec->rx_coalesce_usecs = stmmac_riwt2usec(priv->rx_riwt, priv);
+ }
return 0;
}
@@ -715,7 +717,7 @@ static int stmmac_set_coalesce(struct net_device *dev,
unsigned int rx_riwt;
/* Check not supported parameters */
- if ((ec->rx_max_coalesced_frames) || (ec->rx_coalesce_usecs_irq) ||
+ if ((ec->rx_coalesce_usecs_irq) ||
(ec->rx_max_coalesced_frames_irq) || (ec->tx_coalesce_usecs_irq) ||
(ec->use_adaptive_rx_coalesce) || (ec->use_adaptive_tx_coalesce) ||
(ec->pkt_rate_low) || (ec->rx_coalesce_usecs_low) ||
@@ -749,6 +751,7 @@ static int stmmac_set_coalesce(struct net_device *dev,
/* Only copy relevant parameters, ignore all others. */
priv->tx_coal_frames = ec->tx_max_coalesced_frames;
priv->tx_coal_timer = ec->tx_coalesce_usecs;
+ priv->rx_coal_frames = ec->rx_max_coalesced_frames;
priv->rx_riwt = rx_riwt;
stmmac_rx_watchdog(priv, priv->ioaddr, priv->rx_riwt, rx_cnt);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3425d4dda03d..c8fe85ef9a7e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2268,20 +2268,21 @@ static void stmmac_tx_timer(struct timer_list *t)
}
/**
- * stmmac_init_tx_coalesce - init tx mitigation options.
+ * stmmac_init_coalesce - init mitigation options.
* @priv: driver private structure
* Description:
- * This inits the transmit coalesce parameters: i.e. timer rate,
+ * This inits the coalesce parameters: i.e. timer rate,
* timer handler and default threshold used for enabling the
* interrupt on completion bit.
*/
-static void stmmac_init_tx_coalesce(struct stmmac_priv *priv)
+static void stmmac_init_coalesce(struct stmmac_priv *priv)
{
u32 tx_channel_count = priv->plat->tx_queues_to_use;
u32 chan;
priv->tx_coal_frames = STMMAC_TX_FRAMES;
priv->tx_coal_timer = STMMAC_COAL_TX_TIMER;
+ priv->rx_coal_frames = STMMAC_RX_FRAMES;
for (chan = 0; chan < tx_channel_count; chan++) {
struct stmmac_tx_queue *tx_q = &priv->tx_queue[chan];
@@ -2651,7 +2652,7 @@ static int stmmac_open(struct net_device *dev)
goto init_error;
}
- stmmac_init_tx_coalesce(priv);
+ stmmac_init_coalesce(priv);
phylink_start(priv->phylink);
@@ -3298,6 +3299,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
while (dirty-- > 0) {
struct dma_desc *p;
+ bool use_rx_wd;
if (priv->extend_desc)
p = (struct dma_desc *)(rx_q->dma_erx + entry);
@@ -3340,7 +3342,11 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
}
dma_wmb();
- stmmac_set_rx_owner(priv, p, priv->use_riwt);
+ rx_q->rx_count_frames++;
+ rx_q->rx_count_frames %= priv->rx_coal_frames;
+ use_rx_wd = priv->use_riwt && rx_q->rx_count_frames;
+
+ stmmac_set_rx_owner(priv, p, use_rx_wd);
dma_wmb();
@@ -4623,7 +4629,7 @@ int stmmac_resume(struct device *dev)
stmmac_clear_descriptors(priv);
stmmac_hw_setup(ndev, false);
- stmmac_init_tx_coalesce(priv);
+ stmmac_init_coalesce(priv);
stmmac_set_rx_mode(ndev);
stmmac_enable_all_queues(priv);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 3/3] net: stmmac: Introducing support for Page Pool
From: Jose Abreu @ 2019-07-03 10:37 UTC (permalink / raw)
To: linux-kernel, netdev, linux-stm32, linux-arm-kernel
Cc: Jose Abreu, Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai
In-Reply-To: <cover.1562149883.git.joabreu@synopsys.com>
Mapping and unmapping DMA region is an high bottleneck in stmmac driver,
specially in the RX path.
This commit introduces support for Page Pool API and uses it in all RX
queues. With this change, we get more stable troughput and some increase
of banwidth with iperf:
- MAC1000 - 950 Mbps
- XGMAC: 9.22 Gbps
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>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Chen-Yu Tsai <wens@csie.org>
---
drivers/net/ethernet/stmicro/stmmac/Kconfig | 1 +
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 10 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 196 ++++++----------------
3 files changed, 63 insertions(+), 144 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index 943189dcccb1..2325b40dff6e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
+++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
@@ -3,6 +3,7 @@ config STMMAC_ETH
tristate "STMicroelectronics Multi-Gigabit Ethernet driver"
depends on HAS_IOMEM && HAS_DMA
select MII
+ select PAGE_POOL
select PHYLINK
select CRC32
imply PTP_1588_CLOCK
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 513f4e2df5f6..5cd966c154f3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -20,6 +20,7 @@
#include <linux/ptp_clock_kernel.h>
#include <linux/net_tstamp.h>
#include <linux/reset.h>
+#include <net/page_pool.h>
struct stmmac_resources {
void __iomem *addr;
@@ -54,14 +55,19 @@ struct stmmac_tx_queue {
u32 mss;
};
+struct stmmac_rx_buffer {
+ struct page *page;
+ dma_addr_t addr;
+};
+
struct stmmac_rx_queue {
u32 rx_count_frames;
u32 queue_index;
+ struct page_pool *page_pool;
+ struct stmmac_rx_buffer *buf_pool;
struct stmmac_priv *priv_data;
struct dma_extended_desc *dma_erx;
struct dma_desc *dma_rx ____cacheline_aligned_in_smp;
- struct sk_buff **rx_skbuff;
- dma_addr_t *rx_skbuff_dma;
unsigned int cur_rx;
unsigned int dirty_rx;
u32 rx_zeroc_thresh;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index c8fe85ef9a7e..9f44e8193208 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1197,26 +1197,14 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv, struct dma_desc *p,
int i, gfp_t flags, u32 queue)
{
struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
- struct sk_buff *skb;
+ struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
- skb = __netdev_alloc_skb_ip_align(priv->dev, priv->dma_buf_sz, flags);
- if (!skb) {
- netdev_err(priv->dev,
- "%s: Rx init fails; skb is NULL\n", __func__);
+ buf->page = page_pool_dev_alloc_pages(rx_q->page_pool);
+ if (!buf->page)
return -ENOMEM;
- }
- rx_q->rx_skbuff[i] = skb;
- rx_q->rx_skbuff_dma[i] = dma_map_single(priv->device, skb->data,
- priv->dma_buf_sz,
- DMA_FROM_DEVICE);
- if (dma_mapping_error(priv->device, rx_q->rx_skbuff_dma[i])) {
- netdev_err(priv->dev, "%s: DMA mapping error\n", __func__);
- dev_kfree_skb_any(skb);
- return -EINVAL;
- }
-
- stmmac_set_desc_addr(priv, p, rx_q->rx_skbuff_dma[i]);
+ buf->addr = buf->page->dma_addr;
+ stmmac_set_desc_addr(priv, p, buf->addr);
if (priv->dma_buf_sz == BUF_SIZE_16KiB)
stmmac_init_desc3(priv, p);
@@ -1232,13 +1220,10 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv, struct dma_desc *p,
static void stmmac_free_rx_buffer(struct stmmac_priv *priv, u32 queue, int i)
{
struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
+ struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
- if (rx_q->rx_skbuff[i]) {
- dma_unmap_single(priv->device, rx_q->rx_skbuff_dma[i],
- priv->dma_buf_sz, DMA_FROM_DEVICE);
- dev_kfree_skb_any(rx_q->rx_skbuff[i]);
- }
- rx_q->rx_skbuff[i] = NULL;
+ page_pool_put_page(rx_q->page_pool, buf->page, false);
+ buf->page = NULL;
}
/**
@@ -1321,10 +1306,6 @@ static int init_dma_rx_desc_rings(struct net_device *dev, gfp_t flags)
queue);
if (ret)
goto err_init_rx_buffers;
-
- netif_dbg(priv, probe, priv->dev, "[%p]\t[%p]\t[%x]\n",
- rx_q->rx_skbuff[i], rx_q->rx_skbuff[i]->data,
- (unsigned int)rx_q->rx_skbuff_dma[i]);
}
rx_q->cur_rx = 0;
@@ -1498,8 +1479,9 @@ static void free_dma_rx_desc_resources(struct stmmac_priv *priv)
sizeof(struct dma_extended_desc),
rx_q->dma_erx, rx_q->dma_rx_phy);
- kfree(rx_q->rx_skbuff_dma);
- kfree(rx_q->rx_skbuff);
+ kfree(rx_q->buf_pool);
+ if (rx_q->page_pool)
+ page_pool_request_shutdown(rx_q->page_pool);
}
}
@@ -1551,20 +1533,28 @@ static int alloc_dma_rx_desc_resources(struct stmmac_priv *priv)
/* RX queues buffers and DMA */
for (queue = 0; queue < rx_count; queue++) {
struct stmmac_rx_queue *rx_q = &priv->rx_queue[queue];
+ struct page_pool_params pp_params = { 0 };
rx_q->queue_index = queue;
rx_q->priv_data = priv;
- rx_q->rx_skbuff_dma = kmalloc_array(DMA_RX_SIZE,
- sizeof(dma_addr_t),
- GFP_KERNEL);
- if (!rx_q->rx_skbuff_dma)
+ pp_params.flags = PP_FLAG_DMA_MAP;
+ pp_params.order = DIV_ROUND_UP(priv->dma_buf_sz, PAGE_SIZE);
+ pp_params.nid = dev_to_node(priv->device);
+ pp_params.dev = priv->device;
+ pp_params.dma_dir = DMA_FROM_DEVICE;
+
+ rx_q->page_pool = page_pool_create(&pp_params);
+ if (IS_ERR(rx_q->page_pool)) {
+ ret = PTR_ERR(rx_q->page_pool);
+ rx_q->page_pool = NULL;
goto err_dma;
+ }
- rx_q->rx_skbuff = kmalloc_array(DMA_RX_SIZE,
- sizeof(struct sk_buff *),
- GFP_KERNEL);
- if (!rx_q->rx_skbuff)
+ rx_q->buf_pool = kmalloc_array(DMA_RX_SIZE,
+ sizeof(*rx_q->buf_pool),
+ GFP_KERNEL);
+ if (!rx_q->buf_pool)
goto err_dma;
if (priv->extend_desc) {
@@ -3295,9 +3285,8 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
int dirty = stmmac_rx_dirty(priv, queue);
unsigned int entry = rx_q->dirty_rx;
- int bfsize = priv->dma_buf_sz;
-
while (dirty-- > 0) {
+ struct stmmac_rx_buffer *buf = &rx_q->buf_pool[entry];
struct dma_desc *p;
bool use_rx_wd;
@@ -3306,49 +3295,22 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
else
p = rx_q->dma_rx + entry;
- if (likely(!rx_q->rx_skbuff[entry])) {
- struct sk_buff *skb;
-
- skb = netdev_alloc_skb_ip_align(priv->dev, bfsize);
- if (unlikely(!skb)) {
- /* so for a while no zero-copy! */
- rx_q->rx_zeroc_thresh = STMMAC_RX_THRESH;
- if (unlikely(net_ratelimit()))
- dev_err(priv->device,
- "fail to alloc skb entry %d\n",
- entry);
- break;
- }
-
- rx_q->rx_skbuff[entry] = skb;
- rx_q->rx_skbuff_dma[entry] =
- dma_map_single(priv->device, skb->data, bfsize,
- DMA_FROM_DEVICE);
- if (dma_mapping_error(priv->device,
- rx_q->rx_skbuff_dma[entry])) {
- netdev_err(priv->dev, "Rx DMA map failed\n");
- dev_kfree_skb(skb);
+ if (!buf->page) {
+ buf->page = page_pool_dev_alloc_pages(rx_q->page_pool);
+ if (!buf->page)
break;
- }
-
- stmmac_set_desc_addr(priv, p, rx_q->rx_skbuff_dma[entry]);
- stmmac_refill_desc3(priv, rx_q, p);
-
- if (rx_q->rx_zeroc_thresh > 0)
- rx_q->rx_zeroc_thresh--;
-
- netif_dbg(priv, rx_status, priv->dev,
- "refill entry #%d\n", entry);
}
- dma_wmb();
+
+ buf->addr = buf->page->dma_addr;
+ stmmac_set_desc_addr(priv, p, buf->addr);
+ stmmac_refill_desc3(priv, rx_q, p);
rx_q->rx_count_frames++;
rx_q->rx_count_frames %= priv->rx_coal_frames;
use_rx_wd = priv->use_riwt && rx_q->rx_count_frames;
- stmmac_set_rx_owner(priv, p, use_rx_wd);
-
dma_wmb();
+ stmmac_set_rx_owner(priv, p, use_rx_wd);
entry = STMMAC_GET_ENTRY(entry, DMA_RX_SIZE);
}
@@ -3373,9 +3335,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
unsigned int next_entry = rx_q->cur_rx;
int coe = priv->hw->rx_csum;
unsigned int count = 0;
- bool xmac;
-
- xmac = priv->plat->has_gmac4 || priv->plat->has_xgmac;
if (netif_msg_rx_status(priv)) {
void *rx_head;
@@ -3389,11 +3348,12 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
stmmac_display_ring(priv, rx_head, DMA_RX_SIZE, true);
}
while (count < limit) {
+ struct stmmac_rx_buffer *buf;
+ struct dma_desc *np, *p;
int entry, status;
- struct dma_desc *p;
- struct dma_desc *np;
entry = next_entry;
+ buf = &rx_q->buf_pool[entry];
if (priv->extend_desc)
p = (struct dma_desc *)(rx_q->dma_erx + entry);
@@ -3423,20 +3383,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
stmmac_rx_extended_status(priv, &priv->dev->stats,
&priv->xstats, rx_q->dma_erx + entry);
if (unlikely(status == discard_frame)) {
+ page_pool_recycle_direct(rx_q->page_pool, buf->page);
priv->dev->stats.rx_errors++;
- if (priv->hwts_rx_en && !priv->extend_desc) {
- /* DESC2 & DESC3 will be overwritten by device
- * with timestamp value, hence reinitialize
- * them in stmmac_rx_refill() function so that
- * device can reuse it.
- */
- dev_kfree_skb_any(rx_q->rx_skbuff[entry]);
- rx_q->rx_skbuff[entry] = NULL;
- dma_unmap_single(priv->device,
- rx_q->rx_skbuff_dma[entry],
- priv->dma_buf_sz,
- DMA_FROM_DEVICE);
- }
+ buf->page = NULL;
} else {
struct sk_buff *skb;
int frame_len;
@@ -3476,58 +3425,18 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
frame_len, status);
}
- /* The zero-copy is always used for all the sizes
- * in case of GMAC4 because it needs
- * to refill the used descriptors, always.
- */
- if (unlikely(!xmac &&
- ((frame_len < priv->rx_copybreak) ||
- stmmac_rx_threshold_count(rx_q)))) {
- skb = netdev_alloc_skb_ip_align(priv->dev,
- frame_len);
- if (unlikely(!skb)) {
- if (net_ratelimit())
- dev_warn(priv->device,
- "packet dropped\n");
- priv->dev->stats.rx_dropped++;
- continue;
- }
-
- dma_sync_single_for_cpu(priv->device,
- rx_q->rx_skbuff_dma
- [entry], frame_len,
- DMA_FROM_DEVICE);
- skb_copy_to_linear_data(skb,
- rx_q->
- rx_skbuff[entry]->data,
- frame_len);
-
- skb_put(skb, frame_len);
- dma_sync_single_for_device(priv->device,
- rx_q->rx_skbuff_dma
- [entry], frame_len,
- DMA_FROM_DEVICE);
- } else {
- skb = rx_q->rx_skbuff[entry];
- if (unlikely(!skb)) {
- if (net_ratelimit())
- netdev_err(priv->dev,
- "%s: Inconsistent Rx chain\n",
- priv->dev->name);
- priv->dev->stats.rx_dropped++;
- continue;
- }
- prefetch(skb->data - NET_IP_ALIGN);
- rx_q->rx_skbuff[entry] = NULL;
- rx_q->rx_zeroc_thresh++;
-
- skb_put(skb, frame_len);
- dma_unmap_single(priv->device,
- rx_q->rx_skbuff_dma[entry],
- priv->dma_buf_sz,
- DMA_FROM_DEVICE);
+ skb = netdev_alloc_skb_ip_align(priv->dev, frame_len);
+ if (unlikely(!skb)) {
+ priv->dev->stats.rx_dropped++;
+ continue;
}
+ dma_sync_single_for_cpu(priv->device, buf->addr,
+ frame_len, DMA_FROM_DEVICE);
+ skb_copy_to_linear_data(skb, page_address(buf->page),
+ frame_len);
+ skb_put(skb, frame_len);
+
if (netif_msg_pktdata(priv)) {
netdev_dbg(priv->dev, "frame received (%dbytes)",
frame_len);
@@ -3547,6 +3456,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
napi_gro_receive(&ch->rx_napi, skb);
+ page_pool_recycle_direct(rx_q->page_pool, buf->page);
+ buf->page = NULL;
+
priv->dev->stats.rx_packets++;
priv->dev->stats.rx_bytes += frame_len;
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 0/3] net: stmmac: Some performance improvements and a fix
From: Jose Abreu @ 2019-07-03 10:37 UTC (permalink / raw)
To: linux-kernel, netdev, linux-stm32, linux-arm-kernel
Cc: Jose Abreu, Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue, Maxime Coquelin, Maxime Ripard, Chen-Yu Tsai
Some performace improvements (01/03 and 03/03) and a fix (02/03), all for -next.
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>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: Chen-Yu Tsai <wens@csie.org>
Jose Abreu (3):
net: stmmac: Implement RX Coalesce Frames setting
net: stmmac: Fix descriptors address being in > 32 bits address space
net: stmmac: Introducing support for Page Pool
drivers/net/ethernet/stmicro/stmmac/Kconfig | 1 +
drivers/net/ethernet/stmicro/stmmac/common.h | 1 +
drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 8 +-
.../net/ethernet/stmicro/stmmac/dwmac1000_dma.c | 8 +-
drivers/net/ethernet/stmicro/stmmac/dwmac100_dma.c | 8 +-
drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c | 8 +-
drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h | 2 +
drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c | 10 +-
drivers/net/ethernet/stmicro/stmmac/hwif.h | 4 +-
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 12 +-
.../net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 7 +-
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 210 +++++++--------------
12 files changed, 107 insertions(+), 172 deletions(-)
--
2.7.4
^ 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