* [PATCH net-next 1/1] qed: Add support for Timestamping the unicast PTP packets.
From: Sudarsana Reddy Kalluru @ 2019-07-02 15:04 UTC (permalink / raw)
To: davem; +Cc: netdev, mkalderon, aelior
This patch masks the lower 4 bits of PARAM-MASK registers as required
by the Hardware to detect/timestamp the unicast PTP packets. The register
definition in the header file captures more details on the individual bits.
Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Signed-off-by: Ariel Elior <aelior@marvell.com>
---
drivers/net/ethernet/qlogic/qed/qed_ptp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_ptp.c b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
index f3ebdc5..4a7acfc 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_ptp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_ptp.c
@@ -243,7 +243,7 @@ static int qed_ptp_hw_cfg_filters(struct qed_dev *cdev,
return -EINVAL;
}
- qed_wr(p_hwfn, p_ptt, NIG_REG_LLH_PTP_PARAM_MASK, 0);
+ qed_wr(p_hwfn, p_ptt, NIG_REG_LLH_PTP_PARAM_MASK, 0xF);
qed_wr(p_hwfn, p_ptt, NIG_REG_LLH_PTP_RULE_MASK, rule_mask);
qed_wr(p_hwfn, p_ptt, NIG_REG_RX_PTP_EN, enable_cfg);
@@ -253,7 +253,7 @@ static int qed_ptp_hw_cfg_filters(struct qed_dev *cdev,
qed_wr(p_hwfn, p_ptt, NIG_REG_TX_LLH_PTP_RULE_MASK, 0x3FFF);
} else {
qed_wr(p_hwfn, p_ptt, NIG_REG_TX_PTP_EN, enable_cfg);
- qed_wr(p_hwfn, p_ptt, NIG_REG_TX_LLH_PTP_PARAM_MASK, 0);
+ qed_wr(p_hwfn, p_ptt, NIG_REG_TX_LLH_PTP_PARAM_MASK, 0xF);
qed_wr(p_hwfn, p_ptt, NIG_REG_TX_LLH_PTP_RULE_MASK, rule_mask);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net] rxrpc: Fix oops in tracepoint
From: David Howells @ 2019-07-02 15:04 UTC (permalink / raw)
To: netdev; +Cc: Marc Dionne, dhowells, linux-afs, linux-kernel
If the rxrpc_eproto tracepoint is enabled, an oops will be cause by the
trace line that rxrpc_extract_header() tries to emit when a protocol error
occurs (typically because the packet is short) because the call argument is
NULL.
Fix this by using ?: to assume 0 as the debug_id if call is NULL.
This can then be induced by:
echo -e '\0\0\0\0\0\0\0\0' | ncat -4u --send-only <addr> 20001
where addr has the following program running on it:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <linux/rxrpc.h>
int main(void)
{
struct sockaddr_rxrpc srx;
int fd;
memset(&srx, 0, sizeof(srx));
srx.srx_family = AF_RXRPC;
srx.srx_service = 0;
srx.transport_type = AF_INET;
srx.transport_len = sizeof(srx.transport.sin);
srx.transport.sin.sin_family = AF_INET;
srx.transport.sin.sin_port = htons(0x4e21);
fd = socket(AF_RXRPC, SOCK_DGRAM, AF_INET6);
bind(fd, (struct sockaddr *)&srx, sizeof(srx));
sleep(20);
return 0;
}
It results in the following oops.
BUG: kernel NULL pointer dereference, address: 0000000000000340
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
...
RIP: 0010:trace_event_raw_event_rxrpc_rx_eproto+0x47/0xac
...
Call Trace:
<IRQ>
rxrpc_extract_header+0x86/0x171
? rcu_read_lock_sched_held+0x5d/0x63
? rxrpc_new_skb+0xd4/0x109
rxrpc_input_packet+0xef/0x14fc
? rxrpc_input_data+0x986/0x986
udp_queue_rcv_one_skb+0xbf/0x3d0
udp_unicast_rcv_skb.isra.8+0x64/0x71
ip_protocol_deliver_rcu+0xe4/0x1b4
ip_local_deliver+0xf0/0x154
__netif_receive_skb_one_core+0x50/0x6c
netif_receive_skb_internal+0x26b/0x2e9
napi_gro_receive+0xf8/0x1da
rtl8169_poll+0x303/0x4c4
net_rx_action+0x10e/0x333
__do_softirq+0x1a5/0x38f
irq_exit+0x54/0xc4
do_IRQ+0xda/0xf8
common_interrupt+0xf/0xf
</IRQ>
...
? cpuidle_enter_state+0x23c/0x34d
cpuidle_enter+0x2a/0x36
do_idle+0x163/0x1ea
cpu_startup_entry+0x1d/0x1f
start_secondary+0x157/0x172
secondary_startup_64+0xa4/0xb0
Fixes: a25e21f0bcd2 ("rxrpc, afs: Use debug_ids rather than pointers in traces")
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Marc Dionne <marc.dionne@auristor.com>
---
include/trace/events/rxrpc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index d85816878a52..cc1d060cbf13 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -1379,7 +1379,7 @@ TRACE_EVENT(rxrpc_rx_eproto,
),
TP_fast_assign(
- __entry->call = call->debug_id;
+ __entry->call = call ? call->debug_id : 0;
__entry->serial = serial;
__entry->why = why;
),
^ permalink raw reply related
* re: mlxsw: spectrum: PTP: Support timestamping on Spectrum-1 - potential null ptr dereference
From: Colin Ian King @ 2019-07-02 15:01 UTC (permalink / raw)
To: Petr Machata, Jiri Pirko, Ido Schimmel, David S. Miller, netdev,
linux-kernel@vger.kernel.org
Hi,
Static analysis with Coverity on today's linux-next has found a
potential null pointer dereference bug with the following commit:
commit d92e4e6e33c8b19635be70fb8935b627d2e4f8fe
Author: Petr Machata <petrm@mellanox.com>
Date: Sun Jun 30 09:04:56 2019 +0300
mlxsw: spectrum: PTP: Support timestamping on Spectrum-1
In function: mlxsw_sp1_ptp_packet_finish the offending code is as follows:
/* Between capturing the packet and finishing it, there is a
window of
* opportunity for the originating port to go away (e.g. due to a
* split). Also make sure the SKB device reference is still valid.
*/
mlxsw_sp_port = mlxsw_sp->ports[local_port];
if (!mlxsw_sp_port && (!skb->dev || skb->dev ==
mlxsw_sp_port->dev)) {
dev_kfree_skb_any(skb);
return;
}
If mlxsw_sp_port is null and skb->dev is not-null then the comparison
"skb->dev == mlxsw_sp_port->dev" ends up with a null pointer dereference.
I think the if statement should be:
if (mlxsw_sp_port && (!skb->dev || skb->dev == mlxsw_sp_port->dev))
..but I'm not 100% sure as I may be missing something a bit more subtle
here.
Colin
^ permalink raw reply
* Re: [PATCH bpf] xdp: fix race on generic receive path
From: Magnus Karlsson @ 2019-07-02 15:01 UTC (permalink / raw)
To: Ilya Maximets
Cc: Network Development, linux-kernel, bpf, xdp-newbies,
David S. Miller, Björn Töpel, Magnus Karlsson,
Jonathan Lemon, Jakub Kicinski, Alexei Starovoitov,
Daniel Borkmann
In-Reply-To: <20190702143634.19688-1-i.maximets@samsung.com>
On Tue, Jul 2, 2019 at 4:36 PM Ilya Maximets <i.maximets@samsung.com> 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.
Thanks for this catch Ilya. Do you have any performance numbers you
could share of the impact of adding this spin lock? The reason I ask
is that if the impact is negligible, then let us just add it. But if
it is too large, we might want to brain storm about some other
possible solutions.
Thanks: Magnus
> Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> include/net/xdp_sock.h | 2 ++
> net/xdp/xsk.c | 32 +++++++++++++++++++++++---------
> 2 files changed, 25 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..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);
> +
> + 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 +143,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_irqrestore(&xs->rx_lock, flags);
> +
> + xs->sk.sk_data_ready(&xs->sk);
> + return 0;
> +
> +out_drop:
> xs->rx_dropped++;
> +out_unlock:
> + spin_unlock_irqrestore(&xs->rx_lock, flags);
> return err;
> }
>
> @@ -765,6 +778,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
* Re: [PATCH 1/1] mlx5: Fix build when CONFIG_MLX5_EN_RXNFC is disabled
From: Jes Sorensen @ 2019-07-02 15:00 UTC (permalink / raw)
To: Saeed Mahameed
Cc: kernel-team@fb.com, jsorensen@fb.com, netdev@vger.kernel.org
In-Reply-To: <ff76fcde486792ad01be1476a66a726d6e1ab933.camel@mellanox.com>
On 6/25/19 2:00 PM, Saeed Mahameed wrote:
> On Tue, 2019-06-25 at 11:27 -0400, Jes Sorensen wrote:
>> From: Jes Sorensen <jsorensen@fb.com>
>>
>> The previous patch broke the build with a static declaration for
>> a public function.
>>
>> Fixes: 8f0916c6dc5c (net/mlx5e: Fix ethtool rxfh commands when
>> CONFIG_MLX5_EN_RXNFC is disabled)
>> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
>> ---
>> drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> index dd764e0471f2..776040d91bd4 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> @@ -1905,7 +1905,8 @@ static int mlx5e_flash_device(struct net_device
>> *dev,
>> /* When CONFIG_MLX5_EN_RXNFC=n we only support ETHTOOL_GRXRINGS
>> * otherwise this function will be defined from en_fs_ethtool.c
>> */
>
> As the above comment states, when CONFIG_MLX5_EN_RXNFC is disabled,
> mlx5e_get_rxnfc is only defined, declared and used in this file, so it
> must be static. Otherwise it will be defined in another file which
> provides much much more functionality for ethtool flow steering.
>
> can you please provide more information of what went wrong on your
> build machine ?
Sorry was swamped here!
Looks like you're right, it only triggers in our build due to some
patches we don't have from upstream. I did the patch against upstream
and applied it to our tree, so should have checked further there.
Cheers,
Jes
^ permalink raw reply
* [PATCH net] rxrpc: Fix send on a connected, but unbound socket
From: David Howells @ 2019-07-02 14:59 UTC (permalink / raw)
To: netdev
Cc: syzbot+7966f2a0b2c7da8939b4, Marc Dionne, dhowells, linux-afs,
linux-kernel
If sendmsg() or sendmmsg() is called on a connected socket that hasn't had
bind() called on it, then an oops will occur when the kernel tries to
connect the call because no local endpoint has been allocated.
Fix this by implicitly binding the socket if it is in the
RXRPC_CLIENT_UNBOUND state, just like it does for the RXRPC_UNBOUND state.
Further, the state should be transitioned to RXRPC_CLIENT_BOUND after this
to prevent further attempts to bind it.
This can be tested with:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/socket.h>
#include <arpa/inet.h>
#include <linux/rxrpc.h>
static const unsigned char inet6_addr[16] = {
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, -1, -1, 0xac, 0x14, 0x14, 0xaa
};
int main(void)
{
struct sockaddr_rxrpc srx;
struct cmsghdr *cm;
struct msghdr msg;
unsigned char control[16];
int fd;
memset(&srx, 0, sizeof(srx));
srx.srx_family = 0x21;
srx.srx_service = 0;
srx.transport_type = AF_INET;
srx.transport_len = 0x1c;
srx.transport.sin6.sin6_family = AF_INET6;
srx.transport.sin6.sin6_port = htons(0x4e22);
srx.transport.sin6.sin6_flowinfo = htons(0x4e22);
srx.transport.sin6.sin6_scope_id = htons(0xaa3b);
memcpy(&srx.transport.sin6.sin6_addr, inet6_addr, 16);
cm = (struct cmsghdr *)control;
cm->cmsg_len = CMSG_LEN(sizeof(unsigned long));
cm->cmsg_level = SOL_RXRPC;
cm->cmsg_type = RXRPC_USER_CALL_ID;
*(unsigned long *)CMSG_DATA(cm) = 0;
msg.msg_name = NULL;
msg.msg_namelen = 0;
msg.msg_iov = NULL;
msg.msg_iovlen = 0;
msg.msg_control = control;
msg.msg_controllen = cm->cmsg_len;
msg.msg_flags = 0;
fd = socket(AF_RXRPC, SOCK_DGRAM, AF_INET);
connect(fd, (struct sockaddr *)&srx, sizeof(srx));
sendmsg(fd, &msg, 0);
return 0;
}
Leading to the following oops:
BUG: kernel NULL pointer dereference, address: 0000000000000018
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
...
RIP: 0010:rxrpc_connect_call+0x42/0xa01
...
Call Trace:
? mark_held_locks+0x47/0x59
? __local_bh_enable_ip+0xb6/0xba
rxrpc_new_client_call+0x3b1/0x762
? rxrpc_do_sendmsg+0x3c0/0x92e
rxrpc_do_sendmsg+0x3c0/0x92e
rxrpc_sendmsg+0x16b/0x1b5
sock_sendmsg+0x2d/0x39
___sys_sendmsg+0x1a4/0x22a
? release_sock+0x19/0x9e
? reacquire_held_locks+0x136/0x160
? release_sock+0x19/0x9e
? find_held_lock+0x2b/0x6e
? __lock_acquire+0x268/0xf73
? rxrpc_connect+0xdd/0xe4
? __local_bh_enable_ip+0xb6/0xba
__sys_sendmsg+0x5e/0x94
do_syscall_64+0x7d/0x1bf
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Fixes: 2341e0775747 ("rxrpc: Simplify connect() implementation and simplify sendmsg() op")
Reported-by: syzbot+7966f2a0b2c7da8939b4@syzkaller.appspotmail.com
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Marc Dionne <marc.dionne@auristor.com>
---
net/rxrpc/af_rxrpc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index f9f4721cdfa7..d09eaf153544 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -545,6 +545,7 @@ static int rxrpc_sendmsg(struct socket *sock, struct msghdr *m, size_t len)
switch (rx->sk.sk_state) {
case RXRPC_UNBOUND:
+ case RXRPC_CLIENT_UNBOUND:
rx->srx.srx_family = AF_RXRPC;
rx->srx.srx_service = 0;
rx->srx.transport_type = SOCK_DGRAM;
@@ -569,10 +570,9 @@ static int rxrpc_sendmsg(struct socket *sock, struct msghdr *m, size_t len)
}
rx->local = local;
- rx->sk.sk_state = RXRPC_CLIENT_UNBOUND;
+ rx->sk.sk_state = RXRPC_CLIENT_BOUND;
/* Fall through */
- case RXRPC_CLIENT_UNBOUND:
case RXRPC_CLIENT_BOUND:
if (!m->msg_name &&
test_bit(RXRPC_SOCK_CONNECTED, &rx->flags)) {
^ permalink raw reply related
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Ivan Khoronzhuk @ 2019-07-02 14:56 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap
In-Reply-To: <20190702165230.6caa36e3@carbon>
On Tue, Jul 02, 2019 at 04:52:30PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 2 Jul 2019 17:44:27 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
>> >From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >
>> >Jesper recently removed page_pool_destroy() (from driver invocation) and
>> >moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
>> >handle in-flight packets/pages. This created an asymmetry in drivers
>> >create/destroy pairs.
>> >
>> >This patch add page_pool user refcnt and reintroduce page_pool_destroy.
>> >This serves two purposes, (1) simplify drivers error handling as driver now
>> >drivers always calls page_pool_destroy() and don't need to track if
>> >xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
>> >where a single RX-queue (with a single page_pool) provides packets for two
>> >net_device'es, and thus needs to register the same page_pool twice with two
>> >xdp_rxq_info structures.
>>
>> As I tend to use xdp level patch there is no more reason to mention (2) case
>> here. XDP patch serves it better and can prevent not only obj deletion but also
>> pool flush, so, this one patch I could better leave only for (1) case.
>
>I don't understand what you are saying.
>
>Do you approve this patch, or do you reject this patch?
>
It's not reject, it's proposition to use both, XDP and page pool patches,
each having its goal.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* [PATCH net-next] rxrpc: Fix uninitialized error code in rxrpc_send_data_packet()
From: David Howells @ 2019-07-02 14:55 UTC (permalink / raw)
To: netdev; +Cc: Geert Uytterhoeven, dhowells, linux-afs, linux-kernel
With gcc 4.1:
net/rxrpc/output.c: In function ‘rxrpc_send_data_packet’:
net/rxrpc/output.c:338: warning: ‘ret’ may be used uninitialized in this function
Indeed, if the first jump to the send_fragmentable label is made, and
the address family is not handled in the switch() statement, ret will be
used uninitialized.
Fix this by BUG()'ing as is done in other places in rxrpc where internal
support for future address families will need adding. It should not be
possible to reach this normally as the address families are checked
up-front.
Fixes: 5a924b8951f835b5 ("rxrpc: Don't store the rxrpc header in the Tx queue sk_buffs")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---
net/rxrpc/output.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/rxrpc/output.c b/net/rxrpc/output.c
index a0b6abfbd277..948e3fe249ec 100644
--- a/net/rxrpc/output.c
+++ b/net/rxrpc/output.c
@@ -519,6 +519,9 @@ int rxrpc_send_data_packet(struct rxrpc_call *call, struct sk_buff *skb,
}
break;
#endif
+
+ default:
+ BUG();
}
if (ret < 0)
^ permalink raw reply related
* Re: [PATCH net-next v6 01/15] rtnetlink: provide permanent hardware address in RTM_NEWLINK
From: Stephen Hemminger @ 2019-07-02 14:55 UTC (permalink / raw)
To: Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Jiri Pirko, Andrew Lunn,
Florian Fainelli, John Linville, Johannes Berg, linux-kernel
In-Reply-To: <b6e0aefbcb58297b3ec0a12ee4be8e5194eee61a.1562067622.git.mkubecek@suse.cz>
On Tue, 2 Jul 2019 13:49:44 +0200 (CEST)
Michal Kubecek <mkubecek@suse.cz> wrote:
> Permanent hardware address of a network device was traditionally provided
> via ethtool ioctl interface but as Jiri Pirko pointed out in a review of
> ethtool netlink interface, rtnetlink is much more suitable for it so let's
> add it to the RTM_NEWLINK message.
>
> Add IFLA_PERM_ADDRESS attribute to RTM_NEWLINK messages unless the
> permanent address is all zeros (i.e. device driver did not fill it). As
> permanent address is not modifiable, reject userspace requests containing
> IFLA_PERM_ADDRESS attribute.
>
> Note: we already provide permanent hardware address for bond slaves;
> unfortunately we cannot drop that attribute for backward compatibility
> reasons.
>
> v5 -> v6: only add the attribute if permanent address is not zero
>
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
Do you want to make an iproute patch to display this?
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply
* Re: [PATCH v5 net-next 1/6] xdp: allow same allocator usage
From: Ivan Khoronzhuk @ 2019-07-02 14:53 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend
In-Reply-To: <20190702164648.56ff0761@carbon>
On Tue, Jul 02, 2019 at 04:46:48PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 2 Jul 2019 13:27:07 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> On Mon, Jul 01, 2019 at 01:40:59PM +0200, Jesper Dangaard Brouer wrote:
>> >
>> >I'm very skeptical about this approach.
>> >
>> >On Sun, 30 Jun 2019 20:23:43 +0300
>> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>> >
>> >> XDP rxqs can be same for ndevs running under same rx napi softirq.
>> >> But there is no ability to register same allocator for both rxqs,
>> >> by fact it can same rxq but has different ndev as a reference.
>> >
>> >This description is not very clear. It can easily be misunderstood.
>> >
>> >It is an absolute requirement that each RX-queue have their own
>> >page_pool object/allocator. (This where the performance comes from) as
>> >the page_pool have NAPI protected array for alloc and XDP_DROP recycle.
>> >
>> >Your driver/hardware seems to have special case, where a single
>> >RX-queue can receive packets for two different net_device'es.
>> >
>> >Do you violate this XDP devmap redirect assumption[1]?
>> >[1] https://github.com/torvalds/linux/blob/v5.2-rc7/kernel/bpf/devmap.c#L324-L329
>> Seems that yes, but that's used only for trace for now.
>> As it runs under napi and flush clear dev_rx i must do it right in the
>> rx_handler. So next patchset version will have one patch less.
>>
>> Thanks!
>>
>> >
>> >
>> >> Due to last changes allocator destroy can be defered till the moment
>> >> all packets are recycled by destination interface, afterwards it's
>> >> freed. In order to schedule allocator destroy only after all users are
>> >> unregistered, add refcnt to allocator object and schedule to destroy
>> >> only it reaches 0.
>> >
>> >The guiding principles when designing an API, is to make it easy to
>> >use, but also make it hard to misuse.
>> >
>> >Your API change makes it easy to misuse the API. As it make it easy to
>> >(re)use the allocator pointer (likely page_pool) for multiple
>> >xdp_rxq_info structs. It is only valid for your use-case, because you
>> >have hardware where a single RX-queue delivers to two different
>> >net_devices. For other normal use-cases, this will be a violation.
>> >
>> >If I was a user of this API, and saw your xdp_allocator_get(), then I
>> >would assume that this was the normal case. As minimum, we need to add
>> >a comment in the code, about this specific/intended use-case. I
>> >through about detecting the misuse, by adding a queue_index to
>> >xdp_mem_allocator, that can be checked against, when calling
>> >xdp_rxq_info_reg_mem_model() with another xdp_rxq_info struct (to catch
>> >the obvious mistake where queue_index mismatch).
>>
>> I can add, but not sure if it has or can have some conflicts with another
>> memory allocators, now or in future. Main here to not became a cornerstone
>> in some not obvious use-cases.
>>
>> So, for now, let it be in this way:
>>
>> --- a/include/net/xdp_priv.h
>> +++ b/include/net/xdp_priv.h
>> @@ -19,6 +19,7 @@ struct xdp_mem_allocator {
>> struct delayed_work defer_wq;
>> unsigned long defer_warn;
>> unsigned long refcnt;
>> + u32 queue_index;
>> };
>>
>> #endif /* __LINUX_NET_XDP_PRIV_H__ */
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index a44621190fdc..c4bf29810f4d 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -324,7 +324,7 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
>> return true;
>> }
>>
>> -static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
>> +static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
>> {
>> struct xdp_mem_allocator *xae, *xa = NULL;
>> struct rhashtable_iter iter;
>> @@ -336,7 +336,6 @@ static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
>>
>> while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
>> if (xae->allocator == allocator) {
>> - xae->refcnt++;
>> xa = xae;
>> break;
>> }
>> @@ -386,9 +385,13 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>> }
>> }
>>
>> - xdp_alloc = xdp_allocator_get(allocator);
>> + xdp_alloc = xdp_allocator_find(allocator);
>> if (xdp_alloc) {
>> + if (xdp_alloc->queue_index != xdp_rxq->queue_index)
>> + return -EINVAL;
>> +
>> xdp_rxq->mem.id = xdp_alloc->mem.id;
>> + xdp_alloc->refcnt++;
>
>This is now adjusted outside lock, not good.
In final it serves:
From f43a0b85838f75814cc93e5a724c4c7e5615f936 Mon Sep 17 00:00:00 2001
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Fri, 28 Jun 2019 03:17:24 +0300
Subject: [PATCH] xdp: allow same allocator usage
XDP rxqs can be same for ndevs running under same rx napi softirq.
But there is no ability to register same allocator for both rxqs,
by fact it can same rxq but has different ndev as a reference.
Due to last changes allocator destroy can be defered till the moment
all packets are recycled by destination interface, afterwards it's
freed. In order to schedule allocator destroy only after all users are
unregistered, add refcnt to allocator object and schedule to destroy
only it reaches 0.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
include/net/xdp_priv.h | 2 ++
net/core/xdp.c | 52 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/include/net/xdp_priv.h b/include/net/xdp_priv.h
index 6a8cba6ea79a..9858a4057842 100644
--- a/include/net/xdp_priv.h
+++ b/include/net/xdp_priv.h
@@ -18,6 +18,8 @@ struct xdp_mem_allocator {
struct rcu_head rcu;
struct delayed_work defer_wq;
unsigned long defer_warn;
+ unsigned long refcnt;
+ u32 queue_index;
};
#endif /* __LINUX_NET_XDP_PRIV_H__ */
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 829377cc83db..090f26e4f793 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -98,6 +98,18 @@ static bool __mem_id_disconnect(int id, bool force)
WARN(1, "Request remove non-existing id(%d), driver bug?", id);
return true;
}
+
+ /* to avoid calling hash lookup twice, decrement refcnt here till it
+ * reaches zero, then it can be called from workqueue afterwards.
+ */
+ if (xa->refcnt)
+ xa->refcnt--;
+
+ if (xa->refcnt) {
+ mutex_unlock(&mem_id_lock);
+ return true;
+ }
+
xa->disconnect_cnt++;
/* Detects in-flight packet-pages for page_pool */
@@ -312,6 +324,30 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
return true;
}
+static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
+{
+ struct xdp_mem_allocator *xae, *xa = NULL;
+ struct rhashtable_iter iter;
+
+ rhashtable_walk_enter(mem_id_ht, &iter);
+ do {
+ rhashtable_walk_start(&iter);
+
+ while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
+ if (xae->allocator == allocator) {
+ xa = xae;
+ break;
+ }
+ }
+
+ rhashtable_walk_stop(&iter);
+
+ } while (xae == ERR_PTR(-EAGAIN));
+ rhashtable_walk_exit(&iter);
+
+ return xa;
+}
+
int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
enum xdp_mem_type type, void *allocator)
{
@@ -347,6 +383,20 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
}
}
+ mutex_lock(&mem_id_lock);
+ xdp_alloc = xdp_allocator_find(allocator);
+ if (xdp_alloc) {
+ /* One allocator per queue is supposed only */
+ if (xdp_alloc->queue_index != xdp_rxq->queue_index)
+ return -EINVAL;
+
+ xdp_rxq->mem.id = xdp_alloc->mem.id;
+ xdp_alloc->refcnt++;
+ mutex_unlock(&mem_id_lock);
+ return 0;
+ }
+ mutex_unlock(&mem_id_lock);
+
xdp_alloc = kzalloc(sizeof(*xdp_alloc), gfp);
if (!xdp_alloc)
return -ENOMEM;
@@ -360,6 +410,8 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
xdp_rxq->mem.id = id;
xdp_alloc->mem = xdp_rxq->mem;
xdp_alloc->allocator = allocator;
+ xdp_alloc->refcnt = 1;
+ xdp_alloc->queue_index = xdp_rxq->queue_index;
/* Insert allocator into ID lookup table */
ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>
>> return 0;
>> }
>>
>> @@ -406,6 +409,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>> xdp_alloc->mem = xdp_rxq->mem;
>> xdp_alloc->allocator = allocator;
>> xdp_alloc->refcnt = 1;
>> + xdp_alloc->queue_index = xdp_rxq->queue_index;
>>
>> /* Insert allocator into ID lookup table */
>> ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>>
>> Jesper, are you Ok with this version?
>
>Please see my other patch, this is based on our first refcnt attempt.
>I think that other patch is a better way forward.
XDP patch serves it better and can prevent not only obj deletion but also
pool flush. So I propose use 2 patches.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply related
* Re: [PATCH net-next v6 04/15] ethtool: introduce ethtool netlink interface
From: Michal Kubecek @ 2019-07-02 14:52 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: <20190702122521.GN2250@nanopsycho>
On Tue, Jul 02, 2019 at 02:25:21PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:49:59PM CEST, mkubecek@suse.cz wrote:
> >+Request header
> >+--------------
> >+
> >+Each request or reply message contains a nested attribute with common header.
> >+Structure of this header is
>
> Missing ":"
OK
> >+
> >+ ETHTOOL_A_HEADER_DEV_INDEX (u32) device ifindex
> >+ ETHTOOL_A_HEADER_DEV_NAME (string) device name
> >+ ETHTOOL_A_HEADER_INFOMASK (u32) info mask
> >+ ETHTOOL_A_HEADER_GFLAGS (u32) flags common for all requests
> >+ ETHTOOL_A_HEADER_RFLAGS (u32) request specific flags
> >+
> >+ETHTOOL_A_HEADER_DEV_INDEX and ETHTOOL_A_HEADER_DEV_NAME identify the device
> >+message relates to. One of them is sufficient in requests, if both are used,
> >+they must identify the same device. Some requests, e.g. global string sets, do
> >+not require device identification. Most GET requests also allow dump requests
> >+without device identification to query the same information for all devices
> >+providing it (each device in a separate message).
> >+
> >+Optional info mask allows to ask only for a part of data provided by GET
>
> How this "infomask" works? What are the bits related to? Is that request
> specific?
The interpretation is request specific, the information returned for
a GET request is divided into multiple parts and client can choose to
request one of them (usually one). In the code so far, infomask bits
correspond to top level (nest) attributes but I would rather not make it
a strict rule.
I'll make the paragraph more verbose.
> >+request types. If omitted or zero, all data is returned. The two flag bitmaps
> >+allow enabling requestoptions; ETHTOOL_A_HEADER_GFLAGS are global flags common
>
> s/requestoptions;/request options./ ?
Yes.
> >+for all request types, flags recognized in ETHTOOL_A_HEADER_RFLAGS and their
> >+interpretation are specific for each request type. Global flags are
> >+
> >+ ETHTOOL_RF_COMPACT use compact format bitsets in reply
>
> Why "RF"? Isn't this "GF"? I would like "ETHTOOL_GFLAG_COMPACT" better.
RF as Request Flags. At the moment, global flags use ETHTOOL_RF_name
pattern and request specific flags ETHTOOL_RF_msgtype_name. GFLAG and
RFLAG would probably show the relation better, so how about
ETHTOOL_GFLAG_name for global
ETHTOOL_RFLAG_msgtype_name for request specific
> >+ ETHTOOL_RF_REPLY send optional reply (SET and ACT requests)
> >+
> >+Request specific flags are described with each request type. For both flag
> >+attributes, new flags should follow the general idea that if the flag is not
> >+set, the behaviour is the same as (or closer to) the behaviour before it was
>
> "closer to" ? That would be unfortunate I believe...
There may be situations where it cannot be exactly the same, e.g.
because the flag affects interpretation of an attribute which was
introduced together with the flag. How about "...the behaviour is
backward compatible"?
> >+List of message types
> >+---------------------
> >+
> >+All constants identifying message types use ETHTOOL_CMD_ prefix and suffix
> >+according to message purpose:
> >+
> >+ _GET userspace request to retrieve data
> >+ _SET userspace request to set data
> >+ _ACT userspace request to perform an action
> >+ _GET_REPLY kernel reply to a GET request
> >+ _SET_REPLY kernel reply to a SET request
> >+ _ACT_REPLY kernel reply to an ACT request
> >+ _NTF kernel notification
> >+
> >+"GET" requests are sent by userspace applications to retrieve device
> >+information. They usually do not contain any message specific attributes.
> >+Kernel replies with corresponding "GET_REPLY" message. For most types, "GET"
> >+request with NLM_F_DUMP and no device identification can be used to query the
> >+information for all devices supporting the request.
> >+
> >+If the data can be also modified, corresponding "SET" message with the same
> >+layout as "GET" reply is used to request changes. Only attributes where
>
> s/"GET" reply"/"GET_REPLY" ?
> Maybe better to emphasize that the "GET_REPLY" is the one corresponding
> with "SET". But perhaps I got this sentence all wrong :/
OK
> >+a change is requested are included in such request (also, not all attributes
> >+may be changed). Replies to most "SET" request consist only of error code and
> >+extack; if kernel provides additional data, it is sent in the form of
> >+corresponding "SET_REPLY" message (if ETHTOOL_RF_REPLY flag was set in request
> >+header).
> >+
> >+Data modification also triggers sending a "NTF" message with a notification.
> >+These usually bear only a subset of attributes which was affected by the
> >+change. The same notification is issued if the data is modified using other
> >+means (mostly ioctl ethtool interface). Unlike notifications from ethtool
> >+netlink code which are only sent if something actually changed, notifications
> >+triggered by ioctl interface may be sent even if the request did not actually
> >+change any data.
>
> Interesting. What's the reason for that?
Most setting commands in ioctl interface do not even query the original
state, they just pass the structure from ioctl() to ethtool_ops handler.
We could add retrieving the original state first but I suppose we would
still have to call the handler anyway even if requested values are the
same (as that's what kernel does now) and it's not clear if omitting the
notification in such case is the right thing to do.
> >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
> >index 3ebfab2bca66..f30e0da88be5 100644
> >--- a/net/ethtool/Makefile
> >+++ b/net/ethtool/Makefile
> >@@ -1,3 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0
> >
> >-obj-y += ioctl.o
> >+obj-y += ioctl.o
> >+
> >+obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
>
> Hmm, I wonder, why not to make this always on? We want users to use
> it, memory savings in case it is off would be minimal. RTNetlink is also
> always on. Ethtool ioctl is also always on.
We have already discussed this in the previous version. Someone claimed
earlier that building a kernel without ethtool interface would make
sense for some minimalistic systems. My plan is to make the ioctl
interface also optional once it's possible for (sufficiently new)
ethtool to work without it.
Michal
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Jesper Dangaard Brouer @ 2019-07-02 14:52 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap, brouer
In-Reply-To: <20190702144426.GD4510@khorivan>
On Tue, 2 Jul 2019 17:44:27 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
> >From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >
> >Jesper recently removed page_pool_destroy() (from driver invocation) and
> >moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
> >handle in-flight packets/pages. This created an asymmetry in drivers
> >create/destroy pairs.
> >
> >This patch add page_pool user refcnt and reintroduce page_pool_destroy.
> >This serves two purposes, (1) simplify drivers error handling as driver now
> >drivers always calls page_pool_destroy() and don't need to track if
> >xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
> >where a single RX-queue (with a single page_pool) provides packets for two
> >net_device'es, and thus needs to register the same page_pool twice with two
> >xdp_rxq_info structures.
>
> As I tend to use xdp level patch there is no more reason to mention (2) case
> here. XDP patch serves it better and can prevent not only obj deletion but also
> pool flush, so, this one patch I could better leave only for (1) case.
I don't understand what you are saying.
Do you approve this patch, or do you reject this patch?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH v5 net-next 1/6] xdp: allow same allocator usage
From: Jesper Dangaard Brouer @ 2019-07-02 14:46 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: grygorii.strashko, hawk, davem, ast, linux-kernel, linux-omap,
xdp-newbies, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend, brouer
In-Reply-To: <20190702102700.GA4510@khorivan>
On Tue, 2 Jul 2019 13:27:07 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Mon, Jul 01, 2019 at 01:40:59PM +0200, Jesper Dangaard Brouer wrote:
> >
> >I'm very skeptical about this approach.
> >
> >On Sun, 30 Jun 2019 20:23:43 +0300
> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >
> >> XDP rxqs can be same for ndevs running under same rx napi softirq.
> >> But there is no ability to register same allocator for both rxqs,
> >> by fact it can same rxq but has different ndev as a reference.
> >
> >This description is not very clear. It can easily be misunderstood.
> >
> >It is an absolute requirement that each RX-queue have their own
> >page_pool object/allocator. (This where the performance comes from) as
> >the page_pool have NAPI protected array for alloc and XDP_DROP recycle.
> >
> >Your driver/hardware seems to have special case, where a single
> >RX-queue can receive packets for two different net_device'es.
> >
> >Do you violate this XDP devmap redirect assumption[1]?
> >[1] https://github.com/torvalds/linux/blob/v5.2-rc7/kernel/bpf/devmap.c#L324-L329
> Seems that yes, but that's used only for trace for now.
> As it runs under napi and flush clear dev_rx i must do it right in the
> rx_handler. So next patchset version will have one patch less.
>
> Thanks!
>
> >
> >
> >> Due to last changes allocator destroy can be defered till the moment
> >> all packets are recycled by destination interface, afterwards it's
> >> freed. In order to schedule allocator destroy only after all users are
> >> unregistered, add refcnt to allocator object and schedule to destroy
> >> only it reaches 0.
> >
> >The guiding principles when designing an API, is to make it easy to
> >use, but also make it hard to misuse.
> >
> >Your API change makes it easy to misuse the API. As it make it easy to
> >(re)use the allocator pointer (likely page_pool) for multiple
> >xdp_rxq_info structs. It is only valid for your use-case, because you
> >have hardware where a single RX-queue delivers to two different
> >net_devices. For other normal use-cases, this will be a violation.
> >
> >If I was a user of this API, and saw your xdp_allocator_get(), then I
> >would assume that this was the normal case. As minimum, we need to add
> >a comment in the code, about this specific/intended use-case. I
> >through about detecting the misuse, by adding a queue_index to
> >xdp_mem_allocator, that can be checked against, when calling
> >xdp_rxq_info_reg_mem_model() with another xdp_rxq_info struct (to catch
> >the obvious mistake where queue_index mismatch).
>
> I can add, but not sure if it has or can have some conflicts with another
> memory allocators, now or in future. Main here to not became a cornerstone
> in some not obvious use-cases.
>
> So, for now, let it be in this way:
>
> --- a/include/net/xdp_priv.h
> +++ b/include/net/xdp_priv.h
> @@ -19,6 +19,7 @@ struct xdp_mem_allocator {
> struct delayed_work defer_wq;
> unsigned long defer_warn;
> unsigned long refcnt;
> + u32 queue_index;
> };
>
> #endif /* __LINUX_NET_XDP_PRIV_H__ */
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index a44621190fdc..c4bf29810f4d 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -324,7 +324,7 @@ static bool __is_supported_mem_type(enum xdp_mem_type type)
> return true;
> }
>
> -static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
> +static struct xdp_mem_allocator *xdp_allocator_find(void *allocator)
> {
> struct xdp_mem_allocator *xae, *xa = NULL;
> struct rhashtable_iter iter;
> @@ -336,7 +336,6 @@ static struct xdp_mem_allocator *xdp_allocator_get(void *allocator)
>
> while ((xae = rhashtable_walk_next(&iter)) && !IS_ERR(xae)) {
> if (xae->allocator == allocator) {
> - xae->refcnt++;
> xa = xae;
> break;
> }
> @@ -386,9 +385,13 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> }
> }
>
> - xdp_alloc = xdp_allocator_get(allocator);
> + xdp_alloc = xdp_allocator_find(allocator);
> if (xdp_alloc) {
> + if (xdp_alloc->queue_index != xdp_rxq->queue_index)
> + return -EINVAL;
> +
> xdp_rxq->mem.id = xdp_alloc->mem.id;
> + xdp_alloc->refcnt++;
This is now adjusted outside lock, not good.
> return 0;
> }
>
> @@ -406,6 +409,7 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
> xdp_alloc->mem = xdp_rxq->mem;
> xdp_alloc->allocator = allocator;
> xdp_alloc->refcnt = 1;
> + xdp_alloc->queue_index = xdp_rxq->queue_index;
>
> /* Insert allocator into ID lookup table */
> ptr = rhashtable_insert_slow(mem_id_ht, &id, &xdp_alloc->node);
>
> Jesper, are you Ok with this version?
Please see my other patch, this is based on our first refcnt attempt.
I think that other patch is a better way forward.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Ivan Khoronzhuk @ 2019-07-02 14:44 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap
In-Reply-To: <156207778364.29180.5111562317930943530.stgit@firesoul>
On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
>From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>
>Jesper recently removed page_pool_destroy() (from driver invocation) and
>moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
>handle in-flight packets/pages. This created an asymmetry in drivers
>create/destroy pairs.
>
>This patch add page_pool user refcnt and reintroduce page_pool_destroy.
>This serves two purposes, (1) simplify drivers error handling as driver now
>drivers always calls page_pool_destroy() and don't need to track if
>xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
>where a single RX-queue (with a single page_pool) provides packets for two
>net_device'es, and thus needs to register the same page_pool twice with two
>xdp_rxq_info structures.
As I tend to use xdp level patch there is no more reason to mention (2) case
here. XDP patch serves it better and can prevent not only obj deletion but also
pool flush, so, this one patch I could better leave only for (1) case.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* [PATCH][next] wil6210: fix wil_cid_valid with negative cid values
From: Colin King @ 2019-07-02 14:40 UTC (permalink / raw)
To: Maya Erez, Kalle Valo, David S . Miller, linux-wireless, wil6210,
netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There are several occasions where a negative cid value is passed
into wil_cid_valid and this is converted into a u8 causing the
range check of cid >= 0 to always succeed. Fix this by making
the cid argument an int to handle any -ve error value of cid.
An example of this behaviour is in wil_cfg80211_dump_station,
where cid is assigned -ENOENT if the call to wil_find_cid_by_idx
fails, and this -ve value is passed to wil_cid_valid. I believe
that the conversion of -ENOENT to the u8 value 254 which is
greater than wil->max_assoc_sta causes wil_find_cid_by_idx to
currently work fine, but I think is by luck and not the
intended behaviour.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/ath/wil6210/wil6210.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h
index 6f456b311a39..25a1adcb38eb 100644
--- a/drivers/net/wireless/ath/wil6210/wil6210.h
+++ b/drivers/net/wireless/ath/wil6210/wil6210.h
@@ -1144,7 +1144,7 @@ static inline void wil_c(struct wil6210_priv *wil, u32 reg, u32 val)
/**
* wil_cid_valid - check cid is valid
*/
-static inline bool wil_cid_valid(struct wil6210_priv *wil, u8 cid)
+static inline bool wil_cid_valid(struct wil6210_priv *wil, int cid)
{
return (cid >= 0 && cid < wil->max_assoc_sta);
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] User mode linux bump maximum MTU tuntap interface [RESAND]
From: Richard Weinberger @ 2019-07-02 14:40 UTC (permalink / raw)
To: Алексей
Cc: netdev, linux-um, Anton Ivanov
In-Reply-To: <fc526c78-2d3f-90ca-8317-a89eb653cbf9@yandex.ru>
CC'ing um folks.
On Tue, Jul 2, 2019 at 3:01 PM Алексей <ne-vlezay80@yandex.ru> wrote:
>
> Hello, the parameter ETH_MAX_PACKET limited to 1500 bytes is the not
> support jumbo frames.
>
> This patch change ETH_MAX_PACKET the 65535 bytes to jumbo frame support
> with user mode linux tuntap driver.
>
>
> PATCH:
>
> -------------------
>
>
> diff -ruNP ../linux_orig/linux-5.1/arch/um/include/shared/net_user.h
> ./arch/um/include/shared/net_user.h
> --- a/arch/um/include/shared/net_user.h 2019-05-06 00:42:58.000000000
> +0000
> +++ b/arch/um/include/shared/net_user.h 2019-07-02 07:14:13.593333356
> +0000
> @@ -9,7 +9,7 @@
> #define ETH_ADDR_LEN (6)
> #define ETH_HEADER_ETHERTAP (16)
> #define ETH_HEADER_OTHER (26) /* 14 for ethernet + VLAN + MPLS for
> crazy people */
> -#define ETH_MAX_PACKET (1500)
> +#define ETH_MAX_PACKET (65535)
>
> #define UML_NET_VERSION (4)
>
> -------------------
>
>
--
Thanks,
//richard
^ permalink raw reply
* Re: [PATCH net-next 5/5] ipv4: use indirect call wrappers for {tcp,udp}_{recv,send}msg()
From: Willem de Bruijn @ 2019-07-02 14:38 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Network Development, David S. Miller
In-Reply-To: <a2806dab7e472b5316da87318f1b8e48ff68cd4b.camel@redhat.com>
On Tue, Jul 2, 2019 at 9:03 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Mon, 2019-07-01 at 15:07 -0400, Willem de Bruijn wrote:
> > On Mon, Jul 1, 2019 at 1:10 PM Paolo Abeni <pabeni@redhat.com> wrote:
> > > This avoids an indirect call per syscall for common ipv4 transports
> > >
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > > net/ipv4/af_inet.c | 12 +++++++++---
> > > 1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> > > index 8421e2f5bbb3..9a2f17d0c5f5 100644
> > > --- a/net/ipv4/af_inet.c
> > > +++ b/net/ipv4/af_inet.c
> > > @@ -797,6 +797,8 @@ int inet_send_prepare(struct sock *sk)
> > > }
> > > EXPORT_SYMBOL_GPL(inet_send_prepare);
> > >
> > > +INDIRECT_CALLABLE_DECLARE(int udp_sendmsg(struct sock *, struct msghdr *,
> > > + size_t));
> >
> > Small nit: this is already defined in include/net/udp.h, which is
> > included. So like tcp_sendmsg, probably no need to declare.
>
> Thank you for the review!
>
> You are right, that declaration can be dropped.
> >
> > If defining inet6_sendmsg and inet6_recvmsg in include/net/ipv6.h,
> > perhaps do the same for the other missing functions, instead of these
> > indirect declarations at the callsite?
>
> Uhm... since inet6_{send,recv}msg exists only for retpoline sake and
> are not exported, I think is probably better move their declaration to
> socket.c via INDIRECT_CALLABLE_DECLARE(), to that ICWs are all self-
> contained.
>
> Unless there are objections about spamming, I can repost the series
> with the above changes.
If just for the spurious declaration, can be merged as is, too.
Either way. Not spammy at all to resend after a day.
^ permalink raw reply
* [PATCH bpf] xdp: fix race on generic receive path
From: Ilya Maximets @ 2019-07-02 14:36 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: <CGME20190702143639eucas1p2b168c68c35b70aac75cad6c72ccc81ad@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>
---
include/net/xdp_sock.h | 2 ++
net/xdp/xsk.c | 32 +++++++++++++++++++++++---------
2 files changed, 25 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..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);
+
+ 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 +143,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_irqrestore(&xs->rx_lock, flags);
+
+ xs->sk.sk_data_ready(&xs->sk);
+ return 0;
+
+out_drop:
xs->rx_dropped++;
+out_unlock:
+ spin_unlock_irqrestore(&xs->rx_lock, flags);
return err;
}
@@ -765,6 +778,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 net-next 3/3] macsec: add brackets and indentation after calling macsec_decrypt
From: Willem de Bruijn @ 2019-07-02 14:35 UTC (permalink / raw)
To: Andreas Steinmetz; +Cc: Sabrina Dubroca, Network Development
In-Reply-To: <b393fc9f9a9e4e49b9cbe6edebb3cd38301ffd92.camel@domdv.de>
On Tue, Jul 2, 2019 at 12:38 AM Andreas Steinmetz <ast@domdv.de> wrote:
>
> Ouch, I missed that when Andreas sent me that patch before. No, it is
> > actually intended. If we skip macsec_decrypt(), we should still
> > account for that packet in the InPktsUnchecked/InPktsDelayed
> > counters. That's in Figure 10-5 in the standard.
> >
> > Thanks for catching this, Willem. That patch should only move the
> > IS_ERR(skb) case under the block where macsec_decrypt() is called, but
> > not move the call to macsec_post_decrypt().
>
> Updated patch below.
>
> Signed-off-by: Andreas Steinmetz <ast@domdv.de>
When making a change in a patch set, please resubmit the entire
patchset (with a v2, and record the changelog).
^ permalink raw reply
* Re: [PATCH net 2/2] macsec: fix checksumming after decryption
From: Willem de Bruijn @ 2019-07-02 14:35 UTC (permalink / raw)
To: Andreas Steinmetz; +Cc: Network Development, Sabrina Dubroca
In-Reply-To: <94382bd8cfbf924779ce86cd6405331f70f65c27.camel@domdv.de>
On Tue, Jul 2, 2019 at 12:25 AM Andreas Steinmetz <ast@domdv.de> wrote:
>
> On Sun, 2019-06-30 at 21:47 -0400, Willem de Bruijn wrote:
> > On Sun, Jun 30, 2019 at 4:48 PM Andreas Steinmetz <ast@domdv.de>
> > wrote:
> > > Fix checksumming after decryption.
> > >
> > > Signed-off-by: Andreas Steinmetz <ast@domdv.de>
> > >
> > > --- a/drivers/net/macsec.c 2019-06-30 22:14:10.250285314 +0200
> > > +++ b/drivers/net/macsec.c 2019-06-30 22:15:11.931230417 +0200
> > > @@ -869,6 +869,7 @@
> > >
> > > static void macsec_finalize_skb(struct sk_buff *skb, u8 icv_len,
> > > u8 hdr_len)
> > > {
> > > + skb->ip_summed = CHECKSUM_NONE;
> > > memmove(skb->data + hdr_len, skb->data, 2 * ETH_ALEN);
> > > skb_pull(skb, hdr_len);
> > > pskb_trim_unique(skb, skb->len - icv_len);
> >
> > Does this belong in macset_reset_skb?
>
> Putting this in macsec_reset_skb would then miss out the "nosci:" part
> of the RX path in macsec_handle_frame().
It is called on each nskb before calling netif_rx.
It indeed is not called when returning RX_HANDLER_PASS, but that is correct?
^ permalink raw reply
* [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Jesper Dangaard Brouer @ 2019-07-02 14:31 UTC (permalink / raw)
To: netdev, Ilias Apalodimas, ivan.khoronzhuk, Jesper Dangaard Brouer
Cc: grygorii.strashko, jakub.kicinski, daniel, john.fastabend, ast,
linux-kernel, linux-omap
In-Reply-To: <20190702153902.0e42b0b2@carbon>
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Jesper recently removed page_pool_destroy() (from driver invocation) and
moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
handle in-flight packets/pages. This created an asymmetry in drivers
create/destroy pairs.
This patch add page_pool user refcnt and reintroduce page_pool_destroy.
This serves two purposes, (1) simplify drivers error handling as driver now
drivers always calls page_pool_destroy() and don't need to track if
xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
where a single RX-queue (with a single page_pool) provides packets for two
net_device'es, and thus needs to register the same page_pool twice with two
xdp_rxq_info structures.
This patch is a modified version of Ivan Khoronzhuk's original patch.
Thus, Jesper gives author ownership to Ivan.
Link: https://lore.kernel.org/netdev/20190625175948.24771-2-ivan.khoronzhuk@linaro.org/
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
To Ivan,
If you agree with this patch, please add your Signed-off-by.
You can also say if you prefer to take this patch and make it
part of your driver patchset, what ever you prefer.
--Jesper
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 6 ++---
drivers/net/ethernet/socionext/netsec.c | 8 ++----
include/net/page_pool.h | 27 +++++++++++++++++++++
net/core/page_pool.c | 8 ++++++
net/core/xdp.c | 3 ++
5 files changed, 43 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 1085040675ae..ce1c7a449eae 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -545,10 +545,8 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
}
err = xdp_rxq_info_reg_mem_model(&rq->xdp_rxq,
MEM_TYPE_PAGE_POOL, rq->page_pool);
- if (err) {
- page_pool_free(rq->page_pool);
+ if (err)
goto err_free;
- }
for (i = 0; i < wq_sz; i++) {
if (rq->wq_type == MLX5_WQ_TYPE_LINKED_LIST_STRIDING_RQ) {
@@ -613,6 +611,7 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
if (rq->xdp_prog)
bpf_prog_put(rq->xdp_prog);
xdp_rxq_info_unreg(&rq->xdp_rxq);
+ page_pool_destroy(rq->page_pool);
mlx5_wq_destroy(&rq->wq_ctrl);
return err;
@@ -643,6 +642,7 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
}
xdp_rxq_info_unreg(&rq->xdp_rxq);
+ page_pool_destroy(rq->page_pool);
mlx5_wq_destroy(&rq->wq_ctrl);
}
diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 5544a722543f..43ab0ce90704 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -1210,15 +1210,11 @@ static void netsec_uninit_pkt_dring(struct netsec_priv *priv, int id)
}
}
- /* Rx is currently using page_pool
- * since the pool is created during netsec_setup_rx_dring(), we need to
- * free the pool manually if the registration failed
- */
+ /* Rx is currently using page_pool */
if (id == NETSEC_RING_RX) {
if (xdp_rxq_info_is_reg(&dring->xdp_rxq))
xdp_rxq_info_unreg(&dring->xdp_rxq);
- else
- page_pool_free(dring->page_pool);
+ page_pool_destroy(dring->page_pool);
}
memset(dring->desc, 0, sizeof(struct netsec_desc) * DESC_NUM);
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index ee9c871d2043..ea974856d0f7 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -101,6 +101,14 @@ struct page_pool {
struct ptr_ring ring;
atomic_t pages_state_release_cnt;
+
+ /* A page_pool is strictly tied to a single RX-queue being
+ * protected by NAPI, due to above pp_alloc_cache. This
+ * refcnt serves two purposes. (1) simplify drivers error
+ * handling, and (2) allow special cases where a single
+ * RX-queue provides packet for two net_device'es.
+ */
+ refcount_t user_cnt;
};
struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
@@ -134,6 +142,15 @@ static inline void page_pool_free(struct page_pool *pool)
#endif
}
+/* Drivers use this instead of page_pool_free */
+static inline void page_pool_destroy(struct page_pool *pool)
+{
+ if (!pool)
+ return;
+
+ page_pool_free(pool);
+}
+
/* Never call this directly, use helpers below */
void __page_pool_put_page(struct page_pool *pool,
struct page *page, bool allow_direct);
@@ -201,4 +218,14 @@ static inline bool is_page_pool_compiled_in(void)
#endif
}
+static inline void page_pool_get(struct page_pool *pool)
+{
+ refcount_inc(&pool->user_cnt);
+}
+
+static inline bool page_pool_put(struct page_pool *pool)
+{
+ return refcount_dec_and_test(&pool->user_cnt);
+}
+
#endif /* _NET_PAGE_POOL_H */
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index b366f59885c1..3272dc7a8c81 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -49,6 +49,9 @@ static int page_pool_init(struct page_pool *pool,
atomic_set(&pool->pages_state_release_cnt, 0);
+ /* Driver calling page_pool_create() also call page_pool_destroy() */
+ refcount_set(&pool->user_cnt, 1);
+
if (pool->p.flags & PP_FLAG_DMA_MAP)
get_device(pool->p.dev);
@@ -70,6 +73,7 @@ struct page_pool *page_pool_create(const struct page_pool_params *params)
kfree(pool);
return ERR_PTR(err);
}
+
return pool;
}
EXPORT_SYMBOL(page_pool_create);
@@ -356,6 +360,10 @@ static void __warn_in_flight(struct page_pool *pool)
void __page_pool_free(struct page_pool *pool)
{
+ /* Only last user actually free/release resources */
+ if (!page_pool_put(pool))
+ return;
+
WARN(pool->alloc.count, "API usage violation");
WARN(!ptr_ring_empty(&pool->ring), "ptr_ring is not empty");
diff --git a/net/core/xdp.c b/net/core/xdp.c
index b29d7b513a18..e57a0eb1feb7 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -372,6 +372,9 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
mutex_unlock(&mem_id_lock);
+ if (type == MEM_TYPE_PAGE_POOL)
+ page_pool_get(xdp_alloc->page_pool);
+
trace_mem_connect(xdp_alloc, xdp_rxq);
return 0;
err:
^ permalink raw reply related
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Nikolay Aleksandrov @ 2019-07-02 14:27 UTC (permalink / raw)
To: Linus Lüssing, Ido Schimmel
Cc: Russell King - ARM Linux admin, Ido Schimmel, Vivien Didelot,
Florian Fainelli, netdev@vger.kernel.org, Jiri Pirko,
andrew@lunn.ch, davem@davemloft.net, bridge, b.a.t.m.a.n
In-Reply-To: <20190630165601.GC2500@otheros>
On 30/06/2019 19:56, Linus Lüssing wrote:
> On Sat, Jun 29, 2019 at 07:29:45PM +0300, Ido Schimmel wrote:
>> I would like to avoid having drivers take the querier state into account
>> as it will only complicate things further.
>
> I absolutely share your pain. Initially in the early prototypes of
> multicast awareness in batman-adv we did not consider the querier state.
> And doing so later did indeed complicate the code a good bit in batman-adv
> (together with the IGMP/MLD suppression issues). I would have loved to
> avoid that.
>
>
>> Is there anything we can do about it? Enable the bridge querier if no
>> other querier was detected? Commit c5c23260594c ("bridge: Add
>> multicast_querier toggle and disable queries by default") disabled
>> queries by default, but I'm only suggesting to turn them on if no other
>> querier was detected on the link. Do you think it's still a problem?
>
> As soon as you start becoming the querier, you will not be able to reliably
> detect anymore whether you are the only querier candidate.
>
> If any random Linux host using a bridge device were potentially becoming
> a querier, that would cause quite some trouble when this host is
> behind some bad, bottleneck connection. This host will receive
> all multicast traffic, not just IGMP/MLD reports. And with a
> congested connection and then unreliable IGMP/MLD, multicast would
> become unreliable overall in this domain. So it's important that
> your querier is not running in the "dark, remote, dusty closet" of
> your network (topologically speaking).
>
+1
We definitely don't want random hosts becoming queriers
>> On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
>>> See commit b00589af3b04 ("bridge: disable snooping if there is no
>>> querier"). I think that's unfortunate behavior that we need because
>>> multicast snooping is enabled by default. If it weren't enabled by
>>> default, then anyone enabling it would also make sure there's a querier
>>> in the network.
>
> I do not quite understand that point. In a way, that's what we
> have right now, isn't it? By default it's disabled, because by
> default there is no querier on the link. So anyone wanting to use
> multicast snooping will need to make sure there's a querier in the
> network.
>
Indeed, also you could create the bridge with explicit mcast parameters if you need
different behaviour on start. Unfortunately I think you'll have to handle
the querier state.
>
> Overall I think the querier (election) mechanism in the standards could
> need an update. While the lowest-address first might have
> worked well back then, in uniform, fully wired networks where the
> position of the querier did not matter, this is not a good
> solution anymore in networks involving wireless, dynamic connections.
> Especially in wireless mesh networks this is a bit of an issue for
> us. Ideally, the querier mechanism were dismissed in favour of simply
> unsolicited, periodic IGMP/MLD reports...
>
> But of course, updating IETF standards is no solution for now.
>
> While more complicated, it would not be impossible to consider the
> querier state, would it? I mean you probably already need to
> consider the case of a user disabling multicast snooping during
> runtime, right? So similarly, you could react to appearing or
> disappearing queriers?
>
> Cheers, Linus
>
Thanks,
Nik
^ permalink raw reply
* Re: [PATCH v5 net-next 6/6] net: ethernet: ti: cpsw: add XDP support
From: Ivan Khoronzhuk @ 2019-07-02 14:24 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: grygorii.strashko, davem, ast, linux-kernel, linux-omap,
ilias.apalodimas, netdev, daniel, jakub.kicinski, john.fastabend
In-Reply-To: <20190702153902.0e42b0b2@carbon>
On Tue, Jul 02, 2019 at 03:39:02PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 2 Jul 2019 14:37:39 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> On Mon, Jul 01, 2019 at 06:19:01PM +0200, Jesper Dangaard Brouer wrote:
>> >On Sun, 30 Jun 2019 20:23:48 +0300
>> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>> >
>> >> +static int cpsw_ndev_create_xdp_rxq(struct cpsw_priv *priv, int ch)
>> >> +{
>> >> + struct cpsw_common *cpsw = priv->cpsw;
>> >> + int ret, new_pool = false;
>> >> + struct xdp_rxq_info *rxq;
>> >> +
>> >> + rxq = &priv->xdp_rxq[ch];
>> >> +
>> >> + ret = xdp_rxq_info_reg(rxq, priv->ndev, ch);
>> >> + if (ret)
>> >> + return ret;
>> >> +
>> >> + if (!cpsw->page_pool[ch]) {
>> >> + ret = cpsw_create_rx_pool(cpsw, ch);
>> >> + if (ret)
>> >> + goto err_rxq;
>> >> +
>> >> + new_pool = true;
>> >> + }
>> >> +
>> >> + ret = xdp_rxq_info_reg_mem_model(rxq, MEM_TYPE_PAGE_POOL,
>> >> + cpsw->page_pool[ch]);
>> >> + if (!ret)
>> >> + return 0;
>> >> +
>> >> + if (new_pool) {
>> >> + page_pool_free(cpsw->page_pool[ch]);
>> >> + cpsw->page_pool[ch] = NULL;
>> >> + }
>> >> +
>> >> +err_rxq:
>> >> + xdp_rxq_info_unreg(rxq);
>> >> + return ret;
>> >> +}
>> >
>> >Looking at this, and Ilias'es XDP-netsec error handling path, it might
>> >be a mistake that I removed page_pool_destroy() and instead put the
>> >responsibility on xdp_rxq_info_unreg().
>>
>> As for me this is started not from page_pool_free, but rather from calling
>> unreg_mem_model from rxq_info_unreg. Then, if page_pool_free is hidden
>> it looks more a while normal to move all chain to be self destroyed.
>>
>> >
>> >As here, we have to detect if page_pool_create() was a success, and then
>> >if xdp_rxq_info_reg_mem_model() was a failure, explicitly call
>> >page_pool_free() because the xdp_rxq_info_unreg() call cannot "free"
>> >the page_pool object given it was not registered.
>>
>> Yes, it looked a little bit ugly from the beginning, but, frankly,
>> I have got used to this already.
>>
>> >
>> >Ivan's patch in[1], might be a better approach, which forced all
>> >drivers to explicitly call page_pool_free(), even-though it just
>> >dec-refcnt and the real call to page_pool_free() happened via
>> >xdp_rxq_info_unreg().
>> >
>> >To better handle error path, I would re-introduce page_pool_destroy(),
>>
>> So, you might to do it later as I understand, and not for my special
>> case but becouse it makes error path to look a little bit more pretty.
>> I'm perfectly fine with this, and better you add this, for now my
>> implementation requires only "xdp: allow same allocator usage" patch,
>> but if you insist I can resend also patch in question afterwards my
>> series is applied (with modification to cpsw & netsec & mlx5 & page_pool).
>>
>> What's your choice? I can add to your series patch needed for cpsw to
>> avoid some misuse.
>
>I will try to create a cleaned-up version of your patch[1] and
>re-introduce page_pool_destroy() for drivers to use, then we can build
>your driver on top of that.
I've corrected patch to xdp core and tested. The "page pool API" change
seems is orthogonal now. So no limits to send v6 that is actually done
and no more strict dependency on page pool API changes whenever that
can happen.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* [PATCH -next] carl9170: remove set but not used variable 'udev'
From: YueHaibing @ 2019-07-02 14:12 UTC (permalink / raw)
To: chunkeey; +Cc: linux-kernel, netdev, linux-wireless, kvalo, davem, YueHaibing
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/net/wireless/ath/carl9170/usb.c: In function carl9170_usb_disconnect:
drivers/net/wireless/ath/carl9170/usb.c:1110:21:
warning: variable udev set but not used [-Wunused-but-set-variable]
It is not use since commit feb09b293327 ("carl9170:
fix misuse of device driver API")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/ath/carl9170/usb.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
index 99f1897..486957a 100644
--- a/drivers/net/wireless/ath/carl9170/usb.c
+++ b/drivers/net/wireless/ath/carl9170/usb.c
@@ -1107,12 +1107,10 @@ static int carl9170_usb_probe(struct usb_interface *intf,
static void carl9170_usb_disconnect(struct usb_interface *intf)
{
struct ar9170 *ar = usb_get_intfdata(intf);
- struct usb_device *udev;
if (WARN_ON(!ar))
return;
- udev = ar->udev;
wait_for_completion(&ar->fw_load_wait);
if (IS_INITIALIZED(ar)) {
--
2.7.4
^ permalink raw reply related
* Re: [PATCH bpf-next v3] virtio_net: add XDP meta data support
From: Yuya Kusakabe @ 2019-07-02 14:11 UTC (permalink / raw)
To: Jason Wang
Cc: ast, daniel, davem, hawk, jakub.kicinski, john.fastabend, kafai,
mst, netdev, songliubraving, yhs
In-Reply-To: <ca724dcf-4ffb-ff49-d307-1b45143712b5@redhat.com>
On 7/2/19 5:33 PM, Jason Wang wrote:
>
> On 2019/7/2 下午4:16, Yuya Kusakabe wrote:
>> This adds XDP meta data support to both receive_small() and
>> receive_mergeable().
>>
>> Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access")
>> Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com>
>> ---
>> v3:
>> - fix preserve the vnet header in receive_small().
>> v2:
>> - keep copy untouched in page_to_skb().
>> - preserve the vnet header in receive_small().
>> - fix indentation.
>> ---
>> drivers/net/virtio_net.c | 45 +++++++++++++++++++++++++++-------------
>> 1 file changed, 31 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 4f3de0ac8b0b..03a1ae6fe267 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -371,7 +371,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> struct receive_queue *rq,
>> struct page *page, unsigned int offset,
>> unsigned int len, unsigned int truesize,
>> - bool hdr_valid)
>> + bool hdr_valid, unsigned int metasize)
>> {
>> struct sk_buff *skb;
>> struct virtio_net_hdr_mrg_rxbuf *hdr;
>> @@ -393,7 +393,7 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> else
>> hdr_padded_len = sizeof(struct padded_vnet_hdr);
>> - if (hdr_valid)
>> + if (hdr_valid && !metasize)
>> memcpy(hdr, p, hdr_len);
>> len -= hdr_len;
>> @@ -405,6 +405,11 @@ static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> copy = skb_tailroom(skb);
>> skb_put_data(skb, p, copy);
>> + if (metasize) {
>> + __skb_pull(skb, metasize);
>> + skb_metadata_set(skb, metasize);
>> + }
>> +
>> len -= copy;
>> offset += copy;
>> @@ -644,6 +649,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> unsigned int delta = 0;
>> struct page *xdp_page;
>> int err;
>> + unsigned int metasize = 0;
>> len -= vi->hdr_len;
>> stats->bytes += len;
>> @@ -683,10 +689,13 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>> xdp.data = xdp.data_hard_start + xdp_headroom;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + len;
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> orig_data = xdp.data;
>> + /* Copy the vnet header to the front of data_hard_start to avoid
>> + * overwriting by XDP meta data */
>> + memcpy(xdp.data_hard_start - vi->hdr_len, xdp.data - vi->hdr_len, vi->hdr_len);
>
>
> What happens if we have a large metadata that occupies all headroom here?
>
> Thanks
Do you mean a large "XDP" metadata? If a large metadata is a large "XDP" metadata, I think we can not use a metadata that occupies all headroom. The size of metadata limited by bpf_xdp_adjust_meta() as below.
bpf_xdp_adjust_meta() in net/core/filter.c:
if (unlikely((metalen & (sizeof(__u32) - 1)) ||
(metalen > 32)))
return -EACCES;
Thanks.
>
>
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> stats->xdp_packets++;
>> @@ -695,9 +704,11 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> /* Recalculate length in case bpf program changed it */
>> delta = orig_data - xdp.data;
>> len = xdp.data_end - xdp.data;
>> + metasize = xdp.data - xdp.data_meta;
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -736,10 +747,12 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> skb_reserve(skb, headroom - delta);
>> skb_put(skb, len);
>> if (!delta) {
>> - buf += header_offset;
>> - memcpy(skb_vnet_hdr(skb), buf, vi->hdr_len);
>> + memcpy(skb_vnet_hdr(skb), buf + VIRTNET_RX_PAD, vi->hdr_len);
>> } /* keep zeroed vnet hdr since packet was changed by bpf */
>> + if (metasize)
>> + skb_metadata_set(skb, metasize);
>> +
>> err:
>> return skb;
>> @@ -760,8 +773,8 @@ static struct sk_buff *receive_big(struct net_device *dev,
>> struct virtnet_rq_stats *stats)
>> {
>> struct page *page = buf;
>> - struct sk_buff *skb = page_to_skb(vi, rq, page, 0, len,
>> - PAGE_SIZE, true);
>> + struct sk_buff *skb =
>> + page_to_skb(vi, rq, page, 0, len, PAGE_SIZE, true, 0);
>> stats->bytes += len - vi->hdr_len;
>> if (unlikely(!skb))
>> @@ -793,6 +806,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> unsigned int truesize;
>> unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>> int err;
>> + unsigned int metasize = 0;
>> head_skb = NULL;
>> stats->bytes += len - vi->hdr_len;
>> @@ -839,8 +853,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> data = page_address(xdp_page) + offset;
>> xdp.data_hard_start = data - VIRTIO_XDP_HEADROOM + vi->hdr_len;
>> xdp.data = data + vi->hdr_len;
>> - xdp_set_data_meta_invalid(&xdp);
>> xdp.data_end = xdp.data + (len - vi->hdr_len);
>> + xdp.data_meta = xdp.data;
>> xdp.rxq = &rq->xdp_rxq;
>> act = bpf_prog_run_xdp(xdp_prog, &xdp);
>> @@ -852,8 +866,9 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> * adjustments. Note other cases do not build an
>> * skb and avoid using offset
>> */
>> - offset = xdp.data -
>> - page_address(xdp_page) - vi->hdr_len;
>> + metasize = xdp.data - xdp.data_meta;
>> + offset = xdp.data - page_address(xdp_page) -
>> + vi->hdr_len - metasize;
>> /* recalculate len if xdp.data or xdp.data_end were
>> * adjusted
>> @@ -863,14 +878,15 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> if (unlikely(xdp_page != page)) {
>> rcu_read_unlock();
>> put_page(page);
>> - head_skb = page_to_skb(vi, rq, xdp_page,
>> - offset, len,
>> - PAGE_SIZE, false);
>> + head_skb = page_to_skb(vi, rq, xdp_page, offset,
>> + len, PAGE_SIZE, false,
>> + metasize);
>> return head_skb;
>> }
>> break;
>> case XDP_TX:
>> stats->xdp_tx++;
>> + xdp.data_meta = xdp.data;
>> xdpf = convert_to_xdp_frame(&xdp);
>> if (unlikely(!xdpf))
>> goto err_xdp;
>> @@ -921,7 +937,8 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> goto err_skb;
>> }
>> - head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog);
>> + head_skb = page_to_skb(vi, rq, page, offset, len, truesize, !xdp_prog,
>> + metasize);
>> curr_skb = head_skb;
>> if (unlikely(!curr_skb))
^ 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