* Re: [PATCH v2 0/2] bpf: permit JIT allocations to be served outside the module region
From: Ard Biesheuvel @ 2018-11-21 20:36 UTC (permalink / raw)
To: Rick Edgecombe
Cc: linux-arm-kernel, Linux Kernel Mailing List, Daniel Borkmann,
Kees Cook, Jessica Yu, Jann Horn, Alexei Starovoitov,
Mark Rutland, Catalin Marinas, Will Deacon,
<netdev@vger.kernel.org>, Eric Dumazet, David S. Miller,
Arnd Bergmann
In-Reply-To: <9d7f77959e1be0a9a3ff511a8fc45518068c85a6.camel@intel.com>
On Wed, 21 Nov 2018 at 20:48, Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> On Wed, 2018-11-21 at 14:17 +0100, Ard Biesheuvel wrote:
> > On arm64, modules are allocated from a 128 MB window which is close to
> > the core kernel, so that relative direct branches are guaranteed to be
> > in range (except in some KASLR configurations). Also, module_alloc()
> > is in charge of allocating KASAN shadow memory when running with KASAN
> > enabled.
> >
> > This means that the way BPF reuses module_alloc()/module_memfree() is
> > undesirable on arm64 (and potentially other architectures as well),
> > and so this series refactors BPF's use of those functions to permit
> > architectures to change this behavior.
> >
> Hi Ard,
>
> I am looking at adding optional BPF JIT in vmalloc functionality for x86 that
> would use this refactor. In fact I have done the same thing with just different
> names.
>
> My implementation intends to use the module space until a usage limit is reached
> and then overflow into vmalloc, so it would be an additional knob like
> "bpf_jit_limit". Wondering if that should be a cross-arch concept that connects
> to this. Does it fit in with what you are trying to do for arm64 here?
>
Hi Rick,
As I understand it, x86 requires the BPF allocations to be located
within 2 GB of the core kernel, so that RIP-relative 32-bit jumps are
in range (I read that in a comment somewhere, or a git commit log
perhaps)
That requirement does not exist on arm64: ordinary function calls and
tail calls emitted by the BPF JIT code have unlimited range, and so
there is simply no reason to prefer the module region for these
allocations. I guess we could achieve the same when reusing your
approach by setting the threshold to zero.
^ permalink raw reply
* Re: [PATCH net-next 2/4] netns: add support of NETNSA_TARGET_NSID
From: Nicolas Dichtel @ 2018-11-21 20:58 UTC (permalink / raw)
To: David Ahern, davem; +Cc: netdev
In-Reply-To: <8fa3ba08-e488-13c8-7a2b-8227c5fd77df@gmail.com>
Le 21/11/2018 à 19:05, David Ahern a écrit :
> On 11/21/18 4:01 AM, Nicolas Dichtel wrote:
>> static int rtnl_net_dumpid(struct sk_buff *skb, struct netlink_callback *cb)
>> {
>> - struct net *net = sock_net(skb->sk);
>> struct rtnl_net_dump_cb net_cb = {
>> - .net = net,
>> + .tgt_net = sock_net(skb->sk),
>> .skb = skb,
>> .cb = cb,
>> .idx = 0,
>> .s_idx = cb->args[0],
>> };
>> + int err = 0;
>>
>> - if (cb->strict_check &&
>> - nlmsg_attrlen(cb->nlh, sizeof(struct rtgenmsg))) {
>> - NL_SET_ERR_MSG(cb->extack, "Unknown data in network namespace id dump request");
>> - return -EINVAL;
>> + if (cb->strict_check) {
>> + err = rtnl_valid_dump_net_req(cb->nlh, skb->sk, &net_cb, cb);
>> + if (err < 0)
>> + goto end;
>> }
>>
>> - spin_lock_bh(&net->nsid_lock);
>> - idr_for_each(&net->netns_ids, rtnl_net_dumpid_one, &net_cb);
>> - spin_unlock_bh(&net->nsid_lock);
>> + spin_lock_bh(&net_cb.tgt_net->nsid_lock);
>> + idr_for_each(&net_cb.tgt_net->netns_ids, rtnl_net_dumpid_one, &net_cb);
>> + spin_unlock_bh(&net_cb.tgt_net->nsid_lock);
>>
>> cb->args[0] = net_cb.idx;
>> - return skb->len;
>> +end:
>> + if (net_cb.ref_net)
>> + put_net(net_cb.tgt_net);
>
> That is going to lead to confusion -- you check that ref_net is set put
> do a put on tgt_net. Other places using a target namespace use the nsid
> as the key to whether a put_net is needed.
The target-nsid is not stored in net_cb (not needed). ref_net is set only if
tgt_net comes from TARGET_NSID. I can add a comment.
^ permalink raw reply
* Re: [PATCH net-next 4/4] netns: enable to dump full nsid translation table
From: Nicolas Dichtel @ 2018-11-21 21:01 UTC (permalink / raw)
To: David Ahern, davem; +Cc: netdev
In-Reply-To: <de737e31-78e1-1d6b-8890-b2418e9ef8b4@gmail.com>
Le 21/11/2018 à 19:09, David Ahern a écrit :
> On 11/21/18 4:01 AM, Nicolas Dichtel wrote:
>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>> index 92730905886c..fc568cd0b560 100644
>> --- a/net/core/net_namespace.c
>> +++ b/net/core/net_namespace.c
>> @@ -740,7 +740,7 @@ static int rtnl_net_get_size(void)
>> }
>>
>> static int rtnl_net_fill(struct sk_buff *skb, u32 portid, u32 seq, int flags,
>> - int cmd, int nsid)
>> + int cmd, int nsid, bool add_ref, int ref_nsid)
>> {
>> struct nlmsghdr *nlh;
>> struct rtgenmsg *rth;
>> @@ -755,6 +755,9 @@ static int rtnl_net_fill(struct sk_buff *skb, u32 portid, u32 seq, int flags,
>> if (nla_put_s32(skb, NETNSA_NSID, nsid))
>> goto nla_put_failure;
>>
>> + if (add_ref && nla_put_s32(skb, NETNSA_CURRENT_NSID, ref_nsid))
>> + goto nla_put_failure;
>
> Why not use ref_nsid >= 0 and drop the add_ref argument?
Because ref_nsid can be -1 (NETNSA_NSID_NOT_ASSIGNED) and I didn't want to add
another magic, I think it's clearer with an explicit bool.
^ permalink raw reply
* consistency for statistics with XDP mode
From: David Ahern @ 2018-11-21 21:06 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: Paweł Staszewski, Jesper Dangaard Brouer, Saeed Mahameed,
Jason Wang, Michael S. Tsirkin, David Miller
Paweł ran some more XDP tests yesterday and from it found a couple of
issues. One is a panic in the mlx5 driver unloading the bpf program
(mlx5e_xdp_xmit); he will send a send a separate email for that problem.
The problem I wanted to discuss here is statistics for XDP context. The
short of it is that we need consistency in the counters across NIC
drivers and virtual devices. Right now stats are specific to a driver
with no clear accounting for the packets and bytes handled in XDP.
For example virtio has some stats as device private data extracted via
ethtool:
$ ethtool -S eth2 | grep xdp
...
rx_queue_3_xdp_packets: 5291
rx_queue_3_xdp_tx: 0
rx_queue_3_xdp_redirects: 5163
rx_queue_3_xdp_drops: 0
...
tx_queue_3_xdp_tx: 5163
tx_queue_3_xdp_tx_drops: 0
And the standard counters appear to track bytes and packets for Rx, but
not Tx if the packet is forwarded in XDP.
Similarly, mlx5 has some counters (thanks to Jesper and Toke for helping
out here):
$ ethtool -S mlx5p1 | grep xdp
rx_xdp_drop: 86468350180
rx_xdp_redirect: 18860584
rx_xdp_tx_xmit: 0
rx_xdp_tx_full: 0
rx_xdp_tx_err: 0
rx_xdp_tx_cqe: 0
tx_xdp_xmit: 0
tx_xdp_full: 0
tx_xdp_err: 0
tx_xdp_cqes: 0
...
rx3_xdp_drop: 86468350180
rx3_xdp_redirect: 18860556
rx3_xdp_tx_xmit: 0
rx3_xdp_tx_full: 0
rx3_xdp_tx_err: 0
rx3_xdp_tx_cqes: 0
...
tx0_xdp_xmit: 0
tx0_xdp_full: 0
tx0_xdp_err: 0
tx0_xdp_cqes: 0
...
And no accounting in standard stats for packets handled in XDP.
And then if I understand Jesper's data correctly, the i40e driver does
not have device specific data:
$ ethtool -S i40e1 | grep xdp
[NOTHING]
But rather bumps the standard counters:
sudo ./xdp_rxq_info --dev i40e1 --action XDP_DROP
Running XDP on dev:i40e1 (ifindex:3) action:XDP_DROP options:no_touch
XDP stats CPU pps issue-pps
XDP-RX CPU 1 36,156,872 0
XDP-RX CPU total 36,156,872
RXQ stats RXQ:CPU pps issue-pps
rx_queue_index 1:1 36,156,878 0
rx_queue_index 1:sum 36,156,878
$ ethtool_stats.pl --dev i40e1
Show adapter(s) (i40e1) statistics (ONLY that changed!)
Ethtool(i40e1 ) stat: 2711292859 ( 2,711,292,859) <= port.rx_bytes /sec
Ethtool(i40e1 ) stat: 6274204 ( 6,274,204) <=
port.rx_dropped /sec
Ethtool(i40e1 ) stat: 42363867 ( 42,363,867) <=
port.rx_size_64 /sec
Ethtool(i40e1 ) stat: 42363950 ( 42,363,950) <=
port.rx_unicast /sec
Ethtool(i40e1 ) stat: 2165051990 ( 2,165,051,990) <= rx-1.bytes /sec
Ethtool(i40e1 ) stat: 36084200 ( 36,084,200) <= rx-1.packets /sec
Ethtool(i40e1 ) stat: 5385 ( 5,385) <= rx_dropped /sec
Ethtool(i40e1 ) stat: 36089727 ( 36,089,727) <= rx_unicast /sec
We really need consistency in the counters and at a minimum, users
should be able to track packet and byte counters for both Rx and Tx
including XDP.
It seems to me the Rx and Tx packet, byte and dropped counters returned
for the standard device stats (/proc/net/dev, ip -s li show, ...) should
include all packets managed by the driver regardless of whether they are
forwarded / dropped in XDP or go up the Linux stack. This also aligns
with mlxsw and the stats it shows which are packets handled by the hardware.
>From there the private stats can include XDP specifics as desired --
like the drops and redirects but that those should be add-ons and even
here some consistency makes life easier for users.
The same standards should be also be applied to virtual devices built on
top of the ports -- e.g, vlans. I have an API now that allows bumping
stats for vlan devices.
Keeping the basic xdp packets in the standard counters allows Paweł, for
example, to continue to monitor /proc/net/dev.
Can we get agreement on this? And from there, get updates to the mlx5
and virtio drivers?
David
^ permalink raw reply
* Re: [PATCH net-next 2/4] netns: add support of NETNSA_TARGET_NSID
From: David Ahern @ 2018-11-21 21:07 UTC (permalink / raw)
To: nicolas.dichtel, davem; +Cc: netdev
In-Reply-To: <e1667898-9189-bb32-0510-4c9d22aaf8cb@6wind.com>
On 11/21/18 1:58 PM, Nicolas Dichtel wrote:
> The target-nsid is not stored in net_cb (not needed). ref_net is set only if
> tgt_net comes from TARGET_NSID. I can add a comment.
ref_net is added by this patch and it is only used (unless I missed
something) to know if the put_net is needed. ie/, drop ref_net in place
of nsid
^ permalink raw reply
* Re: [PATCH net-next 4/4] netns: enable to dump full nsid translation table
From: David Ahern @ 2018-11-21 21:08 UTC (permalink / raw)
To: nicolas.dichtel, davem; +Cc: netdev
In-Reply-To: <3a7133b9-858c-5c6c-8402-323a07d8e875@6wind.com>
On 11/21/18 2:01 PM, Nicolas Dichtel wrote:
> Le 21/11/2018 à 19:09, David Ahern a écrit :
>> On 11/21/18 4:01 AM, Nicolas Dichtel wrote:
>>> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
>>> index 92730905886c..fc568cd0b560 100644
>>> --- a/net/core/net_namespace.c
>>> +++ b/net/core/net_namespace.c
>>> @@ -740,7 +740,7 @@ static int rtnl_net_get_size(void)
>>> }
>>>
>>> static int rtnl_net_fill(struct sk_buff *skb, u32 portid, u32 seq, int flags,
>>> - int cmd, int nsid)
>>> + int cmd, int nsid, bool add_ref, int ref_nsid)
>>> {
>>> struct nlmsghdr *nlh;
>>> struct rtgenmsg *rth;
>>> @@ -755,6 +755,9 @@ static int rtnl_net_fill(struct sk_buff *skb, u32 portid, u32 seq, int flags,
>>> if (nla_put_s32(skb, NETNSA_NSID, nsid))
>>> goto nla_put_failure;
>>>
>>> + if (add_ref && nla_put_s32(skb, NETNSA_CURRENT_NSID, ref_nsid))
>>> + goto nla_put_failure;
>>
>> Why not use ref_nsid >= 0 and drop the add_ref argument?
> Because ref_nsid can be -1 (NETNSA_NSID_NOT_ASSIGNED) and I didn't want to add
> another magic, I think it's clearer with an explicit bool.
>
it pushes the input args to 8.
^ permalink raw reply
* Re: [PATCH net-next] net: mvneta: remove redundant check for eee->tx_lpi_timer < 0
From: Thomas Petazzoni @ 2018-11-22 7:44 UTC (permalink / raw)
To: YueHaibing; +Cc: davem, linux-kernel, netdev
In-Reply-To: <20181122064200.11572-1-yuehaibing@huawei.com>
Hello,
On Thu, 22 Nov 2018 14:42:00 +0800, YueHaibing wrote:
> fixes the smatch warning:
>
> drivers/net/ethernet/marvell/mvneta.c:4252 mvneta_ethtool_set_eee() warn:
> unsigned 'eee->tx_lpi_timer' is never less than zero.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Acked-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: consistency for statistics with XDP mode
From: Toke Høiland-Jørgensen @ 2018-11-21 21:14 UTC (permalink / raw)
To: David Ahern, netdev@vger.kernel.org
Cc: Paweł Staszewski, Jesper Dangaard Brouer, Saeed Mahameed,
Jason Wang, Michael S. Tsirkin, David Miller
In-Reply-To: <1d9a6548-4d1d-6624-e808-6ab0460a8655@gmail.com>
David Ahern <dsahern@gmail.com> writes:
> Paweł ran some more XDP tests yesterday and from it found a couple of
> issues. One is a panic in the mlx5 driver unloading the bpf program
> (mlx5e_xdp_xmit); he will send a send a separate email for that
> problem.
Same as this one, I guess?
https://marc.info/?l=linux-netdev&m=153855905619717&w=2
> The problem I wanted to discuss here is statistics for XDP context. The
> short of it is that we need consistency in the counters across NIC
> drivers and virtual devices. Right now stats are specific to a driver
> with no clear accounting for the packets and bytes handled in XDP.
>
> For example virtio has some stats as device private data extracted via
> ethtool:
> $ ethtool -S eth2 | grep xdp
> ...
> rx_queue_3_xdp_packets: 5291
> rx_queue_3_xdp_tx: 0
> rx_queue_3_xdp_redirects: 5163
> rx_queue_3_xdp_drops: 0
> ...
> tx_queue_3_xdp_tx: 5163
> tx_queue_3_xdp_tx_drops: 0
>
> And the standard counters appear to track bytes and packets for Rx, but
> not Tx if the packet is forwarded in XDP.
>
> Similarly, mlx5 has some counters (thanks to Jesper and Toke for helping
> out here):
>
> $ ethtool -S mlx5p1 | grep xdp
> rx_xdp_drop: 86468350180
> rx_xdp_redirect: 18860584
> rx_xdp_tx_xmit: 0
> rx_xdp_tx_full: 0
> rx_xdp_tx_err: 0
> rx_xdp_tx_cqe: 0
> tx_xdp_xmit: 0
> tx_xdp_full: 0
> tx_xdp_err: 0
> tx_xdp_cqes: 0
> ...
> rx3_xdp_drop: 86468350180
> rx3_xdp_redirect: 18860556
> rx3_xdp_tx_xmit: 0
> rx3_xdp_tx_full: 0
> rx3_xdp_tx_err: 0
> rx3_xdp_tx_cqes: 0
> ...
> tx0_xdp_xmit: 0
> tx0_xdp_full: 0
> tx0_xdp_err: 0
> tx0_xdp_cqes: 0
> ...
>
> And no accounting in standard stats for packets handled in XDP.
>
> And then if I understand Jesper's data correctly, the i40e driver does
> not have device specific data:
>
> $ ethtool -S i40e1 | grep xdp
> [NOTHING]
>
>
> But rather bumps the standard counters:
>
> sudo ./xdp_rxq_info --dev i40e1 --action XDP_DROP
>
> Running XDP on dev:i40e1 (ifindex:3) action:XDP_DROP options:no_touch
> XDP stats CPU pps issue-pps
> XDP-RX CPU 1 36,156,872 0
> XDP-RX CPU total 36,156,872
>
> RXQ stats RXQ:CPU pps issue-pps
> rx_queue_index 1:1 36,156,878 0
> rx_queue_index 1:sum 36,156,878
>
>
> $ ethtool_stats.pl --dev i40e1
>
> Show adapter(s) (i40e1) statistics (ONLY that changed!)
> Ethtool(i40e1 ) stat: 2711292859 ( 2,711,292,859) <= port.rx_bytes /sec
> Ethtool(i40e1 ) stat: 6274204 ( 6,274,204) <=
> port.rx_dropped /sec
> Ethtool(i40e1 ) stat: 42363867 ( 42,363,867) <=
> port.rx_size_64 /sec
> Ethtool(i40e1 ) stat: 42363950 ( 42,363,950) <=
> port.rx_unicast /sec
> Ethtool(i40e1 ) stat: 2165051990 ( 2,165,051,990) <= rx-1.bytes /sec
> Ethtool(i40e1 ) stat: 36084200 ( 36,084,200) <= rx-1.packets /sec
> Ethtool(i40e1 ) stat: 5385 ( 5,385) <= rx_dropped /sec
> Ethtool(i40e1 ) stat: 36089727 ( 36,089,727) <= rx_unicast /sec
>
>
> We really need consistency in the counters and at a minimum, users
> should be able to track packet and byte counters for both Rx and Tx
> including XDP.
>
> It seems to me the Rx and Tx packet, byte and dropped counters returned
> for the standard device stats (/proc/net/dev, ip -s li show, ...) should
> include all packets managed by the driver regardless of whether they are
> forwarded / dropped in XDP or go up the Linux stack. This also aligns
> with mlxsw and the stats it shows which are packets handled by the hardware.
>
> From there the private stats can include XDP specifics as desired --
> like the drops and redirects but that those should be add-ons and even
> here some consistency makes life easier for users.
>
> The same standards should be also be applied to virtual devices built on
> top of the ports -- e.g, vlans. I have an API now that allows bumping
> stats for vlan devices.
>
> Keeping the basic xdp packets in the standard counters allows Paweł, for
> example, to continue to monitor /proc/net/dev.
>
> Can we get agreement on this? And from there, get updates to the mlx5
> and virtio drivers?
I'd say it sounds reasonable to include XDP in the normal traffic
counters, but having the detailed XDP-specific counters is quite useful
as well... So can't we do both (for all drivers)?
-Toke
^ permalink raw reply
* Re: [PATCH net-next,v3 04/12] cls_api: add translator to flow_action representation
From: Marcelo Ricardo Leitner @ 2018-11-21 21:15 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, davem, thomas.lendacky, f.fainelli, ariel.elior,
michael.chan, santosh, madalin.bucur, yisen.zhuang, salil.mehta,
jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch, jakub.kicinski,
peppe.cavallaro, grygorii.strashko, andrew, vivien.didelot,
alexandre.torgue, joabreu, linux-net-drivers, ganeshgr, ogerlitz,
Manish.Chopra
In-Reply-To: <20181121025132.14305-5-pablo@netfilter.org>
On Wed, Nov 21, 2018 at 03:51:24AM +0100, Pablo Neira Ayuso wrote:
> This patch implements a new function to translate from native TC action
> to the new flow_action representation. Moreover, this patch also updates
> cls_flower to use this new function.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> v3: add tcf_exts_num_actions() and pass it to flow_rule_alloc() to calculate
> the size of the array of actions.
>
> include/net/pkt_cls.h | 5 +++
> net/sched/cls_api.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++
> net/sched/cls_flower.c | 21 +++++++--
> 3 files changed, 139 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 359876ee32be..abb035f84321 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -620,6 +620,11 @@ tcf_match_indev(struct sk_buff *skb, int ifindex)
> }
> #endif /* CONFIG_NET_CLS_IND */
>
> +unsigned int tcf_exts_num_actions(struct tcf_exts *exts);
> +
> +int tc_setup_flow_action(struct flow_action *flow_action,
> + const struct tcf_exts *exts);
> +
> int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
> enum tc_setup_type type, void *type_data, bool err_stop);
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index d92f44ac4c39..6f8b953dabc4 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -31,6 +31,14 @@
> #include <net/netlink.h>
> #include <net/pkt_sched.h>
> #include <net/pkt_cls.h>
> +#include <net/tc_act/tc_mirred.h>
> +#include <net/tc_act/tc_vlan.h>
> +#include <net/tc_act/tc_tunnel_key.h>
> +#include <net/tc_act/tc_pedit.h>
> +#include <net/tc_act/tc_csum.h>
> +#include <net/tc_act/tc_gact.h>
> +#include <net/tc_act/tc_skbedit.h>
> +#include <net/tc_act/tc_mirred.h>
>
> extern const struct nla_policy rtm_tca_policy[TCA_MAX + 1];
>
> @@ -2567,6 +2575,114 @@ int tc_setup_cb_call(struct tcf_block *block, struct tcf_exts *exts,
> }
> EXPORT_SYMBOL(tc_setup_cb_call);
>
> +int tc_setup_flow_action(struct flow_action *flow_action,
> + const struct tcf_exts *exts)
> +{
> + const struct tc_action *act;
> + int i, j, k;
> +
> + if (!exts)
> + return 0;
> +
> + j = 0;
> + tcf_exts_for_each_action(i, act, exts) {
> + struct flow_action_entry *key;
> +
> + key = &flow_action->entries[j];
> + if (is_tcf_gact_ok(act)) {
> + key->id = FLOW_ACTION_ACCEPT;
> + } else if (is_tcf_gact_shot(act)) {
> + key->id = FLOW_ACTION_DROP;
> + } else if (is_tcf_gact_trap(act)) {
> + key->id = FLOW_ACTION_TRAP;
> + } else if (is_tcf_gact_goto_chain(act)) {
> + key->id = FLOW_ACTION_GOTO;
> + key->chain_index = tcf_gact_goto_chain_index(act);
> + } else if (is_tcf_mirred_egress_redirect(act)) {
> + key->id = FLOW_ACTION_REDIRECT;
> + key->dev = tcf_mirred_dev(act);
> + } else if (is_tcf_mirred_egress_mirror(act)) {
> + key->id = FLOW_ACTION_MIRRED;
> + key->dev = tcf_mirred_dev(act);
> + } else if (is_tcf_vlan(act)) {
> + switch (tcf_vlan_action(act)) {
> + case TCA_VLAN_ACT_PUSH:
> + key->id = FLOW_ACTION_VLAN_PUSH;
> + key->vlan.vid = tcf_vlan_push_vid(act);
> + key->vlan.proto = tcf_vlan_push_proto(act);
> + key->vlan.prio = tcf_vlan_push_prio(act);
> + break;
> + case TCA_VLAN_ACT_POP:
> + key->id = FLOW_ACTION_VLAN_POP;
> + break;
> + case TCA_VLAN_ACT_MODIFY:
> + key->id = FLOW_ACTION_VLAN_MANGLE;
> + key->vlan.vid = tcf_vlan_push_vid(act);
> + key->vlan.proto = tcf_vlan_push_proto(act);
> + key->vlan.prio = tcf_vlan_push_prio(act);
> + break;
> + default:
> + goto err_out;
> + }
> + } else if (is_tcf_tunnel_set(act)) {
> + key->id = FLOW_ACTION_TUNNEL_ENCAP;
> + key->tunnel = tcf_tunnel_info(act);
> + } else if (is_tcf_tunnel_release(act)) {
> + key->id = FLOW_ACTION_TUNNEL_DECAP;
> + key->tunnel = tcf_tunnel_info(act);
> + } else if (is_tcf_pedit(act)) {
> + for (k = 0; k < tcf_pedit_nkeys(act); k++) {
> + switch (tcf_pedit_cmd(act, k)) {
> + case TCA_PEDIT_KEY_EX_CMD_SET:
> + key->id = FLOW_ACTION_MANGLE;
> + break;
> + case TCA_PEDIT_KEY_EX_CMD_ADD:
> + key->id = FLOW_ACTION_ADD;
> + break;
> + default:
> + goto err_out;
> + }
> + key->mangle.htype = tcf_pedit_htype(act, k);
> + key->mangle.mask = tcf_pedit_mask(act, k);
> + key->mangle.val = tcf_pedit_val(act, k);
> + key->mangle.offset = tcf_pedit_offset(act, k);
> + key = &flow_action->entries[++j];
> + }
> + } else if (is_tcf_csum(act)) {
> + key->id = FLOW_ACTION_CSUM;
> + key->csum_flags = tcf_csum_update_flags(act);
> + } else if (is_tcf_skbedit_mark(act)) {
> + key->id = FLOW_ACTION_MARK;
> + key->mark = tcf_skbedit_mark(act);
> + } else {
> + goto err_out;
> + }
> +
> + if (!is_tcf_pedit(act))
> + j++;
> + }
> + return 0;
> +err_out:
> + return -EOPNOTSUPP;
> +}
> +EXPORT_SYMBOL(tc_setup_flow_action);
> +
> +unsigned int tcf_exts_num_actions(struct tcf_exts *exts)
> +{
> + unsigned int num_acts = 0;
> + struct tc_action *act;
> + int i;
> +
> + tcf_exts_for_each_action(i, act, exts) {
> + if (is_tcf_pedit(act))
> + num_acts += tcf_pedit_nkeys(act);
> + else
> + num_acts++;
> + }
> + return num_acts;
> +}
> +EXPORT_SYMBOL(tcf_exts_num_actions);
> +
> static __net_init int tcf_net_init(struct net *net)
> {
> struct tcf_net *tn = net_generic(net, tcf_net_id);
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index d2971fbfc3d9..8898943b8ee6 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -382,7 +382,7 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
> bool skip_sw = tc_skip_sw(f->flags);
> int err;
>
> - cls_flower.rule = flow_rule_alloc();
> + cls_flower.rule = flow_rule_alloc(tcf_exts_num_actions(&f->exts));
As previous patch did:
-struct flow_rule *flow_rule_alloc(void);
+struct flow_rule *flow_rule_alloc(unsigned int num_actions);
the build is broken without this change (bisect-ability).
(applies to similar lines too)
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: fix a libbpf loader issue
From: Daniel Borkmann @ 2018-11-21 21:23 UTC (permalink / raw)
To: Yonghong Song, ast, netdev; +Cc: kernel-team
In-Reply-To: <20181121192242.1802994-1-yhs@fb.com>
On 11/21/2018 08:22 PM, Yonghong Song wrote:
> Commit 2993e0515bb4 ("tools/bpf: add support to read .BTF.ext sections")
> added support to read .BTF.ext sections from an object file, create
> and pass prog_btf_fd and func_info to the kernel.
>
> The program btf_fd (prog->btf_fd) is initialized to be -1 to please
> zclose so we do not need special handling dur prog close.
> Passing -1 to the kernel, however, will cause loading error.
> Passing btf_fd 0 to the kernel if prog->btf_fd is invalid
> fixed the problem.
>
> Fixes: 2993e0515bb4 ("tools/bpf: add support to read .BTF.ext sections")
> Reported-by: Andrey Ignatov <rdna@fb.com>
> Reported-by: Emre Cantimur <haydum@fb.com>
> Tested-by: Andrey Ignatov <rdna@fb.com>
> Signed-off-by: Yonghong Song <yhs@fb.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH v2 2/2] arm64/bpf: don't allocate BPF JIT programs in module memory
From: Ard Biesheuvel @ 2018-11-22 8:02 UTC (permalink / raw)
To: Daniel Borkmann
Cc: linux-arm-kernel, Alexei Starovoitov, Rick Edgecombe,
Eric Dumazet, Jann Horn, Kees Cook, Jessica Yu, Arnd Bergmann,
Catalin Marinas, Will Deacon, Mark Rutland, David S. Miller,
Linux Kernel Mailing List, <netdev@vger.kernel.org>
In-Reply-To: <945415e1-0ff8-65ce-15fa-33cea0a7d1c9@iogearbox.net>
On Thu, 22 Nov 2018 at 00:20, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 11/21/2018 02:17 PM, Ard Biesheuvel wrote:
> > The arm64 module region is a 128 MB region that is kept close to
> > the core kernel, in order to ensure that relative branches are
> > always in range. So using the same region for programs that do
> > not have this restriction is wasteful, and preferably avoided.
> >
> > Now that the core BPF JIT code permits the alloc/free routines to
> > be overridden, implement them by simple vmalloc_exec()/vfree()
> > calls, which can be served from anywere. This also solves an
> > issue under KASAN, where shadow memory is needlessly allocated for
> > all BPF programs (which don't require KASAN shadow pages since
> > they are not KASAN instrumented)
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > arch/arm64/net/bpf_jit_comp.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index a6fdaea07c63..f91b7c157841 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -940,3 +940,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
> > tmp : orig_prog);
> > return prog;
> > }
> > +
> > +void *bpf_jit_alloc_exec(unsigned long size)
> > +{
> > + return vmalloc_exec(size);
> > +}
> > +
> > +void bpf_jit_free_exec(const void *addr)
> > +{
> > + return vfree(size);
> > +}
>
> Hmm, could you elaborate in the commit log on the potential performance
> regression for JITed progs on arm64 after this change?
>
This does not affect the generated code, so I don't anticipate a
performance hit. Did you have anything in particular in mind?
> I think this change would also break JITing of BPF to BPF calls. You might
> have the same issue as ppc64 folks where the offset might not fit into imm
> anymore and would have to transfer it via fp->aux->func[off]->bpf_func
> instead.
If we are relying on BPF programs to remain within 128 MB of each
other, then we already have a potential problem, given that the
module_alloc() spills over into a 4 GB window if the 128 MB window is
exhausted. Perhaps we should do something like
void *bpf_jit_alloc_exec(unsigned long size) {
return __vmalloc_node_range(size, MODULE_ALIGN,
BPF_REGION_START, BPF_REGION_END,
GFP_KERNEL, PAGE_KERNEL_EXEC, 0, NUMA_NO_NODE,
__builtin_return_address(0));
}
and make [BPF_REGION_START, BPF_REGION_END) a separate 128 MB window
at the top of the vmalloc space. That way, it is guaranteed that BPF
programs are within branching range of each other, and we still solve
the original problem. I also like that it becomes impossible to infer
anything about the state of the vmalloc space, placement of the kernel
and modules etc from the placement of the BPF programs (in case it
leaks this information in one way or the other)
That would only give you space for 128M/4K == 32768 programs (or
128M/64K == 2048 on 64k pages kernels). So I guess we'd still need a
spillover window as well, in which case we'd need a fix for the
BPF-to-BPF branching issue (but we need that at the moment anyway)
^ permalink raw reply
* Re: consistency for statistics with XDP mode
From: Paweł Staszewski @ 2018-11-21 21:29 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, David Ahern,
netdev@vger.kernel.org
Cc: Jesper Dangaard Brouer, Saeed Mahameed, Jason Wang,
Michael S. Tsirkin, David Miller
In-Reply-To: <874lca2kvv.fsf@toke.dk>
W dniu 21.11.2018 o 22:14, Toke Høiland-Jørgensen pisze:
> David Ahern <dsahern@gmail.com> writes:
>
>> Paweł ran some more XDP tests yesterday and from it found a couple of
>> issues. One is a panic in the mlx5 driver unloading the bpf program
>> (mlx5e_xdp_xmit); he will send a send a separate email for that
>> problem.
> Same as this one, I guess?
>
> https://marc.info/?l=linux-netdev&m=153855905619717&w=2
Yes same as this one.
When there is no traffic (for example with xdp_fwd program loaded) or
there is not much traffic like 1k frames per second for icmp - i can
load/unload without crashing kernel
But when i push tests with pktgen and use more than >50k pps for udp -
then unbinding xdp_fwd program makes kernel to panic :)
>
>> The problem I wanted to discuss here is statistics for XDP context. The
>> short of it is that we need consistency in the counters across NIC
>> drivers and virtual devices. Right now stats are specific to a driver
>> with no clear accounting for the packets and bytes handled in XDP.
>>
>> For example virtio has some stats as device private data extracted via
>> ethtool:
>> $ ethtool -S eth2 | grep xdp
>> ...
>> rx_queue_3_xdp_packets: 5291
>> rx_queue_3_xdp_tx: 0
>> rx_queue_3_xdp_redirects: 5163
>> rx_queue_3_xdp_drops: 0
>> ...
>> tx_queue_3_xdp_tx: 5163
>> tx_queue_3_xdp_tx_drops: 0
>>
>> And the standard counters appear to track bytes and packets for Rx, but
>> not Tx if the packet is forwarded in XDP.
>>
>> Similarly, mlx5 has some counters (thanks to Jesper and Toke for helping
>> out here):
>>
>> $ ethtool -S mlx5p1 | grep xdp
>> rx_xdp_drop: 86468350180
>> rx_xdp_redirect: 18860584
>> rx_xdp_tx_xmit: 0
>> rx_xdp_tx_full: 0
>> rx_xdp_tx_err: 0
>> rx_xdp_tx_cqe: 0
>> tx_xdp_xmit: 0
>> tx_xdp_full: 0
>> tx_xdp_err: 0
>> tx_xdp_cqes: 0
>> ...
>> rx3_xdp_drop: 86468350180
>> rx3_xdp_redirect: 18860556
>> rx3_xdp_tx_xmit: 0
>> rx3_xdp_tx_full: 0
>> rx3_xdp_tx_err: 0
>> rx3_xdp_tx_cqes: 0
>> ...
>> tx0_xdp_xmit: 0
>> tx0_xdp_full: 0
>> tx0_xdp_err: 0
>> tx0_xdp_cqes: 0
>> ...
>>
>> And no accounting in standard stats for packets handled in XDP.
>>
>> And then if I understand Jesper's data correctly, the i40e driver does
>> not have device specific data:
>>
>> $ ethtool -S i40e1 | grep xdp
>> [NOTHING]
>>
>>
>> But rather bumps the standard counters:
>>
>> sudo ./xdp_rxq_info --dev i40e1 --action XDP_DROP
>>
>> Running XDP on dev:i40e1 (ifindex:3) action:XDP_DROP options:no_touch
>> XDP stats CPU pps issue-pps
>> XDP-RX CPU 1 36,156,872 0
>> XDP-RX CPU total 36,156,872
>>
>> RXQ stats RXQ:CPU pps issue-pps
>> rx_queue_index 1:1 36,156,878 0
>> rx_queue_index 1:sum 36,156,878
>>
>>
>> $ ethtool_stats.pl --dev i40e1
>>
>> Show adapter(s) (i40e1) statistics (ONLY that changed!)
>> Ethtool(i40e1 ) stat: 2711292859 ( 2,711,292,859) <= port.rx_bytes /sec
>> Ethtool(i40e1 ) stat: 6274204 ( 6,274,204) <=
>> port.rx_dropped /sec
>> Ethtool(i40e1 ) stat: 42363867 ( 42,363,867) <=
>> port.rx_size_64 /sec
>> Ethtool(i40e1 ) stat: 42363950 ( 42,363,950) <=
>> port.rx_unicast /sec
>> Ethtool(i40e1 ) stat: 2165051990 ( 2,165,051,990) <= rx-1.bytes /sec
>> Ethtool(i40e1 ) stat: 36084200 ( 36,084,200) <= rx-1.packets /sec
>> Ethtool(i40e1 ) stat: 5385 ( 5,385) <= rx_dropped /sec
>> Ethtool(i40e1 ) stat: 36089727 ( 36,089,727) <= rx_unicast /sec
>>
>>
>> We really need consistency in the counters and at a minimum, users
>> should be able to track packet and byte counters for both Rx and Tx
>> including XDP.
>>
>> It seems to me the Rx and Tx packet, byte and dropped counters returned
>> for the standard device stats (/proc/net/dev, ip -s li show, ...) should
>> include all packets managed by the driver regardless of whether they are
>> forwarded / dropped in XDP or go up the Linux stack. This also aligns
>> with mlxsw and the stats it shows which are packets handled by the hardware.
>>
>> From there the private stats can include XDP specifics as desired --
>> like the drops and redirects but that those should be add-ons and even
>> here some consistency makes life easier for users.
>>
>> The same standards should be also be applied to virtual devices built on
>> top of the ports -- e.g, vlans. I have an API now that allows bumping
>> stats for vlan devices.
>>
>> Keeping the basic xdp packets in the standard counters allows Paweł, for
>> example, to continue to monitor /proc/net/dev.
>>
>> Can we get agreement on this? And from there, get updates to the mlx5
>> and virtio drivers?
> I'd say it sounds reasonable to include XDP in the normal traffic
> counters, but having the detailed XDP-specific counters is quite useful
> as well... So can't we do both (for all drivers)?
>
> -Toke
>
^ permalink raw reply
* Re: [PATCH V2 mlx5-next 00/12] mlx5 core generic EQ API for RDMA ODP
From: Jason Gunthorpe @ 2018-11-21 21:32 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Saeed Mahameed, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <20181120181113.GC5394@mtr-leonro.mtl.com>
On Tue, Nov 20, 2018 at 11:11:16AM -0700, Leon Romanovsky wrote:
> On Mon, Nov 19, 2018 at 10:52:30AM -0800, Saeed Mahameed wrote:
> > Hi,
> >
> > This patchset is for mlx5-next shared branch, and will be applied there
> > once the review is done.
> >
> > This patchset introduces mostly refactoring work and EQ related code updates to
> > allow moving the ODP rdma only logic from mlx5_core into mlx5 ib where it
> > belongs, and will allow future updates and optimizations for the rdma ODP
> > (On Demand Paging) feature to go only to rdma tree.
> >
> > Patch #1: Fixes the offsets of stored irq affinity hints inside mlx5
> > irq info array.
> >
> > Patch #2,3,4: Remove unused fields, code and logic
> >
> > Patch #5: Move all EQ related logic from main.c to eq.c to allow clear
> > and seamless refactoring for creating generic EQ management API.
> >
> > Patch #6: Create mlx5 core EQs in one place, in order to have one entry
> > point to call from main file.
> >
> > Patch #7,8: Move EQ related structures into eq_table mlx5 structure and
> > make eq_table fields and logic private to eq.c file.
> >
> > Patch #9,10: Create one generic EQ struct and use it in different
> > EQ types (usages) e.g. (Async, Command, FW pages, completion and ODP)
> > Introduce generic EQ API to allow creating Generic EQs regardless of
> > their types, will be uesd to create all mlx5 core EQs in mlx5_core and
> > ODP EQ in mlx5_ib.
> >
> > Patch #11: Move ODP logic out from mlx5_core eq.c into mlx5 rdma driver.
> > odp.c file.
> >
> > Patch #12: Make the trivial EQE access methods inline.
> >
> > v1->v2:
> > - Remove vertical alignment
> > - Fix spilling "Chip" -> "Cheap"
> >
> > Thanks,
> > Saeed.
> >
> >
> > Saeed Mahameed (12):
> > net/mlx5: EQ, Use the right place to store/read IRQ affinity hint
> > net/mlx5: EQ, Remove unused fields and structures
> > net/mlx5: EQ, No need to store eq index as a field
> > net/mlx5: EQ, Remove redundant completion EQ list lock
> > net/mlx5: EQ, Move all EQ logic to eq.c
> > net/mlx5: EQ, Create all EQs in one place
> > net/mlx5: EQ, irq_info and rmap belong to eq_table
> > net/mlx5: EQ, Privatize eq_table and friends
> > net/mlx5: EQ, Different EQ types
> > net/mlx5: EQ, Generic EQ
> > {net,IB}/mlx5: Move Page fault EQ and ODP logic to RDMA
> > net/mlx5: EQ, Make EQE access methods inline
> >
>
> Pushed to shared mlx5-next:
>
> 6d2d6fc83a28 net/mlx5: EQ, Make EQE access methods inline
> d5d284b829a6 {net,IB}/mlx5: Move Page fault EQ and ODP logic to RDMA
> 7701707cb94e net/mlx5: EQ, Generic EQ
> 16d760839cee net/mlx5: EQ, Different EQ types
> f2f3df550139 net/mlx5: EQ, Privatize eq_table and friends
> d674a9aa4344 net/mlx5: EQ, irq_info and rmap belong to eq_table
> c8e21b3b576b net/mlx5: EQ, Create all EQs in one place
> ca828cb4686f net/mlx5: EQ, Move all EQ logic to eq.c
> aaa553a64438 net/mlx5: EQ, Remove redundant completion EQ list lock
> 2883f352571b net/mlx5: EQ, No need to store eq index as a field
> 4de45c758636 net/mlx5: EQ, Remove unused fields and structures
> 1e86ace4c140 net/mlx5: EQ, Use the right place to store/read IRQ affinity hint
Okay, merged to rdma-next now
Thanks,
Jason
^ permalink raw reply
* Re: [iproute2-next PATCH v4] tc: flower: Classify packets based port ranges
From: David Ahern @ 2018-11-21 21:42 UTC (permalink / raw)
To: Amritha Nambiar, stephen, netdev
Cc: jakub.kicinski, sridhar.samudrala, jhs, xiyou.wangcong, jiri
In-Reply-To: <154278102460.66868.7581842092203300039.stgit@anamhost.jf.intel.com>
On 11/20/18 11:17 PM, Amritha Nambiar wrote:
> diff --git a/tc/f_flower.c b/tc/f_flower.c
> index 65fca04..722647d 100644
> --- a/tc/f_flower.c
> +++ b/tc/f_flower.c
> @@ -494,6 +494,68 @@ static int flower_parse_port(char *str, __u8 ip_proto,
> return 0;
> }
>
> +static int flower_port_range_attr_type(__u8 ip_proto, enum flower_endpoint type,
> + __be16 *min_port_type,
> + __be16 *max_port_type)
> +{
> + if (ip_proto == IPPROTO_TCP || ip_proto == IPPROTO_UDP ||
> + ip_proto == IPPROTO_SCTP) {
> + if (type == FLOWER_ENDPOINT_SRC) {
> + *min_port_type = TCA_FLOWER_KEY_PORT_SRC_MIN;
> + *max_port_type = TCA_FLOWER_KEY_PORT_SRC_MAX;
> + } else {
> + *min_port_type = TCA_FLOWER_KEY_PORT_DST_MIN;
> + *max_port_type = TCA_FLOWER_KEY_PORT_DST_MAX;
> + }
> + } else {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +static int flower_parse_port_range(__be16 *min, __be16 *max, __u8 ip_proto,
why not just min and max directly since they are not set here but only
referenced by value. Also, you do not parse anything in this function so
the helper is misnamed.
But I think this can be done simpler using what was done in ip/iprule.c ...
> + enum flower_endpoint endpoint,
> + struct nlmsghdr *n)
> +{
> + __be16 min_port_type, max_port_type;
> +
> + if (htons(*max) <= htons(*min)) {
> + fprintf(stderr, "max value should be greater than min value\n");
> + return -1;
> + }
> +
> + if (flower_port_range_attr_type(ip_proto, endpoint, &min_port_type,
> + &max_port_type))
> + return -1;
> +
> + addattr16(n, MAX_MSG, min_port_type, *min);
> + addattr16(n, MAX_MSG, max_port_type, *max);
> +
> + return 0;
> +}
> +
> +static int get_range(__be16 *min, __be16 *max, char *argv)
> +{
> + char *r;
> +
> + r = strchr(argv, '-');
> + if (r) {
> + *r = '\0';
> + if (get_be16(min, argv, 10)) {
> + fprintf(stderr, "invalid min range\n");
> + return -1;
> + }
> + if (get_be16(max, r + 1, 10)) {
> + fprintf(stderr, "invalid max range\n");
> + return -1;
> + }
> + } else {
> + return -1;
> + }
> + return 0;
> +}
> +
> #define TCP_FLAGS_MAX_MASK 0xfff
>
> static int flower_parse_tcp_flags(char *str, int flags_type, int mask_type,
> @@ -1061,20 +1123,47 @@ static int flower_parse_opt(struct filter_util *qu, char *handle,
> return -1;
> }
> } else if (matches(*argv, "dst_port") == 0) {
> + __be16 min, max;
> +
> NEXT_ARG();
> - ret = flower_parse_port(*argv, ip_proto,
> - FLOWER_ENDPOINT_DST, n);
> - if (ret < 0) {
> - fprintf(stderr, "Illegal \"dst_port\"\n");
> - return -1;
> +
> + if (!get_range(&min, &max, *argv)) {
> + ret = flower_parse_port_range(&min, &max,
> + ip_proto,
> + FLOWER_ENDPOINT_DST,
> + n);
> + if (ret < 0) {
> + fprintf(stderr, "Illegal \"dst_port range\"\n");
> + return -1;
> + }
> + } else {
> + ret = flower_parse_port(*argv, ip_proto,
> + FLOWER_ENDPOINT_DST, n);
> + if (ret < 0) {
> + fprintf(stderr, "Illegal \"dst_port\"\n");
> + return -1;
> + }
Take a look at ip/iprule.c, line 921:
} else if (strcmp(*argv, "sport") == 0) {
...
}
Using sscanf and handling the ret to be 1 or 2 should simplify the above.
> }
> } else if (matches(*argv, "src_port") == 0) {
> + __be16 min, max;
> +
> NEXT_ARG();
> - ret = flower_parse_port(*argv, ip_proto,
> - FLOWER_ENDPOINT_SRC, n);
> - if (ret < 0) {
> - fprintf(stderr, "Illegal \"src_port\"\n");
> - return -1;
> + if (!get_range(&min, &max, *argv)) {
> + ret = flower_parse_port_range(&min, &max,
> + ip_proto,
> + FLOWER_ENDPOINT_SRC,
> + n);
> + if (ret < 0) {
> + fprintf(stderr, "Illegal \"src_port range\"\n");
> + return -1;
> + }
> + } else {
> + ret = flower_parse_port(*argv, ip_proto,
> + FLOWER_ENDPOINT_SRC, n);
> + if (ret < 0) {
> + fprintf(stderr, "Illegal \"src_port\"\n");
> + return -1;
> + }
> }
> } else if (matches(*argv, "tcp_flags") == 0) {
> NEXT_ARG();
> @@ -1490,6 +1579,22 @@ static void flower_print_port(char *name, struct rtattr *attr)
> print_hu(PRINT_ANY, name, namefrm, rta_getattr_be16(attr));
> }
>
> +static void flower_print_port_range(char *name, struct rtattr *min_attr,
> + struct rtattr *max_attr)
> +{
> + SPRINT_BUF(namefrm);
> + SPRINT_BUF(out);
> + size_t done;
> +
> + if (!min_attr || !max_attr)
> + return;
> +
> + done = sprintf(out, "%u", rta_getattr_be16(min_attr));
> + sprintf(out + done, "-%u", rta_getattr_be16(max_attr));
> + sprintf(namefrm, "\n %s %%s", name);
> + print_string(PRINT_ANY, name, namefrm, out);
> +}
> +
> static void flower_print_tcp_flags(const char *name, struct rtattr *flags_attr,
> struct rtattr *mask_attr)
> {
> @@ -1678,6 +1783,7 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
> struct rtattr *opt, __u32 handle)
> {
> struct rtattr *tb[TCA_FLOWER_MAX + 1];
> + __be16 min_port_type, max_port_type;
> int nl_type, nl_mask_type;
> __be16 eth_type = 0;
> __u8 ip_proto = 0xff;
> @@ -1796,6 +1902,16 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
> if (nl_type >= 0)
> flower_print_port("src_port", tb[nl_type]);
>
> + if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_DST,
> + &min_port_type, &max_port_type))
> + flower_print_port_range("dst_port range",
I am no json expert, but I do not recall any other place where a space
is used in the name field for json output.
Can tc flower use something similar to ip ru with single port or port
range handled like this?
},{
"priority": 32764,
"src": "172.16.1.0",
"srclen": 24,
"ipproto": "tcp",
"sport": 1100,
"table": "main"
},{
"priority": 32765,
"src": "172.16.1.0",
"srclen": 24,
"ipproto": "tcp",
"sport_start": 1000,
"sport_end": 1010,
"table": "main"
},{
> + tb[min_port_type], tb[max_port_type]);
> +
> + if (!flower_port_range_attr_type(ip_proto, FLOWER_ENDPOINT_SRC,
> + &min_port_type, &max_port_type))
> + flower_print_port_range("src_port range",
> + tb[min_port_type], tb[max_port_type]);
> +
> flower_print_tcp_flags("tcp_flags", tb[TCA_FLOWER_KEY_TCP_FLAGS],
> tb[TCA_FLOWER_KEY_TCP_FLAGS_MASK]);
>
>
^ permalink raw reply
* Re: [PATCH net] net/sched: act_police: add missing spinlock initialization
From: Jiri Pirko @ 2018-11-21 21:43 UTC (permalink / raw)
To: Davide Caratti
Cc: Eric Dumazet, Jamal Hadi Salim, Cong Wang, David S. Miller,
netdev, Ivan Vecera
In-Reply-To: <070f7a50d9fe76cb459b28d03fa322ce3dd59cb1.1542819002.git.dcaratti@redhat.com>
Wed, Nov 21, 2018 at 06:23:53PM CET, dcaratti@redhat.com wrote:
>commit f2cbd4852820 ("net/sched: act_police: fix race condition on state
>variables") introduces a new spinlock, but forgets its initialization.
>Ensure that tcf_police_init() initializes 'tcfp_lock' every time a 'police'
>action is newly created, to avoid the following lockdep splat:
>
> INFO: trying to register non-static key.
> the code is fine but needs lockdep annotation.
> turning off the locking correctness validator.
> <...>
> Call Trace:
> dump_stack+0x85/0xcb
> register_lock_class+0x581/0x590
> __lock_acquire+0xd4/0x1330
> ? tcf_police_init+0x2fa/0x650 [act_police]
> ? lock_acquire+0x9e/0x1a0
> lock_acquire+0x9e/0x1a0
> ? tcf_police_init+0x2fa/0x650 [act_police]
> ? tcf_police_init+0x55a/0x650 [act_police]
> _raw_spin_lock_bh+0x34/0x40
> ? tcf_police_init+0x2fa/0x650 [act_police]
> tcf_police_init+0x2fa/0x650 [act_police]
> tcf_action_init_1+0x384/0x4c0
> tcf_action_init+0xf6/0x160
> tcf_action_add+0x73/0x170
> tc_ctl_action+0x122/0x160
> rtnetlink_rcv_msg+0x2a4/0x490
> ? netlink_deliver_tap+0x99/0x400
> ? validate_linkmsg+0x370/0x370
> netlink_rcv_skb+0x4d/0x130
> netlink_unicast+0x196/0x230
> netlink_sendmsg+0x2e5/0x3e0
> sock_sendmsg+0x36/0x40
> ___sys_sendmsg+0x280/0x2f0
> ? _raw_spin_unlock+0x24/0x30
> ? handle_pte_fault+0xafe/0xf30
> ? find_held_lock+0x2d/0x90
> ? syscall_trace_enter+0x1df/0x360
> ? __sys_sendmsg+0x5e/0xa0
> __sys_sendmsg+0x5e/0xa0
> do_syscall_64+0x60/0x210
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x7f1841c7cf10
> Code: c3 48 8b 05 82 6f 2c 00 f7 db 64 89 18 48 83 cb ff eb dd 0f 1f 80 00 00 00 00 83 3d 8d d0 2c 00 00 75 10 b8 2e 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 ae cc 00 00 48 89 04 24
> RSP: 002b:00007ffcf9df4d68 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007f1841c7cf10
> RDX: 0000000000000000 RSI: 00007ffcf9df4dc0 RDI: 0000000000000003
> RBP: 000000005bf56105 R08: 0000000000000002 R09: 00007ffcf9df8edc
> R10: 00007ffcf9df47e0 R11: 0000000000000246 R12: 0000000000671be0
> R13: 00007ffcf9df4e84 R14: 0000000000000008 R15: 0000000000000000
>
>Fixes: f2cbd4852820 ("net/sched: act_police: fix race condition on state variables")
>Reported-by: Cong Wang <xiyou.wangcong@gmail.com>
>Signed-off-by: Davide Caratti <dcaratti@redhat.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* Re: [PATCH] net: amd: add of_node_put()
From: Sergei Shtylyov @ 2018-11-22 8:28 UTC (permalink / raw)
To: Yangtao Li, davem, yuehaibing; +Cc: netdev, linux-kernel
In-Reply-To: <20181121125824.22168-1-tiny.windzz@gmail.com>
Hello!
On 21.11.2018 15:58, Yangtao Li wrote:
> of_find_node_by_path() acquires a reference to the node
> returned by it and that reference needs to be dropped by its caller.
> bl_idle_init() doesn't do that, so fix it.
Again, maybe sparc_lance_probe_one() doesn't do that?
> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com>
> ---
> drivers/net/ethernet/amd/sunlance.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/amd/sunlance.c b/drivers/net/ethernet/amd/sunlance.c
> index b4fc0ed5bce8..9d4899826823 100644
> --- a/drivers/net/ethernet/amd/sunlance.c
> +++ b/drivers/net/ethernet/amd/sunlance.c
> @@ -1419,7 +1419,7 @@ static int sparc_lance_probe_one(struct platform_device *op,
>
> prop = of_get_property(nd, "tpe-link-test?", NULL);
> if (!prop)
> - goto no_link_test;
> + goto node_put;
>
> if (strcmp(prop, "true")) {
> printk(KERN_NOTICE "SunLance: warning: overriding option "
> @@ -1428,6 +1428,8 @@ static int sparc_lance_probe_one(struct platform_device *op,
> "to ecd@skynet.be\n");
> auxio_set_lte(AUXIO_LTE_ON);
> }
> +node_put:
> + of_node_put(nd);
> no_link_test:
> lp->auto_select = 1;
> lp->tpe = 0;
MBR, Sergei
^ permalink raw reply
* [PATCH bpf] tools: bpftool: fix potential NULL pointer dereference in do_load
From: Jakub Kicinski @ 2018-11-21 21:53 UTC (permalink / raw)
To: alexei.starovoitov, daniel
Cc: oss-drivers, netdev, Jakub Kicinski, Wen Yang, Julia Lawall
This patch fixes a possible null pointer dereference in
do_load, detected by the semantic patch deref_null.cocci,
with the following warning:
./tools/bpf/bpftool/prog.c:1021:23-25: ERROR: map_replace is NULL but dereferenced.
The following code has potential null pointer references:
881 map_replace = reallocarray(map_replace, old_map_fds + 1,
882 sizeof(*map_replace));
883 if (!map_replace) {
884 p_err("mem alloc failed");
885 goto err_free_reuse_maps;
886 }
...
1019 err_free_reuse_maps:
1020 for (i = 0; i < old_map_fds; i++)
1021 close(map_replace[i].fd);
1022 free(map_replace);
Fixes: 3ff5a4dc5d89 ("tools: bpftool: allow reuse of maps with bpftool prog load")
Co-developed-by: Wen Yang <wen.yang99@zte.com.cn>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
CC: Wen Yang <wen.yang99@zte.com.cn>
CC: Julia Lawall <julia.lawall@lip6.fr>
---
tools/bpf/bpftool/prog.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 5302ee282409..b3c32ab8c1dc 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -845,6 +845,7 @@ static int do_load(int argc, char **argv)
}
NEXT_ARG();
} else if (is_prefix(*argv, "map")) {
+ void *new_map_replace;
char *endptr, *name;
int fd;
@@ -878,12 +879,15 @@ static int do_load(int argc, char **argv)
if (fd < 0)
goto err_free_reuse_maps;
- map_replace = reallocarray(map_replace, old_map_fds + 1,
- sizeof(*map_replace));
- if (!map_replace) {
+ new_map_replace = reallocarray(map_replace,
+ old_map_fds + 1,
+ sizeof(*map_replace));
+ if (!new_map_replace) {
p_err("mem alloc failed");
goto err_free_reuse_maps;
}
+ map_replace = new_map_replace;
+
map_replace[old_map_fds].idx = idx;
map_replace[old_map_fds].name = name;
map_replace[old_map_fds].fd = fd;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next v3] libbpf: make sure bpf headers are c++ include-able
From: Daniel Borkmann @ 2018-11-21 22:17 UTC (permalink / raw)
To: Stanislav Fomichev, netdev, ast; +Cc: yhs
In-Reply-To: <20181121172944.146320-1-sdf@google.com>
On 11/21/2018 06:29 PM, Stanislav Fomichev wrote:
> Wrap headers in extern "C", to turn off C++ mangling.
> This simplifies including libbpf in c++ and linking against it.
>
> v2 changes:
> * do the same for btf.h
>
> v3 changes:
> * test_libbpf.cpp to test for possible future c++ breakages
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] libbpf: Add version script for DSO
From: Alexei Starovoitov @ 2018-11-21 22:22 UTC (permalink / raw)
To: Yonghong Song, Andrey Ignatov, netdev@vger.kernel.org
Cc: ast@kernel.org, daniel@iogearbox.net, Kernel Team
In-Reply-To: <7d24f2fd-0cba-e6dc-8b12-881583978ba9@fb.com>
On 11/21/18 12:18 PM, Yonghong Song wrote:
>
>
> On 11/21/18 9:40 AM, Andrey Ignatov wrote:
>> More and more projects use libbpf and one day it'll likely be packaged
>> and distributed as DSO and that requires ABI versioning so that both
>> compatible and incompatible changes to ABI can be introduced in a safe
>> way in the future without breaking executables dynamically linked with a
>> previous version of the library.
>>
>> Usual way to do ABI versioning is version script for the linker. Add
>> such a script for libbpf. All global symbols currently exported via
>> LIBBPF_API macro are added to the version script libbpf.map.
>>
>> The version name LIBBPF_0.0.1 is constructed from the name of the
>> library + version specified by $(LIBBPF_VERSION) in Makefile.
>>
>> Version script does not duplicate the work done by LIBBPF_API macro, it
>> rather complements it. The macro is used at compile time and can be used
>> by compiler to do optimization that can't be done at link time, it is
>> purely about global symbol visibility. The version script, in turn, is
>> used at link time and takes care of ABI versioning. Both techniques are
>> described in details in [1].
>>
>> Whenever ABI is changed in the future, version script should be changed
>> appropriately.
>
> Maybe we should clarify the policy of how version numbers should be
> change? Each commit which changes default global symbol ABI? Each kernel
> release?
>
>>
>> [1] https://www.akkadia.org/drepper/dsohowto.pdf
>>
>> Signed-off-by: Andrey Ignatov <rdna@fb.com>
>> ---
>> tools/lib/bpf/Makefile | 4 +-
>> tools/lib/bpf/libbpf.map | 120 +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 123 insertions(+), 1 deletion(-)
>> create mode 100644 tools/lib/bpf/libbpf.map
>>
>> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
>> index 425b480bda75..d76c41fa2d39 100644
>> --- a/tools/lib/bpf/Makefile
>> +++ b/tools/lib/bpf/Makefile
>> @@ -145,6 +145,7 @@ include $(srctree)/tools/build/Makefile.include
>>
>> BPF_IN := $(OUTPUT)libbpf-in.o
>> LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
>> +VERSION_SCRIPT := libbpf.map
>>
>> CMD_TARGETS = $(LIB_FILE)
>>
>> @@ -170,7 +171,8 @@ $(BPF_IN): force elfdep bpfdep
>> $(Q)$(MAKE) $(build)=libbpf
>>
>> $(OUTPUT)libbpf.so: $(BPF_IN)
>> - $(QUIET_LINK)$(CC) --shared $^ -o $@
>> + $(QUIET_LINK)$(CC) --shared -Wl,--version-script=$(VERSION_SCRIPT) \
>> + $^ -o $@
>>
>> $(OUTPUT)libbpf.a: $(BPF_IN)
>> $(QUIET_LINK)$(RM) $@; $(AR) rcs $@ $^
>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>> new file mode 100644
>> index 000000000000..9fe416b68c7d
>> --- /dev/null
>> +++ b/tools/lib/bpf/libbpf.map
>> @@ -0,0 +1,120 @@
>> +LIBBPF_0.0.1 {
>> + global:
>> + bpf_btf_get_fd_by_id;
>
> Do you think we could use this opportunities to
> make naming more consistent? For example,
> bpf_btf_get_fd_by_id => btf__get_fd_by_id?
I think this one is fine since it matches
bpf_[map|prog]_get_fd_by_id()
and it's a wrapper.
>> + bpf_create_map;
>> + bpf_create_map_in_map;
>> + bpf_create_map_in_map_node;
>> + bpf_create_map_name;
>> + bpf_create_map_node;
>> + bpf_create_map_xattr;
>> + bpf_load_btf;
>> + bpf_load_program;
>> + bpf_load_program_xattr;
>> + bpf_map__btf_key_type_id;
>> + bpf_map__btf_value_type_id;
>> + bpf_map__def;
>> + bpf_map_delete_elem; > + bpf_map__fd;
>> + bpf_map_get_fd_by_id;
>> + bpf_map_get_next_id;
>> + bpf_map_get_next_key; > + bpf_map__is_offload_neutral;
>> + bpf_map_lookup_and_delete_elem;
>> + bpf_map_lookup_elem;
>> + bpf_map__name;
>> + bpf_map__next;
>> + bpf_map__pin;
>> + bpf_map__prev;
>> + bpf_map__priv;
>> + bpf_map__reuse_fd;
>> + bpf_map__set_ifindex;
>> + bpf_map__set_priv;
>> + bpf_map__unpin;
>> + bpf_map_update_elem;
>> + bpf_object__btf_fd;
>> + bpf_object__close;
>> + bpf_object__find_map_by_name;
>> + bpf_object__find_map_by_offset;
>> + bpf_object__find_program_by_title;
>> + bpf_object__kversion;
>> + bpf_object__load;
>> + bpf_object__name;
>> + bpf_object__next;
>> + bpf_object__open;
>> + bpf_object__open_buffer;
>> + bpf_object__open_xattr;
>> + bpf_object__pin;
>> + bpf_object__pin_maps;
>> + bpf_object__pin_programs;
>> + bpf_object__priv;
>> + bpf_object__set_priv;
>> + bpf_object__unload;
>> + bpf_object__unpin_maps;
>> + bpf_object__unpin_programs;
>> + bpf_obj_get;
>> + bpf_obj_get_info_by_fd;
>> + bpf_obj_pin;
>> + bpf_perf_event_read_simple;
>> + bpf_prog_attach;
>> + bpf_prog_detach;
>> + bpf_prog_detach2;
>> + bpf_prog_get_fd_by_id;
>> + bpf_prog_get_next_id;
>> + bpf_prog_load;
>> + bpf_prog_load_xattr;
>> + bpf_prog_query;
>> + bpf_program__fd;
>> + bpf_program__is_kprobe;
>> + bpf_program__is_perf_event;
>> + bpf_program__is_raw_tracepoint;
>> + bpf_program__is_sched_act;
>> + bpf_program__is_sched_cls;
>> + bpf_program__is_socket_filter;
>> + bpf_program__is_tracepoint;
>> + bpf_program__is_xdp;
>> + bpf_program__load;
>> + bpf_program__next;
>> + bpf_program__nth_fd;
>> + bpf_program__pin;
>> + bpf_program__pin_instance;
>> + bpf_program__prev;
>> + bpf_program__priv;
>> + bpf_program__set_expected_attach_type;
>> + bpf_program__set_ifindex;
>> + bpf_program__set_kprobe;
>> + bpf_program__set_perf_event;
>> + bpf_program__set_prep;
>> + bpf_program__set_priv;
>> + bpf_program__set_raw_tracepoint;
>> + bpf_program__set_sched_act;
>> + bpf_program__set_sched_cls;
>> + bpf_program__set_socket_filter;
>> + bpf_program__set_tracepoint;
>> + bpf_program__set_type;
>> + bpf_program__set_xdp;
>> + bpf_program__title;
>> + bpf_program__unload;
>> + bpf_program__unpin;
>> + bpf_program__unpin_instance;
>> + bpf_prog_test_run;
>> + bpf_raw_tracepoint_open;
>> + bpf_set_link_xdp_fd;
>> + bpf_task_fd_query;
>> + bpf_verify_program;
>> + btf__fd;
>> + btf__find_by_name;
>> + btf__free;
>> + btf_get_from_id;
> btf_get_from_id => btf__get_from_id?
this one makes sense indeed. Martin, thoughts?
>> + btf__name_by_offset;
>> + btf__new;
>> + btf__resolve_size;
>> + btf__resolve_type;
>> + btf__type_by_id;
>> + libbpf_attach_type_by_name;
>> + libbpf_get_error;
>> + libbpf_prog_type_by_name;
>> + libbpf_set_print;
>> + libbpf_strerror;
>
> Anything else? We have btf__, bpf_program__ prefixes with double "_"
> while with libbpf_, bpf_<wrapper> as single "_". Not sure whether they
> need change or not.
the convention is that syscall wrappers like bpf_map_lookup_elem()
have single _ and map one-to-one to syscall commands.
Double _ is for objects. That's a coding style of perf.
Today btf, bpf_program, bpf_map, bpf_objects are objects.
libbpf_ is a prefix for auxiliary funcs.
We need to document it in a readme file.
^ permalink raw reply
* Re: [PATCH v2 14/14] nvme-tcp: add NVMe over TCP host driver
From: Sagi Grimberg @ 2018-11-21 22:27 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvme, linux-block, netdev, David S. Miller, Keith Busch
In-Reply-To: <20181121085645.GA29747@lst.de>
>> +struct nvme_tcp_send_ctx {
>> + struct bio *curr_bio;
>> + struct iov_iter iter;
>> + size_t offset;
>> + size_t data_sent;
>> + enum nvme_tcp_send_state state;
>> +};
>> +
>> +struct nvme_tcp_recv_ctx {
>> + struct iov_iter iter;
>> + struct bio *curr_bio;
>> +};
>
> I don't understand these structures. There should only be
> a bio to be send or receive, not both. Why do we need two
> curr_bio pointers?
We don't really need both...
> To me it seems like both structures should just go away and
> move into nvme_tcp_request ala:
>
>
> struct bio *curr_bio;
>
> /* send state */
> struct iov_iter send_iter;
> size_t send_offset;
> enum nvme_tcp_send_state send_state;
> size_t data_sent;
>
> /* receive state */
> struct iov_iter recv_iter;
>
Sure, will move this.
^ permalink raw reply
* Re: Issue with RTL8111 NIC after upgrade to kernel 4.19
From: Heiner Kallweit @ 2018-11-21 22:28 UTC (permalink / raw)
To: Marc Dionne
Cc: andrew, norbert.jurkeit, nic_swsd, Florian Fainelli, David Miller,
netdev, Linux Kernel Mailing List, michael.wiktowy, jcline
In-Reply-To: <CAB9dFdvw8QqzaVYV3bOMGtACyvqWnE9uy96m1tu2C+jjzQR9_Q@mail.gmail.com>
On 21.11.2018 22:53, Marc Dionne wrote:
> On Wed, Nov 21, 2018 at 4:52 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 21.11.2018 21:49, Heiner Kallweit wrote:
>>> On 21.11.2018 21:32, Heiner Kallweit wrote:
>>>> On 21.11.2018 21:20, Andrew Lunn wrote:
>>>>>> request_module() is supposed to be synchronous, however after some
>>>>>> reading this may not be 100% guaranteed. Maybe the module init
>>>>>> function on some systems isn't finished yet when request_module()
>>>>>> returns. As a result the genphy driver may be used instead of
>>>>>> the PHY version-specific driver.
>>>>>
>>>>> Hi Heiner
>>>>>
>>>>> That would be true for all PHYs i think. We would of noticed this
>>>>> problem with other systems using other PHY drivers.
>>>>>
>>>>> Andrew
>>>>>
>>>> It could be a timing issue affecting certain systems only. At least
>>>> for now I don't have a good explanation why loading the module via
>>>> request_module() and loading it upfront manually makes a difference.
>>>>
>>>> One affected user just reported the PHY to be a RTL8211B. This is
>>>> what I expected, because this PHY crashes when writing to the MMD
>>>> registers (the MMD registers are used otherwise by this PHY).
>>>> See also commit 0231b1a074c6 ("net: phy: realtek: Use the dummy
>>>> stubs for MMD register access for rtl8211b").
>>>>
>>>> Let's see whether the other affected systems use the same PHY
>>>> version.
>>>>
>>> Next report is also about a RTL8211B and as I assumed:
>>> - W/o manually loading the realtek module the genphy driver is used
>>> and network fails.
>>> - W/ manually loading the realtek module the proper RTL8211B PHY
>>> driver is used and network works.
>>>
>>> So it seems that even after request_module() the PHY driver isn't
>>> yet available when device and driver are matched.
>>>
>>> If further reports support this (pre-)analysis, then indeed it
>>> seems to be a timing issue and a proper fix most likely is
>>> difficult. As a workaround I could imagine to add a delay loop
>>> after request_module() checking for a Realtek PHY driver via
>>> driver_find(). When adding one small delay after this we should
>>> be sufficiently sure that all Realtek PHY drivers are registered.
>>>
>> Uups, no. We talk about phylib here, not about the r8169 driver.
>> So we need a different solution.
>>
>>>> Heiner
>
> Thanks for the explanation, better than my crude attempt at
> understanding what was going on.
>
> If you have any proposed fixes or diagnostic patches based on current
> mainline I can quickly compile and test them here on an affected
> system. It doesn't fail consistently for me (as others have
> reported), but that could be because it depends on the timing.
>
Thanks for the offer. Can you try the following diagnostic patch
and check whether it helps?
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 55202a0ac..84f417f8b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -607,6 +607,8 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
*/
request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT, MDIO_ID_ARGS(phy_id));
+ msleep(1000);
+
device_initialize(&mdiodev->dev);
return dev;
> Marc
>
^ permalink raw reply related
* Re: [PATCH v2 14/14] nvme-tcp: add NVMe over TCP host driver
From: Sagi Grimberg @ 2018-11-21 22:28 UTC (permalink / raw)
To: Mikhail Skorzhinskii
Cc: linux-nvme, linux-block, netdev, David S. Miller, Keith Busch,
Christoph Hellwig
In-Reply-To: <8736rufxmg.fsf@solarflare.com>
On 11/21/18 4:01 AM, Mikhail Skorzhinskii wrote:
> Sagi Grimberg <sagi@grimberg.me> writes:
> > +static inline void nvme_tcp_queue_request(struct nvme_tcp_request *req)
> > +{
> > + struct nvme_tcp_queue *queue = req->queue;
> > +
> > + spin_lock_bh(&queue->lock);
> > + list_add_tail(&req->entry, &queue->send_list);
> > + spin_unlock_bh(&queue->lock);
> > +
> > + queue_work_on(queue->io_cpu, nvme_tcp_wq, &queue->io_work);
> > +}
>
> May be I missing something, but why bother with bottom half version of
> locking?
>
> There are few places where this lock could be accessed:
>
> (1) From ->queue_rq() call;
> (2) From submitting new AEN request;
> (3) From receiving new R2T;
>
> Which one if these originates from bottom half? Not 100% about queue_rq
> data path, but (2) and (3) looks perfectly safe for me.
Actually, (3) in former versions was invoked in a soft-irq context which
was why this included the bh disable, but I guess it can be removed now.
^ permalink raw reply
* Re: [PATCH v2 07/14] nvme-core: add work elements to struct nvme_ctrl
From: Sagi Grimberg @ 2018-11-21 22:28 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvme, linux-block, netdev, David S. Miller, Keith Busch
In-Reply-To: <20181121130432.GB1373@lst.de>
>> connect_work and err_work will be reused by nvme-tcp so
>> share those in nvme_ctrl for rdma and fc to share.
>
> As said before when you sent this invididually: I'd rather not move
> struct members to the common struture until we actually start using
> it there. So for now please add it to the containing TCP-specific
> structure, although I'm looking forward to actually see code around
> it consolidated eventually.
FFFine....
^ permalink raw reply
* Re: [PATCH v2 13/14] nvmet-tcp: add NVMe over TCP target driver
From: Sagi Grimberg @ 2018-11-21 22:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvme, linux-block, netdev, David S. Miller, Keith Busch
In-Reply-To: <20181121130712.GG1373@lst.de>
>> #define NVME_TCP_DISC_PORT 8009
>> #define NVME_TCP_ADMIN_CCSZ SZ_8K
>> +#define NVME_TCP_DIGEST_LENGTH 4
>
> This should go into the previous patch.
Correct.
^ permalink raw reply
* Re: [PATCH bpf-next v3 1/3] bpf, libbpf: introduce bpf_object__probe_caps to test BPF capabilities
From: Daniel Borkmann @ 2018-11-21 22:30 UTC (permalink / raw)
To: Stanislav Fomichev, netdev, ast
In-Reply-To: <20181121011121.159355-1-sdf@google.com>
On 11/21/2018 02:11 AM, Stanislav Fomichev wrote:
> It currently only checks whether kernel supports map/prog names.
> This capability check will be used in the next two commits to skip setting
> prog/map names.
>
> Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Looks great, thanks for following through. Applied, thanks!
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox