* Re: [PATCH rdma-next 00/12] Add SRQ and XRC support for ODP MRs
From: Leon Romanovsky @ 2019-01-24 12:25 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: RDMA mailing list, Majd Dibbiny, Moni Shoua, Saeed Mahameed,
linux-netdev
In-Reply-To: <20190122064851.6032-1-leon@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]
On Tue, Jan 22, 2019 at 08:48:39AM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Hi,
>
> This series extend ODP to work with SRQ and XRC. Being both per-operation
> (e.g. RDMA write, RDMA read and atomic) and per-transport (e.g. RC, UD and XRC),
> we extend IB/core and mlx5 driver to provide needed information to user space.
>
> Thanks
>
> Moni Shoua (12):
> IB/mlx5: Fix locking SRQ object in ODP event
> IB/core: Allocate bit for SRQ ODP support
> IB/uverbs: Expose XRC ODP device capabilities
> IB/mlx5: Remove useless check in ODP handler
> IB/mlx5: Clean mlx5_ib_mr_responder_pfault_handler() signature
> IB/mlx5: Add XRC initiator ODP support
> IB/mlx5: Let read user wqe also from SRQ buffer
> IB/mlx5: Add ODP SRQ support
> IB/mlx5: Advertise SRQ ODP support for supported transports
> net/mlx5: Add XRC transport to ODP device capabilities layout
> IB/mlx5: Advertise XRC ODP support
> net/mlx5: Set ODP SRQ support in firmware
Doug, Jason,
Please pay attention that patches below should go to shared branch:
net/mlx5: Add XRC transport to ODP device capabilities layout
net/mlx5: Set ODP SRQ support in firmware
Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH] ipvs: Fix signed integer overflow when setsockopt timeout
From: zhangxiaoxu (A) @ 2019-01-24 12:22 UTC (permalink / raw)
To: Simon Horman; +Cc: wensong, ja, netdev, lvs-devel, Pablo Neira Ayuso
In-Reply-To: <20190114121513.mgj45pt3rrgomrqo@verge.net.au>
ping.
On 1/14/2019 8:15 PM, Simon Horman wrote:
> On Thu, Jan 10, 2019 at 04:39:06PM +0800, ZhangXiaoxu wrote:
>> There is a UBSAN bug report as below:
>> UBSAN: Undefined behaviour in net/netfilter/ipvs/ip_vs_ctl.c:2227:21
>> signed integer overflow:
>> -2147483647 * 1000 cannot be represented in type 'int'
>>
>> Reproduce program:
>> #include <stdio.h>
>> #include <sys/types.h>
>> #include <sys/socket.h>
>>
>> #define IPPROTO_IP 0
>> #define IPPROTO_RAW 255
>>
>> #define IP_VS_BASE_CTL (64+1024+64)
>> #define IP_VS_SO_SET_TIMEOUT (IP_VS_BASE_CTL+10)
>>
>> /* The argument to IP_VS_SO_GET_TIMEOUT */
>> struct ipvs_timeout_t {
>> int tcp_timeout;
>> int tcp_fin_timeout;
>> int udp_timeout;
>> };
>>
>> int main() {
>> int ret = -1;
>> int sockfd = -1;
>> struct ipvs_timeout_t to;
>>
>> sockfd = socket(AF_INET, SOCK_RAW, IPPROTO_RAW);
>> if (sockfd == -1) {
>> printf("socket init error\n");
>> return -1;
>> }
>>
>> to.tcp_timeout = -2147483647;
>> to.tcp_fin_timeout = -2147483647;
>> to.udp_timeout = -2147483647;
>>
>> ret = setsockopt(sockfd,
>> IPPROTO_IP,
>> IP_VS_SO_SET_TIMEOUT,
>> (char *)(&to),
>> sizeof(to));
>>
>> printf("setsockopt return %d\n", ret);
>> return ret;
>> }
>>
>> Return -EINVAL if the timeout value is negative or max than 'INT_MAX / HZ'.
>>
>> Signed-off-by: ZhangXiaoxu <zhangxiaoxu5@huawei.com>
>
> Thanks, this looks good to me.
>
> Pablo, could you consider applying this to nf?
>
> Acked-by: Simon Horman <horms@verge.net.au>
>
>> ---
>> net/netfilter/ipvs/ip_vs_ctl.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
>> index 432141f..444aaca 100644
>> --- a/net/netfilter/ipvs/ip_vs_ctl.c
>> +++ b/net/netfilter/ipvs/ip_vs_ctl.c
>> @@ -2221,6 +2221,19 @@ static int ip_vs_set_timeout(struct netns_ipvs *ipvs, struct ip_vs_timeout_user
>> u->udp_timeout);
>>
>> #ifdef CONFIG_IP_VS_PROTO_TCP
>> + if (u->tcp_timeout < 0 || u->tcp_timeout > (INT_MAX / HZ) ||
>> + u->tcp_fin_timeout < 0 || u->tcp_fin_timeout > (INT_MAX / HZ)) {
>> + return -EINVAL;
>> + }
>> +#endif
>> +
>> +#ifdef CONFIG_IP_VS_PROTO_UDP
>> + if (u->udp_timeout < 0 || u->udp_timeout > (INT_MAX / HZ)) {
>> + return -EINVAL;
>> + }
>> +#endif
>> +
>> +#ifdef CONFIG_IP_VS_PROTO_TCP
>> if (u->tcp_timeout) {
>> pd = ip_vs_proto_data_get(ipvs, IPPROTO_TCP);
>> pd->timeout_table[IP_VS_TCP_S_ESTABLISHED]
>> --
>> 2.7.4
>>
>
> .
>
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/8] libbpf: Add a helper for retrieving a prog via index
From: Jesper Dangaard Brouer @ 2019-01-24 12:09 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Maciej Fijalkowski, ast, netdev, jakub.kicinski, brouer
In-Reply-To: <99b1958f-d04c-2b4a-81c1-e9942512df37@iogearbox.net>
On Thu, 24 Jan 2019 12:56:36 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:
> > Please feel free to deprecate or remove the xdp_redirect_cpu --prognum
> > option. I would prefer if this was indeed converted to selecting the
> > program based on the name in the _kern.c file, instead of a number.
>
> Given this is BPF sample code, there is no need to deprecate, lets just
> remove --prognum option and add a new one for selection by name.
I agree, just remove --prognum option.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: eBPF equivalent of SKF_AD_HATYPE / skb->dev->type
From: Maciej Żenczykowski @ 2019-01-24 11:59 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Alexei Starovoitov, Linux NetDev
In-Reply-To: <ad37a96a-1fe1-7b00-25fd-ea3ed5279f4d@iogearbox.net>
Yeah, I'd like to be able to attach the same program to
cellular/wifi/ethernet(usb) interface at ingress.
I'll cook it up when I can, for now I'll just make do with 2 different
ones to handle the with/without ethernet header cases.
On Thu, Jan 24, 2019 at 3:40 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 01/24/2019 12:27 PM, Maciej Żenczykowski wrote:
> > In classic BPF 'SKF_AD_HATYPE' can be used to fetch skb->dev->type.
> >
> > Are my eyes deceiving me, or is there a similar way to get at it from eBPF?
> >
> > (while we're at it, skb->dev->mtu would probably be useful too)
>
> Yeah we can add it if you have a concrete use case for access out of the BPF
> program, feel free to send a patch. So far nobody ran into the need for it
> which is why it hasn't been added yet.
>
> Thanks,
> Daniel
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/8] libbpf: Add a helper for retrieving a prog via index
From: Daniel Borkmann @ 2019-01-24 11:56 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Maciej Fijalkowski; +Cc: ast, netdev, jakub.kicinski
In-Reply-To: <20190123152451.7fcdb226@redhat.com>
On 01/23/2019 03:24 PM, Jesper Dangaard Brouer wrote:
> On Wed, 23 Jan 2019 14:41:59 +0100
> Maciej Fijalkowski <maciejromanfijalkowski@gmail.com> wrote:
>
>> On Wed, 23 Jan 2019 11:41:11 +0100
>> Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>>> On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
>>>> xdp_redirect_cpu has a 6 different XDP programs that can be attached to
>>>> network interface. This sample has a option --prognum that allows user
>>>> for specifying which particular program from a given set will be
>>>> attached to network interface.
>>>> In order to make it easier when converting the mentioned sample to
>>>> libbpf usage, add a function to libbpf that will return program's fd for
>>>> a given index.
>>>>
>>>> Note that there is already a bpf_object__find_prog_by_idx, which could
>>>> be exported and might be used for that purpose, but it operates on the
>>>> number of ELF section and here we need an index from a programs array
>>>> within the bpf_object.
>>>
>>> Series in general looks good to me. Few minor comments, mainly in relation
>>> to the need for libbpf extensions.
>>>
>>> Would it not be a better interface to the user to instead choose the prog
>>> based on section name and then retrieve it via bpf_object__find_program_by_title()
>>> instead of prognum (which feels less user friendly) at least?
>>
>> I couldn't decide which one from:
>> * adding a libbpf helper
>> * changing the xdp_redirect_cpu behaviour
>>
>> would be more invasive when I was converting this sample to libbpf
>> support.
>>
>> Your suggestion sounds good, but I'm wondering about the actual
>> implementation. I suppose that we would choose the program via
>> command line. Some program section names in this sample are a bit
>> long and it might be irritating for user to type in for example
>> "xdp_cpu_map5_lb_hash_ip_pairs", no? Or maybe we can live with this.
>> In case of typo and program being not found it would be good to print
>> out the section names to choose from in usage().
>
> Please feel free to deprecate or remove the xdp_redirect_cpu --prognum
> option. I would prefer if this was indeed converted to selecting the
> program based on the name in the _kern.c file, instead of a number.
Given this is BPF sample code, there is no need to deprecate, lets just
remove --prognum option and add a new one for selection by name.
> Listing the avail programs will be helpful, and you can also
> shorten/rename the program names.
>
> For easy of use, consider doing like 'ip' tool from[1] iproute2, and
> find the first best matching string. Copy pasted code below signature
> for inspiration.
^ permalink raw reply
* Re: stable backport for the BPF speculation series? [was: Re: [PATCH bpf v3 0/9] bpf fix to prevent oob under speculation]
From: Daniel Borkmann @ 2019-01-24 11:53 UTC (permalink / raw)
To: Jann Horn, Greg Kroah-Hartman
Cc: David S. Miller, Alexei Starovoitov, Alexei Starovoitov,
jakub.kicinski, Network Development
In-Reply-To: <CAG48ez1Cm4EP9UwDyZRfW5q+CEyQDzd0RMCk4GwfcrsUFQuVJg@mail.gmail.com>
On 01/23/2019 06:12 PM, Jann Horn wrote:
> On Wed, Jan 23, 2019 at 6:04 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Tue, Jan 22, 2019 at 03:36:54PM +0100, Jann Horn wrote:
>>> On Thu, Jan 3, 2019 at 1:08 AM Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>> On Thu, Jan 03, 2019 at 12:58:26AM +0100, Daniel Borkmann wrote:
>>>>> This set fixes an out of bounds case under speculative execution
>>>>> by implementing masking of pointer alu into the verifier. For
>>>>> details please see the individual patches.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> v2 -> v3:
>>>>> - 8/9: change states_equal condition into old->speculative &&
>>>>> !cur->speculative, thanks Jakub!
>>>>> - 8/9: remove incorrect speculative state test in
>>>>> propagate_liveness(), thanks Jakub!
>>>>> v1 -> v2:
>>>>> - Typo fixes in commit msg and a comment, thanks David!
>>>>
>>>> Applied, Thanks
>>>
>>> This series and the followup fix ("bpf: fix sanitation of alu op with
>>> pointer / scalar type from different paths") have been in Linus' tree
>>> for six days, but from what I can tell, they aren't queued up for
>>> stable yet.
>>
>> What are the git commit ids of the patches you think should be
>> backported?
>
> Daniel Borkmann said at
> https://marc.info/?l=linux-netdev&m=154820859831443&w=2 :
>
> | Will get this to stable towards end of week. We wanted to let this sit
> | for a while in Linus' tree given the complexity of the fix to get some
> | more coverage. We also need 9d5564ddcf2a ("bpf: fix inner map masking
> |to prevent oob under speculation") in addition.
>
> , so I expect that he's going to submit a request for stable inclusion
> in the next few days. The git commits are:
Yep, correct.
^ permalink raw reply
* Re: eBPF equivalent of SKF_AD_HATYPE / skb->dev->type
From: Daniel Borkmann @ 2019-01-24 11:40 UTC (permalink / raw)
To: Maciej Żenczykowski, Alexei Starovoitov; +Cc: Linux NetDev
In-Reply-To: <CANP3RGcxAOY6mBBqFa26Cc4s3++T0QH1z+mcLFbPGdbxZXqTYQ@mail.gmail.com>
On 01/24/2019 12:27 PM, Maciej Żenczykowski wrote:
> In classic BPF 'SKF_AD_HATYPE' can be used to fetch skb->dev->type.
>
> Are my eyes deceiving me, or is there a similar way to get at it from eBPF?
>
> (while we're at it, skb->dev->mtu would probably be useful too)
Yeah we can add it if you have a concrete use case for access out of the BPF
program, feel free to send a patch. So far nobody ran into the need for it
which is why it hasn't been added yet.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v3 bpf-next] bpf: allow BPF programs access skb_shared_info->gso_segs field
From: Daniel Borkmann @ 2019-01-24 11:30 UTC (permalink / raw)
To: Eric Dumazet, Alexei Starovoitov
Cc: netdev, Eric Dumazet, Eddie Hao, Martin KaFai Lau
In-Reply-To: <20190123172227.203429-1-edumazet@google.com>
On 01/23/2019 06:22 PM, Eric Dumazet wrote:
> This adds the ability to read gso_segs from a BPF program.
>
> v3: Use BPF_REG_AX instead of BPF_REG_TMP for the temporary register,
> as suggested by Martin.
>
> v2: refined Eddie Hao patch to address Alexei feedback.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Eddie Hao <eddieh@google.com>
> Cc: Martin KaFai Lau <kafai@fb.com>
Looks good, applied, thanks!
^ permalink raw reply
* Re: [PATCH bpf-next] bpftool: feature probing, change default action
From: Daniel Borkmann @ 2019-01-24 11:29 UTC (permalink / raw)
To: Prashant Bhole, Alexei Starovoitov; +Cc: netdev, Quentin Monnet, Jakub Kicinski
In-Reply-To: <20190124025157.6812-1-bhole_prashant_q7@lab.ntt.co.jp>
On 01/24/2019 03:51 AM, Prashant Bhole wrote:
> When 'bpftool feature' is executed it shows incorrect help string.
>
> test# bpftool feature
> Usage: bpftool bpftool probe [COMPONENT] [macros [prefix PREFIX]]
> bpftool bpftool help
>
> COMPONENT := { kernel | dev NAME }
>
> Instead of fixing the help text by tweaking argv[] indices, this
> patch changes the default action to 'probe'. It makes the behavior
> consistent with other subcommands, where first subcommand without
> extra parameter results in 'show' action.
>
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
Applied, thanks!
^ permalink raw reply
* eBPF equivalent of SKF_AD_HATYPE / skb->dev->type
From: Maciej Żenczykowski @ 2019-01-24 11:27 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann; +Cc: Linux NetDev
In classic BPF 'SKF_AD_HATYPE' can be used to fetch skb->dev->type.
Are my eyes deceiving me, or is there a similar way to get at it from eBPF?
(while we're at it, skb->dev->mtu would probably be useful too)
Cheers,
Maciej
^ permalink raw reply
* Re: [PATCH] net: dev_is_mac_header_xmit() true for ARPHRD_RAWIP
From: Daniel Borkmann @ 2019-01-24 11:27 UTC (permalink / raw)
To: Maciej Żenczykowski, Maciej Żenczykowski,
David S . Miller, Alexei Starovoitov
Cc: linux-kernel, netdev
In-Reply-To: <20190124110702.140834-1-zenczykowski@gmail.com>
On 01/24/2019 12:07 PM, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
>
> __bpf_redirect() and act_mirred checks this boolean
> to determine whether to prefix an ethernet header.
>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
I presume David would pick this up for net tree, so:
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply
* Re: [PATCH v2 bpf-next] bpf: use the proper optlen when doing strncpy in bpf_getsockopt()
From: Daniel Borkmann @ 2019-01-24 11:24 UTC (permalink / raw)
To: Yafang Shao, kafai, brakmo, ast; +Cc: netdev, shaoyafang
In-Reply-To: <1548306994-877-1-git-send-email-laoar.shao@gmail.com>
On 01/24/2019 06:16 AM, Yafang Shao wrote:
> As strncpy(..., TCP_CA_NAME_MAX-1) is used in bpf_setsockopt(),
> this change will make it more consistent with the bpf_setsockopt() above.
>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
> Acked-by: Lawrence Brakmo <brakmo@fb.com>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
While not wrong, I also don't think it improves much either. As mentioned,
that code is totally fine as it currently is.
^ permalink raw reply
* Re: [RFC bpf-next] bpf: allow users to opt out of releasing objects when urefs are gone
From: Daniel Borkmann @ 2019-01-24 11:17 UTC (permalink / raw)
To: Jakub Kicinski, alexei.starovoitov; +Cc: yhs, sdf, brouer, netdev, oss-drivers
In-Reply-To: <20190121201021.30785-1-jakub.kicinski@netronome.com>
On 01/21/2019 09:10 PM, Jakub Kicinski wrote:
> Commit c9da161c6517 ("bpf: fix clearing on persistent program array maps")
> added protection against dependency loops between programs and maps leading
> to zombie objects which would never be freed. FD maps are flushed when
> last user reference is gone.
>
> This is confusing to users of bpftool, as updates to tail call maps have
> no effect, unless those maps are pinned (or open by another entity).
Probably would make sense to only allow updates by providing the pinned
map path in case of tail call map (and e.g. not by id)?
> Today, flushing should not be a requirement for privileged user, as root
> has access to GET_FD_BY_ID commends, and therefore can manually flush
> the maps. Add a flag to opt out of automatic flushing.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> Hi!
>
> I wonder what the reactions to this (untested) patch would be? I don't
> really care strongly, but perhaps others experienced a similar issue?
My main worry with this is that this might lead to hard to debug memory
consumption issues, for example, once apps start to make wider use of
this flag (and even worse would load maps with BPF progs with circular
references), then they won't be cleaned up on detach and keep holding
potentially huge amount of kernel memory. It's true that similar issues
would happen when long running app leaks tail call map fd or the map
is a stale leftover node in bpf fs, but it feels there's still better
tooling out of user space for tracing back their correlation (e.g. fds,
file access to pinned entry) and given the fact that then there's a
guaranteed flush once urefs are fully dropped. Not having it and scanning
through GET_FD_BY_ID range for such cases to determine a stale tail call
map, its potential origin, and breaking up dependencies seems much harder
(or at least there's proper tooling missing for it today).
> ---
> include/uapi/linux/bpf.h | 3 +++
> kernel/bpf/arraymap.c | 13 ++++++++++---
> kernel/bpf/syscall.c | 9 ++++++++-
> net/core/sock_map.c | 3 ++-
> 4 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 91c43884f295..771f5cd957a7 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -291,6 +291,9 @@ enum bpf_attach_type {
> /* Zero-initialize hash function seed. This should only be used for testing. */
> #define BPF_F_ZERO_SEED (1U << 6)
>
> +/* Don't flush fd map when last reference is gone */
> +#define BPF_F_FD_MAP_NO_UREF_FLUSH (1U << 7)
> +
> /* flags for BPF_PROG_QUERY */
> #define BPF_F_QUERY_EFFECTIVE (1U << 0)
>
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 25632a75d630..ca8a4504c612 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -23,6 +23,8 @@
>
> #define ARRAY_CREATE_FLAG_MASK \
> (BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
> +#define FD_ARRAY_CREATE_FLAG_MASK \
> + (ARRAY_CREATE_FLAG_MASK | BPF_F_FD_MAP_NO_UREF_FLUSH)
>
> static void bpf_array_free_percpu(struct bpf_array *array)
> {
> @@ -54,7 +56,7 @@ static int bpf_array_alloc_percpu(struct bpf_array *array)
> }
>
> /* Called from syscall */
> -int array_map_alloc_check(union bpf_attr *attr)
> +static int __array_map_alloc_check(union bpf_attr *attr, u32 allowed_flags)
> {
> bool percpu = attr->map_type == BPF_MAP_TYPE_PERCPU_ARRAY;
> int numa_node = bpf_map_attr_numa_node(attr);
> @@ -62,7 +64,7 @@ int array_map_alloc_check(union bpf_attr *attr)
> /* check sanity of attributes */
> if (attr->max_entries == 0 || attr->key_size != 4 ||
> attr->value_size == 0 ||
> - attr->map_flags & ~ARRAY_CREATE_FLAG_MASK ||
> + attr->map_flags & ~allowed_flags ||
> (percpu && numa_node != NUMA_NO_NODE))
> return -EINVAL;
>
> @@ -75,6 +77,11 @@ int array_map_alloc_check(union bpf_attr *attr)
> return 0;
> }
>
> +int array_map_alloc_check(union bpf_attr *attr)
> +{
> + return __array_map_alloc_check(attr, ARRAY_CREATE_FLAG_MASK);
> +}
> +
> static struct bpf_map *array_map_alloc(union bpf_attr *attr)
> {
> bool percpu = attr->map_type == BPF_MAP_TYPE_PERCPU_ARRAY;
> @@ -431,7 +438,7 @@ static int fd_array_map_alloc_check(union bpf_attr *attr)
> /* only file descriptors can be stored in this type of map */
> if (attr->value_size != sizeof(u32))
> return -EINVAL;
> - return array_map_alloc_check(attr);
> + return __array_map_alloc_check(attr, FD_ARRAY_CREATE_FLAG_MASK);
> }
>
> static void fd_array_map_free(struct bpf_map *map)
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index b155cd17c1bd..42f10766f62e 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -123,6 +123,12 @@ static struct bpf_map *find_and_alloc_map(union bpf_attr *attr)
> err = ops->map_alloc_check(attr);
> if (err)
> return ERR_PTR(err);
> + /* Unprivileged users can't GET_FD_BY_ID so they always
> + * need the flush protection.
> + */
> + if ((attr->map_flags & BPF_F_FD_MAP_NO_UREF_FLUSH) &&
> + !capable(CAP_SYS_ADMIN))
> + return ERR_PTR(-EPERM);
> }
> if (attr->map_ifindex)
> ops = &bpf_map_offload_ops;
> @@ -293,7 +299,8 @@ static void bpf_map_free_deferred(struct work_struct *work)
> static void bpf_map_put_uref(struct bpf_map *map)
> {
> if (atomic_dec_and_test(&map->usercnt)) {
> - if (map->ops->map_release_uref)
> + if (map->ops->map_release_uref &&
> + !(map->map_flags & BPF_F_FD_MAP_NO_UREF_FLUSH))
> map->ops->map_release_uref(map);
> }
> }
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index be6092ac69f8..ea5a16d88bec 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -19,7 +19,8 @@ struct bpf_stab {
> };
>
> #define SOCK_CREATE_FLAG_MASK \
> - (BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
> + (BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY | \
> + BPF_F_FD_MAP_NO_UREF_FLUSH)
>
> static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
> {
>
^ permalink raw reply
* [PATCH] net: dev_is_mac_header_xmit() true for ARPHRD_RAWIP
From: Maciej Żenczykowski @ 2019-01-24 11:07 UTC (permalink / raw)
To: Maciej Żenczykowski, David S . Miller, Alexei Starovoitov,
Daniel Borkmann
Cc: linux-kernel, netdev
From: Maciej Żenczykowski <maze@google.com>
__bpf_redirect() and act_mirred checks this boolean
to determine whether to prefix an ethernet header.
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
include/linux/if_arp.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/if_arp.h b/include/linux/if_arp.h
index 6756fea18b69..e44746de95cd 100644
--- a/include/linux/if_arp.h
+++ b/include/linux/if_arp.h
@@ -54,6 +54,7 @@ static inline bool dev_is_mac_header_xmit(const struct net_device *dev)
case ARPHRD_IPGRE:
case ARPHRD_VOID:
case ARPHRD_NONE:
+ case ARPHRD_RAWIP:
return false;
default:
return true;
--
2.20.1.321.g9e740568ce-goog
^ permalink raw reply related
* Re: [PATCH 6/8] power: reset: at91-reset: add support for sam9x60 SoC
From: Nicolas.Ferre @ 2019-01-24 10:34 UTC (permalink / raw)
To: sre
Cc: alexandre.belloni, Ludovic.Desroches, linux-kernel,
linux-arm-kernel, linux-pm, netdev, davem, linux-usb, stern,
gregkh, robh+dt, devicetree
In-Reply-To: <20190123183443.ppxdfstmau2xtodi@earth.universe>
Hi Sebastian,
On 23/01/2019 at 19:34, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Jan 16, 2019 at 10:57:42AM +0100, Nicolas Ferre wrote:
>> Add support for additional reset causes and the proper compatibility
>> string for sam9x60 SoC. The restart function is the same as the samx7.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
>> ---
>> drivers/power/reset/at91-reset.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
>> index f44a9ffcc2ab..44ca983a49a1 100644
>> --- a/drivers/power/reset/at91-reset.c
>> +++ b/drivers/power/reset/at91-reset.c
>> @@ -44,6 +44,9 @@ enum reset_type {
>> RESET_TYPE_WATCHDOG = 2,
>> RESET_TYPE_SOFTWARE = 3,
>> RESET_TYPE_USER = 4,
>> + RESET_TYPE_CPU_FAIL = 6,
>> + RESET_TYPE_XTAL_FAIL = 7,
>> + RESET_TYPE_ULP2 = 8,
>
> what happened to 5? :)
That a good question ;-)
It's marked as "Reserved"... which opens up a whole new field of
speculation :-)
[..]
>> { .compatible = "atmel,samx7-rstc", .data = samx7_restart },
>> + { .compatible = "microchip,sam9x60-rstc", .data = samx7_restart },
>> { /* sentinel */ }
>> };
>> MODULE_DEVICE_TABLE(of, at91_reset_of_match);
>
> Patch looks fine to me. But I will wait a bit with merging, so that
> Alexandre or Ludovic have a chance to provide feedback.
What about merging this patch with the whole series through the at91
then arm-soc trees?
Best regards,
--
Nicolas Ferre
^ permalink raw reply
* Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
From: Simon Horman @ 2019-01-24 10:01 UTC (permalink / raw)
To: Fabrizio Castro
Cc: Geert Uytterhoeven, Wolfgang Grandegger, Marc Kleine-Budde,
Rob Herring, Mark Rutland, Michael Turquette, Stephen Boyd,
David S. Miller, Magnus Damm, Geert Uytterhoeven, Chris Paterson,
Biju Das, linux-can@vger.kernel.org, netdev@vger.kernel.org,
devicetree@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-clk@vger.kernel.org
In-Reply-To: <TYXPR01MB1775B75CED683E38B9171EA8C0990@TYXPR01MB1775.jpnprd01.prod.outlook.com>
On Wed, Jan 23, 2019 at 12:18:45PM +0000, Fabrizio Castro wrote:
> Hello Geert,
>
> Thank you for your feedback!
>
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: 23 January 2019 12:08
> > Subject: Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
> >
> > Hi Fabrizio,
> >
> > On Wed, Jan 23, 2019 at 1:01 PM Fabrizio Castro
> > <fabrizio.castro@bp.renesas.com> wrote:
> > > > From: Simon Horman <horms@verge.net.au>
> > > > Sent: 23 January 2019 11:38
> > > > Subject: Re: [PATCH 2/3] arm64: dts: renesas: r8a774a1: Add clkp2 clock to CAN nodes
> > > >
> > > > On Wed, Jan 23, 2019 at 10:51:23AM +0100, Simon Horman wrote:
> > > > > On Fri, Jan 18, 2019 at 01:13:53PM +0100, Simon Horman wrote:
> > > > > > On Thu, Jan 17, 2019 at 02:54:15PM +0000, Fabrizio Castro wrote:
> > > > > > > According to the latest information, clkp2 is available on RZ/G2.
> > > > > > > Modify CAN0 and CAN1 nodes accordingly.
> > > > > > >
> > > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > > > > > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>
> >
> > > > > Thanks again, applied for v5.1.
> > > >
> > > > Sorry, I was a little hasty there.
> > > >
> > > > This patch depends on the presence of R8A774A1_CLK_CANFD which
> > > > (rightly) is added in a different patch in this series which is to
> > > > go upstream via a different tree.
> > > >
> > > > I have dropped this patch for now. I think there are two solution to this
> > > > problem.
> > > >
> > > > 1. Provide a version of this patch which uses a numeric index instead of
> > > > R8A774A1_CLK_CANFD. And then, once R8A774A1_CLK_CANFD is present in an
> > > > RC release provide a patch to switch to using R8A774A1_CLK_CANFD.
> > > >
> > > > 2. Defer this patch until R8A774A1_CLK_CANFD is present in an RC release.
> > >
> > > Yeah, my personal preference is solution 2.
> >
> > Note that solution 2 will probably defer the DT patch to v5.2.
>
> Yeah, my understanding is that even if we chose solution 1 we would still
> need to be waiting for v5.2 for the patch to switch to using
> R8A774A1_CLK_CANFD to appear in a rc, therefore I think solution 2 is
> fine.
Your understanding is the same as mine.
I do seem some slight value in option 1 in terms of getting the change,
less the minor issue of using an index rather than a symbol, upstream
earlier. But I do not feel strongly about this.
I am marking this patch as deferred. Please repost or otherwise ping me
when you would like to revisit this topic.
^ permalink raw reply
* Re: [PATCH 6/8] power: reset: at91-reset: add support for sam9x60 SoC
From: Sebastian Reichel @ 2019-01-23 18:34 UTC (permalink / raw)
To: Nicolas Ferre
Cc: Alexandre Belloni, Ludovic Desroches, linux-kernel,
linux-arm-kernel, linux-pm, netdev, David S . Miller, linux-usb,
Alan Stern, Greg Kroah-Hartman, Rob Herring, devicetree
In-Reply-To: <7e68a0298b8802edaead1f9c011f9c2e191ecf6a.1547629763.git.nicolas.ferre@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]
Hi,
On Wed, Jan 16, 2019 at 10:57:42AM +0100, Nicolas Ferre wrote:
> Add support for additional reset causes and the proper compatibility
> string for sam9x60 SoC. The restart function is the same as the samx7.
>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> ---
> drivers/power/reset/at91-reset.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c
> index f44a9ffcc2ab..44ca983a49a1 100644
> --- a/drivers/power/reset/at91-reset.c
> +++ b/drivers/power/reset/at91-reset.c
> @@ -44,6 +44,9 @@ enum reset_type {
> RESET_TYPE_WATCHDOG = 2,
> RESET_TYPE_SOFTWARE = 3,
> RESET_TYPE_USER = 4,
> + RESET_TYPE_CPU_FAIL = 6,
> + RESET_TYPE_XTAL_FAIL = 7,
> + RESET_TYPE_ULP2 = 8,
what happened to 5? :)
> };
>
> static void __iomem *at91_ramc_base[2], *at91_rstc_base;
> @@ -164,6 +167,15 @@ static void __init at91_reset_status(struct platform_device *pdev)
> case RESET_TYPE_USER:
> reason = "user reset";
> break;
> + case RESET_TYPE_CPU_FAIL:
> + reason = "CPU clock failure detection";
> + break;
> + case RESET_TYPE_XTAL_FAIL:
> + reason = "32.768 kHz crystal failure detection";
> + break;
> + case RESET_TYPE_ULP2:
> + reason = "ULP2 reset";
> + break;
> default:
> reason = "unknown reset";
> break;
> @@ -183,6 +195,7 @@ static const struct of_device_id at91_reset_of_match[] = {
> { .compatible = "atmel,at91sam9g45-rstc", .data = at91sam9g45_restart },
> { .compatible = "atmel,sama5d3-rstc", .data = sama5d3_restart },
> { .compatible = "atmel,samx7-rstc", .data = samx7_restart },
> + { .compatible = "microchip,sam9x60-rstc", .data = samx7_restart },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, at91_reset_of_match);
Patch looks fine to me. But I will wait a bit with merging, so that
Alexandre or Ludovic have a chance to provide feedback.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg API
From: Eran Ben Elisha @ 2019-01-24 9:57 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev@vger.kernel.org, Jiri Pirko, David S. Miller,
Saeed Mahameed, Moshe Shemesh
In-Reply-To: <20190124090806.GA2357@nanopsycho>
On 1/24/2019 11:08 AM, Jiri Pirko wrote:
> Thu, Jan 24, 2019 at 08:39:12AM CET, eranbe@mellanox.com wrote:
>>
>>
>> On 1/23/2019 4:39 PM, Jiri Pirko wrote:
>>> Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@mellanox.com wrote:
>>>> As part of the devlink health reporter diagnose ops callback, the mlx5e TX
>>>> reporter used devlink health buffers API. Which will soon be depracated.
>>>> Modify the reporter to use the new devlink msg API.
>>>>
>>>> The actual set of the new diagnose function will be done later in the
>>>> downstream patch, only when devlink will actually expose a new diagnose
>>>> operation that uses the new API.
>>>>
>>>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>>>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>>>> ---
>>>> .../mellanox/mlx5/core/en/reporter_tx.c | 123 ++++--------------
>>>> 1 file changed, 26 insertions(+), 97 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> index d9675afbb924..fc92850c214a 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> @@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter,
>>>> }
>>>>
>>>> static int
>>>> -mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
>>>> +mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx,
>>>> u32 sqn, u8 state, u8 stopped)
>>>
>>> What is "state" and "stopped"? Is "stopped" a bool? Is "state" an enum.
>>>
>> stopped is the reply from netif_xmit_stopped, and it is a bool.
>
> If it is bool, you should add it into message as NLA_FLAG, not NLA_U8
>
>
>> state is the HW state of the SQ.
>> it is defined in the ifc file:
>> enum {
>> MLX5_SQC_STATE_RST = 0x0,
>> MLX5_SQC_STATE_RDY = 0x1,
>> MLX5_SQC_STATE_ERR = 0x3,
>> };
>> I could have translated it into strings, but I though it would be fine
>> to leave it as is.
>
> It's fine as u8
>
>>
>>>
>>>> {
>>>> - int err, i;
>>>> - int nest = 0;
>>>> - char name[20];
>>>> -
>>>> - err = devlink_health_buffer_nest_start(buffer,
>>>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>>>> - if (err)
>>>> - goto buffer_error;
>>>> - nest++;
>>>> -
>>>> - err = devlink_health_buffer_nest_start(buffer,
>>>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>>> - if (err)
>>>> - goto buffer_error;
>>>> - nest++;
>>>> -
>>>> - sprintf(name, "SQ 0x%x", sqn);
>>>> - err = devlink_health_buffer_put_object_name(buffer, name);
>>>> - if (err)
>>>> - goto buffer_error;
>>>> -
>>>> - err = devlink_health_buffer_nest_start(buffer,
>>>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>>> - if (err)
>>>> - goto buffer_error;
>>>> - nest++;
>>>> -
>>>> - err = devlink_health_buffer_nest_start(buffer,
>>>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>>>> - if (err)
>>>> - goto buffer_error;
>>>> - nest++;
>>>> -
>>>> - err = devlink_health_buffer_nest_start(buffer,
>>>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>>> - if (err)
>>>> - goto buffer_error;
>>>> - nest++;
>>>> -
>>>> - err = devlink_health_buffer_put_object_name(buffer, "HW state");
>>>> - if (err)
>>>> - goto buffer_error;
>>>> + int err;
>>>>
>>>> - err = devlink_health_buffer_nest_start(buffer,
>>>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>>> + err = devlink_msg_object_start(msg_ctx, "SQ");
>>>> if (err)
>>>> - goto buffer_error;
>>>> - nest++;
>>>> + return err;
>>>>
>>>> - err = devlink_health_buffer_put_value_u8(buffer, state);
>>>> + err = devlink_msg_object_start(msg_ctx, NULL);
>>>> if (err)
>>>> - goto buffer_error;
>>>> -
>>>> - devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */
>>>> - nest--;
>>>> -
>>>> - devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */
>>>> - nest--;
>>>> + return err;
>>>>
>>>> - err = devlink_health_buffer_nest_start(buffer,
>>>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>>>> + err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn,
>>>> + sizeof(sqn), NLA_U32);
>>>> if (err)
>>>> - goto buffer_error;
>>>> - nest++;
>>>> + return err;
>>>>
>>>> - err = devlink_health_buffer_put_object_name(buffer, "stopped");
>>>> + err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state,
>>>> + sizeof(state), NLA_U8);
>>>> if (err)
>>>> - goto buffer_error;
>>>> + return err;
>>>>
>>>> - err = devlink_health_buffer_nest_start(buffer,
>>>> - DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>>>> + err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped,
>>>> + sizeof(stopped), NLA_U8);
>>>> if (err)
>>>> - goto buffer_error;
>>>> - nest++;
>>>> + return err;
>>>>
>>>> - err = devlink_health_buffer_put_value_u8(buffer, stopped);
>>>> + err = devlink_msg_object_end(msg_ctx, NULL);
>>>> if (err)
>>>> - goto buffer_error;
>>>> -
>>>> - for (i = 0; i < nest; i++)
>>>> - devlink_health_buffer_nest_end(buffer);
>>>> -
>>>> - return 0;
>>>> + return err;
>>>>
>>>> -buffer_error:
>>>> - for (i = 0; i < nest; i++)
>>>> - devlink_health_buffer_nest_cancel(buffer);
>>>> + err = devlink_msg_object_end(msg_ctx, "SQ");
>>>> return err;
>>>> }
>>>>
>>>> static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> - struct devlink_health_buffer **buffers_array,
>>>> - unsigned int buffer_size,
>>>> - unsigned int num_buffers)
>>>> + struct devlink_msg_ctx *msg_ctx)
>>>> {
>>>> struct mlx5e_priv *priv = devlink_health_reporter_priv(reporter);
>>>> - unsigned int buff = 0;
>>>> - int i = 0, err = 0;
>>>> -
>>>> - if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN)
>>>> - return -ENOMEM;
>>>> + int i, err = 0;
>>>>
>>>> mutex_lock(&priv->state_lock);
>>>>
>>>> @@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> return 0;
>>>> }
>>>>
>>>> - while (i < priv->channels.num * priv->channels.params.num_tc) {
>>>> + for (i = 0; i < priv->channels.num * priv->channels.params.num_tc;
>>>> + i++) {
>>>> struct mlx5e_txqsq *sq = priv->txq2sq[i];
>>>> u8 state;
>>>>
>>>> @@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> if (err)
>>>> break;
>>>>
>>>> - err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff],
>>>> - sq->sqn, state,
>>>> + err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn,
>>>> + state,
>>>> netif_xmit_stopped(sq->txq));
>>>
>>> This should be an array. On SQ entry : one member of array.
>>>
>>> So if you want to change it, you need to do this in 2 patches. One API,
>>> one the actual layout. :/
>>>
>>>
>>>
>>>> - if (err) {
>>>> - if (++buff == num_buffers)
>>>> - break;
>>>> - } else {
>>>> - i++;
>>>> - }
>>>> + if (err)
>>>> + break;
>>>> }
>>>>
>>>> mutex_unlock(&priv->state_lock);
>>>> @@ -330,11 +264,6 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>>>> .name = "TX",
>>>> .recover = mlx5e_tx_reporter_recover,
>>>> - .diagnose_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC *
>>>> - MLX5E_TX_REPORTER_PER_SQ_MAX_LEN,
>>>> - .diagnose = mlx5e_tx_reporter_diagnose,
>>>
>>> So you change the callback, remove it so the dissection is broken.
>>
>> This is needed in order to have this patch compiled.
>
> You have to figure it out without breakage. This is not the way to
> do this.
This would cause all devlink and mlx5e patches to be altogether when
moving to the new API, which I don't like at all.
(#2) I think we shall take your approach and do a revert and put it all
again on top.
>
>>
>>>
>>>
>>>
>>>> - .dump_size = 0,
>>>> - .dump = NULL,
>>>
>>>
>>> This has 0 relation to the patch. Should be separate.
>>
>> ack.
>>
>>>
>>>
>>>> };
>>>>
>>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>>> --
>>>> 2.17.1
>>>>
^ permalink raw reply
* Re: [PATCH net-next 3/7] devlink: move devlink health reporter to use devlink msg API
From: Eran Ben Elisha @ 2019-01-24 9:57 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev@vger.kernel.org, Jiri Pirko, David S. Miller,
Saeed Mahameed, Moshe Shemesh
In-Reply-To: <20190124090917.GB2357@nanopsycho>
On 1/24/2019 11:09 AM, Jiri Pirko wrote:
> Thu, Jan 24, 2019 at 08:45:05AM CET, eranbe@mellanox.com wrote:
>>
>>
>> On 1/23/2019 4:42 PM, Jiri Pirko wrote:
>>> Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>>>> Move devlink reporter diagnose and dump operations to use the new msg API.
>>>> Redefine the signature of diagnose and dump operations and move the mlx5e
>>>> reporter to use it with the new format.
>>>>
>>>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>>>> Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>>>> ---
>>>> .../mellanox/mlx5/core/en/reporter_tx.c | 1 +
>>>> include/net/devlink.h | 9 +-
>>>> net/core/devlink.c | 95 +++++--------------
>>>> 3 files changed, 28 insertions(+), 77 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> index fc92850c214a..7238cda670ba 100644
>>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>>>> @@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>>>> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
>>>> .name = "TX",
>>>> .recover = mlx5e_tx_reporter_recover,
>>>> + .diagnose = mlx5e_tx_reporter_diagnose,
>>>
>>> Unrelated to this patch.
>>
>> ack.
>>
>>>
>>>
>>>> };
>>>>
>>>> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>>>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>>>> index fe323e9b14e1..d66de8b80cc2 100644
>>>> --- a/include/net/devlink.h
>>>> +++ b/include/net/devlink.h
>>>> @@ -442,17 +442,12 @@ struct devlink_health_reporter;
>>>>
>>>> struct devlink_health_reporter_ops {
>>>> char *name;
>>>> - unsigned int dump_size;
>>>> - unsigned int diagnose_size;
>>>> int (*recover)(struct devlink_health_reporter *reporter,
>>>> void *priv_ctx);
>>>> int (*dump)(struct devlink_health_reporter *reporter,
>>>> - struct devlink_health_buffer **buffers_array,
>>>> - unsigned int buffer_size, unsigned int num_buffers,
>>>> - void *priv_ctx);
>>>> + struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
>>>> int (*diagnose)(struct devlink_health_reporter *reporter,
>>>> - struct devlink_health_buffer **buffers_array,
>>>> - unsigned int buffer_size, unsigned int num_buffers);
>>>> + struct devlink_msg_ctx *msg_ctx);
>>>> };
>>>>
>>>> struct devlink_ops {
>>>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>>>> index 57ca096849b3..347b638e6f32 100644
>>>> --- a/net/core/devlink.c
>>>> +++ b/net/core/devlink.c
>>>> @@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
>>>>
>>>> struct devlink_health_reporter {
>>>> struct list_head list;
>>>> - struct devlink_health_buffer **dump_buffers_array;
>>>> + struct devlink_msg_ctx *dump_msg_ctx;
>>>> struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>>>> - struct devlink_health_buffer **diagnose_buffers_array;
>>>> - struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
>>>
>>> Howcome you don't need the mutex anymore?
>>
>> Now, as data is allocated on the fly while diagnose_doit(), no need to
>> store the diagnose over the reporter anymore. So no need for any mutex
>> locking in order to prepare and send it.
>
> Why the data is allocated on the fly? Shouldn't it be a separate patch
> as it looks like a separate change?
>
This start to be really complicated (but possible).
(#1) I think we shall take your approach and do a revert and put it all
again on top.
>
>>
>>>
>>>
>>>> void *priv;
>>>> const struct devlink_health_reporter_ops *ops;
>>>> struct devlink *devlink;
>>>> @@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
>>>> goto unlock;
>>>> }
>>>>
>>>> - if (WARN_ON(ops->dump && !ops->dump_size) ||
>>>> - WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>>>> - WARN_ON(auto_recover && !ops->recover) ||
>>>> + if (WARN_ON(auto_recover && !ops->recover) ||
>>>> WARN_ON(graceful_period && !ops->recover)) {
>>>> reporter = ERR_PTR(-EINVAL);
>>>> goto unlock;
>>>> @@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
>>>> goto unlock;
>>>> }
>>>>
>>>> - if (ops->dump) {
>>>> - reporter->dump_buffers_array =
>>>> - devlink_health_buffers_create(ops->dump_size);
>>>> - if (!reporter->dump_buffers_array) {
>>>> - kfree(reporter);
>>>> - reporter = ERR_PTR(-ENOMEM);
>>>> - goto unlock;
>>>> - }
>>>> - }
>>>> -
>>>> - if (ops->diagnose) {
>>>> - reporter->diagnose_buffers_array =
>>>> - devlink_health_buffers_create(ops->diagnose_size);
>>>> - if (!reporter->diagnose_buffers_array) {
>>>> - devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>>> - DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>>>> - kfree(reporter);
>>>> - reporter = ERR_PTR(-ENOMEM);
>>>> - goto unlock;
>>>> - }
>>>> - }
>>>> -
>>>> list_add_tail(&reporter->list, &devlink->reporter_list);
>>>> mutex_init(&reporter->dump_lock);
>>>> - mutex_init(&reporter->diagnose_lock);
>>>>
>>>> reporter->priv = priv;
>>>> reporter->ops = ops;
>>>> @@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
>>>> {
>>>> mutex_lock(&reporter->devlink->lock);
>>>> list_del(&reporter->list);
>>>> - devlink_health_buffers_destroy(reporter->dump_buffers_array,
>>>> - DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>>> - devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>>>> - DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>>>> + if (reporter->dump_msg_ctx)
>>>> + devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>>> kfree(reporter);
>>>> mutex_unlock(&reporter->devlink->lock);
>>>> }
>>>> @@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
>>>> if (reporter->dump_avail)
>>>> return 0;
>>>>
>>>> - devlink_health_buffers_reset(reporter->dump_buffers_array,
>>>> - DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>>> - err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>>>> - DEVLINK_HEALTH_BUFFER_SIZE,
>>>> - DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>>>> - priv_ctx);
>>>> + reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>>>> + if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>>>> + err = PTR_ERR(reporter->dump_msg_ctx);
>>>> + reporter->dump_msg_ctx = NULL;
>>>> + return err;
>>>> + }
>>>> +
>>>> + err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>>>> + priv_ctx);
>>>> if (!err) {
>>>> reporter->dump_avail = true;
>>>> reporter->dump_ts = jiffies;
>>>> @@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>>> {
>>>> struct devlink *devlink = info->user_ptr[0];
>>>> struct devlink_health_reporter *reporter;
>>>> - u64 num_of_buffers;
>>>> + struct devlink_msg_ctx *msg_ctx;
>>>> int err;
>>>>
>>>> reporter = devlink_health_reporter_get_from_info(devlink, info);
>>>> @@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
>>>> if (!reporter->ops->diagnose)
>>>> return -EOPNOTSUPP;
>>>>
>>>> - num_of_buffers =
>>>> - DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>>>> + msg_ctx = devlink_msg_ctx_alloc();
>>>> + if (IS_ERR_OR_NULL(msg_ctx))
>>>> + return PTR_ERR(msg_ctx);
>>>>
>>>> - mutex_lock(&reporter->diagnose_lock);
>>>> - devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>>>> - num_of_buffers);
>>>> -
>>>> - err = reporter->ops->diagnose(reporter,
>>>> - reporter->diagnose_buffers_array,
>>>> - DEVLINK_HEALTH_BUFFER_SIZE,
>>>> - num_of_buffers);
>>>> + err = reporter->ops->diagnose(reporter, msg_ctx);
>>>
>>> So this is not needed to be in reporter now? Why it was needed before?
>>
>> see reply above.
>>
>>>
>>>
>>>
>>>> if (err)
>>>> goto out;
>>>>
>>>> - err = devlink_health_buffer_snd(info,
>>>> - DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>>> - 0, reporter->diagnose_buffers_array,
>>>> - num_of_buffers);
>>>> - if (err)
>>>> - goto out;
>>>> -
>>>> - mutex_unlock(&reporter->diagnose_lock);
>>>> - return 0;
>>>> + err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>>>> + 0, msg_ctx);
>>>>
>>>> out:
>>>> - mutex_unlock(&reporter->diagnose_lock);
>>>> + devlink_msg_ctx_free(msg_ctx);
>>>> return err;
>>>> }
>>>>
>>>> @@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
>>>> {
>>>> reporter->dump_avail = false;
>>>> reporter->dump_ts = 0;
>>>> - devlink_health_buffers_reset(reporter->dump_buffers_array,
>>>> - DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>>>> + devlink_msg_ctx_free(reporter->dump_msg_ctx);
>>>> + reporter->dump_msg_ctx = NULL;
>>>> }
>>>>
>>>> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>>> @@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>>> {
>>>> struct devlink *devlink = info->user_ptr[0];
>>>> struct devlink_health_reporter *reporter;
>>>> - u64 num_of_buffers;
>>>> int err;
>>>>
>>>> reporter = devlink_health_reporter_get_from_info(devlink, info);
>>>> @@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>>>> if (!reporter->ops->dump)
>>>> return -EOPNOTSUPP;
>>>>
>>>> - num_of_buffers =
>>>> - DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>>>> -
>>>> mutex_lock(&reporter->dump_lock);
>>>> err = devlink_health_do_dump(reporter, NULL);
>>>> if (err)
>>>> goto out;
>>>>
>>>> - err = devlink_health_buffer_snd(info,
>>>> - DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>>> - 0, reporter->dump_buffers_array,
>>>> - num_of_buffers);
>>>> + err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>>>> + 0, reporter->dump_msg_ctx);
>>>>
>>>> out:
>>>> mutex_unlock(&reporter->dump_lock);
>>>> --
>>>> 2.17.1
>>>>
^ permalink raw reply
* Re: [PATCH bpf-next v2 13/16] ppc: bpf: implement jitting of JMP32
From: Jiong Wang @ 2019-01-24 9:53 UTC (permalink / raw)
To: Sandipan Das; +Cc: Jiong Wang, ast, daniel, netdev, oss-drivers, Naveen N . Rao
In-Reply-To: <b02b3ee0-21f0-3e9f-7d38-ca8e660fa6cf@linux.ibm.com>
Sandipan Das writes:
> Hi Jiong,
>
> On 21/01/19 6:45 PM, Jiong Wang wrote:
>> This patch implements code-gen for new JMP32 instructions on ppc.
>>
>> For JMP32 | JSET, instruction encoding for PPC_RLWINM_DOT is added to check
>> the result of ANDing low 32-bit of operands.
>>
>> Cc: Naveen N. Rao <naveen.n.rao@linux.ibm.com>
>> Cc: Sandipan Das <sandipan@linux.ibm.com>
>> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
>> ---
>> arch/powerpc/include/asm/ppc-opcode.h | 1 +
>> arch/powerpc/net/bpf_jit.h | 4 ++
>> arch/powerpc/net/bpf_jit_comp64.c | 98 ++++++++++++++++++++++++++++++-----
>> 3 files changed, 89 insertions(+), 14 deletions(-)
>> [...]
>
> I ran the verifier selftests on a ppc64 test system and found that the jmp32 tests
> were failing because the instructions were not being decoded by the JIT compiler.
> The codegen logic looks good to me and the tests passed after I fixed the decoding
> logic. Here are the changes that I had to make:
Ah, I missed the outer layer decoding logic.
Thanks for the review, test and fix. Will integrate the following in v3.
Regards,
Jiong
>
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index 2e40c2b251ba..15bba765fa79 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -768,36 +768,58 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
> case BPF_JMP | BPF_JGT | BPF_X:
> case BPF_JMP | BPF_JSGT | BPF_K:
> case BPF_JMP | BPF_JSGT | BPF_X:
> + case BPF_JMP32 | BPF_JGT | BPF_K:
> + case BPF_JMP32 | BPF_JGT | BPF_X:
> + case BPF_JMP32 | BPF_JSGT | BPF_K:
> + case BPF_JMP32 | BPF_JSGT | BPF_X:
> true_cond = COND_GT;
> goto cond_branch;
> case BPF_JMP | BPF_JLT | BPF_K:
> case BPF_JMP | BPF_JLT | BPF_X:
> case BPF_JMP | BPF_JSLT | BPF_K:
> case BPF_JMP | BPF_JSLT | BPF_X:
> + case BPF_JMP32 | BPF_JLT | BPF_K:
> + case BPF_JMP32 | BPF_JLT | BPF_X:
> + case BPF_JMP32 | BPF_JSLT | BPF_K:
> + case BPF_JMP32 | BPF_JSLT | BPF_X:
> true_cond = COND_LT;
> goto cond_branch;
> case BPF_JMP | BPF_JGE | BPF_K:
> case BPF_JMP | BPF_JGE | BPF_X:
> case BPF_JMP | BPF_JSGE | BPF_K:
> case BPF_JMP | BPF_JSGE | BPF_X:
> + case BPF_JMP32 | BPF_JGE | BPF_K:
> + case BPF_JMP32 | BPF_JGE | BPF_X:
> + case BPF_JMP32 | BPF_JSGE | BPF_K:
> + case BPF_JMP32 | BPF_JSGE | BPF_X:
> true_cond = COND_GE;
> goto cond_branch;
> case BPF_JMP | BPF_JLE | BPF_K:
> case BPF_JMP | BPF_JLE | BPF_X:
> case BPF_JMP | BPF_JSLE | BPF_K:
> case BPF_JMP | BPF_JSLE | BPF_X:
> + case BPF_JMP32 | BPF_JLE | BPF_K:
> + case BPF_JMP32 | BPF_JLE | BPF_X:
> + case BPF_JMP32 | BPF_JSLE | BPF_K:
> + case BPF_JMP32 | BPF_JSLE | BPF_X:
> true_cond = COND_LE;
> goto cond_branch;
> case BPF_JMP | BPF_JEQ | BPF_K:
> case BPF_JMP | BPF_JEQ | BPF_X:
> + case BPF_JMP32 | BPF_JEQ | BPF_K:
> + case BPF_JMP32 | BPF_JEQ | BPF_X:
> true_cond = COND_EQ;
> goto cond_branch;
> case BPF_JMP | BPF_JNE | BPF_K:
> case BPF_JMP | BPF_JNE | BPF_X:
> + case BPF_JMP32 | BPF_JNE | BPF_K:
> + case BPF_JMP32 | BPF_JNE | BPF_X:
> true_cond = COND_NE;
> goto cond_branch;
> case BPF_JMP | BPF_JSET | BPF_K:
> case BPF_JMP | BPF_JSET | BPF_X:
> + case BPF_JMP32 | BPF_JSET | BPF_K:
> + case BPF_JMP32 | BPF_JSET | BPF_X:
> true_cond = COND_NE;
> /* Fall through */
^ permalink raw reply
* RE: [PATCH 3/7] net/ethernet: Add parse_protocol header_ops support
From: Maxim Mikityanskiy @ 2019-01-24 9:47 UTC (permalink / raw)
To: Willem de Bruijn
Cc: David S. Miller, Saeed Mahameed, Willem de Bruijn, Jason Wang,
Eric Dumazet, netdev@vger.kernel.org, Eran Ben Elisha,
Tariq Toukan
In-Reply-To: <CAF=yD-+B6atsSbYV6Dk3qTdxtxmpqZXEen_eGFDqkHYJOe=zCQ@mail.gmail.com>
> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Sent: 23 January, 2019 16:15
> To: Maxim Mikityanskiy <maximmi@mellanox.com>
> Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> <tariqt@mellanox.com>
> Subject: Re: [PATCH 3/7] net/ethernet: Add parse_protocol header_ops support
>
> On Mon, Jan 14, 2019 at 8:21 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> wrote:
> >
> > The previous commit introduced parse_protocol callback which should
> > extract the protocol number from the L2 header. Make all Ethernet
> > devices support it.
> >
> > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> > ---
> > include/linux/etherdevice.h | 1 +
> > net/ethernet/eth.c | 13 +++++++++++++
> > 2 files changed, 14 insertions(+)
> >
> > diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> > index 2c0af7b00715..e2f3b21cd72a 100644
> > --- a/include/linux/etherdevice.h
> > +++ b/include/linux/etherdevice.h
> > @@ -44,6 +44,7 @@ int eth_header_cache(const struct neighbour *neigh,
> struct hh_cache *hh,
> > __be16 type);
> > void eth_header_cache_update(struct hh_cache *hh, const struct net_device
> *dev,
> > const unsigned char *haddr);
> > +__be16 eth_header_parse_protocol(const struct sk_buff *skb);
>
> Does not need to be exposed in the header file or exported.
Are you sure? All the other Ethernet header_ops callbacks are exported
and declared in the header. I'm not sure about the reason why it is done
in such a way, but my guess is that it will be useful if some driver
decides to replace one callback in header_ops but to use the default
ones for the rest of callbacks. If the default callbacks were not
accessible externally, it wouldn't be possible. So, I'm following the
existing pattern here, and there are reasons for that. What do you think
about it?
^ permalink raw reply
* Re: [PATCH v11 perf, bpf-next 7/9] perf tools: synthesize PERF_RECORD_* for loaded BPF programs
From: Arnaldo Carvalho de Melo @ 2019-01-24 9:49 UTC (permalink / raw)
To: Song Liu
Cc: Jiri Olsa, Jiri Olsa, Namhyung Kim, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, peterz@infradead.org, ast@kernel.org,
daniel@iogearbox.net, Kernel Team
In-Reply-To: <CF447261-1B9C-423B-9508-60B32EAC4A5C@fb.com>
Em Tue, Jan 22, 2019 at 07:24:43PM +0000, Song Liu escreveu:
> +#include "machine.h"
> #include "bpf-event.h"
> #include "debug.h"
> #include "symbol.h"
> @@ -187,7 +188,7 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_tool *tool,
> *bpf_event = (struct bpf_event){
> .header = {
> .type = PERF_RECORD_BPF_EVENT,
> - .size = sizeof(struct bpf_event),
> + .size = sizeof(struct bpf_event) + machine->id_hdr_size,
> },
> .type = PERF_BPF_EVENT_PROG_LOAD,
> .flags = 0,
>
> Would you send the official patch? Or would you prefer me sending it?
Nevermind, I'll fix this, its just that I'm now travelling.
- Arnaldo
^ permalink raw reply
* RE: [PATCH 1/7] net: Don't set transport offset to invalid value
From: Maxim Mikityanskiy @ 2019-01-24 9:47 UTC (permalink / raw)
To: Willem de Bruijn
Cc: David S. Miller, Saeed Mahameed, Willem de Bruijn, Jason Wang,
Eric Dumazet, netdev@vger.kernel.org, Eran Ben Elisha,
Tariq Toukan
In-Reply-To: <CAF=yD-+ZwVv7RgNoRzCbP0S6rLPqtjwTsXUs07p_WZqvdx=VnQ@mail.gmail.com>
> > but the general idea is that we
> > report this status, so if you say that my version is also good for you,
> > I'll leave it as is. It was just a rationale for my decision.
>
> It's fine. But please avoid the code churn in xenvif_tx_submit
> with to extra indentation. This is equivalent:
>
> - if (skb_is_gso(skb)) {
> + if (skb_is_gso(skb) && th_set) {
>
> More fundamentally, the code has the assumption that th_set
> always holds if skb_is_gso(skb). Why add the check? This is
> another example that the return value is not really needed.
What about
if (skb_is_gso(skb)) {
BUG_ON(!skb_transport_header_was_set(skb));
?
I think it's cleaner than skipping the action here if dissect failed and
propagating a potential bug further.
> > > If this is the only reason for the boolean return value, using
> > > skb_transport_header_was_set() is more standard (I immediately know
> > > what's happening when I read it), slightly less code change and avoids
> > > introducing a situation where the majority of callers ignore a return
> > > value. I think it's preferable. But these merits are certainly
> > > debatable, so either is fine.
> >
> > From my side, I wanted to avoid calling skb_transport_header_was_set
> > twice, so I made skb_try_probe_transport_header return whether it
> > succeeded or not. I think "try" in the function name indicates this idea
> > pretty clearly. This result status is pretty useful, it just happened
> > that it's not needed in many places,
>
> Which is an indication that it's perhaps not needed.
Well, from the point of view of the function, it looks reasonable to
notify the caller whether the call was successful or not... You know,
many functions return error codes.
However, this case is rather special, because it turned out that we
don't actually need that error status immediately, but it can be
requested much later, and we already have skb_transport_header_was_set
for that.
So, considering these points, the return value can be removed, as all
use cases are "probe now, check later", not "probe now and handle errors
immediately".
^ permalink raw reply
* [PATCH net-next 1/1] openvswitch: Declare ovs key structures using macros
From: Eli Britstein @ 2019-01-24 9:46 UTC (permalink / raw)
To: Pravin B Shelar
Cc: netdev, Ben Pfaff, dev, Roi Dayan, Simon Horman, Eli Britstein
Declare ovs key structures using macros to enable retrieving fields
information, with no functional change.
Signed-off-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
include/uapi/linux/openvswitch.h | 102 ++++++++++++++++++++++++++-------------
1 file changed, 69 insertions(+), 33 deletions(-)
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index dbe0cbe4f1b7..dc8246f871fd 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -387,73 +387,109 @@ enum ovs_frag_type {
#define OVS_FRAG_TYPE_MAX (__OVS_FRAG_TYPE_MAX - 1)
+#define OVS_KEY_FIELD_ARR(type, name, elements) type name[elements];
+#define OVS_KEY_FIELD(type, name) type name;
+
+#define OVS_KEY_ETHERNET_FIELDS \
+ OVS_KEY_FIELD_ARR(__u8, eth_src, ETH_ALEN) \
+ OVS_KEY_FIELD_ARR(__u8, eth_dst, ETH_ALEN)
+
struct ovs_key_ethernet {
- __u8 eth_src[ETH_ALEN];
- __u8 eth_dst[ETH_ALEN];
+ OVS_KEY_ETHERNET_FIELDS
};
struct ovs_key_mpls {
__be32 mpls_lse;
};
+#define OVS_KEY_IPV4_FIELDS \
+ OVS_KEY_FIELD(__be32, ipv4_src) \
+ OVS_KEY_FIELD(__be32, ipv4_dst) \
+ OVS_KEY_FIELD(__u8, ipv4_proto) \
+ OVS_KEY_FIELD(__u8, ipv4_tos) \
+ OVS_KEY_FIELD(__u8, ipv4_ttl) \
+ OVS_KEY_FIELD(__u8, ipv4_frag /* One of OVS_FRAG_TYPE_*. */)
+
struct ovs_key_ipv4 {
- __be32 ipv4_src;
- __be32 ipv4_dst;
- __u8 ipv4_proto;
- __u8 ipv4_tos;
- __u8 ipv4_ttl;
- __u8 ipv4_frag; /* One of OVS_FRAG_TYPE_*. */
+ OVS_KEY_IPV4_FIELDS
};
+#define OVS_KEY_IPV6_FIELDS \
+ OVS_KEY_FIELD_ARR(__be32, ipv6_src, 4) \
+ OVS_KEY_FIELD_ARR(__be32, ipv6_dst, 4) \
+ OVS_KEY_FIELD(__be32, ipv6_label /* 20-bits in least-significant bits. */) \
+ OVS_KEY_FIELD(__u8, ipv6_proto) \
+ OVS_KEY_FIELD(__u8, ipv6_tclass) \
+ OVS_KEY_FIELD(__u8, ipv6_hlimit) \
+ OVS_KEY_FIELD(__u8, ipv6_frag /* One of OVS_FRAG_TYPE_*. */)
+
struct ovs_key_ipv6 {
- __be32 ipv6_src[4];
- __be32 ipv6_dst[4];
- __be32 ipv6_label; /* 20-bits in least-significant bits. */
- __u8 ipv6_proto;
- __u8 ipv6_tclass;
- __u8 ipv6_hlimit;
- __u8 ipv6_frag; /* One of OVS_FRAG_TYPE_*. */
+ OVS_KEY_IPV6_FIELDS
};
+#define OVS_KEY_TCP_FIELDS \
+ OVS_KEY_FIELD(__be16, tcp_src) \
+ OVS_KEY_FIELD(__be16, tcp_dst)
+
struct ovs_key_tcp {
- __be16 tcp_src;
- __be16 tcp_dst;
+ OVS_KEY_TCP_FIELDS
};
+#define OVS_KEY_UDP_FIELDS \
+ OVS_KEY_FIELD(__be16, udp_src) \
+ OVS_KEY_FIELD(__be16, udp_dst)
+
struct ovs_key_udp {
- __be16 udp_src;
- __be16 udp_dst;
+ OVS_KEY_UDP_FIELDS
};
+#define OVS_KEY_SCTP_FIELDS \
+ OVS_KEY_FIELD(__be16, sctp_src) \
+ OVS_KEY_FIELD(__be16, sctp_dst)
+
struct ovs_key_sctp {
- __be16 sctp_src;
- __be16 sctp_dst;
+ OVS_KEY_SCTP_FIELDS
};
+#define OVS_KEY_ICMP_FIELDS \
+ OVS_KEY_FIELD(__u8, icmp_type) \
+ OVS_KEY_FIELD(__u8, icmp_code)
+
struct ovs_key_icmp {
- __u8 icmp_type;
- __u8 icmp_code;
+ OVS_KEY_ICMP_FIELDS
};
+#define OVS_KEY_ICMPV6_FIELDS \
+ OVS_KEY_FIELD(__u8, icmpv6_type) \
+ OVS_KEY_FIELD(__u8, icmpv6_code)
+
struct ovs_key_icmpv6 {
- __u8 icmpv6_type;
- __u8 icmpv6_code;
+ OVS_KEY_ICMPV6_FIELDS
};
+#define OVS_KEY_ARP_FIELDS \
+ OVS_KEY_FIELD(__be32, arp_sip) \
+ OVS_KEY_FIELD(__be32, arp_tip) \
+ OVS_KEY_FIELD(__be16, arp_op) \
+ OVS_KEY_FIELD_ARR(__u8, arp_sha, ETH_ALEN) \
+ OVS_KEY_FIELD_ARR(__u8, arp_tha, ETH_ALEN)
+
struct ovs_key_arp {
- __be32 arp_sip;
- __be32 arp_tip;
- __be16 arp_op;
- __u8 arp_sha[ETH_ALEN];
- __u8 arp_tha[ETH_ALEN];
+ OVS_KEY_ARP_FIELDS
};
+#define OVS_KEY_ND_FIELDS \
+ OVS_KEY_FIELD_ARR(__be32, nd_target, 4) \
+ OVS_KEY_FIELD_ARR(__u8, nd_sll, ETH_ALEN) \
+ OVS_KEY_FIELD_ARR(__u8, nd_tll, ETH_ALEN)
+
struct ovs_key_nd {
- __be32 nd_target[4];
- __u8 nd_sll[ETH_ALEN];
- __u8 nd_tll[ETH_ALEN];
+ OVS_KEY_ND_FIELDS
};
+#undef OVS_KEY_FIELD_ARR
+#undef OVS_KEY_FIELD
+
#define OVS_CT_LABELS_LEN_32 4
#define OVS_CT_LABELS_LEN (OVS_CT_LABELS_LEN_32 * sizeof(__u32))
struct ovs_key_ct_labels {
--
2.14.4
^ permalink raw reply related
* Re: [PATCH net-next v7 0/8] devlink: Add configuration parameters support for devlink_port
From: Vasundhara Volam @ 2019-01-24 9:46 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Michal Kubecek, Netdev, David Miller, michael.chan@broadcom.com,
Jiri Pirko
In-Reply-To: <20190122141842.26908b27@cakuba.netronome.com>
On Wed, Jan 23, 2019 at 3:48 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 18 Jan 2019 15:33:19 +0100, Michal Kubecek wrote:
> > On Fri, Jan 18, 2019 at 12:39:37PM +0530, Vasundhara Volam wrote:
> > > There is difference of opinion on adding WOL parameter to devlink, between
> > > Jakub Kicinski and Michael Chan.
> > >
> > > Quote from Jakud Kicinski:
> > > ********
> > > As explained previously I think it's a very bad idea to add existing
> > > configuration options to devlink, just because devlink has the ability
> > > to persist the setting in NVM. Especially that for WoL you have to get
> > > the link up so you potentially have all link config stuff as well. And
> > > that n-tuple filters are one of the WoL options, meaning we'd need the
> > > ability to persist n-tuple filters via devlink.
> > >
> > > The effort would be far better spent helping with migrating ethtool to
> > > netlink, and allowing persisting there.
> > >
> > > I have not heard any reason why devlink is a better fit. I can imagine
> > > you're just doing it here because it's less effort for you since
> > > ethtool is not yet migrated.
> > > ********
> > >
> > > Quote from Michael Chan:
> > > ********
> > > The devlink's WoL parameter is a persistent WoL parameter stored in the
> > > NIC's NVRAM. It is different from ethtool's WoL parameter in a number of
> > > ways. ethtool WoL is not persistent over AC power cycle and is considered
> > > OS-present WoL. As such, ethtool WoL can use a more sophisticated pattern
> > > including n-tuple with IP address in addition to the more basic types
> > > (e.g. magic packet). Whereas OS-absent power up WoL should only include
> > > magic packet and other simple types.
> >
> > If I understand correctly, it's that way now. I'm not sure there is a
> > technical reason preventing more complex WoL types in the OS-absent case
> > in the future. Also, even with traditional ethtool WoL setting, most
> > NICs only support some of the types (I'm not sure if there is a NIC
> > which would support all of them.)
> >
> > > The devlink WoL setting does not have to match the ethtool WoL
> > > setting.
> >
> > IMHO this is not really a problem. We can either use an additional flag
> > telling kernel/driver if we are setting runtime or persistent WoL mask
> > or we can pass (up to) two bitmaps.
>
> I think reusing new netlink ethtool with a special flag would be a nice,
> complete solution. We could also address link settings this way (which
> are a pre-requisite for WoL).
>
> I have no strong preference on the mechanism, but for ease of setting
> as well as dumping would it be workable to use a nesting, like this:
>
> Run time settings:
> [ETHTOOL_SETTINGS_BLA]
> [ETHTOOL_BLA_VAL_1]
> [ETHTOOL_BLA_VAL_2]
> ...
>
> Persistent:
> [ETHTOOL_PERSISTENT]
> [ETHTOOL_SETTINGS_BLA]
> [ETHTOOL_BLA_VAL_1]
> [ETHTOOL_BLA_VAL_2]
>
> IOW encapsulate settings into a "persistent" attribute?
Not sure if current devlink framework allows to encapsulate additional
settings now.
But we can think of extending it to support this when there is a requirement.
>
> How does that look to you, Michal?
>
> > > The card will autoneg up to the speed supported by Vaux so no special
> > > devlink link setting is needed.
> > > ********
> >
> > Like Jakub, I'm not convinced there is a strong technical reason to have
> > each of the WoL settings handled through a different interface. I don't
> > say, though, that ethtool is necessarily the right one. If there is
> > a consensus that it better fits into devlink, I can imagine that both
> > could be accessible through devlink (for start, in drivers which choose
> > so, e.g. because they want to implement the persistent setting).
> >
> > Michal Kubecek
^ 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