* Re: i40e - Is i40e_force_link_state doing the right thing ?
From: Chaitanya Lala @ 2018-05-15 21:47 UTC (permalink / raw)
To: Stachura, Mariusz
Cc: netdev@vger.kernel.org, Williams, Mitch A, Bowers, AndrewX,
Kirsher, Jeffrey T
In-Reply-To: <1F86EBA5C8668B468CEE1C33B96E862B36905DA1@IRSMSX104.ger.corp.intel.com>
Hi Mariusz, ...
On Tue, May 15, 2018 at 2:24 PM, Stachura, Mariusz
<mariusz.stachura@intel.com> wrote:
> On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala <chaitanya.lala@gmail.com> wrote:
>> Hi,
>>
>> I am trying to bring up a Intel XL710 4x10G Intel card using the
>> latest mainline top-of-tree.
>> The problem is that "ifconfig up" and "ifconfig down" do not take
>> effect at the link state level.
>> I tracked the problem down to i40e_force_link_state() when it is
>> called from i40e_down().
>> It calls i40e_force_link_state with "is_up" == false. In-turn it
>> calls, i40e_aq_set_link_restart_an(hw, true, NULL).
>>
>> Should the second argument of i40e_aq_set_link_restart_an be "is_up"
>> vs the current "true"
>> i.e. i40e_aq_set_link_restart_an(hw, is_up, NULL). ? When I make this
>> change, the link state syncs-up with the interface administrative
>> state.
>>
>> Is this a bug ?
>>
>> Thanks,
>> Chaitanya
>
> Hello Chaitanya,
>
> i40e_down() calls i40e_force_link_state with "is_up" == false only if interface's private flag "link-down-on-close" is set. By default the link is left up for manageability and VF traffic, user can use this flag to power down the interface on the link level. Does that work for you?
> The command is:
> "ethtool --set-priv-flags IFNAME link-down-on-close on" and then
This flag is _on_ in my setup and hencet i40e_force_link_state is
being called with is_up == false in my setup. The problem is that
irrespective of
value of "is_up" flag, i40e_force_link_state invokes
i40e_aq_set_link_restart_an with second argument (enable_link) as
"true". So i40e_aq_set_link_restart_an
is always trying to enable link even if is_up was false. Is that
correct behavior ?
I have pasted code with my annotations below marked with "//XXX".
/**
* i40e_force_link_state - Force the link status
* @pf: board private structure
* @is_up: whether the link state should be forced up or down
**/
static i40e_status i40e_force_link_state(struct i40e_pf *pf, bool
is_up) // XXX is_up was _false_ in my test case because I have
link-down-on-close "on"
{
struct i40e_aq_get_phy_abilities_resp abilities;
struct i40e_aq_set_phy_config config = {0};
struct i40e_hw *hw = &pf->hw;
i40e_status err;
u64 mask;
/* Get the current phy config */
err = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
NULL);
if (err) {
dev_err(&pf->pdev->dev,
"failed to get phy cap., ret = %s last_status = %s\n",
i40e_stat_str(hw, err),
i40e_aq_str(hw, hw->aq.asq_last_status));
return err;
}
/* If link needs to go up, but was not forced to go down,
* no need for a flap
*/
if (is_up && abilities.phy_type != 0)
return I40E_SUCCESS;
/* To force link we need to set bits for all supported PHY types,
* but there are now more than 32, so we need to split the bitmap
* across two fields.
*/
mask = I40E_PHY_TYPES_BITMASK;
config.phy_type = is_up ? cpu_to_le32((u32)(mask & 0xffffffff)) : 0;
config.phy_type_ext = is_up ? (u8)((mask >> 32) & 0xff) : 0;
/* Copy the old settings, except of phy_type */
config.abilities = abilities.abilities;
config.link_speed = abilities.link_speed;
config.eee_capability = abilities.eee_capability;
config.eeer = abilities.eeer_val;
config.low_power_ctrl = abilities.d3_lpan;
err = i40e_aq_set_phy_config(hw, &config, NULL);
if (err) {
dev_err(&pf->pdev->dev,
"set phy config ret = %s last_status = %s\n",
i40e_stat_str(&pf->hw, err),
i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status));
return err;
}
/* Update the link info */
err = i40e_update_link_info(hw);
if (err) {
/* Wait a little bit (on 40G cards it sometimes takes a really
* long time for link to come back from the atomic reset)
* and try once more
*/
msleep(1000);
i40e_update_link_info(hw);
}
i40e_aq_set_link_restart_an(hw, true, NULL); // XXX Second argument is
unconditionally true. Should it not be is_up instead of true ?
return I40E_SUCCESS;
}
Thanks,
Chaitanya
> "ifconfig IFNAME down"
>
> Best regards!
> Mariusz
> --------------------------------------------------------------------
>
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
>
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.
^ permalink raw reply
* RE: i40e - Is i40e_force_link_state doing the right thing ?
From: Stachura, Mariusz @ 2018-05-15 21:24 UTC (permalink / raw)
To: Chaitanya Lala, netdev@vger.kernel.org, Williams, Mitch A,
Bowers, AndrewX, Kirsher, Jeffrey T
In-Reply-To: <CABOGejK8bnwU0iqO5-GRO8FX6tCaMYtS+UORPrug3E7LqeWg1Q@mail.gmail.com>
On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala <chaitanya.lala@gmail.com> wrote:
> Hi,
>
> I am trying to bring up a Intel XL710 4x10G Intel card using the
> latest mainline top-of-tree.
> The problem is that "ifconfig up" and "ifconfig down" do not take
> effect at the link state level.
> I tracked the problem down to i40e_force_link_state() when it is
> called from i40e_down().
> It calls i40e_force_link_state with "is_up" == false. In-turn it
> calls, i40e_aq_set_link_restart_an(hw, true, NULL).
>
> Should the second argument of i40e_aq_set_link_restart_an be "is_up"
> vs the current "true"
> i.e. i40e_aq_set_link_restart_an(hw, is_up, NULL). ? When I make this
> change, the link state syncs-up with the interface administrative
> state.
>
> Is this a bug ?
>
> Thanks,
> Chaitanya
Hello Chaitanya,
i40e_down() calls i40e_force_link_state with "is_up" == false only if interface's private flag "link-down-on-close" is set. By default the link is left up for manageability and VF traffic, user can use this flag to power down the interface on the link level. Does that work for you?
The command is:
"ethtool --set-priv-flags IFNAME link-down-on-close on" and then
"ifconfig IFNAME down"
Best regards!
Mariusz
--------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
others is strictly prohibited.
^ permalink raw reply
* Re: [PATCH 00/14] Modify action API for implementing lockless actions
From: Vlad Buslov @ 2018-05-15 21:21 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: netdev, davem, xiyou.wangcong, jiri, pablo, kadlec, fw, ast,
daniel, edumazet, keescook, linux-kernel, netfilter-devel,
coreteam, kliteyn
In-Reply-To: <97ccb118-a3dc-6eb0-b127-9c04adf9e2fd@mojatatu.com>
On Tue 15 May 2018 at 18:25, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 14/05/18 04:46 PM, Vlad Buslov wrote:
>>
>> On Mon 14 May 2018 at 18:03, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 14/05/18 10:27 AM, Vlad Buslov wrote:
>
>
>> Hello Jamal,
>>
>> I'm trying to run tdc, but keep getting following error even on clean
>> branch without my patches:
>
> Vlad, not sure if you saw my email:
> Apply Roman's patch and try again
>
> https://marc.info/?l=linux-netdev&m=152639369112020&w=2
>
> cheers,
> jamal
With patch applied I get following error:
Test 7d50: Add skbmod action to set destination mac
exit: 255 0
dst MAC address <11:22:33:44:55:66>
RTNETLINK answers: No such file or directory
We have an error talking to the kernel
-----> teardown stage *** Could not execute: "$TC actions flush action skbmod"
-----> teardown stage *** Error message: "RTNETLINK answers: Invalid argument
We have an error flushing
"
-----> teardown stage *** Aborting test run.
<_io.BufferedReader name=3> *** stdout ***
<_io.BufferedReader name=5> *** stderr ***
"-----> teardown stage" did not complete successfully
Exception <class '__main__.PluginMgrTestFail'> ('teardown', 'dst MAC address <11:22:33:44:55:66>\nRTNETLINK answers: No such file or directory\nWe have an error talking to the kernel\n', '"-----> teardown stage" did not complete successful
ly') (caught in test_runner, running test 87 7d50 Add skbmod action to set destination mac stage teardown)
---------------
traceback
File "./tdc.py", line 293, in test_runner
tap += run_one_test(pm, args, index, tidx)
File "./tdc.py", line 229, in run_one_test
prepare_env(args, pm, 'teardown', '-----> teardown stage', tidx['teardown'], procout)
File "./tdc.py", line 177, in prepare_env
'"{}" did not complete successfully'.format(prefix))
---------------
accumulated output for this test:
dst MAC address <11:22:33:44:55:66>
RTNETLINK answers: No such file or directory
We have an error talking to the kernel
---------------
Regards,
Vlad
^ permalink raw reply
* Re: [PATCH bpf-next v6 2/4] bpf: sockmap, add hash map support
From: Y Song @ 2018-05-15 21:09 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: John Fastabend, Alexei Starovoitov, netdev, David Miller
In-Reply-To: <cc2e3011-0309-842c-4dbc-d3e82d5fc2d0@iogearbox.net>
On Tue, May 15, 2018 at 12:01 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 05/14/2018 07:00 PM, John Fastabend wrote:
>> Sockmap is currently backed by an array and enforces keys to be
>> four bytes. This works well for many use cases and was originally
>> modeled after devmap which also uses four bytes keys. However,
>> this has become limiting in larger use cases where a hash would
>> be more appropriate. For example users may want to use the 5-tuple
>> of the socket as the lookup key.
>>
>> To support this add hash support.
>>
>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>> Acked-by: David S. Miller <davem@davemloft.net>
>> ---
>> include/linux/bpf.h | 8 +
>> include/linux/bpf_types.h | 1 +
>> include/uapi/linux/bpf.h | 52 ++++-
>> kernel/bpf/core.c | 1 +
>> kernel/bpf/sockmap.c | 494 ++++++++++++++++++++++++++++++++++++++++++++--
>> kernel/bpf/verifier.c | 14 +-
>> net/core/filter.c | 58 ++++++
>> 7 files changed, 610 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index a38e474..ed0122b 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -668,6 +668,7 @@ static inline void bpf_map_offload_map_free(struct bpf_map *map)
>>
>> #if defined(CONFIG_STREAM_PARSER) && defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_INET)
>> struct sock *__sock_map_lookup_elem(struct bpf_map *map, u32 key);
>> +struct sock *__sock_hash_lookup_elem(struct bpf_map *map, void *key);
>> int sock_map_prog(struct bpf_map *map, struct bpf_prog *prog, u32 type);
>> #else
>> static inline struct sock *__sock_map_lookup_elem(struct bpf_map *map, u32 key)
>> @@ -675,6 +676,12 @@ static inline struct sock *__sock_map_lookup_elem(struct bpf_map *map, u32 key)
>> return NULL;
>> }
>>
>> +static inline struct sock *__sock_hash_lookup_elem(struct bpf_map *map,
>> + void *key)
>> +{
>> + return NULL;
>> +}
>> +
>> static inline int sock_map_prog(struct bpf_map *map,
>> struct bpf_prog *prog,
>> u32 type)
>> @@ -724,6 +731,7 @@ static inline void __xsk_map_flush(struct bpf_map *map)
>> extern const struct bpf_func_proto bpf_get_stackid_proto;
>> extern const struct bpf_func_proto bpf_get_stack_proto;
>> extern const struct bpf_func_proto bpf_sock_map_update_proto;
>> +extern const struct bpf_func_proto bpf_sock_hash_update_proto;
>>
>> /* Shared helpers among cBPF and eBPF. */
>> void bpf_user_rnd_init_once(void);
>> diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
>> index d7df1b32..b67f879 100644
>> --- a/include/linux/bpf_types.h
>> +++ b/include/linux/bpf_types.h
>> @@ -47,6 +47,7 @@
>> BPF_MAP_TYPE(BPF_MAP_TYPE_DEVMAP, dev_map_ops)
>> #if defined(CONFIG_STREAM_PARSER) && defined(CONFIG_INET)
>> BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKMAP, sock_map_ops)
>> +BPF_MAP_TYPE(BPF_MAP_TYPE_SOCKHASH, sock_hash_ops)
>> #endif
>> BPF_MAP_TYPE(BPF_MAP_TYPE_CPUMAP, cpu_map_ops)
>> #if defined(CONFIG_XDP_SOCKETS)
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 02e4112..1205d86 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -118,6 +118,7 @@ enum bpf_map_type {
>> BPF_MAP_TYPE_SOCKMAP,
>> BPF_MAP_TYPE_CPUMAP,
>> BPF_MAP_TYPE_XSKMAP,
>> + BPF_MAP_TYPE_SOCKHASH,
>> };
>>
>> enum bpf_prog_type {
>> @@ -1855,6 +1856,52 @@ struct bpf_stack_build_id {
>> * Egress device index on success, 0 if packet needs to continue
>> * up the stack for further processing or a negative error in case
>> * of failure.
>> + * int bpf_sock_hash_update(struct bpf_sock_ops_kern *skops, struct bpf_map *map, void *key, u64 flags)
>
> When you rebase please fix this up properly next time and add a newline in between
> the helpers. I fixed this up while applying.
I guess the tools/include/uapi/linux/bpf.h may also need fixup to be
in sync with main bpf.h.
^ permalink raw reply
* Re: Poor TCP performance with XPS enabled after scrubbing skb
From: Eric Dumazet @ 2018-05-15 21:08 UTC (permalink / raw)
To: Flavio Leitner, netdev; +Cc: Paolo Abeni
In-Reply-To: <20180515193128.GA11901@plex.lan>
On 05/15/2018 12:31 PM, Flavio Leitner wrote:
> Hi,
>
> There is a significant throughput issue (~50% drop) for a single TCP
> stream when the skb is scrubbed and XPS is enabled.
>
> If I turn CONFIG_XPS off, then the issue never happens and the test
> reaches line rate. The same happens if I echo 0 to tx-*/xps_cpus.
>
> It looks like that when the skb is scrubbed, there is no more reference
> to the struct sock,
And this is really the problem here, since it breaks back pressure (and TCP Small queues)
I am not sure why skb_orphan() is used in this scrubbing really.
^ permalink raw reply
* Re: [PATCH net-next v3 0/7] Microsemi Ocelot Ethernet switch support
From: David Miller @ 2018-05-15 20:45 UTC (permalink / raw)
To: alexandre.belloni
Cc: Allan.Nielsen, razvan.stefanescu, po.liu, thomas.petazzoni,
andrew, f.fainelli, netdev, linux-kernel, linux-mips, jhogan
In-Reply-To: <20180514200500.2953-1-alexandre.belloni@bootlin.com>
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
Date: Mon, 14 May 2018 22:04:53 +0200
> The ocelot dts changes are here for reference and should probably go
> through the MIPS tree once the bindings are accepted.
Ok, series applied to net-next minus patches #5 and #6.
Thank you.
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to get new driver attributes
From: Doug Ledford @ 2018-05-15 20:39 UTC (permalink / raw)
To: Steve Wise, 'David Ahern', leon; +Cc: stephen, netdev, linux-rdma
In-Reply-To: <004901d3ec8b$4b9ac5c0$e2d05140$@opengridcomputing.com>
[-- Attachment #1: Type: text/plain, Size: 2390 bytes --]
On Tue, 2018-05-15 at 15:28 -0500, Steve Wise wrote:
> > -----Original Message-----
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Doug Ledford
> > Sent: Tuesday, May 15, 2018 2:53 PM
> > To: David Ahern <dsahern@gmail.com>; Steve Wise
> > <swise@opengridcomputing.com>; leon@kernel.org
> > Cc: stephen@networkplumber.org; netdev@vger.kernel.org; linux-
> > rdma@vger.kernel.org
> > Subject: Re: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to
> > get new driver attributes
> >
> > On Tue, 2018-05-15 at 13:37 -0600, David Ahern wrote:
> > > On 5/14/18 9:42 AM, Steve Wise wrote:
> > > > diff --git a/rdma/include/uapi/rdma/rdma_netlink.h
> >
> > b/rdma/include/uapi/rdma/rdma_netlink.h
> > > > index 60416ed..40be0d8 100644
> > > > --- a/rdma/include/uapi/rdma/rdma_netlink.h
> > > > +++ b/rdma/include/uapi/rdma/rdma_netlink.h
> > > >
> > > > @@ -387,6 +399,20 @@ enum rdma_nldev_attr {
> > > > RDMA_NLDEV_ATTR_RES_PD_ENTRY, /* nested table */
> > > > RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY, /* u32 */
> > > > RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY, /* u32 */
> > > > + /*
> > > > + * driver-specific attributes.
> > > > + */
> > > > + RDMA_NLDEV_ATTR_DRIVER, /* nested table */
> > > > + RDMA_NLDEV_ATTR_DRIVER_ENTRY, /* nested table */
> > > > + RDMA_NLDEV_ATTR_DRIVER_STRING, /* string */
> > > > + /*
> > > > + * u8 values from enum rdma_nldev_print_type
> > > > + */
> > > > + RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE, /* u8 */
> > > > + RDMA_NLDEV_ATTR_DRIVER_S32, /* s32 */
> > > > + RDMA_NLDEV_ATTR_DRIVER_U32, /* u32 */
> > > > + RDMA_NLDEV_ATTR_DRIVER_S64, /* s64 */
> > > > + RDMA_NLDEV_ATTR_DRIVER_U64, /* u64 */
> > >
> > > and again here.
> >
> > This chunk, however, is a problem. We'll need to fix that in the kernel
> > and in this patch too.
>
> I'll fix this series once I fix the kernel side. Doug, should I send a patch that basically moves the DRIVER attributes to the bottom?
>
> Sorry about missing this!
I missed it too. I knew about the first hunk, but thought this one was
at the end. Anyway, I've already pushed the fix to my wip/dl-for-next
branch, so you can get it there now.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: i40e - Is i40e_force_link_state doing the right thing ?
From: Alexander Duyck @ 2018-05-15 20:31 UTC (permalink / raw)
To: Chaitanya Lala, intel-wired-lan
Cc: Netdev, mariusz.stachura, Mitch Williams, Bowers, AndrewX,
Jeff Kirsher
In-Reply-To: <CABOGejK8bnwU0iqO5-GRO8FX6tCaMYtS+UORPrug3E7LqeWg1Q@mail.gmail.com>
On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala
<chaitanya.lala@gmail.com> wrote:
> Hi,
>
> I am trying to bring up a Intel XL710 4x10G Intel card using the
> latest mainline top-of-tree.
> The problem is that "ifconfig up" and "ifconfig down" do not take
> effect at the link state level.
> I tracked the problem down to i40e_force_link_state() when it is
> called from i40e_down().
> It calls i40e_force_link_state with "is_up" == false. In-turn it
> calls, i40e_aq_set_link_restart_an(hw, true, NULL).
>
> Should the second argument of i40e_aq_set_link_restart_an be "is_up"
> vs the current "true"
> i.e. i40e_aq_set_link_restart_an(hw, is_up, NULL). ? When I make this
> change, the link state syncs-up with
> the interface administrative state.
>
> Is this a bug ?
>
> Thanks,
> Chaitanya
If you are calling i40e_down the assumption is you are bringing the
interface down, so as not to pass traffic. That is why the "is_up" is
set to false.
Could you provide the dmesg results for when you load the driver and
execute the "ifconfig up" and "ifconfig down" commands? That would
help us to understand what might be going on.
Thanks.
- Alex
^ permalink raw reply
* RE: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to get new driver attributes
From: Steve Wise @ 2018-05-15 20:29 UTC (permalink / raw)
To: 'David Ahern', 'Doug Ledford', leon
Cc: stephen, netdev, linux-rdma
In-Reply-To: <fd64ce39-a284-b09f-85b0-8cd7ba0d1207@gmail.com>
> -----Original Message-----
> From: David Ahern <dsahern@gmail.com>
> Sent: Tuesday, May 15, 2018 3:02 PM
> To: Doug Ledford <dledford@redhat.com>; Steve Wise
> <swise@opengridcomputing.com>; leon@kernel.org
> Cc: stephen@networkplumber.org; netdev@vger.kernel.org; linux-
> rdma@vger.kernel.org
> Subject: Re: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to
> get new driver attributes
>
> On 5/15/18 2:00 PM, Doug Ledford wrote:
> > I just sent an incremental fix to the list under separate cover. You
> > can squash that fix into Steve's patch and it should resolve the issue.
> > Or Steve can respin the set. Either way.
>
> Once the patch has been committed to the upstream repo, spin a new set
> with the correct header update.
Will do!
Steve.
^ permalink raw reply
* RE: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to get new driver attributes
From: Steve Wise @ 2018-05-15 20:28 UTC (permalink / raw)
To: 'Doug Ledford', 'David Ahern', leon
Cc: stephen, netdev, linux-rdma
In-Reply-To: <39cd4caf34da44885021d8646699889ced482008.camel@redhat.com>
> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Doug Ledford
> Sent: Tuesday, May 15, 2018 2:53 PM
> To: David Ahern <dsahern@gmail.com>; Steve Wise
> <swise@opengridcomputing.com>; leon@kernel.org
> Cc: stephen@networkplumber.org; netdev@vger.kernel.org; linux-
> rdma@vger.kernel.org
> Subject: Re: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to
> get new driver attributes
>
> On Tue, 2018-05-15 at 13:37 -0600, David Ahern wrote:
> > On 5/14/18 9:42 AM, Steve Wise wrote:
> > > diff --git a/rdma/include/uapi/rdma/rdma_netlink.h
> b/rdma/include/uapi/rdma/rdma_netlink.h
> > > index 60416ed..40be0d8 100644
> > > --- a/rdma/include/uapi/rdma/rdma_netlink.h
> > > +++ b/rdma/include/uapi/rdma/rdma_netlink.h
> > >
> > > @@ -387,6 +399,20 @@ enum rdma_nldev_attr {
> > > RDMA_NLDEV_ATTR_RES_PD_ENTRY, /* nested table */
> > > RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY, /* u32 */
> > > RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY, /* u32 */
> > > + /*
> > > + * driver-specific attributes.
> > > + */
> > > + RDMA_NLDEV_ATTR_DRIVER, /* nested table */
> > > + RDMA_NLDEV_ATTR_DRIVER_ENTRY, /* nested table */
> > > + RDMA_NLDEV_ATTR_DRIVER_STRING, /* string */
> > > + /*
> > > + * u8 values from enum rdma_nldev_print_type
> > > + */
> > > + RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE, /* u8 */
> > > + RDMA_NLDEV_ATTR_DRIVER_S32, /* s32 */
> > > + RDMA_NLDEV_ATTR_DRIVER_U32, /* u32 */
> > > + RDMA_NLDEV_ATTR_DRIVER_S64, /* s64 */
> > > + RDMA_NLDEV_ATTR_DRIVER_U64, /* u64 */
> >
> > and again here.
>
> This chunk, however, is a problem. We'll need to fix that in the kernel
> and in this patch too.
I'll fix this series once I fix the kernel side. Doug, should I send a patch that basically moves the DRIVER attributes to the bottom?
Sorry about missing this!
Steve
^ permalink raw reply
* RE: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to get new driver attributes
From: Steve Wise @ 2018-05-15 20:25 UTC (permalink / raw)
To: 'Doug Ledford', 'David Ahern', leon
Cc: stephen, netdev, linux-rdma
In-Reply-To: <eb6edd23b76a176b876002cc19c546a0669cf66f.camel@redhat.com>
> On Tue, 2018-05-15 at 13:37 -0600, David Ahern wrote:
> > On 5/14/18 9:42 AM, Steve Wise wrote:
> > > diff --git a/rdma/include/uapi/rdma/rdma_netlink.h
> b/rdma/include/uapi/rdma/rdma_netlink.h
> > > index 60416ed..40be0d8 100644
> > > --- a/rdma/include/uapi/rdma/rdma_netlink.h
> > > +++ b/rdma/include/uapi/rdma/rdma_netlink.h
> > > @@ -249,10 +249,22 @@ enum rdma_nldev_command {
> > > RDMA_NLDEV_NUM_OPS
> > > };
> > >
> > > +enum {
> > > + RDMA_NLDEV_ATTR_ENTRY_STRLEN = 16,
> > > +};
> > > +
> > > +enum rdma_nldev_print_type {
> > > + RDMA_NLDEV_PRINT_TYPE_UNSPEC,
> > > + RDMA_NLDEV_PRINT_TYPE_HEX,
> > > +};
> > > +
> > > enum rdma_nldev_attr {
> > > /* don't change the order or add anything between, this is ABI! */
> >
> > I asked this before and did not get a response. As the comment above
> > states with an emphasis (!) ...
Sorry David, I missed your question previously. ☹
> >
> > > RDMA_NLDEV_ATTR_UNSPEC,
> > >
> > > + /* Pad attribute for 64b alignment */
> > > + RDMA_NLDEV_ATTR_PAD = RDMA_NLDEV_ATTR_UNSPEC,
> > > +
> >
> > ... are you really adding new attributes in the middle?
>
> Not really. The new item is being explicitly set to the same value as
> the item above it. It therefore becomes two entries with the same enum
> value. The rest of the enum is all unchanged.
Correct.
The reason this was done was because a kernel had already been released where 64b nlattrs were being padded with 0 instead of defining an explicit pad attribute. Jason thought the kernel side should define an explicit PAD attribute and use it. To preserve the ABI we defined it but set it to 0 (aka ATTR_UNSPEC aka the first in the enum).
>
> > > /* Identifier for ib_device */
> > > RDMA_NLDEV_ATTR_DEV_INDEX, /* u32 */
> > >
> > > @@ -387,6 +399,20 @@ enum rdma_nldev_attr {
> > > RDMA_NLDEV_ATTR_RES_PD_ENTRY, /* nested table */
> > > RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY, /* u32 */
> > > RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY, /* u32 */
> > > + /*
> > > + * driver-specific attributes.
> > > + */
> > > + RDMA_NLDEV_ATTR_DRIVER, /* nested table */
> > > + RDMA_NLDEV_ATTR_DRIVER_ENTRY, /* nested table */
> > > + RDMA_NLDEV_ATTR_DRIVER_STRING, /* string */
> > > + /*
> > > + * u8 values from enum rdma_nldev_print_type
> > > + */
> > > + RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE, /* u8 */
> > > + RDMA_NLDEV_ATTR_DRIVER_S32, /* s32 */
> > > + RDMA_NLDEV_ATTR_DRIVER_U32, /* u32 */
> > > + RDMA_NLDEV_ATTR_DRIVER_S64, /* s64 */
> > > + RDMA_NLDEV_ATTR_DRIVER_U64, /* u64 */
> >
> > and again here.
> >
Ugh, this looks like a mistake maybe due to me rebasing and not noticing this commit added the name/index attrs.
5b2cc79de878 leonro@mellanox.com RDMA/nldev: Provide netdevice name and index
Both of these are in -next to be merged upstream together.
Should I do anything?
^ permalink raw reply
* Re: [RFC PATCH bpf-next 11/12] i40e: implement AF_XDP zero-copy support for Rx
From: Alexander Duyck @ 2018-05-15 20:25 UTC (permalink / raw)
To: Björn Töpel
Cc: magnus.karlsson, Karlsson, Magnus, Duyck, Alexander H,
John Fastabend, Alexei Starovoitov, Jesper Dangaard Brouer,
Willem de Bruijn, Daniel Borkmann, Michael S. Tsirkin, Netdev,
Björn Töpel, michael.lundkvist, Brandeburg, Jesse,
Anjali Singhai Jain, qi.z.zhang, intel-wired-lan
In-Reply-To: <20180515190615.23099-12-bjorn.topel@gmail.com>
On Tue, May 15, 2018 at 12:06 PM, Björn Töpel <bjorn.topel@gmail.com> wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> A lot of things here. First we add support for the new
> XDP_SETUP_XSK_UMEM command in ndo_bpf. This allows the AF_XDP socket
> to pass a UMEM to the driver. The driver will then DMA map all the
> frames in the UMEM for the driver. Next, the Rx code will allocate
> frames from the UMEM fill queue, instead of the regular page
> allocator.
>
> Externally, for the rest of the XDP code, the driver the driver
> internal UMEM allocator will appear as a MEM_TYPE_ZERO_COPY.
>
> Keep in mind that having frames coming from userland requires some
> extra care taken when passing them to the regular kernel stack. In
> these cases the ZC frame must be copied.
>
> The commit also introduces a completely new clean_rx_irq/allocator
> functions for zero-copy, and means (functions pointers) to set
> allocators and clean_rx functions.
>
> Finally, a lot of this are *not* implemented here. To mention some:
>
> * No passing to the stack via XDP_PASS (clone/copy to skb).
> * No XDP redirect to other than sockets (convert_to_xdp_frame does not
> clone the frame yet).
>
> And yes, too much C&P and too big commit. :-)
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
A few minor comments below.
> ---
> drivers/net/ethernet/intel/i40e/i40e.h | 20 ++
> drivers/net/ethernet/intel/i40e/i40e_main.c | 202 +++++++++++++-
> drivers/net/ethernet/intel/i40e/i40e_txrx.c | 400 ++++++++++++++++++++++++++--
> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 30 ++-
> 4 files changed, 619 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e.h b/drivers/net/ethernet/intel/i40e/i40e.h
> index 7a80652e2500..e6ee6c9bf094 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e.h
> @@ -786,6 +786,12 @@ struct i40e_vsi {
>
> /* VSI specific handlers */
> irqreturn_t (*irq_handler)(int irq, void *data);
> +
> + /* AF_XDP zero-copy */
> + struct xdp_umem **xsk_umems;
> + u16 num_xsk_umems_used;
> + u16 num_xsk_umems;
> +
> } ____cacheline_internodealigned_in_smp;
>
> struct i40e_netdev_priv {
> @@ -1090,6 +1096,20 @@ static inline bool i40e_enabled_xdp_vsi(struct i40e_vsi *vsi)
> return !!vsi->xdp_prog;
> }
>
> +static inline struct xdp_umem *i40e_xsk_umem(struct i40e_ring *ring)
> +{
> + bool xdp_on = i40e_enabled_xdp_vsi(ring->vsi);
> + int qid = ring->queue_index;
> +
> + if (ring_is_xdp(ring))
> + qid -= ring->vsi->alloc_queue_pairs;
> +
> + if (!ring->vsi->xsk_umems || !ring->vsi->xsk_umems[qid] || !xdp_on)
> + return NULL;
> +
> + return ring->vsi->xsk_umems[qid];
> +}
> +
> int i40e_create_queue_channel(struct i40e_vsi *vsi, struct i40e_channel *ch);
> int i40e_set_bw_limit(struct i40e_vsi *vsi, u16 seid, u64 max_tx_rate);
> int i40e_add_del_cloud_filter(struct i40e_vsi *vsi,
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index b4c23cf3979c..dc3d668a741e 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -5,6 +5,7 @@
> #include <linux/of_net.h>
> #include <linux/pci.h>
> #include <linux/bpf.h>
> +#include <net/xdp_sock.h>
>
> /* Local includes */
> #include "i40e.h"
> @@ -3054,6 +3055,9 @@ static int i40e_configure_tx_ring(struct i40e_ring *ring)
> i40e_status err = 0;
> u32 qtx_ctl = 0;
>
> + if (ring_is_xdp(ring))
> + ring->xsk_umem = i40e_xsk_umem(ring);
> +
> /* some ATR related tx ring init */
> if (vsi->back->flags & I40E_FLAG_FD_ATR_ENABLED) {
> ring->atr_sample_rate = vsi->back->atr_sample_rate;
> @@ -3163,13 +3167,31 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
> struct i40e_hw *hw = &vsi->back->hw;
> struct i40e_hmc_obj_rxq rx_ctx;
> i40e_status err = 0;
> + int ret;
>
> bitmap_zero(ring->state, __I40E_RING_STATE_NBITS);
>
> /* clear the context structure first */
> memset(&rx_ctx, 0, sizeof(rx_ctx));
>
> - ring->rx_buf_len = vsi->rx_buf_len;
> + ring->xsk_umem = i40e_xsk_umem(ring);
> + if (ring->xsk_umem) {
> + ring->clean_rx_irq = i40e_clean_rx_irq_zc;
> + ring->alloc_rx_buffers = i40e_alloc_rx_buffers_zc;
> + ring->rx_buf_len = ring->xsk_umem->props.frame_size -
> + ring->xsk_umem->frame_headroom -
> + XDP_PACKET_HEADROOM;
> + ring->zca.free = i40e_zca_free;
> + ret = xdp_rxq_info_reg_mem_model(&ring->xdp_rxq,
> + MEM_TYPE_ZERO_COPY,
> + &ring->zca);
> + if (ret)
> + return ret;
> + } else {
> + ring->clean_rx_irq = i40e_clean_rx_irq;
> + ring->alloc_rx_buffers = i40e_alloc_rx_buffers;
> + ring->rx_buf_len = vsi->rx_buf_len;
> + }
>
> rx_ctx.dbuff = DIV_ROUND_UP(ring->rx_buf_len,
> BIT_ULL(I40E_RXQ_CTX_DBUFF_SHIFT));
> @@ -3225,7 +3247,7 @@ static int i40e_configure_rx_ring(struct i40e_ring *ring)
> ring->tail = hw->hw_addr + I40E_QRX_TAIL(pf_q);
> writel(0, ring->tail);
>
> - i40e_alloc_rx_buffers(ring, I40E_DESC_UNUSED(ring));
> + ring->alloc_rx_buffers(ring, I40E_DESC_UNUSED(ring));
>
> return 0;
> }
> @@ -12050,6 +12072,179 @@ static int i40e_queue_pair_enable(struct i40e_vsi *vsi, int queue_pair)
> return err;
> }
>
> +static int i40e_alloc_xsk_umems(struct i40e_vsi *vsi)
> +{
> + if (vsi->xsk_umems)
> + return 0;
> +
> + vsi->num_xsk_umems_used = 0;
> + vsi->num_xsk_umems = vsi->alloc_queue_pairs;
> + vsi->xsk_umems = kcalloc(vsi->num_xsk_umems, sizeof(*vsi->xsk_umems),
> + GFP_KERNEL);
> + if (!vsi->xsk_umems) {
> + vsi->num_xsk_umems = 0;
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static int i40e_add_xsk_umem(struct i40e_vsi *vsi, struct xdp_umem *umem,
> + u16 qid)
> +{
> + int err;
> +
> + err = i40e_alloc_xsk_umems(vsi);
> + if (err)
> + return err;
> +
> + vsi->xsk_umems[qid] = umem;
> + vsi->num_xsk_umems_used++;
> +
> + return 0;
> +}
> +
> +static void i40e_remove_xsk_umem(struct i40e_vsi *vsi, u16 qid)
> +{
> + vsi->xsk_umems[qid] = NULL;
> + vsi->num_xsk_umems_used--;
> +
> + if (vsi->num_xsk_umems == 0) {
> + kfree(vsi->xsk_umems);
> + vsi->xsk_umems = NULL;
> + vsi->num_xsk_umems = 0;
> + }
> +}
> +
> +static int i40e_xsk_umem_dma_map(struct i40e_vsi *vsi, struct xdp_umem *umem)
> +{
> + struct i40e_pf *pf = vsi->back;
> + struct device *dev;
> + unsigned int i, j;
> + dma_addr_t dma;
> +
> + dev = &pf->pdev->dev;
> +
> + for (i = 0; i < umem->props.nframes; i++) {
> + dma = dma_map_single_attrs(dev, umem->frames[i].addr,
> + umem->props.frame_size,
> + DMA_BIDIRECTIONAL, I40E_RX_DMA_ATTR);
> + if (dma_mapping_error(dev, dma))
> + goto out_unmap;
> +
> + umem->frames[i].dma = dma;
> + }
> +
> + return 0;
> +
> +out_unmap:
> + for (j = 0; j < i; j++) {
> + dma_unmap_single_attrs(dev, umem->frames[i].dma,
> + umem->props.frame_size,
> + DMA_BIDIRECTIONAL,
> + I40E_RX_DMA_ATTR);
> + umem->frames[i].dma = 0;
> + }
> +
> + return -1;
> +}
> +
> +static void i40e_xsk_umem_dma_unmap(struct i40e_vsi *vsi, struct xdp_umem *umem)
> +{
> + struct i40e_pf *pf = vsi->back;
> + struct device *dev;
> + unsigned int i;
> +
> + dev = &pf->pdev->dev;
> +
> + for (i = 0; i < umem->props.nframes; i++) {
> + dma_unmap_single_attrs(dev, umem->frames[i].dma,
> + umem->props.frame_size,
> + DMA_BIDIRECTIONAL,
> + I40E_RX_DMA_ATTR);
> +
> + umem->frames[i].dma = 0;
> + }
> +}
> +
> +static int i40e_xsk_umem_enable(struct i40e_vsi *vsi, struct xdp_umem *umem,
> + u16 qid)
> +{
> + bool if_running;
> + int err;
> +
> + if (vsi->type != I40E_VSI_MAIN)
> + return -EINVAL;
> +
> + if (qid >= vsi->num_queue_pairs)
> + return -EINVAL;
> +
> + if (vsi->xsk_umems && vsi->xsk_umems[qid])
> + return -EBUSY;
> +
> + err = i40e_xsk_umem_dma_map(vsi, umem);
> + if (err)
> + return err;
> +
> + if_running = netif_running(vsi->netdev) && i40e_enabled_xdp_vsi(vsi);
> +
> + if (if_running) {
> + err = i40e_queue_pair_disable(vsi, qid);
> + if (err)
> + return err;
> + }
> +
> + err = i40e_add_xsk_umem(vsi, umem, qid);
> + if (err)
> + return err;
> +
> + if (if_running) {
> + err = i40e_queue_pair_enable(vsi, qid);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int i40e_xsk_umem_disable(struct i40e_vsi *vsi, u16 qid)
> +{
> + bool if_running;
> + int err;
> +
> + if (!vsi->xsk_umems || qid >= vsi->num_xsk_umems ||
> + !vsi->xsk_umems[qid])
> + return -EINVAL;
> +
> + if_running = netif_running(vsi->netdev) && i40e_enabled_xdp_vsi(vsi);
> +
> + if (if_running) {
> + err = i40e_queue_pair_disable(vsi, qid);
> + if (err)
> + return err;
> + }
> +
> + i40e_xsk_umem_dma_unmap(vsi, vsi->xsk_umems[qid]);
> + i40e_remove_xsk_umem(vsi, qid);
> +
> + if (if_running) {
> + err = i40e_queue_pair_enable(vsi, qid);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int i40e_xsk_umem_setup(struct i40e_vsi *vsi, struct xdp_umem *umem,
> + u16 qid)
> +{
> + if (umem)
> + return i40e_xsk_umem_enable(vsi, umem, qid);
> +
> + return i40e_xsk_umem_disable(vsi, qid);
> +}
> +
> /**
> * i40e_xdp - implements ndo_bpf for i40e
> * @dev: netdevice
> @@ -12071,6 +12266,9 @@ static int i40e_xdp(struct net_device *dev,
> xdp->prog_attached = i40e_enabled_xdp_vsi(vsi);
> xdp->prog_id = vsi->xdp_prog ? vsi->xdp_prog->aux->id : 0;
> return 0;
> + case XDP_SETUP_XSK_UMEM:
> + return i40e_xsk_umem_setup(vsi, xdp->xsk.umem,
> + xdp->xsk.queue_id);
> default:
> return -EINVAL;
> }
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 5efa68de935b..f89ac524652c 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -5,6 +5,7 @@
> #include <net/busy_poll.h>
> #include <linux/bpf_trace.h>
> #include <net/xdp.h>
> +#include <net/xdp_sock.h>
> #include "i40e.h"
> #include "i40e_trace.h"
> #include "i40e_prototype.h"
> @@ -1373,31 +1374,35 @@ void i40e_clean_rx_ring(struct i40e_ring *rx_ring)
> }
>
> /* Free all the Rx ring sk_buffs */
> - for (i = 0; i < rx_ring->count; i++) {
> - struct i40e_rx_buffer *rx_bi = &rx_ring->rx_bi[i];
> + if (!rx_ring->xsk_umem) {
> + for (i = 0; i < rx_ring->count; i++) {
I'm not a fan of all this extra indenting. This could be much more
easily handled with just a goto and a label.
> + struct i40e_rx_buffer *rx_bi = &rx_ring->rx_bi[i];
>
> - if (!rx_bi->page)
> - continue;
> -
> - /* Invalidate cache lines that may have been written to by
> - * device so that we avoid corrupting memory.
> - */
> - dma_sync_single_range_for_cpu(rx_ring->dev,
> - rx_bi->dma,
> - rx_bi->page_offset,
> - rx_ring->rx_buf_len,
> - DMA_FROM_DEVICE);
> -
> - /* free resources associated with mapping */
> - dma_unmap_page_attrs(rx_ring->dev, rx_bi->dma,
> - i40e_rx_pg_size(rx_ring),
> - DMA_FROM_DEVICE,
> - I40E_RX_DMA_ATTR);
> -
> - __page_frag_cache_drain(rx_bi->page, rx_bi->pagecnt_bias);
> + if (!rx_bi->page)
> + continue;
>
> - rx_bi->page = NULL;
> - rx_bi->page_offset = 0;
> + /* Invalidate cache lines that may have been
> + * written to by device so that we avoid
> + * corrupting memory.
> + */
> + dma_sync_single_range_for_cpu(rx_ring->dev,
> + rx_bi->dma,
> + rx_bi->page_offset,
> + rx_ring->rx_buf_len,
> + DMA_FROM_DEVICE);
> +
> + /* free resources associated with mapping */
> + dma_unmap_page_attrs(rx_ring->dev, rx_bi->dma,
> + i40e_rx_pg_size(rx_ring),
> + DMA_FROM_DEVICE,
> + I40E_RX_DMA_ATTR);
> +
> + __page_frag_cache_drain(rx_bi->page,
> + rx_bi->pagecnt_bias);
> +
> + rx_bi->page = NULL;
> + rx_bi->page_offset = 0;
> + }
> }
>
> bi_size = sizeof(struct i40e_rx_buffer) * rx_ring->count;
> @@ -2214,8 +2219,6 @@ static struct sk_buff *i40e_run_xdp(struct i40e_ring *rx_ring,
> if (!xdp_prog)
> goto xdp_out;
>
> - prefetchw(xdp->data_hard_start); /* xdp_frame write */
> -
> act = bpf_prog_run_xdp(xdp_prog, xdp);
> switch (act) {
> case XDP_PASS:
> @@ -2284,7 +2287,7 @@ static inline void i40e_xdp_ring_update_tail(struct i40e_ring *xdp_ring)
> *
> * Returns amount of work completed
> **/
> -static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
> +int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
> {
> unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> struct sk_buff *skb = rx_ring->skb;
> @@ -2426,6 +2429,349 @@ static int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget)
> return failure ? budget : (int)total_rx_packets;
> }
>
How much of the code below is actually reused anywhere else? I would
almost be inclined to say that maybe the zero-copy path should be
moved to a new file since so much of this is being duplicated from the
original tx/rx code path. I can easily see this becoming confusing as
to which is which when a bug gets found and needs to be fixed.
> +static struct sk_buff *i40e_run_xdp_zc(struct i40e_ring *rx_ring,
> + struct xdp_buff *xdp)
> +{
> + int err, result = I40E_XDP_PASS;
> + struct i40e_ring *xdp_ring;
> + struct bpf_prog *xdp_prog;
> + u32 act;
> +
> + rcu_read_lock();
> + xdp_prog = READ_ONCE(rx_ring->xdp_prog);
> +
> + act = bpf_prog_run_xdp(xdp_prog, xdp);
> + switch (act) {
> + case XDP_PASS:
> + break;
> + case XDP_TX:
> + xdp_ring = rx_ring->vsi->xdp_rings[rx_ring->queue_index];
> + result = i40e_xmit_xdp_tx_ring(xdp, xdp_ring);
> + break;
> + case XDP_REDIRECT:
> + err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog);
> + result = !err ? I40E_XDP_TX : I40E_XDP_CONSUMED;
> + break;
> + default:
> + bpf_warn_invalid_xdp_action(act);
> + case XDP_ABORTED:
> + trace_xdp_exception(rx_ring->netdev, xdp_prog, act);
> + /* fallthrough -- handle aborts by dropping packet */
> + case XDP_DROP:
> + result = I40E_XDP_CONSUMED;
> + break;
> + }
> +
> + rcu_read_unlock();
> + return ERR_PTR(-result);
> +}
> +
> +static bool i40e_alloc_frame_zc(struct i40e_ring *rx_ring,
> + struct i40e_rx_buffer *bi)
> +{
> + struct xdp_umem *umem = rx_ring->xsk_umem;
> + void *addr = bi->addr;
> + u32 *id;
> +
> + if (addr) {
> + rx_ring->rx_stats.page_reuse_count++;
> + return true;
> + }
> +
> + id = xsk_umem_peek_id(umem);
> + if (unlikely(!id)) {
> + rx_ring->rx_stats.alloc_page_failed++;
> + return false;
> + }
> +
> + bi->dma = umem->frames[*id].dma + umem->frame_headroom +
> + XDP_PACKET_HEADROOM;
> + bi->addr = umem->frames[*id].addr + umem->frame_headroom +
> + XDP_PACKET_HEADROOM;
> + bi->id = *id;
> +
> + xsk_umem_discard_id(umem);
> + return true;
> +}
> +
> +bool i40e_alloc_rx_buffers_zc(struct i40e_ring *rx_ring, u16 cleaned_count)
> +{
> + u16 ntu = rx_ring->next_to_use;
> + union i40e_rx_desc *rx_desc;
> + struct i40e_rx_buffer *bi;
> +
> + rx_desc = I40E_RX_DESC(rx_ring, ntu);
> + bi = &rx_ring->rx_bi[ntu];
> +
> + do {
> + if (!i40e_alloc_frame_zc(rx_ring, bi))
> + goto no_buffers;
> +
> + /* sync the buffer for use by the device */
> + dma_sync_single_range_for_device(rx_ring->dev, bi->dma, 0,
> + rx_ring->rx_buf_len,
> + DMA_BIDIRECTIONAL);
> +
> + /* Refresh the desc even if buffer_addrs didn't change
> + * because each write-back erases this info.
> + */
> + rx_desc->read.pkt_addr = cpu_to_le64(bi->dma);
> +
> + rx_desc++;
> + bi++;
> + ntu++;
> + if (unlikely(ntu == rx_ring->count)) {
> + rx_desc = I40E_RX_DESC(rx_ring, 0);
> + bi = rx_ring->rx_bi;
> + ntu = 0;
> + }
> +
> + /* clear the status bits for the next_to_use descriptor */
> + rx_desc->wb.qword1.status_error_len = 0;
> +
> + cleaned_count--;
> + } while (cleaned_count);
> +
> + if (rx_ring->next_to_use != ntu)
> + i40e_release_rx_desc(rx_ring, ntu);
> +
> + return false;
> +
> +no_buffers:
> + if (rx_ring->next_to_use != ntu)
> + i40e_release_rx_desc(rx_ring, ntu);
> +
> + /* make sure to come back via polling to try again after
> + * allocation failure
> + */
> + return true;
> +}
> +
> +static struct i40e_rx_buffer *i40e_get_rx_buffer_zc(struct i40e_ring *rx_ring,
> + const unsigned int size)
> +{
> + struct i40e_rx_buffer *rx_buffer;
> +
> + rx_buffer = &rx_ring->rx_bi[rx_ring->next_to_clean];
> +
> + /* we are reusing so sync this buffer for CPU use */
> + dma_sync_single_range_for_cpu(rx_ring->dev,
> + rx_buffer->dma, 0,
> + size,
> + DMA_BIDIRECTIONAL);
> +
> + return rx_buffer;
> +}
> +
> +static void i40e_reuse_rx_buffer_zc(struct i40e_ring *rx_ring,
> + struct i40e_rx_buffer *old_buff)
> +{
> + struct i40e_rx_buffer *new_buff;
> + u16 nta = rx_ring->next_to_alloc;
> +
> + new_buff = &rx_ring->rx_bi[nta];
> +
> + /* update, and store next to alloc */
> + nta++;
> + rx_ring->next_to_alloc = (nta < rx_ring->count) ? nta : 0;
> +
> + /* transfer page from old buffer to new buffer */
> + new_buff->dma = old_buff->dma;
> + new_buff->addr = old_buff->addr;
> + new_buff->id = old_buff->id;
> +}
> +
> +/* Called from the XDP return API in NAPI context. */
> +void i40e_zca_free(struct zero_copy_allocator *alloc, unsigned long handle)
> +{
> + struct i40e_rx_buffer *new_buff;
> + struct i40e_ring *rx_ring;
> + u16 nta;
> +
> + rx_ring = container_of(alloc, struct i40e_ring, zca);
> + nta = rx_ring->next_to_alloc;
> +
> + new_buff = &rx_ring->rx_bi[nta];
> +
> + /* update, and store next to alloc */
> + nta++;
> + rx_ring->next_to_alloc = (nta < rx_ring->count) ? nta : 0;
> +
> + new_buff->dma = rx_ring->xsk_umem->frames[handle].dma;
> + new_buff->addr = rx_ring->xsk_umem->frames[handle].addr;
> + new_buff->id = (u32)handle;
> +}
> +
> +static struct sk_buff *i40e_zc_frame_to_skb(struct i40e_ring *rx_ring,
> + struct i40e_rx_buffer *rx_buffer,
> + struct xdp_buff *xdp)
> +{
> + // XXX implement alloc skb and copy
> + i40e_reuse_rx_buffer_zc(rx_ring, rx_buffer);
> + return NULL;
> +}
> +
> +static void i40e_clean_programming_status_zc(struct i40e_ring *rx_ring,
> + union i40e_rx_desc *rx_desc,
> + u64 qw)
> +{
> + struct i40e_rx_buffer *rx_buffer;
> + u32 ntc = rx_ring->next_to_clean;
> + u8 id;
> +
> + /* fetch, update, and store next to clean */
> + rx_buffer = &rx_ring->rx_bi[ntc++];
> + ntc = (ntc < rx_ring->count) ? ntc : 0;
> + rx_ring->next_to_clean = ntc;
> +
> + prefetch(I40E_RX_DESC(rx_ring, ntc));
> +
> + /* place unused page back on the ring */
> + i40e_reuse_rx_buffer_zc(rx_ring, rx_buffer);
> + rx_ring->rx_stats.page_reuse_count++;
> +
> + /* clear contents of buffer_info */
> + rx_buffer->addr = NULL;
> +
> + id = (qw & I40E_RX_PROG_STATUS_DESC_QW1_PROGID_MASK) >>
> + I40E_RX_PROG_STATUS_DESC_QW1_PROGID_SHIFT;
> +
> + if (id == I40E_RX_PROG_STATUS_DESC_FD_FILTER_STATUS)
> + i40e_fd_handle_status(rx_ring, rx_desc, id);
> +}
> +
> +int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> +{
> + unsigned int total_rx_bytes = 0, total_rx_packets = 0;
> + u16 cleaned_count = I40E_DESC_UNUSED(rx_ring);
> + bool failure = false, xdp_xmit = false;
> + struct sk_buff *skb;
> + struct xdp_buff xdp;
> +
> + xdp.rxq = &rx_ring->xdp_rxq;
> +
> + while (likely(total_rx_packets < (unsigned int)budget)) {
> + struct i40e_rx_buffer *rx_buffer;
> + union i40e_rx_desc *rx_desc;
> + unsigned int size;
> + u16 vlan_tag;
> + u8 rx_ptype;
> + u64 qword;
> + u32 ntc;
> +
> + /* return some buffers to hardware, one at a time is too slow */
> + if (cleaned_count >= I40E_RX_BUFFER_WRITE) {
> + failure = failure ||
> + i40e_alloc_rx_buffers_zc(rx_ring,
> + cleaned_count);
> + cleaned_count = 0;
> + }
> +
> + rx_desc = I40E_RX_DESC(rx_ring, rx_ring->next_to_clean);
> +
> + /* status_error_len will always be zero for unused descriptors
> + * because it's cleared in cleanup, and overlaps with hdr_addr
> + * which is always zero because packet split isn't used, if the
> + * hardware wrote DD then the length will be non-zero
> + */
> + qword = le64_to_cpu(rx_desc->wb.qword1.status_error_len);
> +
> + /* This memory barrier is needed to keep us from reading
> + * any other fields out of the rx_desc until we have
> + * verified the descriptor has been written back.
> + */
> + dma_rmb();
> +
> + if (unlikely(i40e_rx_is_programming_status(qword))) {
> + i40e_clean_programming_status_zc(rx_ring, rx_desc,
> + qword);
> + cleaned_count++;
> + continue;
> + }
> + size = (qword & I40E_RXD_QW1_LENGTH_PBUF_MASK) >>
> + I40E_RXD_QW1_LENGTH_PBUF_SHIFT;
> + if (!size)
> + break;
> +
> + rx_buffer = i40e_get_rx_buffer_zc(rx_ring, size);
> +
> + /* retrieve a buffer from the ring */
> + xdp.data = rx_buffer->addr;
> + xdp_set_data_meta_invalid(&xdp);
> + xdp.data_hard_start = xdp.data - XDP_PACKET_HEADROOM;
> + xdp.data_end = xdp.data + size;
> + xdp.handle = rx_buffer->id;
> +
> + skb = i40e_run_xdp_zc(rx_ring, &xdp);
> +
> + if (IS_ERR(skb)) {
> + if (PTR_ERR(skb) == -I40E_XDP_TX)
> + xdp_xmit = true;
> + else
> + i40e_reuse_rx_buffer_zc(rx_ring, rx_buffer);
> + total_rx_bytes += size;
> + total_rx_packets++;
> + } else {
> + skb = i40e_zc_frame_to_skb(rx_ring, rx_buffer, &xdp);
> + if (!skb) {
> + rx_ring->rx_stats.alloc_buff_failed++;
> + break;
> + }
> + }
> +
> + rx_buffer->addr = NULL;
> + cleaned_count++;
> +
> + /* don't care about non-EOP frames in XDP mode */
> + ntc = rx_ring->next_to_clean + 1;
> + ntc = (ntc < rx_ring->count) ? ntc : 0;
> + rx_ring->next_to_clean = ntc;
> + prefetch(I40E_RX_DESC(rx_ring, ntc));
> +
> + if (i40e_cleanup_headers(rx_ring, skb, rx_desc)) {
> + skb = NULL;
> + continue;
> + }
> +
> + /* probably a little skewed due to removing CRC */
> + total_rx_bytes += skb->len;
> +
> + qword = le64_to_cpu(rx_desc->wb.qword1.status_error_len);
> + rx_ptype = (qword & I40E_RXD_QW1_PTYPE_MASK) >>
> + I40E_RXD_QW1_PTYPE_SHIFT;
> +
> + /* populate checksum, VLAN, and protocol */
> + i40e_process_skb_fields(rx_ring, rx_desc, skb, rx_ptype);
> +
> + vlan_tag = (qword & BIT(I40E_RX_DESC_STATUS_L2TAG1P_SHIFT)) ?
> + le16_to_cpu(rx_desc->wb.qword0.lo_dword.l2tag1) : 0;
> +
> + i40e_receive_skb(rx_ring, skb, vlan_tag);
> + skb = NULL;
> +
> + /* update budget accounting */
> + total_rx_packets++;
> + }
> +
> + if (xdp_xmit) {
> + struct i40e_ring *xdp_ring =
> + rx_ring->vsi->xdp_rings[rx_ring->queue_index];
> +
> + i40e_xdp_ring_update_tail(xdp_ring);
> + xdp_do_flush_map();
> + }
> +
> + u64_stats_update_begin(&rx_ring->syncp);
> + rx_ring->stats.packets += total_rx_packets;
> + rx_ring->stats.bytes += total_rx_bytes;
> + u64_stats_update_end(&rx_ring->syncp);
> + rx_ring->q_vector->rx.total_packets += total_rx_packets;
> + rx_ring->q_vector->rx.total_bytes += total_rx_bytes;
> +
> + /* guarantee a trip back through this routine if there was a failure */
> + return failure ? budget : (int)total_rx_packets;
> +}
> +
> static inline u32 i40e_buildreg_itr(const int type, u16 itr)
> {
> u32 val;
> @@ -2576,7 +2922,7 @@ int i40e_napi_poll(struct napi_struct *napi, int budget)
> budget_per_ring = max(budget/q_vector->num_ringpairs, 1);
>
> i40e_for_each_ring(ring, q_vector->rx) {
> - int cleaned = i40e_clean_rx_irq(ring, budget_per_ring);
> + int cleaned = ring->clean_rx_irq(ring, budget_per_ring);
>
> work_done += cleaned;
> /* if we clean as many as budgeted, we must not be done */
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> index fdd2c55f03a6..9d5d9862e9f1 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
> @@ -296,13 +296,22 @@ struct i40e_tx_buffer {
>
> struct i40e_rx_buffer {
> dma_addr_t dma;
> - struct page *page;
> + union {
> + struct {
> + struct page *page;
> #if (BITS_PER_LONG > 32) || (PAGE_SIZE >= 65536)
> - __u32 page_offset;
> + __u32 page_offset;
> #else
> - __u16 page_offset;
> + __u16 page_offset;
> #endif
> - __u16 pagecnt_bias;
> + __u16 pagecnt_bias;
> + };
> + struct {
> + /* for umem */
> + void *addr;
> + u32 id;
> + };
> + };
> };
>
> struct i40e_queue_stats {
> @@ -344,6 +353,8 @@ enum i40e_ring_state_t {
> #define I40E_RX_SPLIT_TCP_UDP 0x4
> #define I40E_RX_SPLIT_SCTP 0x8
>
> +void i40e_zc_recycle(struct zero_copy_allocator *alloc, unsigned long handle);
> +
> /* struct that defines a descriptor ring, associated with a VSI */
> struct i40e_ring {
> struct i40e_ring *next; /* pointer to next ring in q_vector */
> @@ -414,6 +425,12 @@ struct i40e_ring {
>
> struct i40e_channel *ch;
> struct xdp_rxq_info xdp_rxq;
> +
> + int (*clean_rx_irq)(struct i40e_ring *, int);
> + bool (*alloc_rx_buffers)(struct i40e_ring *, u16);
> + struct xdp_umem *xsk_umem;
> +
> + struct zero_copy_allocator zca; /* ZC allocator anchor */
> } ____cacheline_internodealigned_in_smp;
>
> static inline bool ring_uses_build_skb(struct i40e_ring *ring)
> @@ -474,6 +491,7 @@ static inline unsigned int i40e_rx_pg_order(struct i40e_ring *ring)
> #define i40e_rx_pg_size(_ring) (PAGE_SIZE << i40e_rx_pg_order(_ring))
>
> bool i40e_alloc_rx_buffers(struct i40e_ring *rxr, u16 cleaned_count);
> +bool i40e_alloc_rx_buffers_zc(struct i40e_ring *rx_ring, u16 cleaned_count);
> netdev_tx_t i40e_lan_xmit_frame(struct sk_buff *skb, struct net_device *netdev);
> void i40e_clean_tx_ring(struct i40e_ring *tx_ring);
> void i40e_clean_rx_ring(struct i40e_ring *rx_ring);
> @@ -489,6 +507,9 @@ int __i40e_maybe_stop_tx(struct i40e_ring *tx_ring, int size);
> bool __i40e_chk_linearize(struct sk_buff *skb);
> int i40e_xdp_xmit(struct net_device *dev, struct xdp_frame *xdpf);
> void i40e_xdp_flush(struct net_device *dev);
> +int i40e_clean_rx_irq(struct i40e_ring *rx_ring, int budget);
> +int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget);
> +void i40e_zca_free(struct zero_copy_allocator *alloc, unsigned long handle);
>
> /**
> * i40e_get_head - Retrieve head from head writeback
> @@ -575,4 +596,5 @@ static inline struct netdev_queue *txring_txq(const struct i40e_ring *ring)
> {
> return netdev_get_tx_queue(ring->netdev, ring->queue_index);
> }
> +
> #endif /* _I40E_TXRX_H_ */
> --
> 2.14.1
>
^ permalink raw reply
* Re: [PATCH net-next 2/2] pfifo_fast: drop unneeded additional lock on dequeue
From: Michael S. Tsirkin @ 2018-05-15 20:17 UTC (permalink / raw)
To: Paolo Abeni
Cc: netdev, David S. Miller, Jamal Hadi Salim, Cong Wang, Jiri Pirko,
John Fastabend
In-Reply-To: <8a1740148995663939837bedb14f29716c7cf6f5.1526392746.git.pabeni@redhat.com>
On Tue, May 15, 2018 at 04:24:37PM +0200, Paolo Abeni wrote:
> After the previous patch, for NOLOCK qdiscs, q->seqlock is
> always held when the dequeue() is invoked, we can drop
> any additional locking to protect such operation.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> include/linux/skb_array.h | 5 +++++
> net/sched/sch_generic.c | 4 ++--
> 2 files changed, 7 insertions(+), 2 deletions(-)
Is the seqlock taken during qdisc_change_tx_queue_len?
We need to prevent that racing with dequeue.
> diff --git a/include/linux/skb_array.h b/include/linux/skb_array.h
> index a6b6e8bb3d7b..62d9b0a6329f 100644
> --- a/include/linux/skb_array.h
> +++ b/include/linux/skb_array.h
> @@ -97,6 +97,11 @@ static inline bool skb_array_empty_any(struct skb_array *a)
> return ptr_ring_empty_any(&a->ring);
> }
>
> +static inline struct sk_buff *__skb_array_consume(struct skb_array *a)
> +{
> + return __ptr_ring_consume(&a->ring);
> +}
> +
> static inline struct sk_buff *skb_array_consume(struct skb_array *a)
> {
> return ptr_ring_consume(&a->ring);
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index a126f16bc30b..760ab1b09f8b 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -656,7 +656,7 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
> if (__skb_array_empty(q))
> continue;
>
> - skb = skb_array_consume_bh(q);
> + skb = __skb_array_consume(q);
> }
> if (likely(skb)) {
> qdisc_qstats_cpu_backlog_dec(qdisc, skb);
> @@ -697,7 +697,7 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
> if (!q->ring.queue)
> continue;
>
> - while ((skb = skb_array_consume_bh(q)) != NULL)
> + while ((skb = __skb_array_consume(q)) != NULL)
> kfree_skb(skb);
> }
>
> --
> 2.14.3
^ permalink raw reply
* i40e - Is i40e_force_link_state doing the right thing ?
From: Chaitanya Lala @ 2018-05-15 20:15 UTC (permalink / raw)
To: netdev, mariusz.stachura, mitch.a.williams, andrewx.bowers,
jeffrey.t.kirsher
Hi,
I am trying to bring up a Intel XL710 4x10G Intel card using the
latest mainline top-of-tree.
The problem is that "ifconfig up" and "ifconfig down" do not take
effect at the link state level.
I tracked the problem down to i40e_force_link_state() when it is
called from i40e_down().
It calls i40e_force_link_state with "is_up" == false. In-turn it
calls, i40e_aq_set_link_restart_an(hw, true, NULL).
Should the second argument of i40e_aq_set_link_restart_an be "is_up"
vs the current "true"
i.e. i40e_aq_set_link_restart_an(hw, is_up, NULL). ? When I make this
change, the link state syncs-up with
the interface administrative state.
Is this a bug ?
Thanks,
Chaitanya
^ permalink raw reply
* Re: [net 1/1] net/mlx5: Fix build break when CONFIG_SMP=n
From: Guenter Roeck @ 2018-05-15 20:11 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: David S. Miller, netdev, Randy Dunlap, Thomas Gleixner
In-Reply-To: <20180514223810.21197-1-saeedm@mellanox.com>
On Mon, May 14, 2018 at 03:38:10PM -0700, Saeed Mahameed wrote:
> Avoid using the kernel's irq_descriptor and return IRQ vector affinity
> directly from the driver.
>
> This fixes the following build break when CONFIG_SMP=n
>
> include/linux/mlx5/driver.h: In function ‘mlx5_get_vector_affinity_hint’:
> include/linux/mlx5/driver.h:1299:13: error:
> ‘struct irq_desc’ has no member named ‘affinity_hint’
>
> Fixes: 6082d9c9c94a ("net/mlx5: Fix mlx5_get_vector_affinity function")
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> CC: Randy Dunlap <rdunlap@infradead.org>
> CC: Guenter Roeck <linux@roeck-us.net>
> CC: Thomas Gleixner <tglx@linutronix.de>
> Tested-by: Israel Rukshin <israelr@mellanox.com>
Tested-by: Guenter Roeck <linux@roeck-us.net>
> ---
>
> For -stable v4.14
>
> include/linux/mlx5/driver.h | 12 +-----------
> 1 file changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
> index 2a156c5dfadd..d703774982ca 100644
> --- a/include/linux/mlx5/driver.h
> +++ b/include/linux/mlx5/driver.h
> @@ -1286,17 +1286,7 @@ enum {
> static inline const struct cpumask *
> mlx5_get_vector_affinity_hint(struct mlx5_core_dev *dev, int vector)
> {
> - struct irq_desc *desc;
> - unsigned int irq;
> - int eqn;
> - int err;
> -
> - err = mlx5_vector2eqn(dev, vector, &eqn, &irq);
> - if (err)
> - return NULL;
> -
> - desc = irq_to_desc(irq);
> - return desc->affinity_hint;
> + return dev->priv.irq_info[vector].mask;
> }
>
> #endif /* MLX5_DRIVER_H */
> --
> 2.17.0
>
^ permalink raw reply
* Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather
From: Willem de Bruijn @ 2018-05-15 20:04 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Network Development, David Miller, Willem de Bruijn
In-Reply-To: <CAF=yD-KUgKoaV-CXcZpRLBVPykkpGmBAm59C=+OWFq4wd6g5xg@mail.gmail.com>
On Tue, May 15, 2018 at 10:14 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Mon, May 14, 2018 at 7:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>>
>>
>> On 05/14/2018 04:30 PM, Willem de Bruijn wrote:
>>
>>> I don't quite follow. The reported crash happens in the protocol layer,
>>> because of this check. With pagedlen we have not allocated
>>> sufficient space for the skb_put.
>>>
>>> if (!(rt->dst.dev->features&NETIF_F_SG)) {
>>> unsigned int off;
>>>
>>> off = skb->len;
>>> if (getfrag(from, skb_put(skb, copy),
>>> offset, copy, off, skb) < 0) {
>>> __skb_trim(skb, off);
>>> err = -EFAULT;
>>> goto error;
>>> }
>>> } else {
>>> int i = skb_shinfo(skb)->nr_frags;
>>>
>>> Are you referring to a separate potential issue in the gso layer?
>>> If a bonding device advertises SG, but a slave does not, then
>>> skb_segment on the slave should build linear segs? I have not
>>> tested that.
>>
>> Given that the device attribute could change under us, we need to not
>> crash, even if initially we thought NETIF_F_SG was available.
>>
>> Unless you want to hold RTNL in UDP xmit :)
>>
>> Ideally, GSO should be always on, as we did for TCP.
>>
>> Otherwise, I can guarantee syzkaller will hit again.
>
> Ah, right. Thanks, Eric!
>
> I'll read that feature bit only once.
This issue is actually deeper and not specific to gso.
With corking it is trivial to turn off sg in between calls.
I'll need to send a separate fix for that.
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to get new driver attributes
From: Doug Ledford @ 2018-05-15 20:05 UTC (permalink / raw)
To: David Ahern, Steve Wise, leon; +Cc: stephen, netdev, linux-rdma
In-Reply-To: <fd64ce39-a284-b09f-85b0-8cd7ba0d1207@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 739 bytes --]
On Tue, 2018-05-15 at 14:01 -0600, David Ahern wrote:
> On 5/15/18 2:00 PM, Doug Ledford wrote:
> > I just sent an incremental fix to the list under separate cover. You
> > can squash that fix into Steve's patch and it should resolve the issue.
> > Or Steve can respin the set. Either way.
>
> Once the patch has been committed to the upstream repo, spin a new set
> with the correct header update.
Well, I wrote it, so it's in my repo now ;-). I just sent it to the
list because that's what you do when you are writing fixup patches even
if you've already committed them.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to get new driver attributes
From: David Ahern @ 2018-05-15 20:01 UTC (permalink / raw)
To: Doug Ledford, Steve Wise, leon; +Cc: stephen, netdev, linux-rdma
In-Reply-To: <d4a8a280c94dd8db969bc52abe057af1fde89a4b.camel@redhat.com>
On 5/15/18 2:00 PM, Doug Ledford wrote:
> I just sent an incremental fix to the list under separate cover. You
> can squash that fix into Steve's patch and it should resolve the issue.
> Or Steve can respin the set. Either way.
Once the patch has been committed to the upstream repo, spin a new set
with the correct header update.
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to get new driver attributes
From: Doug Ledford @ 2018-05-15 20:00 UTC (permalink / raw)
To: David Ahern, Steve Wise, leon; +Cc: stephen, netdev, linux-rdma
In-Reply-To: <c9d9c8dd-682e-ab38-a3af-6159adbebba7@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 836 bytes --]
On Tue, 2018-05-15 at 13:53 -0600, David Ahern wrote:
> sure for the above, but not the ones below.
>
> Before this patch, this program:
> $ cat a.c
> #include <uapi/rdma/rdma_netlink.h>
> #include <stdio.h>
>
> int main(void)
> {
> printf("RDMA_NLDEV_ATTR_NDEV_INDEX = %d\n", RDMA_NLDEV_ATTR_NDEV_INDEX);
>
> return 0;
> }
>
> prints this:
> $ ./a.out
> RDMA_NLDEV_ATTR_NDEV_INDEX = 50
>
> After this patch,
> $ ./a.out
> RDMA_NLDEV_ATTR_NDEV_INDEX = 58
I just sent an incremental fix to the list under separate cover. You
can squash that fix into Steve's patch and it should resolve the issue.
Or Steve can respin the set. Either way.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: ASSALAMUALAIKUM.
From: MRS NUBIA MUHAMMAD @ 2018-05-15 19:57 UTC (permalink / raw)
Dearest,
Greetings in the name of Allah,please let this not sound strange to
you for my only surviving lawyer who would have done this died early
this year.I prayed and got your email id from your country guestbook.I
am Mrs Nubia Muhammad from Egypt,I am 72 year old,i am suffering from
a long time cancer of the lungs,from all indication my conditions is
really deteriorating and it is quite obvious that,according to my
doctors they have advised me that i may not live for more than two
months,this is because the cancer stage has gotten to a very bad
stage.I am married to Late (Muhammad Essem Shafik) who was former
Embassy of the Arab Republic of Egypt in South Africa for nine
years,Before he died in 2005.
I married to my late husband for Thirty years without a child,my
husband died in a fatal motor accident Before his death we were truely
Islamic believers.Since his death I decided not to re-marry,I sold all
my inherited belongings and deposited all the sum of £5.2 Millions GBP
Pounds with Bank in South Africa.Though what disturbs me mostly is the
cancer.Having known my condition I decided to donate this fund to
you,i want you as ALLAH fearing person,to also use this money to
Help,orphanages and widows,I took this decision,before i rest in peace
because my time will so on be up.
I took this decision because I don`t have any child that will inherit
this money and my husband's relatives are not around and I don`t want
my husband hard earned money to go invain. I don`t want a situation
where these money will be misused,hence the reason for taking this
bold decision.I am not afraid of death hence i know where am going to
ALLAH.Presently,I'm with my laptop in a hospital here in London where
I have been undergoing treatment for cancer of the lungs.
As soon as I receive your reply I shall give you the contact of the
Bank.I will also issue you a letter of authority that will prove you
as the new beneficiary of my fund.Please assure me that you will act
accordingly as I stated.Hoping to hear from you soon.
May Allah bless you.
Mrs Nubia Muhammad
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to get new driver attributes
From: David Ahern @ 2018-05-15 19:53 UTC (permalink / raw)
To: Doug Ledford, Steve Wise, leon; +Cc: stephen, netdev, linux-rdma
In-Reply-To: <eb6edd23b76a176b876002cc19c546a0669cf66f.camel@redhat.com>
On 5/15/18 1:47 PM, Doug Ledford wrote:
> On Tue, 2018-05-15 at 13:37 -0600, David Ahern wrote:
>> On 5/14/18 9:42 AM, Steve Wise wrote:
>>> diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h
>>> index 60416ed..40be0d8 100644
>>> --- a/rdma/include/uapi/rdma/rdma_netlink.h
>>> +++ b/rdma/include/uapi/rdma/rdma_netlink.h
>>> @@ -249,10 +249,22 @@ enum rdma_nldev_command {
>>> RDMA_NLDEV_NUM_OPS
>>> };
>>>
>>> +enum {
>>> + RDMA_NLDEV_ATTR_ENTRY_STRLEN = 16,
>>> +};
>>> +
>>> +enum rdma_nldev_print_type {
>>> + RDMA_NLDEV_PRINT_TYPE_UNSPEC,
>>> + RDMA_NLDEV_PRINT_TYPE_HEX,
>>> +};
>>> +
>>> enum rdma_nldev_attr {
>>> /* don't change the order or add anything between, this is ABI! */
>>
>> I asked this before and did not get a response. As the comment above
>> states with an emphasis (!) ...
>>
>>> RDMA_NLDEV_ATTR_UNSPEC,
>>>
>>> + /* Pad attribute for 64b alignment */
>>> + RDMA_NLDEV_ATTR_PAD = RDMA_NLDEV_ATTR_UNSPEC,
>>> +
>>
>> ... are you really adding new attributes in the middle?
>
> Not really. The new item is being explicitly set to the same value as
> the item above it. It therefore becomes two entries with the same enum
> value. The rest of the enum is all unchanged.
sure for the above, but not the ones below.
Before this patch, this program:
$ cat a.c
#include <uapi/rdma/rdma_netlink.h>
#include <stdio.h>
int main(void)
{
printf("RDMA_NLDEV_ATTR_NDEV_INDEX = %d\n", RDMA_NLDEV_ATTR_NDEV_INDEX);
return 0;
}
prints this:
$ ./a.out
RDMA_NLDEV_ATTR_NDEV_INDEX = 50
After this patch,
$ ./a.out
RDMA_NLDEV_ATTR_NDEV_INDEX = 58
>
>>> /* Identifier for ib_device */
>>> RDMA_NLDEV_ATTR_DEV_INDEX, /* u32 */
>>>
>>> @@ -387,6 +399,20 @@ enum rdma_nldev_attr {
>>> RDMA_NLDEV_ATTR_RES_PD_ENTRY, /* nested table */
>>> RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY, /* u32 */
>>> RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY, /* u32 */
>>> + /*
>>> + * driver-specific attributes.
>>> + */
>>> + RDMA_NLDEV_ATTR_DRIVER, /* nested table */
>>> + RDMA_NLDEV_ATTR_DRIVER_ENTRY, /* nested table */
>>> + RDMA_NLDEV_ATTR_DRIVER_STRING, /* string */
>>> + /*
>>> + * u8 values from enum rdma_nldev_print_type
>>> + */
>>> + RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE, /* u8 */
>>> + RDMA_NLDEV_ATTR_DRIVER_S32, /* s32 */
>>> + RDMA_NLDEV_ATTR_DRIVER_U32, /* u32 */
>>> + RDMA_NLDEV_ATTR_DRIVER_S64, /* s64 */
>>> + RDMA_NLDEV_ATTR_DRIVER_U64, /* u64 */
>>
>> and again here.
>>
>>>
>>> /*
>>> * Provides logical name and index of netdevice which is
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" icat n
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 1/3] rdma: update rdma_netlink.h to get new driver attributes
From: Doug Ledford @ 2018-05-15 19:52 UTC (permalink / raw)
To: David Ahern, Steve Wise, leon; +Cc: stephen, netdev, linux-rdma
In-Reply-To: <f419f21a-d57d-ba6c-419e-4fdf24eaa662@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]
On Tue, 2018-05-15 at 13:37 -0600, David Ahern wrote:
> On 5/14/18 9:42 AM, Steve Wise wrote:
> > diff --git a/rdma/include/uapi/rdma/rdma_netlink.h b/rdma/include/uapi/rdma/rdma_netlink.h
> > index 60416ed..40be0d8 100644
> > --- a/rdma/include/uapi/rdma/rdma_netlink.h
> > +++ b/rdma/include/uapi/rdma/rdma_netlink.h
> >
> > @@ -387,6 +399,20 @@ enum rdma_nldev_attr {
> > RDMA_NLDEV_ATTR_RES_PD_ENTRY, /* nested table */
> > RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY, /* u32 */
> > RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY, /* u32 */
> > + /*
> > + * driver-specific attributes.
> > + */
> > + RDMA_NLDEV_ATTR_DRIVER, /* nested table */
> > + RDMA_NLDEV_ATTR_DRIVER_ENTRY, /* nested table */
> > + RDMA_NLDEV_ATTR_DRIVER_STRING, /* string */
> > + /*
> > + * u8 values from enum rdma_nldev_print_type
> > + */
> > + RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE, /* u8 */
> > + RDMA_NLDEV_ATTR_DRIVER_S32, /* s32 */
> > + RDMA_NLDEV_ATTR_DRIVER_U32, /* u32 */
> > + RDMA_NLDEV_ATTR_DRIVER_S64, /* s64 */
> > + RDMA_NLDEV_ATTR_DRIVER_U64, /* u64 */
>
> and again here.
This chunk, however, is a problem. We'll need to fix that in the kernel
and in this patch too.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 32/32] random: convert to ->poll_mask
From: Christoph Hellwig @ 2018-05-15 19:48 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180515194833.6906-1-hch@lst.de>
The big change is that random_read_wait and random_write_wait are merged
into a single waitqueue that uses keyed wakeups. Because wait_event_*
doesn't know about that this will lead to occassional spurious wakeups
in _random_read and add_hwgenerator_randomness, but wait_event_* is
designed to handle these and were are not in a a hot path there.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Acked-by: Theodore Ts'o <tytso@mit.edu>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/char/random.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index cd888d4ee605..a8fb0020ba5c 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -402,8 +402,7 @@ static struct poolinfo {
/*
* Static global variables
*/
-static DECLARE_WAIT_QUEUE_HEAD(random_read_wait);
-static DECLARE_WAIT_QUEUE_HEAD(random_write_wait);
+static DECLARE_WAIT_QUEUE_HEAD(random_wait);
static struct fasync_struct *fasync;
static DEFINE_SPINLOCK(random_ready_list_lock);
@@ -722,8 +721,8 @@ static void credit_entropy_bits(struct entropy_store *r, int nbits)
/* should we wake readers? */
if (entropy_bits >= random_read_wakeup_bits &&
- wq_has_sleeper(&random_read_wait)) {
- wake_up_interruptible(&random_read_wait);
+ wq_has_sleeper(&random_wait)) {
+ wake_up_interruptible_poll(&random_wait, POLLIN);
kill_fasync(&fasync, SIGIO, POLL_IN);
}
/* If the input pool is getting full, send some
@@ -1397,7 +1396,7 @@ static size_t account(struct entropy_store *r, size_t nbytes, int min,
trace_debit_entropy(r->name, 8 * ibytes);
if (ibytes &&
(r->entropy_count >> ENTROPY_SHIFT) < random_write_wakeup_bits) {
- wake_up_interruptible(&random_write_wait);
+ wake_up_interruptible_poll(&random_wait, POLLOUT);
kill_fasync(&fasync, SIGIO, POLL_OUT);
}
@@ -1839,7 +1838,7 @@ _random_read(int nonblock, char __user *buf, size_t nbytes)
if (nonblock)
return -EAGAIN;
- wait_event_interruptible(random_read_wait,
+ wait_event_interruptible(random_wait,
ENTROPY_BITS(&input_pool) >=
random_read_wakeup_bits);
if (signal_pending(current))
@@ -1876,14 +1875,17 @@ urandom_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
return ret;
}
+static struct wait_queue_head *
+random_get_poll_head(struct file *file, __poll_t events)
+{
+ return &random_wait;
+}
+
static __poll_t
-random_poll(struct file *file, poll_table * wait)
+random_poll_mask(struct file *file, __poll_t events)
{
- __poll_t mask;
+ __poll_t mask = 0;
- poll_wait(file, &random_read_wait, wait);
- poll_wait(file, &random_write_wait, wait);
- mask = 0;
if (ENTROPY_BITS(&input_pool) >= random_read_wakeup_bits)
mask |= EPOLLIN | EPOLLRDNORM;
if (ENTROPY_BITS(&input_pool) < random_write_wakeup_bits)
@@ -1990,7 +1992,8 @@ static int random_fasync(int fd, struct file *filp, int on)
const struct file_operations random_fops = {
.read = random_read,
.write = random_write,
- .poll = random_poll,
+ .get_poll_head = random_get_poll_head,
+ .poll_mask = random_poll_mask,
.unlocked_ioctl = random_ioctl,
.fasync = random_fasync,
.llseek = noop_llseek,
@@ -2323,7 +2326,7 @@ void add_hwgenerator_randomness(const char *buffer, size_t count,
* We'll be woken up again once below random_write_wakeup_thresh,
* or when the calling thread is about to terminate.
*/
- wait_event_interruptible(random_write_wait, kthread_should_stop() ||
+ wait_event_interruptible(random_wait, kthread_should_stop() ||
ENTROPY_BITS(&input_pool) <= random_write_wakeup_bits);
mix_pool_bytes(poolp, buffer, count);
credit_entropy_bits(poolp, entropy);
--
2.17.0
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH 31/32] timerfd: convert to ->poll_mask
From: Christoph Hellwig @ 2018-05-15 19:48 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180515194833.6906-1-hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/timerfd.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/fs/timerfd.c b/fs/timerfd.c
index cdad49da3ff7..d84a2bee4f82 100644
--- a/fs/timerfd.c
+++ b/fs/timerfd.c
@@ -226,21 +226,20 @@ static int timerfd_release(struct inode *inode, struct file *file)
kfree_rcu(ctx, rcu);
return 0;
}
-
-static __poll_t timerfd_poll(struct file *file, poll_table *wait)
+
+static struct wait_queue_head *timerfd_get_poll_head(struct file *file,
+ __poll_t eventmask)
{
struct timerfd_ctx *ctx = file->private_data;
- __poll_t events = 0;
- unsigned long flags;
- poll_wait(file, &ctx->wqh, wait);
+ return &ctx->wqh;
+}
- spin_lock_irqsave(&ctx->wqh.lock, flags);
- if (ctx->ticks)
- events |= EPOLLIN;
- spin_unlock_irqrestore(&ctx->wqh.lock, flags);
+static __poll_t timerfd_poll_mask(struct file *file, __poll_t eventmask)
+{
+ struct timerfd_ctx *ctx = file->private_data;
- return events;
+ return ctx->ticks ? EPOLLIN : 0;
}
static ssize_t timerfd_read(struct file *file, char __user *buf, size_t count,
@@ -364,7 +363,8 @@ static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long arg
static const struct file_operations timerfd_fops = {
.release = timerfd_release,
- .poll = timerfd_poll,
+ .get_poll_head = timerfd_get_poll_head,
+ .poll_mask = timerfd_poll_mask,
.read = timerfd_read,
.llseek = noop_llseek,
.show_fdinfo = timerfd_show,
--
2.17.0
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH 30/32] eventfd: switch to ->poll_mask
From: Christoph Hellwig @ 2018-05-15 19:48 UTC (permalink / raw)
To: viro; +Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180515194833.6906-1-hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/eventfd.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/fs/eventfd.c b/fs/eventfd.c
index 08d3bd602f73..61c9514da5e9 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -101,14 +101,20 @@ static int eventfd_release(struct inode *inode, struct file *file)
return 0;
}
-static __poll_t eventfd_poll(struct file *file, poll_table *wait)
+static struct wait_queue_head *
+eventfd_get_poll_head(struct file *file, __poll_t events)
+{
+ struct eventfd_ctx *ctx = file->private_data;
+
+ return &ctx->wqh;
+}
+
+static __poll_t eventfd_poll_mask(struct file *file, __poll_t eventmask)
{
struct eventfd_ctx *ctx = file->private_data;
__poll_t events = 0;
u64 count;
- poll_wait(file, &ctx->wqh, wait);
-
/*
* All writes to ctx->count occur within ctx->wqh.lock. This read
* can be done outside ctx->wqh.lock because we know that poll_wait
@@ -305,7 +311,8 @@ static const struct file_operations eventfd_fops = {
.show_fdinfo = eventfd_show_fdinfo,
#endif
.release = eventfd_release,
- .poll = eventfd_poll,
+ .get_poll_head = eventfd_get_poll_head,
+ .poll_mask = eventfd_poll_mask,
.read = eventfd_read,
.write = eventfd_write,
.llseek = noop_llseek,
--
2.17.0
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ 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