* Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library
From: Jason A. Donenfeld @ 2018-09-21 4:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andrew Lunn, Arnd Bergmann, Ard Biesheuvel, Eric Biggers, LKML,
Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <8FA361E3-FBCB-469E-88DC-F4085BD91175@amacapital.net>
Hi Andy,
On Fri, Sep 21, 2018 at 5:23 AM Andy Lutomirski <luto@amacapital.net> wrote:
> At the risk on suggesting something awful: on x86_64, since we turn preemption off for simd, it wouldn’t be *completely* insane to do the crypto on the irq stack. It would look like:
>
> kernel_fpu_call(func, arg);
>
> And this helper would disable preemption, enable FPU, switch to the irq stack, call func(arg), disable FPU, enable preemption, and return. And we can have large IRQ stacks.
>
> I refuse to touch this with a ten-foot pole until the lazy FPU restore patches land.
Haha. That's fun, and maybe we'll do that at some point, but I have
some other reasons too for being on a workqueue now.
>
> All that being said, why are these frames so large? It sounds like something may be spilling that ought not to.
They're not. Well, they're not anymore. I had a silly thing before
like "u8 buffer[1 << 12]" in some debugging code, which is what
prompted the ccflag-y addition. I cleaned up the mistakes like that
and frames are now reasonable everywhere. Non-issue.
Jason
^ permalink raw reply
* Re: array bounds warning in xfrm_output_resume
From: David Ahern @ 2018-09-20 22:53 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev@vger.kernel.org
In-Reply-To: <20180920140617.uybaha3n7iz6ckvn@breakpoint.cc>
On 9/20/18 7:06 AM, Florian Westphal wrote:
> David Ahern <dsahern@gmail.com> wrote:
>>> $ make O=kbuild/perf -j 24 -s
>>> In file included from /home/dsa/kernel-3.git/include/linux/kernel.h:10:0,
>>> from /home/dsa/kernel-3.git/include/linux/list.h:9,
>>> from /home/dsa/kernel-3.git/include/linux/module.h:9,
>>> from /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c:13:
>>> /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c: In function
>>> ‘xfrm_output_resume’:
>>> /home/dsa/kernel-3.git/include/linux/compiler.h:252:20: warning: array
>>> subscript is above array bounds [-Warray-bounds]
>>> __read_once_size(&(x), __u.__c, sizeof(x)); \
>
> Does this thing avoid the warning?
nope, still see it.
^ permalink raw reply
* Re: [PATCH net-next v5 02/20] zinc: introduce minimal cryptography library
From: Andy Lutomirski @ 2018-09-21 4:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jason A. Donenfeld, Andrew Lunn, Arnd Bergmann, Eric Biggers,
LKML, Netdev, Linux Crypto Mailing List, David Miller,
Greg Kroah-Hartman, Samuel Neves, Andrew Lutomirski,
Jean-Philippe Aumasson
In-Reply-To: <CAKv+Gu-sciHeWVij8hWGF78HV_CLddMv7Xdf1GRSY+6B2yt48A@mail.gmail.com>
> On Sep 20, 2018, at 9:30 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> On 20 September 2018 at 21:15, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Hi Andy,
>>
>>> On Fri, Sep 21, 2018 at 5:23 AM Andy Lutomirski <luto@amacapital.net> wrote:
>>> At the risk on suggesting something awful: on x86_64, since we turn preemption off for simd, it wouldn’t be *completely* insane to do the crypto on the irq stack. It would look like:
>>>
>>> kernel_fpu_call(func, arg);
>>>
>>> And this helper would disable preemption, enable FPU, switch to the irq stack, call func(arg), disable FPU, enable preemption, and return. And we can have large IRQ stacks.
>>>
>>> I refuse to touch this with a ten-foot pole until the lazy FPU restore patches land.
>>
>> Haha. That's fun, and maybe we'll do that at some point, but I have
>> some other reasons too for being on a workqueue now.
>>
>
> Kernel mode crypto is callable from any context, and SIMD can be used
> in softirq context on arm64 (and on x86, even from hardirq context
> IIRC if the interrupt is taken from userland), in which case we'd
> already be on the irq stack.
The x86_64 irq stack handles nesting already.
^ permalink raw reply
* Re: [PATCH] net: netronome: remove redundant continue
From: zhong jiang @ 2018-09-21 5:21 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, dirk.vandermerwe, simon.horman, netdev, linux-kernel
In-Reply-To: <20180920093857.6227710f@cakuba.netronome.com>
On 2018/9/21 0:38, Jakub Kicinski wrote:
> On Thu, 20 Sep 2018 16:02:21 +0800, zhong jiang wrote:
>> The continue will not truely skip any code. hence it is safe to
>> remove it.
>>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> I think this came up during review at some point. I still prefer to
> keep the continue. The body of the loop performs initialization of
> objects, if an object is removed we shouldn't carry on with the body.
> It's easy to miss that the object got freed otherwise, there is no
> error being set and no warning printed...
IMO, we should bring it back when the case truely occur you have said. At present.
We should not take too much into account. Maybe it will not occur.
Thanks,
zhong jiang
>> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> index 0b1ac9c..50d7b58 100644
>> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
>> @@ -230,10 +230,8 @@ static void nfp_net_pf_free_vnics(struct nfp_pf *pf)
>> ctrl_bar += NFP_PF_CSR_SLICE_SIZE;
>>
>> /* Kill the vNIC if app init marked it as invalid */
>> - if (nn->port && nn->port->type == NFP_PORT_INVALID) {
>> + if (nn->port && nn->port->type == NFP_PORT_INVALID)
>> nfp_net_pf_free_vnic(pf, nn);
>> - continue;
>> - }
>> }
>>
>> if (list_empty(&pf->vnics))
>
> .
>
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Song Liu @ 2018-09-20 23:43 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jeff Kirsher, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <ee9b9a55-c967-cec3-7df1-79f84b06154c@gmail.com>
> On Sep 20, 2018, at 4:22 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 09/20/2018 03:42 PM, Song Liu wrote:
>>
>>
>>> On Sep 20, 2018, at 2:01 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
>>>
>>> On Thu, 2018-09-20 at 13:35 -0700, Eric Dumazet wrote:
>>>> On 09/20/2018 12:01 PM, Song Liu wrote:
>>>>> The NIC driver should only enable interrupts when napi_complete_done()
>>>>> returns true. This patch adds the check for ixgbe.
>>>>>
>>>>> Cc: stable@vger.kernel.org # 4.10+
>>>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>>>> Suggested-by: Eric Dumazet <edumazet@google.com>
>>>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>>>> ---
>>>>
>>>>
>>>> Well, unfortunately we do not know why this is needed,
>>>> this is why I have not yet sent this patch formally.
>>>>
>>>> netpoll has correct synchronization :
>>>>
>>>> poll_napi() places into napi->poll_owner current cpu number before
>>>> calling poll_one_napi()
>>>>
>>>> netpoll_poll_lock() does also use napi->poll_owner
>>>>
>>>> When netpoll calls ixgbe poll() method, it passed a budget of 0,
>>>> meaning napi_complete_done() is not called.
>>>>
>>>> As long as we can not explain the problem properly in the changelog,
>>>> we should investigate, otherwise we will probably see coming dozens of
>>>> patches
>>>> trying to fix a 'potential hazard'.
>>>
>>> Agreed, which is why I have our validation and developers looking into it,
>>> while we test the current patch from Song.
>>
>> I figured out what is the issue here. And I have a proposal to fix it. I
>> have verified that this fixes the issue in our tests. But Alexei suggests
>> that it may not be the right way to fix.
>>
>> Here is what happened:
>>
>> netpoll tries to send skb with netpoll_start_xmit(). If that fails, it
>> calls netpoll_poll_dev(), which calls ndo_poll_controller(). Then, in
>> the driver, ndo_poll_controller() calls napi_schedule() for ALL NAPIs
>> within the same NIC.
>>
>> This is problematic, because at the end napi_schedule() calls:
>>
>> ____napi_schedule(this_cpu_ptr(&softnet_data), n);
>>
>> which attached these NAPIs to softnet_data on THIS CPU. This is done
>> via napi->poll_list.
>>
>> Then suddenly ksoftirqd on this CPU owns multiple NAPIs. And it will
>> not give up the ownership until it calls napi_complete_done(). However,
>> for a very busy server, we usually use 16 CPUs to poll NAPI, so this
>> CPU can easily be overloaded. And as a result, each call of napi->poll()
>> will hit budget (of 64), and it will not call napi_complete_done(),
>> and the NAPI stays in the poll_list of this CPU.
>>
>> When this happens, the host usually cannot get out of this state until
>> we throttle/stop client traffic.
>>
>>
>> I am pretty confident this is what happened. Please let me know if
>> anything above doesn't make sense.
>>
>>
>> Here is my proposal to fix it: Instead of polling all NAPIs within one
>> NIC, I would have netpoll to only poll the NAPI that will free space
>> for netpoll_start_xmit(). I attached my two RFC patches to the end of
>> this email.
>>
>> I chatted with Alexei about this. He think polling only one NAPI may
>> not guarantee netpoll make progress with the TX queue we are aiming
>> for. Also, the bigger problem may be the fact that NAPIs could get
>> pinned to one CPU and cannot get released.
>>
>> At this point, I really don't know what is the best way to fix this.
>>
>> I will also work on a repro with netperf.
>
> Thanks !
>
>>
>> Please let me know your suggestions.
>>
>
> Yeah, maybe that NICs using NAPI could not provide an ndo_poll_controller() method at all,
> since it is very risky (potentially grab many NAPI, and end up in this locked situation)
>
> poll_napi() could attempt to free skbs one napi at a time,
> without the current cpu stealing all NAPI.
>
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 57557a6a950cc9cdff959391576a03381d328c1a..a992971d366090ba69d5c1af32eadd554d6880cf 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -205,13 +205,8 @@ static void netpoll_poll_dev(struct net_device *dev)
> }
>
> ops = dev->netdev_ops;
> - if (!ops->ndo_poll_controller) {
> - up(&ni->dev_lock);
> - return;
> - }
> -
> - /* Process pending work on NIC */
> - ops->ndo_poll_controller(dev);
> + if (ops->ndo_poll_controller)
> + ops->ndo_poll_controller(dev);
>
> poll_napi(dev);
>
I tried to totally skip ndo_poll_controller() here. It did avoid hitting
the issue. However, netpoll will drop (fail to send) more packets.
Thanks,
Song
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Eric Dumazet @ 2018-09-20 23:49 UTC (permalink / raw)
To: Song Liu, Eric Dumazet
Cc: Jeff Kirsher, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <E4A7BD0D-B091-4FBD-94A9-F0104729DCF6@fb.com>
On 09/20/2018 04:43 PM, Song Liu wrote:
>
> I tried to totally skip ndo_poll_controller() here. It did avoid hitting
> the issue. However, netpoll will drop (fail to send) more packets.
>
Why is it failing ?
If you are under high memory pressure, then maybe if you absolutely want memory to send
netpoll packets, you want to grab all NAPI contexts as a way to prevent other cpus
from feeding incoming packets to the host and add more memory pressure ;)
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in _decode_session6
From: Alexei Starovoitov @ 2018-09-21 6:21 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Eric Dumazet, syzbot, Alexei Starovoitov, Daniel Borkmann,
David Miller, Herbert Xu, Alexey Kuznetsov, LKML, netdev,
Steffen Klassert, syzkaller-bugs, Hideaki YOSHIFUJI
On Thu, Sep 6, 2018 at 12:17 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> but I have a hard time reproducing the issue, so will appreciate
>> if somebody can test the following patch:
>
> syzbot can:
> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
was the patch tested?
it seems to me syzbot doesn't care about kernel quality but rather
about the number of issues syzbot can find.
^ permalink raw reply
* Re: [PATCH net-next, 1/3] hv_netvsc: Add support for LRO/RSC in the vSwitch
From: Stephen Hemminger @ 2018-09-21 0:41 UTC (permalink / raw)
To: Haiyang Zhang
Cc: davem@davemloft.net, netdev@vger.kernel.org, olaf@aepfle.de,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
vkuznets
In-Reply-To: <BN6PR21MB01614DCE1099980A7B1286FFCA130@BN6PR21MB0161.namprd21.prod.outlook.com>
On Thu, 20 Sep 2018 20:56:46 +0000
Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Thursday, September 20, 2018 4:48 PM
> > To: Haiyang Zhang <haiyangz@linuxonhyperv.com>
> > Cc: Haiyang Zhang <haiyangz@microsoft.com>; davem@davemloft.net;
> > netdev@vger.kernel.org; olaf@aepfle.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; vkuznets <vkuznets@redhat.com>
> > Subject: Re: [PATCH net-next, 1/3] hv_netvsc: Add support for LRO/RSC in the
> > vSwitch
> >
> > On Thu, 20 Sep 2018 17:06:59 +0000
> > Haiyang Zhang <haiyangz@linuxonhyperv.com> wrote:
> >
> > > +static inline void rsc_add_data
> > > + (struct netvsc_channel *nvchan,
> > > + const struct ndis_pkt_8021q_info *vlan,
> > > + const struct ndis_tcp_ip_checksum_info *csum_info,
> > > + void *data, u32 len)
> > > +{
> >
> > Could this be changed to look more like a function and skip the inline.
> > The compiler will end up inlining it anyway.
> >
> > static void rsc_add_data(struct netvsc_channel *nvchan,
>
> How about this?
> static inline
> void rsc_add_data(struct netvsc_channel *nvchan,
>
Sure that matches other code in that file
^ permalink raw reply
* [PATCH net] net/ipv6: Display all addresses in output of /proc/net/if_inet6
From: Jeff Barnhill @ 2018-09-21 0:45 UTC (permalink / raw)
To: netdev; +Cc: davem, kuznet, yoshfuji, Jeff Barnhill
The backend handling for /proc/net/if_inet6 in addrconf.c doesn't properly
handle starting/stopping the iteration. The problem is that at some point
during the iteration, an overflow is detected and the process is
subsequently stopped. The item being shown via seq_printf() when the
overflow occurs is not actually shown, though. When start() is
subsequently called to resume iterating, it returns the next item, and
thus the item that was being processed when the overflow occurred never
gets printed.
Alter the meaning of the private data member "offset". Currently, when it
is not 0 (which only happens at the very beginning), "offset" represents
the next hlist item to be printed. After this change, "offset" always
represents the current item.
This is also consistent with the private data member "bucket", which
represents the current bucket, and also the use of "pos" as defined in
seq_file.txt:
The pos passed to start() will always be either zero, or the most
recent pos used in the previous session.
Signed-off-by: Jeff Barnhill <0xeffeff@gmail.com>
---
net/ipv6/addrconf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index d51a8c0b3372..c63ccce6425f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4201,7 +4201,6 @@ static struct inet6_ifaddr *if6_get_first(struct seq_file *seq, loff_t pos)
p++;
continue;
}
- state->offset++;
return ifa;
}
@@ -4225,13 +4224,12 @@ static struct inet6_ifaddr *if6_get_next(struct seq_file *seq,
return ifa;
}
+ state->offset = 0;
while (++state->bucket < IN6_ADDR_HSIZE) {
- state->offset = 0;
hlist_for_each_entry_rcu(ifa,
&inet6_addr_lst[state->bucket], addr_lst) {
if (!net_eq(dev_net(ifa->idev->dev), net))
continue;
- state->offset++;
return ifa;
}
}
--
2.14.1
^ permalink raw reply related
* Re: [PATCH iproute2-next] iplink: add ipvtap support
From: David Ahern @ 2018-09-21 0:55 UTC (permalink / raw)
To: Hangbin Liu, netdev
Cc: Stephen Hemminger, Phil Sutter, Sainath Grandhi, Davide Caratti
In-Reply-To: <1537326209-30837-1-git-send-email-liuhangbin@gmail.com>
On 9/18/18 8:03 PM, Hangbin Liu wrote:
> IPVLAN and IPVTAP are using the same functions and parameters. So we can
> just add a new link_util with id ipvtap. Others are the same.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> ip/iplink.c | 4 ++--
> ip/iplink_ipvlan.c | 28 ++++++++++++++++++----------
> man/man8/ip-link.8.in | 4 ++++
> 3 files changed, 24 insertions(+), 12 deletions(-)
>
applied to iproute2-next. Thanks
^ permalink raw reply
* RE: [PATCH net-next] net/tls: Add support for async encryption of records for performance
From: Vakul Garg @ 2018-09-21 1:14 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, borisp@mellanox.com, aviadye@mellanox.com,
davejwatson@fb.com, doronrk@fb.com
In-Reply-To: <20180920.111833.80229845984902983.davem@davemloft.net>
> -----Original Message-----
> From: David Miller <davem@davemloft.net>
> Sent: Thursday, September 20, 2018 11:49 PM
> To: Vakul Garg <vakul.garg@nxp.com>
> Cc: netdev@vger.kernel.org; borisp@mellanox.com;
> aviadye@mellanox.com; davejwatson@fb.com; doronrk@fb.com
> Subject: Re: [PATCH net-next] net/tls: Add support for async encryption of
> records for performance
>
> From: Vakul Garg <vakul.garg@nxp.com>
> Date: Wed, 19 Sep 2018 20:51:35 +0530
>
> > This patch enables encryption of multiple records in parallel when an
> > async capable crypto accelerator is present in system.
>
> This seems to be trading off zero copy with async support.
>
> Async crypto device support is not the common case at all, and synchronous
> crypto via cpu crypto acceleration instructions is so much more likely.
>
> Oh I see, the new logic is only triggered with ASYNC_CAPABLE is set?
>
> > +static inline bool is_tx_ready(struct tls_context *tls_ctx,
> > + struct tls_sw_context_tx *ctx)
> > +{
>
> Two space between "inline" and "bool", please make it one.
Fixed.
Seems checkpatch misses it.
>
> > static void tls_write_space(struct sock *sk) {
> > struct tls_context *ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
>
> Longest to shortest line (reverse christmas tree) ordering for local variable
> declarations please.
Can't do this. The second variable assignment is dependent upon previous one.
> >
> > + list_for_each_prev(pos, &ctx->tx_ready_list) {
> > + struct tls_rec *rec = (struct tls_rec *)pos;
> > + u64 seq = be64_to_cpup((const __be64 *)&rec->aad_space);
>
> Likewise.
>
I can split variable declaration 'seq' and its assignment into two separate lines.
But I am not sure if increasing number of lines in order to comply reverse Christmas tree
is a good thing for this case.
> > -static int tls_do_encryption(struct tls_context *tls_ctx,
> > +int tls_tx_records(struct sock *sk, int flags) {
> > + struct tls_rec *rec, *tmp;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + int rc = 0;
> > + int tx_flags;
>
> Likewise.
Could partially address since ctx assignment depends upon tls_ctx assignment.
>
> > +static void tls_encrypt_done(struct crypto_async_request *req, int
> > +err) {
> > + struct aead_request *aead_req = (struct aead_request *)req;
> > + struct sock *sk = req->data;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + struct tls_rec *rec;
> > + int pending;
> > + bool ready = false;
>
> Likewise.
Placed 'ready' above pending 'pending'. Rest unchanged because of dependencies.
>
> > +static int tls_do_encryption(struct sock *sk,
> > + struct tls_context *tls_ctx,
> > struct tls_sw_context_tx *ctx,
> > struct aead_request *aead_req,
> > size_t data_len)
> > {
> > int rc;
> > + struct tls_rec *rec = ctx->open_rec;
>
> Likewise.
>
> > @@ -473,11 +630,12 @@ static int memcopy_from_iter(struct sock *sk,
> > struct iov_iter *from, {
> > struct tls_context *tls_ctx = tls_get_ctx(sk);
> > struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > - struct scatterlist *sg = ctx->sg_plaintext_data;
> > + struct tls_rec *rec = ctx->open_rec;
> > + struct scatterlist *sg = rec->sg_plaintext_data;
> > int copy, i, rc = 0;
>
> Likewise.
Can't change because of dependencies.
>
> > +struct tls_rec *get_rec(struct sock *sk) {
> > + int mem_size;
> > + struct tls_context *tls_ctx = tls_get_ctx(sk);
> > + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> > + struct tls_rec *rec;
>
> Likewise.
>
Declared 'mem_size' below 'rec'.
> > @@ -510,21 +707,33 @@ int tls_sw_sendmsg(struct sock *sk, struct
> msghdr *msg, size_t size)
> > int record_room;
> > bool full_record;
> > int orig_size;
> > + struct tls_rec *rec;
> > bool is_kvec = msg->msg_iter.type & ITER_KVEC;
> > + struct crypto_tfm *tfm = crypto_aead_tfm(ctx->aead_send);
> > + bool async_capable = tfm->__crt_alg->cra_flags &
> CRYPTO_ALG_ASYNC;
> > + int num_async = 0;
> > + int num_zc = 0;
>
> Likewise.
Fixed
> > @@ -661,6 +904,8 @@ int tls_sw_sendpage(struct sock *sk, struct page
> *page,
> > struct scatterlist *sg;
> > bool full_record;
> > int record_room;
> > + struct tls_rec *rec;
> > + int num_async = 0;
>
> Likewise.
Fixed.
Sending v2.
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Song Liu @ 2018-09-21 7:17 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jeff Kirsher, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <028b4cea-0e3f-fab5-7a74-cf003bbd1134@gmail.com>
> On Sep 20, 2018, at 4:49 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> On 09/20/2018 04:43 PM, Song Liu wrote:
>>
>
>> I tried to totally skip ndo_poll_controller() here. It did avoid hitting
>> the issue. However, netpoll will drop (fail to send) more packets.
>>
>
> Why is it failing ?
>
> If you are under high memory pressure, then maybe if you absolutely want memory to send
> netpoll packets, you want to grab all NAPI contexts as a way to prevent other cpus
> from feeding incoming packets to the host and add more memory pressure ;)
>
I did the test with Eric's latest patch (and disable ndo_poll_controller
in driver). The result didn't show significant increase in drop packets.
I guess packet drops in my earlier test was caused by some other changes
I mixed there.
So I think this patch does fix the issue. Thanks Eric!
For ixgbe, I think we need to check napi_complete_done() return value
anyway. Otherwise, the driver will enable IRQ in polling mode.
Song
^ permalink raw reply
* Re: [PATCH net-next 17/22] hv_netvsc: fix return type of ndo_start_xmit function
From: YueHaibing @ 2018-09-21 1:37 UTC (permalink / raw)
To: Stephen Hemminger
Cc: davem, dmitry.tarnyagin, wg, mkl, michal.simek, hsweeten,
madalin.bucur, pantelis.antoniou, claudiu.manoil, leoyang.li,
linux, sammy, ralf, nico, steve.glendinning, f.fainelli,
grygorii.strashko, w-kwok2, m-karicheri2, t.sailer, jreuter, kys,
haiyangz, wei.liu2, paul.durrant, arvid.brodin, pshelar, dev,
linux-mips, xen-devel, netdev, linux-usb
In-Reply-To: <20180920074341.3acef75c@xeon-e3>
On 2018/9/20 22:43, Stephen Hemminger wrote:
> On Thu, 20 Sep 2018 20:33:01 +0800
> YueHaibing <yuehaibing@huawei.com> wrote:
>
>> The method ndo_start_xmit() is defined as returning an 'netdev_tx_t',
>> which is a typedef for an enum type, so make sure the implementation in
>> this driver has returns 'netdev_tx_t' value, and change the function
>> return type to netdev_tx_t.
>>
>> Found by coccinelle.
>>
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>> drivers/net/hyperv/netvsc_drv.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
>> index 3af6d8d..056c472 100644
>> --- a/drivers/net/hyperv/netvsc_drv.c
>> +++ b/drivers/net/hyperv/netvsc_drv.c
>> @@ -511,7 +511,8 @@ static int netvsc_vf_xmit(struct net_device *net, struct net_device *vf_netdev,
>> return rc;
>> }
>>
>> -static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>> +static netdev_tx_t
>> +netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>> {
>> struct net_device_context *net_device_ctx = netdev_priv(net);
>> struct hv_netvsc_packet *packet = NULL;
>> @@ -528,8 +529,11 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
>> */
>> vf_netdev = rcu_dereference_bh(net_device_ctx->vf_netdev);
>> if (vf_netdev && netif_running(vf_netdev) &&
>> - !netpoll_tx_running(net))
>> - return netvsc_vf_xmit(net, vf_netdev, skb);
>> + !netpoll_tx_running(net)) {
>> + ret = netvsc_vf_xmit(net, vf_netdev, skb);
>> + if (ret)
>> + return NETDEV_TX_BUSY;
>> + }
>
> Sorry, the new code is wrong. It will fall through if ret == 0 (NETDEV_TX_OK)
> Please review and test your patches.
I'm sorry for this, will correct it as Haiyang's suggestion.
>
> .
>
^ permalink raw reply
* RE: [PATCH 00/21] SMMU enablement for NXP LS1043A and LS1046A
From: Laurentiu Tudor @ 2018-09-21 7:32 UTC (permalink / raw)
To: Leo Li
Cc: robin.murphy@arm.com,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Netdev, lkml,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Madalin-cristian Bucur, Roy Pledge, Shawn Guo, David Miller
In-Reply-To: <CADRPPNTot7U6vARqtrRrw4XqhDQPSXP1QO9ZbGpNQByJPcU37Q@mail.gmail.com>
> -----Original Message-----
> From: Li Yang [mailto:leoyang.li@nxp.com]
> Sent: Thursday, September 20, 2018 10:07 PM
>
> On Thu, Sep 20, 2018 at 5:39 AM Laurentiu Tudor <laurentiu.tudor@nxp.com>
> wrote:
> >
> >
> >
> > On 19.09.2018 17:37, Robin Murphy wrote:
> > > On 19/09/18 15:18, Laurentiu Tudor wrote:
> > >> Hi Robin,
> > >>
> > >> On 19.09.2018 16:25, Robin Murphy wrote:
> > >>> Hi Laurentiu,
> > >>>
> > >>> On 19/09/18 13:35, laurentiu.tudor@nxp.com wrote:
> > >>>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> > >>>>
> > >>>> This patch series adds SMMU support for NXP LS1043A and LS1046A
> chips
> > >>>> and consists mostly in important driver fixes and the required
> device
> > >>>> tree updates. It touches several subsystems and consists of three
> main
> > >>>> parts:
> > >>>> - changes in soc/drivers/fsl/qbman drivers adding iommu mapping
> of
> > >>>> reserved memory areas, fixes and defered probe support
> > >>>> - changes in drivers/net/ethernet/freescale/dpaa_eth drivers
> > >>>> consisting in misc dma mapping related fixes and probe
> ordering
> > >>>> - addition of the actual arm smmu device tree node together with
> > >>>> various adjustments to the device trees
> > >>>>
> > >>>> Performance impact
> > >>>>
> > >>>> Running iperf benchmarks in a back-to-back setup (both sides
> > >>>> having smmu enabled) on a 10GBps port show an important
> > >>>> networking performance degradation of around %40 (9.48Gbps
> > >>>> linerate vs 5.45Gbps). If you need performance but without
> > >>>> SMMU support you can use "iommu.passthrough=1" to disable
> > >>>> SMMU.
> > >>>>
> > >>>> USB issue and workaround
> > >>>>
> > >>>> There's a problem with the usb controllers in these chips
> > >>>> generating smaller, 40-bit wide dma addresses instead of the
> > >>>> 48-bit
> > >>>> supported at the smmu input. So you end up in a situation
> > >>>> where the
> > >>>> smmu is mapped with 48-bit address translations, but the
> device
> > >>>> generates transactions with clipped 40-bit addresses, thus
> smmu
> > >>>> context faults are triggered. I encountered a similar
> > >>>> situation for
> > >>>> mmc that I managed to fix in software [1] however for USB I
> > >>>> did not
> > >>>> find a proper place in the code to add a similar fix. The
> only
> > >>>> workaround I found was to add this kernel parameter which
> > >>>> limits the
> > >>>> usb dma to 32-bit size: "xhci-hcd.quirks=0x800000".
> > >>>> This workaround if far from ideal, so any suggestions for a
> code
> > >>>> based workaround in this area would be greatly appreciated.
> > >>>
> > >>> If you have a nominally-64-bit device with a
> > >>> narrower-than-the-main-interconnect link in front of it, that should
> > >>> already be fixed in 4.19-rc by bus_dma_mask picking up DT dma-
> ranges,
> > >>> provided the interconnect hierarchy can be described appropriately
> (or
> > >>> at least massaged sufficiently to satisfy the binding), e.g.:
> > >>>
> > >>> / {
> > >>> ...
> > >>>
> > >>> soc {
> > >>> ranges;
> > >>> dma-ranges = <0 0 10000 0>;
> > >>>
> > >>> dev_48bit { ... };
> > >>>
> > >>> periph_bus {
> > >>> ranges;
> > >>> dma-ranges = <0 0 100 0>;
> > >>>
> > >>> dev_40bit { ... };
> > >>> };
> > >>> };
> > >>> };
> > >>>
> > >>> and if that fails to work as expected (except for PCI hosts where
> > >>> handling dma-ranges properly still needs sorting out), please do let
> us
> > >>> know ;)
> > >>>
> > >>
> > >> Just to confirm, Is this [1] the change I was supposed to test?
> > >
> > > Not quite - dma-ranges is only valid for nodes representing a bus, so
> > > putting it directly in the USB device nodes doesn't work (FWIW that's
> > > why PCI is broken, because the parser doesn't expect the
> > > bus-as-leaf-node case). That's teh point of that intermediate simple-
> bus
> > > node represented by "periph_bus" in my example (sorry, I should have
> put
> > > compatibles in to make it clearer) - often that's actually true to
> life
> > > (i.e. "soc" is something like a CCI and "periph_bus" is something like
> > > an AXI NIC gluing a bunch of lower-bandwidth DMA masters to one of the
> > > CCI ports) but at worst it's just a necessary evil to make the binding
> > > happy (if it literally only represents the point-to-point link between
> > > the device master port and interconnect slave port).
> > >
> >
> > Quick update: so I adjusted to device tree according to your example and
> > it works so now I can get rid of that nasty kernel arg based workaround,
> > yey! :-)
>
> Great that we have a generic solution like I hoped for! So you will
> submit a new revision of the series to include these dts updates,
> right?
>
Yes, I already have it prepared. Just delaying the v2 for a few days maybe there will be some more feedback.
---
Best Regards, Laurentiu
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: add a missing error return
From: Jason Wang @ 2018-09-21 2:29 UTC (permalink / raw)
To: Dan Carpenter, Michael S. Tsirkin
Cc: kvm, virtualization, netdev, kernel-janitors
In-Reply-To: <20180920100158.GA9551@mwanda>
On 2018年09月20日 18:01, Dan Carpenter wrote:
> We accidentally left out this error return so it leads to some use after
> free bugs later on.
>
> Fixes: 0a0be13b8fe2 ("vhost_net: batch submitting XDP buffers to underlayer sockets")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index dd4e0a301635..1bff6bc8161a 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1244,6 +1244,7 @@ static int vhost_net_open(struct inode *inode, struct file *f)
> kfree(vqs);
> kvfree(n);
> kfree(queue);
> + return -ENOMEM;
> }
> n->vqs[VHOST_NET_VQ_TX].xdp = xdp;
>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks!
^ permalink raw reply
* Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array
From: Marek Szyprowski @ 2018-09-21 8:18 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Andrew Lunn, Ulf Hansson, linux-doc, linux-iio, Linus Walleij,
Dominik Brodowski, Peter Rosin, netdev, linux-i2c,
Peter Meerwald-Stadler, devel, Florian Fainelli, Jonathan Corbet,
Krzysztof Kozlowski, Kishon Vijay Abraham I, Tony Lindgren,
Lukas Wunner, Geert Uytterhoeven, linux-serial, Jiri Slaby,
Michael Hennerich, Uwe Kleine-König, linux-gpio,
Russell King
In-Reply-To: <15226900.TQMLYV7PZ0@z50>
Hi Janusz,
On 2018-09-20 18:21, Janusz Krzysztofik wrote:
> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote:
>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote:
>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote:
>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may contain
>>>> information on direct mapping of array members to pins of a single GPIO
>>>> chip in hardware order. In such cases, bitmaps of values can be passed
>>>> directly from/to the chip's .get/set_multiple() callbacks without
>>>> wasting time on iterations.
>>>>
>>>> Add respective code to gpiod_get/set_array_bitmap_complex() functions.
>>>> Pins not applicable for fast path are processed as before, skipping
>>>> over the 'fast' ones.
>>>>
>>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>> I've just noticed that this patch landed in today's linux-next. Sadly it
>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts).
>>>
>>> Booting hangs after detecting MMC cards. Reverting this patch fixes the
>>> boot. I will try later to add some debugs and investigate it further what
>>> really happens when booting hangs.
>> Hi Marek,
>>
>> Thanks for reporting. Could you please try the following fix?
> Hi again,
>
> I realized the patch was not correct, j, not i, should be updated in second
> hunk. Please try the following one.
>
> Thanks,
> Janusz
>
> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001
> From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Date: Thu, 20 Sep 2018 17:37:21 +0200
> Subject: [PATCH] gpiolib: Fix bitmap index not updated
> While skipping fast path bits, bitmap index is not updated with next
> found zero bit position. Fix it.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
This one also doesn't help. A quick compare of logs with this version and
a working system shows, that with your patch (and fix) there are no calls to
gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If
you need any more information (what kind of logs will help?), let me know.
> ---
> drivers/gpio/gpiolib.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a53d17745d21..369bdd358fcc 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
> __set_bit(hwgpio, mask);
>
> if (array_info)
> - find_next_zero_bit(array_info->get_mask,
> + i = find_next_zero_bit(array_info->get_mask,
> array_size, i);
> else
> i++;
> @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
> trace_gpio_value(desc_to_gpio(desc), 1, value);
>
> if (array_info)
> - find_next_zero_bit(array_info->get_mask, i, j);
> + j = find_next_zero_bit(array_info->get_mask, i,
> + j);
> else
> j++;
> }
> @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
> }
>
> if (array_info)
> - find_next_zero_bit(array_info->set_mask,
> + i = find_next_zero_bit(array_info->set_mask,
> array_size, i);
> else
> i++;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* [PATCH RESEND] PCI: hv: Fix return value check in hv_pci_assign_slots()
From: Wei Yongjun @ 2018-09-21 2:53 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Lorenzo Pieralisi, Bjorn Helgaas
Cc: Wei Yongjun, devel, linux-pci, netdev, kernel-janitors
In-Reply-To: <1537425631-28460-1-git-send-email-weiyongjun1@huawei.com>
In case of error, the function pci_create_slot() returns ERR_PTR() and
never returns NULL. The NULL test in the return value check should be
replaced with IS_ERR().
Fixes: a15f2c08c708 ("PCI: hv: support reporting serial number as slot information")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
Since the orig patch is merged from net tree, cc netdev@vger.kernel.org
---
drivers/pci/controller/pci-hyperv.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index ee80e79..9ba4d12 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1484,8 +1484,10 @@ static void hv_pci_assign_slots(struct hv_pcibus_device *hbus)
snprintf(name, SLOT_NAME_SIZE, "%u", hpdev->desc.ser);
hpdev->pci_slot = pci_create_slot(hbus->pci_bus, slot_nr,
name, NULL);
- if (!hpdev->pci_slot)
+ if (IS_ERR(hpdev->pci_slot)) {
pr_warn("pci_create slot %s failed\n", name);
+ hpdev->pci_slot = NULL;
+ }
}
}
^ permalink raw reply related
* Re: [PATCH net] r8169: fix autoneg issue on resume with RTL8168E
From: David Miller @ 2018-09-21 2:59 UTC (permalink / raw)
To: hkallweit1; +Cc: nic_swsd, netdev, neil
In-Reply-To: <ad74f3bd-ff60-1c8e-81cd-44ab76f942de@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 20 Sep 2018 22:47:09 +0200
> It was reported that chip version 33 (RTL8168E) ends up with
> 10MBit/Half on a 1GBit link after resuming from S3 (with different
> link partners). For whatever reason the PHY on this chip doesn't
> properly start a renegotiation when soft-reset.
> Explicitly requesting a renegotiation fixes this.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Fixes: a2965f12fde6 ("r8169: remove rtl8169_set_speed_xmii")
> Reported-by: Neil MacLeod <neil@nmacleod.com>
> Tested-by: Neil MacLeod <neil@nmacleod.com>
Applied, thank you.
Please always make "Fixes: " the first tag in a set of tags.
I fixed it up for you this time.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next 0/3] net: wean netfilter from fib_nh
From: David Miller @ 2018-09-21 3:02 UTC (permalink / raw)
To: dsahern; +Cc: netdev, netfilter-devel, pablo, fw, dsahern
In-Reply-To: <20180920205049.15143-1-dsahern@kernel.org>
From: dsahern@kernel.org
Date: Thu, 20 Sep 2018 13:50:46 -0700
> From: David Ahern <dsahern@gmail.com>
>
> Two netfilter modules reference fib_nh. In both cases the code is
> only checking if a nexthop in a fib_info uses a specific device.
> Both instances essentially duplicate code from __fib_validate_source,
> so move that code into a helper and flip the netfilter modules to
> use it.
Series applied, thanks David.
^ permalink raw reply
* Re: KASAN: slab-out-of-bounds Read in _decode_session6
From: Dmitry Vyukov @ 2018-09-21 8:53 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Eric Dumazet, syzbot, Alexei Starovoitov, Daniel Borkmann,
David Miller, Herbert Xu, Alexey Kuznetsov, LKML, netdev,
Steffen Klassert, syzkaller-bugs, Hideaki YOSHIFUJI
In-Reply-To: <CAADnVQL8=8O1W8-G-UP1akLijdGN82hnOJAY=sknTH_S0Y1rVQ@mail.gmail.com>
On Fri, Sep 21, 2018 at 8:21 AM, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
> On Thu, Sep 6, 2018 at 12:17 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>>> but I have a hard time reproducing the issue, so will appreciate
>>> if somebody can test the following patch:
>>
>> syzbot can:
>> https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
>
> was the patch tested?
Hi Alexei,
syzbot tests patches on request. I don't see anybody requested any
testing for this bug. When testing is requested syzbot replies with
results generally within 30 mins. You can read more about patch
testing here:
https://github.com/google/syzkaller/blob/master/docs/syzbot.md#testing-patches
> it seems to me syzbot doesn't care about kernel quality but rather
> about the number of issues syzbot can find.
Finding and reporting bugs is a prerequisite for fixing them and
improving kernel quality. syzbot simply automates that part of bug
handling process, something that otherwise would needed to be done by
kernel developers. But active developer involvement and interest are
still required as not all parts are automatable.
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] bpf: emit RECORD_MMAP events for bpf prog load/unload
From: Alexei Starovoitov @ 2018-09-21 3:14 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Arnaldo Carvalho de Melo, Alexei Starovoitov, David S . Miller,
daniel, netdev, kernel-team
In-Reply-To: <20180920135651.GW24124@hirez.programming.kicks-ass.net>
On Thu, Sep 20, 2018 at 03:56:51PM +0200, Peter Zijlstra wrote:
> On Thu, Sep 20, 2018 at 10:25:45AM -0300, Arnaldo Carvalho de Melo wrote:
> > PeterZ provided a patch introducing PERF_RECORD_MUNMAP, went nowhere due
> > to having to cope with munmapping parts of existing mmaps, etc.
> >
> > I'm still more in favour of introduce PERF_RECORD_MUNMAP, even if for
> > now it would be used just in this clean case for undoing a
> > PERF_RECORD_MMAP for a BPF program.
> >
> > The ABI is already complicated, starting to use something called
> > PERF_RECORD_MMAP for unmmaping by just using a NULL name... too clever,
> > I think.
>
> Agreed, the PERF_RECORD_MUNMAP patch was fairly trivial, the difficult
> part was getting the perf tool to dtrt for that use-case. But if we need
> unmap events, doing the unmap record now is the right thing.
Thanks for the pointers!
The answer is a bit long. pls bear with me.
I have considered adding MUNMAP to match existing MMAP, but went
without it because I didn't want to introduce new bit in perf_event_attr
and emit these new events in a misbalanced conditional way for prog load/unload.
Like old perf is asking kernel for mmap events via mmap bit, so prog load events
will be in perf.data, but old perf report won't recognize them anyway.
Whereas new perf would certainly want to catch bpf events and will set
both mmap and mumap bits.
Then if I add MUNMAP event without new bit and emit MMAP/MUNMAP
conditionally based on single mmap bit they will confuse old perf
and it will print warning about 'unknown events'.
Both situations are ugly, hence I went with reuse of MMAP event
for both load/unload.
In such case old perf silently ignores them. Which is what I wanted.
When we upgrade the kernel we cannot synchronize the kernel upgrade
(or downgrade) with user space perf package upgrade.
Hence not confusing old perf is important.
With new kernel new bpf mmap events get into perf.data and
new perf picks them up.
Few more considerations:
I consider synthetic perf events to be non-ABI. Meaning they're
emitted by perf user space into perf.data and there is a convention
on names, but it's not a kernel abi. Like RECORD_MMAP with
event.filename == "[module_name]" is an indication for perf report
to parse elf/build-id of dso==module_name.
There is no such support in the kernel. Kernel doesn't emit
such events for module load/unload. If in the future
we decide to extend kernel with such events they don't have
to match what user space perf does today.
Why this is important? To get to next step.
As Arnaldo pointed out this patch set is missing support for
JITed prog annotations and displaying asm code. Absolutely correct.
This set only helps perf to reveal the names of bpf progs that _were_
running at the time of perf record, but there is no way yet for
perf report to show asm code of the prog that was running.
In that sense bpf is drastically different from java, other jits
and normal profiling.
bpf JIT happens in the kernel and only kernel knows the mapping
between original source and JITed code.
In addition there are bpf2bpf functions. In the future there will
be bpf libraries, more type info, line number support, etc.
I strongly believe perf RECORD_* events should NOT care about
the development that happens on the bpf side.
The only thing kernel will be telling user space is that bpf prog
with prog_id=X was loaded.
Then based on prog_id the 'perf record' phase can query the kernel
for bpf related information. There is already a way to fetch
JITed image based on prog_id.
Then perf will emit synthetic RECORD_FOOBAR into perf.data
that will contain bpf related info (like complete JITed image)
and perf report can process it later and annotate things in UI.
It may seem that there is a race here.
Like when 'perf record' see 'bpf prog was loaded with prog_id=X' event
it will ask the kernel about prog_id=X, but that prog could be
unloaded already.
In such case prog_id will not exist and perf record can ignore such event.
So no race.
The kernel doesn't need to pass all information about bpf prog to
the user space via RECORD_*. Instead 'perf record' can emit
synthetic events into perf.data.
I was thinking to extend RECORD_MMAP with prog_id already
(instead of passing kallsyms's bpf prog name in event->mmap.filename)
but bpf functions don't have their own prog_id. Multiple bpf funcs
with different JITed blobs are considered to be a part of single prog_id.
So as a step one I'm only extending RECORD_MMAP with addr and kallsym
name of bpf function/prog.
As a step two the plan is to add notification mechanism for prog load/unload
that will include prog_id and design new synthetic RECORD_* events in
perf user space that will contain JITed code, line info, BTF, etc.
TLDR:
step 1 (this patch set)
Single bpf prog_load can call multiple
bpf_prog_kallsyms_add() -> RECORD_MMAP with addr+kallsym only
Similarly unload calls multiple
bpf_prog_kallsyms_del() -> RECORD_MMAP with addr only
step 2 (future work)
single event for bpf prog_load with prog_id only.
Either via perf ring buffer or ftrace or tracepoints or some
other notification mechanism.
It may seem that step 2 obsoletes step 1. It can, but I think
it will complement it. There is a lot more code there and
a lot more discussions to have.
Step 1 is already big improvement.
Thoughts?
^ permalink raw reply
* [PATCH] ath5k: Remove unused BUG_ON
From: Nathan Chancellor @ 2018-09-21 9:25 UTC (permalink / raw)
To: Jiri Slaby, Nick Kossifidis, Luis R. Rodriguez, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Nathan Chancellor
Clang warns that the address of a pointer will always evaluated as true
in a boolean context:
drivers/net/wireless/ath/ath5k/debug.c:1031:14: warning: address of
array 'ah->sbands' will always evaluate to 'true'
[-Wpointer-bool-conversion]
BUG_ON(!ah->sbands);
~~~~~^~~~~~
./include/asm-generic/bug.h:61:45: note: expanded from macro 'BUG_ON'
#define BUG_ON(condition) do { if (unlikely(condition)) BUG(); } while (0)
^~~~~~~~~
./include/linux/compiler.h:77:42: note: expanded from macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
1 warning generated.
Given that this condition is always false because of the logical not,
just remove it to fix the warning.
Link: https://github.com/ClangBuiltLinux/linux/issues/142
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/wireless/ath/ath5k/debug.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath5k/debug.c b/drivers/net/wireless/ath/ath5k/debug.c
index e01faf641288..94f70047d3fc 100644
--- a/drivers/net/wireless/ath/ath5k/debug.c
+++ b/drivers/net/wireless/ath/ath5k/debug.c
@@ -1028,8 +1028,6 @@ ath5k_debug_dump_bands(struct ath5k_hw *ah)
if (likely(!(ah->debug.level & ATH5K_DEBUG_DUMPBANDS)))
return;
- BUG_ON(!ah->sbands);
-
for (b = 0; b < NUM_NL80211_BANDS; b++) {
struct ieee80211_supported_band *band = &ah->sbands[b];
char bname[6];
--
2.19.0
^ permalink raw reply related
* [PATCH] net/mlx4: Use cpumask_available for eq->affinity_mask
From: Nathan Chancellor @ 2018-09-21 9:44 UTC (permalink / raw)
To: Tariq Toukan, David S. Miller
Cc: netdev, linux-rdma, linux-kernel, Nathan Chancellor
Clang warns that the address of a pointer will always evaluated as true
in a boolean context:
drivers/net/ethernet/mellanox/mlx4/eq.c:243:11: warning: address of
array 'eq->affinity_mask' will always evaluate to 'true'
[-Wpointer-bool-conversion]
if (!eq->affinity_mask || cpumask_empty(eq->affinity_mask))
~~~~~^~~~~~~~~~~~~
1 warning generated.
Use cpumask_available, introduced in commit f7e30f01a9e2 ("cpumask: Add
helper cpumask_available()"), which does the proper checking and avoids
this warning.
Link: https://github.com/ClangBuiltLinux/linux/issues/86
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/ethernet/mellanox/mlx4/eq.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/eq.c b/drivers/net/ethernet/mellanox/mlx4/eq.c
index 1f3372c1802e..2df92dbd38e1 100644
--- a/drivers/net/ethernet/mellanox/mlx4/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx4/eq.c
@@ -240,7 +240,8 @@ static void mlx4_set_eq_affinity_hint(struct mlx4_priv *priv, int vec)
struct mlx4_dev *dev = &priv->dev;
struct mlx4_eq *eq = &priv->eq_table.eq[vec];
- if (!eq->affinity_mask || cpumask_empty(eq->affinity_mask))
+ if (!cpumask_available(eq->affinity_mask) ||
+ cpumask_empty(eq->affinity_mask))
return;
hint_err = irq_set_affinity_hint(eq->irq, eq->affinity_mask);
--
2.19.0
^ permalink raw reply related
* [PATCH] rsi: Remove unnecessary boolean condition
From: Nathan Chancellor @ 2018-09-21 9:48 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, netdev, linux-kernel, Nathan Chancellor
Clang warns that the address of a pointer will always evaluated as true
in a boolean context.
drivers/net/wireless/rsi/rsi_91x_mac80211.c:927:50: warning: address of
array 'key->key' will always evaluate to 'true'
[-Wpointer-bool-conversion]
if (vif->type == NL80211_IFTYPE_STATION && key->key &&
~~ ~~~~~^~~
1 warning generated.
Link: https://github.com/ClangBuiltLinux/linux/issues/136
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/wireless/rsi/rsi_91x_mac80211.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_mac80211.c b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
index 4e510cbe0a89..e56fc83faf0e 100644
--- a/drivers/net/wireless/rsi/rsi_91x_mac80211.c
+++ b/drivers/net/wireless/rsi/rsi_91x_mac80211.c
@@ -924,7 +924,7 @@ static int rsi_hal_key_config(struct ieee80211_hw *hw,
if (status)
return status;
- if (vif->type == NL80211_IFTYPE_STATION && key->key &&
+ if (vif->type == NL80211_IFTYPE_STATION &&
(key->cipher == WLAN_CIPHER_SUITE_WEP104 ||
key->cipher == WLAN_CIPHER_SUITE_WEP40)) {
if (!rsi_send_block_unblock_frame(adapter->priv, false))
--
2.19.0
^ permalink raw reply related
* 答复: [PATCH][next-next][v2] netlink: avoid to allocate full skb when sending to many devices
From: Li,Rongqing @ 2018-09-21 3:27 UTC (permalink / raw)
To: Eric Dumazet, netdev@vger.kernel.org
In-Reply-To: <549f8bea-bd5f-70da-bbf3-c2331ffbf964@gmail.com>
: Re: [PATCH][next-next][v2] netlink: avoid to allocate full skb when
> sending to many devices
>
>
>
> On 09/20/2018 06:43 AM, Eric Dumazet wrote:
> >
>
Sorry, I should cc to you.
> > And lastly this patch looks way too complicated to me.
> > You probably can write something much simpler.
>
But it should not increase the negative performance
> Something like :
>
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index
> 930d17fa906c9ebf1cf7b6031ce0a22f9f66c0e4..e0a81beb4f37751421dbbe794c
> cf3d5a46bdf900 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -278,22 +278,26 @@ static bool netlink_filter_tap(const struct sk_buff
> *skb)
> return false;
> }
>
> -static int __netlink_deliver_tap_skb(struct sk_buff *skb,
> +static int __netlink_deliver_tap_skb(struct sk_buff **pskb,
> struct net_device *dev) {
> - struct sk_buff *nskb;
> + struct sk_buff *nskb, *skb = *pskb;
> struct sock *sk = skb->sk;
> int ret = -ENOMEM;
>
> if (!net_eq(dev_net(dev), sock_net(sk)))
> return 0;
>
> - dev_hold(dev);
> -
> - if (is_vmalloc_addr(skb->head))
> + if (is_vmalloc_addr(skb->head)) {
> nskb = netlink_to_full_skb(skb, GFP_ATOMIC);
> - else
> - nskb = skb_clone(skb, GFP_ATOMIC);
> + if (!nskb)
> + return -ENOMEM;
> + consume_skb(skb);
The original skb can not be freed, since it will be used after send to tap in __netlink_sendskb
> + skb = nskb;
> + *pskb = skb;
> + }
> + dev_hold(dev);
> + nskb = skb_clone(skb, GFP_ATOMIC);
since original skb can not be freed, skb_clone will lead to a leak.
> if (nskb) {
> nskb->dev = dev;
> nskb->protocol = htons((u16) sk->sk_protocol); @@ -318,7 +322,7
> @@ static void __netlink_deliver_tap(struct sk_buff *skb, struct
> netlink_tap_net *n
> return;
>
> list_for_each_entry_rcu(tmp, &nn->netlink_tap_all, list) {
> - ret = __netlink_deliver_tap_skb(skb, tmp->dev);
> + ret = __netlink_deliver_tap_skb(&skb, tmp->dev);
> if (unlikely(ret))
> break;
> }
>
The below change seems simple, but it increase skb allocation and
free one time,
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index e3a0538ec0be..b9631137f0fe 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -290,10 +290,8 @@ static int __netlink_deliver_tap_skb(struct sk_buff *skb,
dev_hold(dev);
- if (is_vmalloc_addr(skb->head))
- nskb = netlink_to_full_skb(skb, GFP_ATOMIC);
- else
- nskb = skb_clone(skb, GFP_ATOMIC);
+ nskb = skb_clone(skb GFP_ATOMIC);
+
if (nskb) {
nskb->dev = dev;
nskb->protocol = htons((u16) sk->sk_protocol);
@@ -317,11 +315,20 @@ static void __netlink_deliver_tap(struct sk_buff *skb, struct netlink_tap_net *n
if (!netlink_filter_tap(skb))
return;
+ if (is_vmalloc_addr(skb->head)) {
+ skb = netlink_to_full_skb(skb, GFP_ATOMIC);
+ if (!skb)
+ return;
+ alloc = true;
+ }
+
list_for_each_entry_rcu(tmp, &nn->netlink_tap_all, list) {
+
ret = __netlink_deliver_tap_skb(skb, tmp->dev);
if (unlikely(ret))
break;
}
+
+ if (alloc)
+ consume_skb(skb);
}
-Q
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox