* Re: [PATCH net-next 04/12] net: sched: add tcf_block_setup()
From: Jiri Pirko @ 2019-06-26 12:12 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, netfilter-devel, 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, marcelo.leitner, mkubecek,
venkatkumar.duvvuru, cphealy
In-Reply-To: <20190625083154.jfzhh22zsl3fu2ik@salvia>
Tue, Jun 25, 2019 at 10:31:54AM CEST, pablo@netfilter.org wrote:
>On Fri, Jun 21, 2019 at 07:16:03PM +0200, Jiri Pirko wrote:
>> Thu, Jun 20, 2019 at 09:49:09PM CEST, pablo@netfilter.org wrote:
>>
>> [...]
>>
>> >
>> >+static LIST_HEAD(tcf_block_cb_list);
>>
>> I still don't like the global list. Have to go throught the code more
>> carefully, but why you can't pass the priv/ctx from tc/netfilter. From
>> tc it would be tcf_block as it is now, from netfilter something else.
>
>This tcf_block_cb_list should go away at some point, once drivers know
>how to deal with multiple subsystems using the setup block
>infrastructure. As I said in my previous email, only one can set up
>the block at this stage, the ones coming later will hit busy.
The driver should know if it can bind or is busy. Also, the bind cmd
should contain type of binder (tc/nft/whatever) or perhaps rather binder
priority (according to the hook order in rx/tx).
^ permalink raw reply
* linux-next: manual merge of the akpm tree with the net tree
From: Stephen Rothwell @ 2019-06-26 12:02 UTC (permalink / raw)
To: Andrew Morton, David Miller, Networking
Cc: Linux Next Mailing List, Linux Kernel Mailing List,
Eiichi Tsukata, Matteo Croce
[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]
Hi all,
Today's linux-next merge of the akpm tree got a conflict in:
net/ipv6/route.c
between commit:
b8e8a86337c2 ("net/ipv6: Fix misuse of proc_dointvec "skip_notify_on_dev_down"")
from the net tree and patch:
"proc/sysctl: add shared variables for range check"
from the akpm tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
--
Cheers,
Stephen Rothwell
diff --cc net/ipv6/route.c
index a0994415484e,c5125cdff32c..000000000000
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@@ -6077,9 -6074,9 +6074,9 @@@ static struct ctl_table ipv6_route_tabl
.data = &init_net.ipv6.sysctl.skip_notify_on_dev_down,
.maxlen = sizeof(int),
.mode = 0644,
- .proc_handler = proc_dointvec,
+ .proc_handler = proc_dointvec_minmax,
- .extra1 = &zero,
- .extra2 = &one,
+ .extra1 = SYSCTL_ZERO,
+ .extra2 = SYSCTL_ONE,
},
{ }
};
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH net-next 1/5] net: sched: em_ipt: match only on ip/ipv6 traffic
From: Nikolay Aleksandrov @ 2019-06-26 11:58 UTC (permalink / raw)
To: netdev
Cc: roopa, pablo, xiyou.wangcong, davem, jiri, jhs, eyal.birger,
Nikolay Aleksandrov
In-Reply-To: <20190626115855.13241-1-nikolay@cumulusnetworks.com>
Restrict matching only to ip/ipv6 traffic and make sure we can use the
headers, otherwise matches will be attempted on any protocol which can
be unexpected by the xt matches. Currently policy supports only ipv4/6.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/sched/em_ipt.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index 243fd22f2248..64dbafe4e94c 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -185,6 +185,19 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
struct nf_hook_state state;
int ret;
+ switch (tc_skb_protocol(skb)) {
+ case htons(ETH_P_IP):
+ if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
+ return 0;
+ break;
+ case htons(ETH_P_IPV6):
+ if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
+ return 0;
+ break;
+ default:
+ return 0;
+ }
+
rcu_read_lock();
if (skb->skb_iif)
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 4/5] net: sched: em_ipt: keep the user-specified nfproto and use it
From: Nikolay Aleksandrov @ 2019-06-26 11:58 UTC (permalink / raw)
To: netdev
Cc: roopa, pablo, xiyou.wangcong, davem, jiri, jhs, eyal.birger,
Nikolay Aleksandrov
In-Reply-To: <20190626115855.13241-1-nikolay@cumulusnetworks.com>
For NFPROTO_UNSPEC xt_matches there's no way to restrict the matching to a
specific family, in order to do so we record the user-specified family
and later enforce it while doing the match.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/sched/em_ipt.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index d4257f5f1d94..cfb93ce340da 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -21,6 +21,7 @@
struct em_ipt_match {
const struct xt_match *match;
u32 hook;
+ u8 nfproto;
u8 match_data[0] __aligned(8);
};
@@ -115,6 +116,7 @@ static int em_ipt_change(struct net *net, void *data, int data_len,
struct em_ipt_match *im = NULL;
struct xt_match *match;
int mdata_len, ret;
+ u8 nfproto;
ret = nla_parse_deprecated(tb, TCA_EM_IPT_MAX, data, data_len,
em_ipt_policy, NULL);
@@ -125,6 +127,16 @@ static int em_ipt_change(struct net *net, void *data, int data_len,
!tb[TCA_EM_IPT_MATCH_DATA] || !tb[TCA_EM_IPT_NFPROTO])
return -EINVAL;
+ nfproto = nla_get_u8(tb[TCA_EM_IPT_NFPROTO]);
+ switch (nfproto) {
+ case NFPROTO_IPV4:
+ case NFPROTO_IPV6:
+ case NFPROTO_UNSPEC:
+ break;
+ default:
+ return -EINVAL;
+ }
+
match = get_xt_match(tb);
if (IS_ERR(match)) {
pr_err("unable to load match\n");
@@ -140,6 +152,7 @@ static int em_ipt_change(struct net *net, void *data, int data_len,
im->match = match;
im->hook = nla_get_u32(tb[TCA_EM_IPT_HOOK]);
+ im->nfproto = nfproto;
nla_memcpy(im->match_data, tb[TCA_EM_IPT_MATCH_DATA], mdata_len);
ret = check_match(net, im, mdata_len);
@@ -187,16 +200,16 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
switch (tc_skb_protocol(skb)) {
case htons(ETH_P_IP):
- if (im->match->family != NFPROTO_UNSPEC &&
- im->match->family != NFPROTO_IPV4)
+ if (im->nfproto != NFPROTO_UNSPEC &&
+ im->nfproto != NFPROTO_IPV4)
return 0;
if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
return 0;
state.pf = NFPROTO_IPV4;
break;
case htons(ETH_P_IPV6):
- if (im->match->family != NFPROTO_UNSPEC &&
- im->match->family != NFPROTO_IPV6)
+ if (im->nfproto != NFPROTO_UNSPEC &&
+ im->nfproto != NFPROTO_IPV6)
return 0;
if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
return 0;
@@ -234,7 +247,7 @@ static int em_ipt_dump(struct sk_buff *skb, struct tcf_ematch *em)
return -EMSGSIZE;
if (nla_put_u8(skb, TCA_EM_IPT_MATCH_REVISION, im->match->revision) < 0)
return -EMSGSIZE;
- if (nla_put_u8(skb, TCA_EM_IPT_NFPROTO, im->match->family) < 0)
+ if (nla_put_u8(skb, TCA_EM_IPT_NFPROTO, im->nfproto) < 0)
return -EMSGSIZE;
if (nla_put(skb, TCA_EM_IPT_MATCH_DATA,
im->match->usersize ?: im->match->matchsize,
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 5/5] net: sched: em_ipt: add support for addrtype matching
From: Nikolay Aleksandrov @ 2019-06-26 11:58 UTC (permalink / raw)
To: netdev
Cc: roopa, pablo, xiyou.wangcong, davem, jiri, jhs, eyal.birger,
Nikolay Aleksandrov
In-Reply-To: <20190626115855.13241-1-nikolay@cumulusnetworks.com>
Allow em_ipt to use addrtype for matching. Restrict the use only to
revision 1 which has IPv6 support. Since it's a NFPROTO_UNSPEC xt match
we use the user-specified nfproto for matching, in case it's unspecified
both v4/v6 will be matched by the rule.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/sched/em_ipt.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index cfb93ce340da..ce0798f6f1f7 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -72,11 +72,25 @@ static int policy_validate_match_data(struct nlattr **tb, u8 mrev)
return 0;
}
+static int addrtype_validate_match_data(struct nlattr **tb, u8 mrev)
+{
+ if (mrev != 1) {
+ pr_err("only addrtype match revision 1 supported");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static const struct em_ipt_xt_match em_ipt_xt_matches[] = {
{
.match_name = "policy",
.validate_match_data = policy_validate_match_data
},
+ {
+ .match_name = "addrtype",
+ .validate_match_data = addrtype_validate_match_data
+ },
{}
};
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 2/5] net: sched: em_ipt: set the family based on the protocol when matching
From: Nikolay Aleksandrov @ 2019-06-26 11:58 UTC (permalink / raw)
To: netdev
Cc: roopa, pablo, xiyou.wangcong, davem, jiri, jhs, eyal.birger,
Nikolay Aleksandrov
In-Reply-To: <20190626115855.13241-1-nikolay@cumulusnetworks.com>
Set the family based on the protocol otherwise protocol-neutral matches
will have wrong information (e.g. NFPROTO_UNSPEC). In preparation for
using NFPROTO_UNSPEC xt matches.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/sched/em_ipt.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index 64dbafe4e94c..23965a071177 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -189,10 +189,12 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
case htons(ETH_P_IP):
if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
return 0;
+ state.pf = NFPROTO_IPV4;
break;
case htons(ETH_P_IPV6):
if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
return 0;
+ state.pf = NFPROTO_IPV6;
break;
default:
return 0;
@@ -203,7 +205,7 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
if (skb->skb_iif)
indev = dev_get_by_index_rcu(em->net, skb->skb_iif);
- nf_hook_state_init(&state, im->hook, im->match->family,
+ nf_hook_state_init(&state, im->hook, state.pf,
indev ?: skb->dev, skb->dev, NULL, em->net, NULL);
acpar.match = im->match;
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 3/5] net: sched: em_ipt: restrict matching to the respective protocol
From: Nikolay Aleksandrov @ 2019-06-26 11:58 UTC (permalink / raw)
To: netdev
Cc: roopa, pablo, xiyou.wangcong, davem, jiri, jhs, eyal.birger,
Nikolay Aleksandrov
In-Reply-To: <20190626115855.13241-1-nikolay@cumulusnetworks.com>
Currently a match will continue even if the user-specified nfproto
doesn't match the packet's, so restrict it only to when they're equal or
the protocol is unspecified.
Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
net/sched/em_ipt.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/net/sched/em_ipt.c b/net/sched/em_ipt.c
index 23965a071177..d4257f5f1d94 100644
--- a/net/sched/em_ipt.c
+++ b/net/sched/em_ipt.c
@@ -187,11 +187,17 @@ static int em_ipt_match(struct sk_buff *skb, struct tcf_ematch *em,
switch (tc_skb_protocol(skb)) {
case htons(ETH_P_IP):
+ if (im->match->family != NFPROTO_UNSPEC &&
+ im->match->family != NFPROTO_IPV4)
+ return 0;
if (!pskb_network_may_pull(skb, sizeof(struct iphdr)))
return 0;
state.pf = NFPROTO_IPV4;
break;
case htons(ETH_P_IPV6):
+ if (im->match->family != NFPROTO_UNSPEC &&
+ im->match->family != NFPROTO_IPV6)
+ return 0;
if (!pskb_network_may_pull(skb, sizeof(struct ipv6hdr)))
return 0;
state.pf = NFPROTO_IPV6;
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 0/5] em_ipt: add support for addrtype
From: Nikolay Aleksandrov @ 2019-06-26 11:58 UTC (permalink / raw)
To: netdev
Cc: roopa, pablo, xiyou.wangcong, davem, jiri, jhs, eyal.birger,
Nikolay Aleksandrov
Hi,
We would like to be able to use the addrtype from tc for ACL rules and
em_ipt seems the best place to add support for the already existing xt
match. The biggest issue is that addrtype revision 1 (with ipv6 support)
is NFPROTO_UNSPEC and currently em_ipt can't differentiate between v4/v6
if such xt match is used because it passes the match's family instead of
the user-specified one. The first 4 patches make em_ipt match only on IP
traffic (currently both policy and addrtype recognize such traffic
only) and make it pass the actual packet's protocol instead of the xt
match family. They also add support for NFPROTO_UNSPEC xt matches.
The last patch allows to add addrtype rules via em_ipt.
Thank you,
Nikolay Aleksandrov
Nikolay Aleksandrov (5):
net: sched: em_ipt: match only on ip/ipv6 traffic
net: sched: em_ipt: set the family based on the protocol when matching
net: sched: em_ipt: restrict matching to the respective protocol
net: sched: em_ipt: keep the user-specified nfproto and use it
net: sched: em_ipt: add support for addrtype matching
net/sched/em_ipt.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 50 insertions(+), 2 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [for-next V2 08/10] linux/dim: Implement rdma_dim
From: Or Gerlitz @ 2019-06-26 11:57 UTC (permalink / raw)
To: Sagi Grimberg
Cc: Saeed Mahameed, David S. Miller, Doug Ledford, Jason Gunthorpe,
Leon Romanovsky, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Yamin Friedman, Max Gurtovoy,
Idan Burstein, Or Gerlitz
In-Reply-To: <bfa2159e-1576-6b3c-c85b-ee98bd4f9a47@grimberg.me>
On Wed, Jun 26, 2019 at 1:03 AM Sagi Grimberg <sagi@grimberg.me> wrote:
>
> > +void rdma_dim(struct dim *dim, u64 completions)
> > +{
> > + struct dim_sample *curr_sample = &dim->measuring_sample;
> > + struct dim_stats curr_stats;
> > + u32 nevents;
> > +
> > + dim_update_sample_with_comps(curr_sample->event_ctr + 1,
> > + curr_sample->pkt_ctr,
> > + curr_sample->byte_ctr,
> > + curr_sample->comp_ctr + completions,
> > + &dim->measuring_sample);
>
> If this is the only caller, why add pkt_ctr and byte_ctr at all?
Hi Sagi,
Thanks for the fast review and feedback, other than the default per
ib/rdma device setup for rdma
dim / adaptive-moderation for which Idan commented on (and lets
discuss it there please) seems
the rest of the comments are fine and Yamin will respond / address
them in the coming days.
Or.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 0/3] libbpf: add perf buffer abstraction and API
From: Toke Høiland-Jørgensen @ 2019-06-26 11:55 UTC (permalink / raw)
To: Andrii Nakryiko, andrii.nakryiko, ast, daniel, bpf, netdev,
kernel-team
Cc: Andrii Nakryiko
In-Reply-To: <20190626061235.602633-1-andriin@fb.com>
Andrii Nakryiko <andriin@fb.com> writes:
> This patchset adds a high-level API for setting up and polling perf buffers
> associated with BPF_MAP_TYPE_PERF_EVENT_ARRAY map. Details of APIs are
> described in corresponding commit.
>
> Patch #1 adds a set of APIs to set up and work with perf buffer.
> Patch #2 enhances libbpf to supprot auto-setting PERF_EVENT_ARRAY map size.
> Patch #3 adds test.
Having this in libbpf is great! Do you have a usage example of how a
program is supposed to read events from the buffer? This is something we
would probably want to add to the XDP tutorial
-Toke
^ permalink raw reply
* Re: XDP multi-buffer incl. jumbo-frames (Was: [RFC V1 net-next 1/1] net: ena: implement XDP drop support)
From: Toke Høiland-Jørgensen @ 2019-06-26 11:52 UTC (permalink / raw)
To: Jesper Dangaard Brouer, Machulsky, Zorik
Cc: Jubran, Samih, davem@davemloft.net, netdev@vger.kernel.org,
Woodhouse, David, Matushevsky, Alexander, Bshara, Saeed,
Wilson, Matt, Liguori, Anthony, Bshara, Nafea, Tzalik, Guy,
Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
Kiyanovski, Arthur, Daniel Borkmann, brouer, Ilias Apalodimas,
Alexei Starovoitov, Jakub Kicinski, xdp-newbies@vger.kernel.org
In-Reply-To: <20190626103829.5360ef2d@carbon>
Jesper Dangaard Brouer <brouer@redhat.com> writes:
> On Tue, 25 Jun 2019 03:19:22 +0000
> "Machulsky, Zorik" <zorik@amazon.com> wrote:
>
>> On 6/23/19, 7:21 AM, "Jesper Dangaard Brouer" <brouer@redhat.com> wrote:
>>
>> On Sun, 23 Jun 2019 10:06:49 +0300 <sameehj@amazon.com> wrote:
>>
>> > This commit implements the basic functionality of drop/pass logic in the
>> > ena driver.
>>
>> Usually we require a driver to implement all the XDP return codes,
>> before we accept it. But as Daniel and I discussed with Zorik during
>> NetConf[1], we are going to make an exception and accept the driver
>> if you also implement XDP_TX.
>>
>> As we trust that Zorik/Amazon will follow and implement XDP_REDIRECT
>> later, given he/you wants AF_XDP support which requires XDP_REDIRECT.
>>
>> Jesper, thanks for your comments and very helpful discussion during
>> NetConf! That's the plan, as we agreed. From our side I would like to
>> reiterate again the importance of multi-buffer support by xdp frame.
>> We would really prefer not to see our MTU shrinking because of xdp
>> support.
>
> Okay we really need to make a serious attempt to find a way to support
> multi-buffer packets with XDP. With the important criteria of not
> hurting performance of the single-buffer per packet design.
>
> I've created a design document[2], that I will update based on our
> discussions: [2] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
>
> The use-case that really convinced me was Eric's packet header-split.
>
>
> Lets refresh: Why XDP don't have multi-buffer support:
>
> XDP is designed for maximum performance, which is why certain driver-level
> use-cases were not supported, like multi-buffer packets (like jumbo-frames).
> As it e.g. complicated the driver RX-loop and memory model handling.
>
> The single buffer per packet design, is also tied into eBPF Direct-Access
> (DA) to packet data, which can only be allowed if the packet memory is in
> contiguous memory. This DA feature is essential for XDP performance.
>
>
> One way forward is to define that XDP only get access to the first
> packet buffer, and it cannot see subsequent buffers. For XDP_TX and
> XDP_REDIRECT to work then XDP still need to carry pointers (plus
> len+offset) to the other buffers, which is 16 bytes per extra buffer.
Yeah, I think this would be reasonable. As long as we can have a
metadata field with the full length + still give XDP programs the
ability to truncate the packet (i.e., discard the subsequent pages) I
think many (most?) use cases will work fine without having access to the
full packet data...
-Toke
^ permalink raw reply
* Re: [PATCH v4 net-next 1/4] net: core: page_pool: add user cnt preventing pool deletion
From: Jesper Dangaard Brouer @ 2019-06-26 11:51 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: davem, grygorii.strashko, saeedm, leon, ast, linux-kernel,
linux-omap, ilias.apalodimas, netdev, daniel, jakub.kicinski,
john.fastabend, brouer
In-Reply-To: <20190626104948.GF6485@khorivan>
On Wed, 26 Jun 2019 13:49:49 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Wed, Jun 26, 2019 at 12:42:16PM +0200, Jesper Dangaard Brouer wrote:
> >On Tue, 25 Jun 2019 20:59:45 +0300
> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >
> >> Add user counter allowing to delete pool only when no users.
> >> It doesn't prevent pool from flush, only prevents freeing the
> >> pool instance. Helps when no need to delete the pool and now
> >> it's user responsibility to free it by calling page_pool_free()
> >> while destroying procedure. It also makes to use page_pool_free()
> >> explicitly, not fully hidden in xdp unreg, which looks more
> >> correct after page pool "create" routine.
> >
> >No, this is wrong.
> below.
>
> >
> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> ---
> >> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 8 +++++---
> >> include/net/page_pool.h | 7 +++++++
> >> net/core/page_pool.c | 7 +++++++
> >> net/core/xdp.c | 3 +++
> >> 4 files changed, 22 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >> index 5e40db8f92e6..cb028de64a1d 100644
> >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> >> @@ -545,10 +545,8 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
> >> }
> >> err = xdp_rxq_info_reg_mem_model(&rq->xdp_rxq,
> >> MEM_TYPE_PAGE_POOL, rq->page_pool);
> >> - if (err) {
> >> - page_pool_free(rq->page_pool);
> >> + if (err)
> >> goto err_free;
> >> - }
> >>
> >> for (i = 0; i < wq_sz; i++) {
> >> if (rq->wq_type == MLX5_WQ_TYPE_LINKED_LIST_STRIDING_RQ) {
> >> @@ -613,6 +611,8 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
> >> if (rq->xdp_prog)
> >> bpf_prog_put(rq->xdp_prog);
> >> xdp_rxq_info_unreg(&rq->xdp_rxq);
> >> + if (rq->page_pool)
> >> + page_pool_free(rq->page_pool);
> >> mlx5_wq_destroy(&rq->wq_ctrl);
> >>
> >> return err;
> >> @@ -643,6 +643,8 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
> >> }
> >>
> >> xdp_rxq_info_unreg(&rq->xdp_rxq);
> >> + if (rq->page_pool)
> >> + page_pool_free(rq->page_pool);
> >
> >No, this is wrong. The hole point with the merged page_pool fixes
> >patchset was that page_pool_free() needs to be delayed until no-more
> >in-flight packets exist.
>
> Probably it's not so obvious, but it's still delayed and deleted only
> after no-more in-flight packets exist. Here question is only who is able
> to do this first based on refcnt.
Hmm... then I find this API is rather misleading, even the function
name page_pool_free is misleading ("free"). (Now, I do see, below, that
page_pool_create() take an extra reference).
But it is still wrong / problematic. As you allow
__page_pool_request_shutdown() to be called with elevated refcnt. Your
use-case is to have more than 1 xdp_rxq_info struct using the same
page_pool. Then you have to call xdp_rxq_info_unreg_mem_model() for
each, which will call __page_pool_request_shutdown().
For this to be safe, your driver have to stop RX for all the
xdp_rxq_info structs that share the page_pool. The page_pool already
have this requirement, but it comes as natural step when shutting down
an RXQ. With your change, you have to take care of stopping the RXQs
first, and then call xdp_rxq_info_unreg_mem_model() for each
xdp_rxq_info afterwards. I assume you do this, but it is just a driver
bug waiting to happen.
> >> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> >> index b366f59885c1..169b0e3c870e 100644
> >> --- a/net/core/page_pool.c
> >> +++ b/net/core/page_pool.c
[...]
> >> @@ -70,6 +71,8 @@ struct page_pool *page_pool_create(const struct page_pool_params *params)
> >> kfree(pool);
> >> return ERR_PTR(err);
> >> }
> >> +
> >> + page_pool_get(pool);
> >> return pool;
> >> }
> >> EXPORT_SYMBOL(page_pool_create);
The thing (perhaps) like about your API change, is that you also allow
the driver to explicitly keep the page_pool object across/after a
xdp_rxq_info_unreg_mem_model(). And this way possibly reuse it for
another RXQ. The problem is of-cause that on driver shutdown, this
will force drivers to implement the same shutdown logic with
schedule_delayed_work as the core xdp.c code already does.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [EXT] [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size
From: Benjamin Poirier @ 2019-06-26 11:39 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev@vger.kernel.org
In-Reply-To: <DM6PR18MB269776CBA6B979855AD215A8ABE20@DM6PR18MB2697.namprd18.prod.outlook.com>
On 2019/06/26 09:36, Manish Chopra wrote:
> > -----Original Message-----
> > From: Benjamin Poirier <bpoirier@suse.com>
> > Sent: Monday, June 17, 2019 1:19 PM
> > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > Subject: [EXT] [PATCH net-next 05/16] qlge: Remove rx_ring.sbq_buf_size
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > Tx rings have sbq_buf_size = 0 but there's no case where the code actually
> > tests on that value. We can remove sbq_buf_size and use a constant instead.
> >
>
> Seems relevant to RX ring, not the TX ring ?
qlge uses "struct rx_ring" for rx and for tx completion rings.
The driver's author is probably laughing now at the success of his plan
to confuse those who would follow in his footsteps.
^ permalink raw reply
* Re: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
From: Benjamin Poirier @ 2019-06-26 11:37 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev@vger.kernel.org
In-Reply-To: <DM6PR18MB2697BAC4CA9B876306BEDBEBABE20@DM6PR18MB2697.namprd18.prod.outlook.com>
On 2019/06/26 09:24, Manish Chopra wrote:
> > -----Original Message-----
> > From: Benjamin Poirier <bpoirier@suse.com>
> > Sent: Monday, June 17, 2019 1:19 PM
> > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > Subject: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> > lbq_buf_size is duplicated to every rx_ring structure whereas lbq_buf_order is
> > present once in the ql_adapter structure. All rings use the same buf size, keep
> > only one copy of it. Also factor out the calculation of lbq_buf_size instead of
> > having two copies.
> >
> > Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> > ---
[...]
>
> Not sure if this change is really required, I think fields relevant to rx_ring should be present in the rx_ring structure.
> There are various other fields like "lbq_len" and "lbq_size" which would be same for all rx rings but still under the relevant rx_ring structure.
Indeed, those members are also removed by this patch series, in patch 11.
^ permalink raw reply
* KASAN: use-after-free Read in corrupted (3)
From: syzbot @ 2019-06-26 11:37 UTC (permalink / raw)
To: aarcange, akpm, christian, ebiederm, elena.reshetova, guro,
keescook, linux-kernel, luto, mhocko, mingo, namit, netdev,
peterz, riel, syzkaller-bugs, wad
Hello,
syzbot found the following crash on:
HEAD commit: 045df37e Merge branch 'cxgb4-Reference-count-MPS-TCAM-entr..
git tree: net-next
console output: https://syzkaller.appspot.com/x/log.txt?x=13c6217ea00000
kernel config: https://syzkaller.appspot.com/x/.config?x=dd16b8dc9d0d210c
dashboard link: https://syzkaller.appspot.com/bug?extid=8a821b383523654227bf
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1389f5b5a00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+8a821b383523654227bf@syzkaller.appspotmail.com
==================================================================
BUG: KASAN: use-after-free in vsnprintf+0x1727/0x19a0 lib/vsprintf.c:2503
Read of size 8 at addr ffff8880952500a0 by task syz-executor.1/9180
CPU: 0 PID: 9180 Comm: syz-executor.1 Not tainted 5.2.0-rc5+ #43
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
Allocated by task 8:
save_stack+0x23/0x90 mm/kasan/common.c:71
set_track mm/kasan/common.c:79 [inline]
__kasan_kmalloc mm/kasan/common.c:489 [inline]
__kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:462
kasan_slab_alloc+0xf/0x20 mm/kasan/common.c:497
slab_post_alloc_hook mm/slab.h:437 [inline]
slab_alloc mm/slab.c:3326 [inline]
kmem_cache_alloc+0x11a/0x6f0 mm/slab.c:3488
vm_area_dup+0x21/0x170 kernel/fork.c:343
dup_mmap kernel/fork.c:528 [inline]
dup_mm+0x8c4/0x13b0 kernel/fork.c:1341
copy_mm kernel/fork.c:1397 [inline]
copy_process.part.0+0x2cde/0x6790 kernel/fork.c:2032
copy_process kernel/fork.c:1800 [inline]
_do_fork+0x25d/0xfe0 kernel/fork.c:2369
__do_sys_clone kernel/fork.c:2476 [inline]
__se_sys_clone kernel/fork.c:2470 [inline]
__x64_sys_clone+0xbf/0x150 kernel/fork.c:2470
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 2502230480:
------------[ cut here ]------------
Bad or missing usercopy whitelist? Kernel memory overwrite attempt detected
to SLAB object 'shmem_inode_cache' (offset 1040, size 1)!
WARNING: CPU: 0 PID: 9180 at mm/usercopy.c:74 usercopy_warn+0xeb/0x110
mm/usercopy.c:74
Kernel panic - not syncing: panic_on_warn set ...
Shutting down cpus with NMI
Kernel Offset: disabled
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [PATCH net-next 01/16] qlge: Remove irq_cnt
From: Benjamin Poirier @ 2019-06-26 11:36 UTC (permalink / raw)
To: Manish Chopra; +Cc: GR-Linux-NIC-Dev, netdev@vger.kernel.org
In-Reply-To: <DM6PR18MB2697814343012B4363482290ABE20@DM6PR18MB2697.namprd18.prod.outlook.com>
On 2019/06/26 08:59, Manish Chopra wrote:
> > -----Original Message-----
> > From: Benjamin Poirier <bpoirier@suse.com>
> > Sent: Monday, June 17, 2019 1:19 PM
> > To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> > NIC-Dev@marvell.com>; netdev@vger.kernel.org
> > Subject: [PATCH net-next 01/16] qlge: Remove irq_cnt
> >
> > qlge uses an irq enable/disable refcounting scheme that is:
> > * poorly implemented
> > Uses a spin_lock to protect accesses to the irq_cnt atomic variable
> > * buggy
> > Breaks when there is not a 1:1 sequence of irq - napi_poll, such as
> > when using SO_BUSY_POLL.
> > * unnecessary
> > The purpose or irq_cnt is to reduce irq control writes when
> > multiple work items result from one irq: the irq is re-enabled
> > after all work is done.
> > Analysis of the irq handler shows that there is only one case where
> > there might be two workers scheduled at once, and those have
> > separate irq masking bits.
>
> I believe you are talking about here for asic error recovery worker and MPI worker.
> Which separate IRQ masking bits are you referring here ?
INTR_EN with intr_dis_mask for completion interrupts
INTR_MASK bit INTR_MASK_PI for mpi interrupts
> > static int ql_validate_flash(struct ql_adapter *qdev, u32 size, const char *str)
> > @@ -2500,21 +2451,22 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> > u32 var;
> > int work_done = 0;
> >
> > - spin_lock(&qdev->hw_lock);
> > - if (atomic_read(&qdev->intr_context[0].irq_cnt)) {
> > - netif_printk(qdev, intr, KERN_DEBUG, qdev->ndev,
> > - "Shared Interrupt, Not ours!\n");
> > - spin_unlock(&qdev->hw_lock);
> > - return IRQ_NONE;
> > - }
> > - spin_unlock(&qdev->hw_lock);
> > + /* Experience shows that when using INTx interrupts, the device does
> > + * not always auto-mask the interrupt.
> > + * When using MSI mode, the interrupt must be explicitly disabled
> > + * (even though it is auto-masked), otherwise a later command to
> > + * enable it is not effective.
> > + */
> > + if (!test_bit(QL_MSIX_ENABLED, &qdev->flags))
> > + ql_disable_completion_interrupt(qdev, 0);
>
> Current code is disabling completion interrupt in case of MSI-X zeroth vector.
> This change will break that behavior. Shouldn't zeroth vector in case of MSI-X also disable completion interrupt ?
> If not, please explain why ?
In msix mode there's no need to explicitly disable completion
interrupts, they are reliably auto-masked, according to my observations.
I tested this on two QLE8142 adapters.
Do you have reason to believe this might not always be the case?
>
> >
> > - var = ql_disable_completion_interrupt(qdev, intr_context->intr);
> > + var = ql_read32(qdev, STS);
> >
> > /*
> > * Check for fatal error.
> > */
> > if (var & STS_FE) {
> > + ql_disable_completion_interrupt(qdev, 0);
>
> Why need to do it again here ? if before this it can disable completion interrupt for INT-X case and MSI-X zeroth vector case ?
I couldn't test this code path, so I preserved the original behavior.
>
> > ql_queue_asic_error(qdev);
> > netdev_err(qdev->ndev, "Got fatal error, STS = %x.\n", var);
> > var = ql_read32(qdev, ERR_STS);
> > @@ -2534,7 +2486,6 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> > */
> > netif_err(qdev, intr, qdev->ndev,
> > "Got MPI processor interrupt.\n");
> > - ql_disable_completion_interrupt(qdev, intr_context->intr);
>
> Why disable interrupt is not required here ?
The interrupt source _is_ masked:
ql_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16));
> While in case of Fatal error case above ql_disable_completion_interrupt() is being called ?
> Also, in case of MSI-X zeroth vector it will not disable completion interrupt but at the end, it will end of qlge_isr() enabling completion interrupt.
> Seems like disabling and enabling might not be in sync in case of MSI-X zeroth vector.
I guess you forgot to consider that completion interrupts are
auto-masked in msix mode.
^ permalink raw reply
* net: never suspend the ethernet PHY on certain boards?
From: Alexander Dahl @ 2019-06-26 11:23 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, Thomas Pfahl
Hei hei,
tl;dr: is there a way to prevent an ethernet PHY to ever power down, preferred
with some dt configuration, not with a hack e.g. patching out suspend
functions?
With the bugfix 0da70f808029476001109b6cb076737bc04cea2e ("net: macb: do not
disable MDIO bus at open/close time", came with kernel v4.19, was backported
to v4.18.7) a problem arises for us, which was masked before for ages, with a
special combination of SoC, ethernet PHY and other chips on the same board,
and the linux drivers for that.
The boards use either a at91sam9g20 or a sama5d27 SoC, both using cadence/macb
as ethernet driver. Both boards have a smsc LAN8720A ethernet phy attached.
The RMII clock is generated by the PHY, which uses a 25 MHz crystal for that.
This clock line is of course fed into the SoC/MAC, but also used (you might
say hijacked) by other chips on the board which depend on that clock being
_always_ on (at least after initial init on boot). The hardware can not be
changed, we speak of several hundred boards already sold in the last years.
O:-)
Symptom is: when calling `ip link set down dev eth0` that clock goes off, the
other (not soc nor phy) chips depending on that clock, freeze.
I could bisect this behaviour change on a vanilla kernel to the commit
mentioned above (actually to the backport commit v4.18.7-4-g716fc5ce90cf,
because I bisected from v4.17.19 to v4.18.20).
What I tracked down so far: macb_close() before the bugfix reset the MPE bit
in the MAC Network Control Register, which probably prevents the MAC to send
MDIO telegrams to the PHY? After the bugfix, that bit is not cleared anymore
(to allow still talking to other PHYs on the same MDIO bus, we don't have that
case). I assume communicating with the PHY is still possible then.
macb_close() also calls phy_stop() which sets the state of the phy driver
state machine to PHY_HALTED, with the next run of that state machine
phy_suspend() is called.
The smsc phy driver has no special suspend/resume functions, but uses
genphy_suspend(), that one sets BMCR_PDOWN in MII_BMCR register of that
(standard compliant) PHY. I suspect after that the PHY powers down and the
clock goes off.
I assume before that bugfix, this power down bit could not be set, because the
MDIO interface in the MAC had been disabled, so the PHY stayed on. (However
there's a possible race because in macb_close() the phy_stop() is called
before macb_reset_hw(), right?)
So far, these are mostly assumptions. I did not use gdb on the drivers or a
logic analyzer on the MDIO lines. I could do to prove, however.
What I could do:
1) Revert that change on my tree, which would mean reverting a generic bugfix
2) Patch smsc phy driver to not suspend anymore
3) Invent some new way to prevent suspend on a configuration basis (dt?)
4) Anything I did not think of yet
I know 1) or 2) are hacks without a chance to make it to mainline. What would
be your suggestions for 3) and 4)?
Greets
Alex
^ permalink raw reply
* [PATCH net-next 3/3] net: dsa: sja1105: Mark in-band AN modes not supported for PHYLINK
From: Vladimir Oltean @ 2019-06-26 11:20 UTC (permalink / raw)
To: rmk+kernel, f.fainelli, vivien.didelot, andrew, davem
Cc: netdev, Vladimir Oltean
In-Reply-To: <20190626112014.7625-1-olteanv@gmail.com>
We need a better way to signal this, perhaps in phylink_validate, but
for now just print this error message as guidance for other people
looking at this driver's code while trying to rework PHYLINK.
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
drivers/net/dsa/sja1105/sja1105_main.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index ad4f604590c0..d82afb835fb7 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -806,6 +806,11 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port,
if (sja1105_phy_mode_mismatch(priv, port, state->interface))
return;
+ if (link_an_mode == MLO_AN_INBAND) {
+ dev_err(ds->dev, "In-band AN not supported!\n");
+ return;
+ }
+
sja1105_adjust_port_config(priv, port, state->speed);
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 2/3] net: dsa: sja1105: Check for PHY mode mismatches with what PHYLINK reports
From: Vladimir Oltean @ 2019-06-26 11:20 UTC (permalink / raw)
To: rmk+kernel, f.fainelli, vivien.didelot, andrew, davem
Cc: netdev, Vladimir Oltean
In-Reply-To: <20190626112014.7625-1-olteanv@gmail.com>
PHYLINK being designed with PHYs in mind that can change MII protocol,
for correct operation it is necessary to ensure that the PHY interface
mode stays the same (otherwise clear the supported bit mask, as
required).
Because this is just a hypothetical situation for now, we don't bother
to check whether we could actually support the new PHY interface mode.
Actually we could modify the xMII table, reset the switch and send an
updated static configuration, but adding that would just be dead code.
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
drivers/net/dsa/sja1105/sja1105_main.c | 47 ++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index da1736093b06..ad4f604590c0 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -766,12 +766,46 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
return sja1105_clocking_setup_port(priv, port);
}
+/* The SJA1105 MAC programming model is through the static config (the xMII
+ * Mode table cannot be dynamically reconfigured), and we have to program
+ * that early (earlier than PHYLINK calls us, anyway).
+ * So just error out in case the connected PHY attempts to change the initial
+ * system interface MII protocol from what is defined in the DT, at least for
+ * now.
+ */
+static bool sja1105_phy_mode_mismatch(struct sja1105_private *priv, int port,
+ phy_interface_t interface)
+{
+ struct sja1105_xmii_params_entry *mii;
+ sja1105_phy_interface_t phy_mode;
+
+ mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
+ phy_mode = mii->xmii_mode[port];
+
+ switch (interface) {
+ case PHY_INTERFACE_MODE_MII:
+ return (phy_mode != XMII_MODE_MII);
+ case PHY_INTERFACE_MODE_RMII:
+ return (phy_mode != XMII_MODE_RMII);
+ case PHY_INTERFACE_MODE_RGMII:
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ case PHY_INTERFACE_MODE_RGMII_TXID:
+ return (phy_mode != XMII_MODE_RGMII);
+ default:
+ return true;
+ }
+}
+
static void sja1105_mac_config(struct dsa_switch *ds, int port,
unsigned int link_an_mode,
const struct phylink_link_state *state)
{
struct sja1105_private *priv = ds->priv;
+ if (sja1105_phy_mode_mismatch(priv, port, state->interface))
+ return;
+
sja1105_adjust_port_config(priv, port, state->speed);
}
@@ -804,6 +838,19 @@ static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
+ /* include/linux/phylink.h says:
+ * When @state->interface is %PHY_INTERFACE_MODE_NA, phylink
+ * expects the MAC driver to return all supported link modes.
+ */
+ if (state->interface != PHY_INTERFACE_MODE_NA &&
+ sja1105_phy_mode_mismatch(priv, port, state->interface)) {
+ dev_warn(ds->dev, "PHY mode mismatch on port %d: "
+ "PHYLINK tried to change to %s\n",
+ port, phy_modes(state->interface));
+ bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
+ return;
+ }
+
/* The MAC does not support pause frames, and also doesn't
* support half-duplex traffic modes.
*/
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 1/3] net: dsa: sja1105: Don't check state->link in phylink_mac_config
From: Vladimir Oltean @ 2019-06-26 11:20 UTC (permalink / raw)
To: rmk+kernel, f.fainelli, vivien.didelot, andrew, davem
Cc: netdev, Vladimir Oltean
In-Reply-To: <20190626112014.7625-1-olteanv@gmail.com>
It has been pointed out that PHYLINK can call mac_config only to update
the phy_interface_type and without knowing what the AN results are.
Experimentally, when this was observed to happen, state->link was also
unset, and therefore was used as a proxy to ignore this call. However it
is also suggested that state->link is undefined for this callback and
should not be relied upon.
So let the previously-dead codepath for SPEED_UNKNOWN be called, and
update the comment to make sure the MAC's behavior is sane.
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
---
drivers/net/dsa/sja1105/sja1105_main.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
index caebf76eaa3e..da1736093b06 100644
--- a/drivers/net/dsa/sja1105/sja1105_main.c
+++ b/drivers/net/dsa/sja1105/sja1105_main.c
@@ -715,7 +715,13 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
switch (speed_mbps) {
case SPEED_UNKNOWN:
- /* No speed update requested */
+ /* PHYLINK called sja1105_mac_config() to inform us about
+ * the state->interface, but AN has not completed and the
+ * speed is not yet valid. UM10944.pdf says that setting
+ * SJA1105_SPEED_AUTO at runtime disables the port, so that is
+ * ok for power consumption in case AN will never complete -
+ * otherwise PHYLINK should come back with a new update.
+ */
speed = SJA1105_SPEED_AUTO;
break;
case SPEED_10:
@@ -766,9 +772,6 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port,
{
struct sja1105_private *priv = ds->priv;
- if (!state->link)
- return;
-
sja1105_adjust_port_config(priv, port, state->speed);
}
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 0/3] Better PHYLINK compliance for SJA1105 DSA
From: Vladimir Oltean @ 2019-06-26 11:20 UTC (permalink / raw)
To: rmk+kernel, f.fainelli, vivien.didelot, andrew, davem
Cc: netdev, Vladimir Oltean
After discussing with Russell King, it appears this driver is making a
few confusions and not performing some checks for consistent operation.
Vladimir Oltean (3):
net: dsa: sja1105: Don't check state->link in phylink_mac_config
net: dsa: sja1105: Check for PHY mode mismatches with what PHYLINK
reports
net: dsa: sja1105: Mark in-band AN modes not supported for PHYLINK
drivers/net/dsa/sja1105/sja1105_main.c | 59 +++++++++++++++++++++++++-
1 file changed, 57 insertions(+), 2 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: Question about nf_conntrack_proto for IPsec
From: Florian Westphal @ 2019-06-26 11:13 UTC (permalink / raw)
To: Naruto Nguyen; +Cc: netfilter-devel, netdev, netfilter
In-Reply-To: <CANpxKHHXzrEpJPSj3x83+WE23G1W0KPz9XbG=fCVzS21+-BpfQ@mail.gmail.com>
Naruto Nguyen <narutonguyen2018@gmail.com> wrote:
> In linux/latest/source/net/netfilter/ folder, I only see we have
> nf_conntrack_proto_tcp.c, nf_conntrack_proto_udp.c and some other
> conntrack implementations for other protocols but I do not see
> nf_conntrack_proto for IPsec, so does it mean connection tracking
> cannot track ESP or AH protocol as a connection. I mean when I use
> "conntrack -L" command, I will not see ESP or AH connection is saved
> in conntrack list. Could you please help me to understand if conntrack
> supports that and any reasons if it does not support?
ESP/AH etc. use the generic tracker, i.e. only one ESP connection
is tracked between each endpoint.
^ permalink raw reply
* Question about nf_conntrack_proto for IPsec
From: Naruto Nguyen @ 2019-06-26 11:06 UTC (permalink / raw)
To: netfilter-devel, netdev, netfilter
Hi everyone,
In linux/latest/source/net/netfilter/ folder, I only see we have
nf_conntrack_proto_tcp.c, nf_conntrack_proto_udp.c and some other
conntrack implementations for other protocols but I do not see
nf_conntrack_proto for IPsec, so does it mean connection tracking
cannot track ESP or AH protocol as a connection. I mean when I use
"conntrack -L" command, I will not see ESP or AH connection is saved
in conntrack list. Could you please help me to understand if conntrack
supports that and any reasons if it does not support?
Thanks a lot,
Brs,
Naruto
^ permalink raw reply
* Re: [PATCH v4 net] af_packet: Block execution of tasks waiting for transmit to complete in AF_PACKET
From: Neil Horman @ 2019-06-26 10:54 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Matteo Croce, David S. Miller
In-Reply-To: <CAF=yD-+fCNGQyoRNAZngof3=_gGbHn9aSCQA_hNvFSsSZtZQxA@mail.gmail.com>
On Tue, Jun 25, 2019 at 06:30:08PM -0400, Willem de Bruijn wrote:
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index a29d66da7394..a7ca6a003ebe 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -2401,6 +2401,9 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
> >
> > ts = __packet_set_timestamp(po, ph, skb);
> > __packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
> > +
> > + if (!packet_read_pending(&po->tx_ring))
> > + complete(&po->skb_completion);
> > }
> >
> > sock_wfree(skb);
> > @@ -2585,7 +2588,7 @@ static int tpacket_parse_header(struct packet_sock *po, void *frame,
> >
> > static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > {
> > - struct sk_buff *skb;
> > + struct sk_buff *skb = NULL;
> > struct net_device *dev;
> > struct virtio_net_hdr *vnet_hdr = NULL;
> > struct sockcm_cookie sockc;
> > @@ -2600,6 +2603,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > int len_sum = 0;
> > int status = TP_STATUS_AVAILABLE;
> > int hlen, tlen, copylen = 0;
> > + long timeo = 0;
> >
> > mutex_lock(&po->pg_vec_lock);
> >
> > @@ -2646,12 +2650,21 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> > if ((size_max > dev->mtu + reserve + VLAN_HLEN) && !po->has_vnet_hdr)
> > size_max = dev->mtu + reserve + VLAN_HLEN;
> >
> > + reinit_completion(&po->skb_completion);
> > +
> > do {
> > ph = packet_current_frame(po, &po->tx_ring,
> > TP_STATUS_SEND_REQUEST);
> > if (unlikely(ph == NULL)) {
> > - if (need_wait && need_resched())
> > - schedule();
> > + if (need_wait && skb) {
> > + timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > + timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
>
> This looks really nice.
>
> But isn't it still susceptible to the race where tpacket_destruct_skb
> is called in between po->xmit and this
> wait_for_completion_interruptible_timeout?
>
Thats not an issue, since the complete is only gated on packet_read_pending
reaching 0 in tpacket_destuct_skb. Previously it was gated on my
wait_on_complete flag being non-zero, so we had to set that prior to calling
po->xmit, or the complete call might never get made, resulting in a hang. Now,
we will always call complete, and the completion api allows for arbitrary
ordering of complete/wait_for_complete (since its internal done variable gets
incremented), making a call to wait_for_complete effectively a fall through if
complete gets called first.
There is an odd path here though. If an application calls sendmsg on a packet
socket here with MSG_DONTWAIT set, then need_wait will be zero, and we will
eventually exit this loop without ever having called wait_for_complete, but
tpacket_destruct_skb will still have called complete when all the frames
complete transmission. In and of itself, thats fine, but it leave the
completion structure in a state where its done variable will have been
incremented at least once (specifically it will be set to N, where N is the
number of frames transmitted during the call where MSG_DONTWAIT is set). If the
application then calls sendmsg on this socket with MSG_DONTWAIT clear, we will
call wait_for_complete, but immediately return from it (due to the previously
made calls to complete). I've corrected this however, but adding that call to
reinit_completion prior to the loop entry, so that we are always guaranteed to
have the completion variable set properly to wait for only the frames being sent
in this particular instance of the sendmsg call.
> The test for skb is shorthand for packet_read_pending != 0, right?
>
Sort of. gating on skb guarantees for us that we have sent at least one frame
in this call to tpacket_snd. If we didn't do that, then it would be possible
for an application to call sendmsg without setting any frames in the buffer to
TP_STATUS_SEND_REQUEST, which would cause us to wait for a completion without
having sent any frames, meaning we would block waiting for an event
(tpacket_destruct_skb), that will never happen. The check for skb ensures that
tpacket_snd_skb will get called, and that we will get a wakeup from a call to
wait_for_complete. It does suggest that packet_read_pending != 0, but thats not
guaranteed, because tpacket_destruct_skb may already have been called (see the
above explination regarding ordering of complete/wait_for_complete).
Neil
^ permalink raw reply
* Re: [PATCH v4 net-next 1/4] net: core: page_pool: add user cnt preventing pool deletion
From: Ivan Khoronzhuk @ 2019-06-26 10:49 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: davem, grygorii.strashko, hawk, saeedm, leon, ast, linux-kernel,
linux-omap, xdp-newbies, ilias.apalodimas, netdev, daniel,
jakub.kicinski, john.fastabend
In-Reply-To: <20190626124216.494eee86@carbon>
On Wed, Jun 26, 2019 at 12:42:16PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 25 Jun 2019 20:59:45 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> Add user counter allowing to delete pool only when no users.
>> It doesn't prevent pool from flush, only prevents freeing the
>> pool instance. Helps when no need to delete the pool and now
>> it's user responsibility to free it by calling page_pool_free()
>> while destroying procedure. It also makes to use page_pool_free()
>> explicitly, not fully hidden in xdp unreg, which looks more
>> correct after page pool "create" routine.
>
>No, this is wrong.
below.
>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 8 +++++---
>> include/net/page_pool.h | 7 +++++++
>> net/core/page_pool.c | 7 +++++++
>> net/core/xdp.c | 3 +++
>> 4 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index 5e40db8f92e6..cb028de64a1d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> @@ -545,10 +545,8 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
>> }
>> err = xdp_rxq_info_reg_mem_model(&rq->xdp_rxq,
>> MEM_TYPE_PAGE_POOL, rq->page_pool);
>> - if (err) {
>> - page_pool_free(rq->page_pool);
>> + if (err)
>> goto err_free;
>> - }
>>
>> for (i = 0; i < wq_sz; i++) {
>> if (rq->wq_type == MLX5_WQ_TYPE_LINKED_LIST_STRIDING_RQ) {
>> @@ -613,6 +611,8 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c,
>> if (rq->xdp_prog)
>> bpf_prog_put(rq->xdp_prog);
>> xdp_rxq_info_unreg(&rq->xdp_rxq);
>> + if (rq->page_pool)
>> + page_pool_free(rq->page_pool);
>> mlx5_wq_destroy(&rq->wq_ctrl);
>>
>> return err;
>> @@ -643,6 +643,8 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq)
>> }
>>
>> xdp_rxq_info_unreg(&rq->xdp_rxq);
>> + if (rq->page_pool)
>> + page_pool_free(rq->page_pool);
>
>No, this is wrong. The hole point with the merged page_pool fixes
>patchset was that page_pool_free() needs to be delayed until no-more
>in-flight packets exist.
Probably it's not so obvious, but it's still delayed and deleted only
after no-more in-flight packets exist. Here question is only who is able
to do this first based on refcnt.
>
>
>> mlx5_wq_destroy(&rq->wq_ctrl);
>> }
>>
>> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>> index f07c518ef8a5..1ec838e9927e 100644
>> --- a/include/net/page_pool.h
>> +++ b/include/net/page_pool.h
>> @@ -101,6 +101,7 @@ struct page_pool {
>> struct ptr_ring ring;
>>
>> atomic_t pages_state_release_cnt;
>> + atomic_t user_cnt;
>> };
>>
>> struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp);
>> @@ -183,6 +184,12 @@ static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
>> return page->dma_addr;
>> }
>>
>> +/* used to prevent pool from deallocation */
>> +static inline void page_pool_get(struct page_pool *pool)
>> +{
>> + atomic_inc(&pool->user_cnt);
>> +}
>> +
>> static inline bool is_page_pool_compiled_in(void)
>> {
>> #ifdef CONFIG_PAGE_POOL
>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
>> index b366f59885c1..169b0e3c870e 100644
>> --- a/net/core/page_pool.c
>> +++ b/net/core/page_pool.c
>> @@ -48,6 +48,7 @@ static int page_pool_init(struct page_pool *pool,
>> return -ENOMEM;
>>
>> atomic_set(&pool->pages_state_release_cnt, 0);
>> + atomic_set(&pool->user_cnt, 0);
>>
>> if (pool->p.flags & PP_FLAG_DMA_MAP)
>> get_device(pool->p.dev);
>> @@ -70,6 +71,8 @@ struct page_pool *page_pool_create(const struct page_pool_params *params)
>> kfree(pool);
>> return ERR_PTR(err);
>> }
>> +
>> + page_pool_get(pool);
>> return pool;
>> }
>> EXPORT_SYMBOL(page_pool_create);
>> @@ -356,6 +359,10 @@ static void __warn_in_flight(struct page_pool *pool)
>>
>> void __page_pool_free(struct page_pool *pool)
>> {
>> + /* free only if no users */
>> + if (!atomic_dec_and_test(&pool->user_cnt))
>> + return;
>> +
>> WARN(pool->alloc.count, "API usage violation");
>> WARN(!ptr_ring_empty(&pool->ring), "ptr_ring is not empty");
>>
>> diff --git a/net/core/xdp.c b/net/core/xdp.c
>> index 829377cc83db..04bdcd784d2e 100644
>> --- a/net/core/xdp.c
>> +++ b/net/core/xdp.c
>> @@ -372,6 +372,9 @@ int xdp_rxq_info_reg_mem_model(struct xdp_rxq_info *xdp_rxq,
>>
>> mutex_unlock(&mem_id_lock);
>>
>> + if (type == MEM_TYPE_PAGE_POOL)
>> + page_pool_get(xdp_alloc->page_pool);
>> +
>> trace_mem_connect(xdp_alloc, xdp_rxq);
>> return 0;
>> err:
>
>
>
>--
>Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
--
Regards,
Ivan Khoronzhuk
^ 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