* Re: __sk_buff.data_end
From: Johannes Berg @ 2017-04-20 14:32 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev
In-Reply-To: <58F8C59F.1040009@iogearbox.net>
On Thu, 2017-04-20 at 16:28 +0200, Daniel Borkmann wrote:
>
> I see what you mean now. Yes, that's fine. We already do something
> similar essentially with skb->ifindex access already (skb->dev +
> dev->ifindex), f.e.:
>
> [...]
> case offsetof(struct __sk_buff, ifindex):
> BUILD_BUG_ON(FIELD_SIZEOF(struct net_device, ifindex)
> != 4);
>
> *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff,
> dev),
> si->dst_reg, si->src_reg,
> offsetof(struct sk_buff, dev));
> *insn++ = BPF_JMP_IMM(BPF_JEQ, si->dst_reg, 0, 1);
> *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
> offsetof(struct net_device,
> ifindex));
> break;
> [...]
Oh, right, good point.
> Which is not too different from the above. You'd probably need to
> populate the struct wifi_data each time if you place it onto the
> stack, but perhaps could be optimized by storing that somewhere
> else (e.g. somewhere via netdev, etc) and walking the pointer from
> there, which would also spare you the cb[] save/restore.
Hmm. I don't see what "somewhere else" I could possibly have though,
given that I want the (kernel-side) context to be "struct sk_buff *"
to allow the skb helpers?
johannes
^ permalink raw reply
* Re: [PATCH v4 net-next RFC] net: Generic XDP
From: Jesper Dangaard Brouer @ 2017-04-20 14:30 UTC (permalink / raw)
To: Andy Gospodarek
Cc: David Miller, alexei.starovoitov, michael.chan, netdev,
xdp-newbies, brouer
In-Reply-To: <20170419142903.GJ4730@C02RW35GFVH8.dhcp.broadcom.net>
On Wed, 19 Apr 2017 10:29:03 -0400
Andy Gospodarek <andy@greyhouse.net> wrote:
> I ran this on top of a card that uses the bnxt_en driver on a desktop
> class system with an i7-6700 CPU @ 3.40GHz, sending a single stream of
> UDP traffic with flow control disabled and saw the following (all stats
> in Million PPS).
>
> xdp1 xdp2 xdp_tx_tunnel
> Generic XDP 7.8 5.5 (1.3 actual) 4.6 (1.1 actual)
> Optimized XDP 11.7 9.7 4.6
>
> One thing to note is that the Generic XDP case shows some different
> results for reported by the application vs actual (seen on the wire). I
> did not debug where the drops are happening and what counter needs to be
> incremented to note this -- I'll add that to my TODO list. The
> Optimized XDP case does not have a difference in reported vs actual
> frames on the wire.
The reported application vs actual (seen on the wire) number sound scary.
How do you evaluate/measure "seen on the wire"?
Perhaps you could use ethtool -S stats to see if anything is fishy?
I recommend using my tool[1] like:
~/git/network-testing/bin/ethtool_stats.pl --dev mlx5p2 --sec 2
[1] https://github.com/netoptimizer/network-testing/blob/master/bin/ethtool_stats.pl
I'm evaluating this patch on a mlx5 NIC, and something is not right...
I'm seeing:
Ethtool(mlx5p2) stat: 349599 ( 349,599) <= tx_multicast_phy /sec
Ethtool(mlx5p2) stat: 4940185 ( 4,940,185) <= tx_packets /sec
Ethtool(mlx5p2) stat: 349596 ( 349,596) <= tx_packets_phy /sec
[...]
Ethtool(mlx5p2) stat: 36898 ( 36,898) <= rx_cache_busy /sec
Ethtool(mlx5p2) stat: 36898 ( 36,898) <= rx_cache_full /sec
Ethtool(mlx5p2) stat: 4903287 ( 4,903,287) <= rx_cache_reuse /sec
Ethtool(mlx5p2) stat: 4940185 ( 4,940,185) <= rx_csum_complete /sec
Ethtool(mlx5p2) stat: 4940185 ( 4,940,185) <= rx_packets /sec
Something is wrong... when I tcpdump on the generator machine, I see
garbled packets with IPv6 multicast addresses.
And it looks like I'm only sending 349,596 tx_packets_phy/sec on the "wire".
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: Heads-up: two regressions in v4.11-rc series
From: Mel Gorman @ 2017-04-20 14:30 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Linus Torvalds, Andrew Morton, Frederic Weisbecker, Tariq Toukan,
LKML, linux-mm, netdev@vger.kernel.org, peterz
In-Reply-To: <20170420110042.73d01e0f@redhat.com>
On Thu, Apr 20, 2017 at 11:00:42AM +0200, Jesper Dangaard Brouer wrote:
> Hi Linus,
>
> Just wanted to give a heads-up on two regressions in 4.11-rc series.
>
> (1) page allocator optimization revert
>
> Mel Gorman and I have been playing with optimizing the page allocator,
> but Tariq spotted that we caused a regression for (NIC) drivers that
> refill DMA RX rings in softirq context.
>
> The end result was a revert, and this is waiting in AKPMs quilt queue:
> http://ozlabs.org/~akpm/mmots/broken-out/revert-mm-page_alloc-only-use-per-cpu-allocator-for-irq-safe-requests.patch
>
This was flagged to Andrew that it should go in for either 4.11 or if
there were concerns about how close to the release we are then put it in
for 4.11-stable. At worst, I can do a resubmit to -stable myself after
it gets merged in the next window if it falls between the cracks.
--
Mel Gorman
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: __sk_buff.data_end
From: Daniel Borkmann @ 2017-04-20 14:28 UTC (permalink / raw)
To: Johannes Berg, Alexei Starovoitov; +Cc: netdev
In-Reply-To: <1492697865.3109.7.camel@sipsolutions.net>
On 04/20/2017 04:17 PM, Johannes Berg wrote:
> On Thu, 2017-04-20 at 16:10 +0200, Daniel Borkmann wrote:
>>
>> I think this would be a rather more complex operation on the BPF
>> side, it would need changes from LLVM (which assumes initial ctx sits
>> in r1), verifier for tracking this ctx2, all the way down to JITs
>> plus some way to handle 1 and 2 argument program calls generically.
>> Much easier to pass additional meta data for the program via cb[],
>> for example.
>
> Yeah, it did seem very complex :)
>
>>> Alternatively I can clear another pointer (u64) in the CB, store a
>>> pointer there, and always emit code following that pointer - should
>>> be possible right?
>>
>> What kind of pointer? If it's something like data_end as read-only,
>> then this needs to be tracked in the verifier in addition, of course.
>> Other option you could do (depending on what you want to achieve) is
>> to have a bpf_probe_read() version as a helper for your prog type
>> that would further walk that pointer/struct (similar to tracing)
>> where this comes w/o any backward compat guarantees, though.
>
> I meant something like this
>
> struct wifi_cb {
> struct wifi_data *wifi_data;
> ...
> void *data_end; // with BUILD_BUG_ON to the right offset
> };
>
> Then struct wifi_data can contain extra data that doesn't fit into
> wifi_cb, like the stuff I evicted for *data_end and *wifi_data. Let's
> say one of those fields is "u64 boottime_ns;" (as I did in my patch
> now), so we have
>
> struct wifi_data {
> u64 boottime_ns;
> };
>
> then I can still have
>
> struct __wifi_sk_buff {
> u32 len;
> u32 data;
> u32 data_end;
> u32 boottime_ns; // this is strange but
> // seems to be done this way?
> };
>
> And then when boottime_ns is accessed, I can have:
>
> case offsetof(struct __wifi_sk_buff, boottime_ns):
> off = si->off;
> off -= offsetof(struct __wifi_sk_buff, boottime_ns);
> off += offsetof(struct sk_buff, cb);
> off += offsetof(struct wifi_cb, wifi_data);
> *insn++ = BPF_LDX_MEM(BPF_SIZEOF(void *), si->dst_reg,
> si->src_reg, off);
> off = offsetof(struct wifi_data, boottime_ns);
> *isns++ = BPF_LDX_MEM(BPF_SIZEOF(u64), si->dst_reg,
> si->src_reg, off);
> break;
>
> no?
>
> It seems to me this should work, and essentially emit code to follow
> the pointer to inside struct wifi_data. Assuming
I see what you mean now. Yes, that's fine. We already do something
similar essentially with skb->ifindex access already (skb->dev +
dev->ifindex), f.e.:
[...]
case offsetof(struct __sk_buff, ifindex):
BUILD_BUG_ON(FIELD_SIZEOF(struct net_device, ifindex) != 4);
*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct sk_buff, dev),
si->dst_reg, si->src_reg,
offsetof(struct sk_buff, dev));
*insn++ = BPF_JMP_IMM(BPF_JEQ, si->dst_reg, 0, 1);
*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->dst_reg,
offsetof(struct net_device, ifindex));
break;
[...]
Which is not too different from the above. You'd probably need to
populate the struct wifi_data each time if you place it onto the
stack, but perhaps could be optimized by storing that somewhere
else (e.g. somewhere via netdev, etc) and walking the pointer from
there, which would also spare you the cb[] save/restore.
^ permalink raw reply
* Re: [PATCH net-next 0/3] l3mdev: Improve use with main table
From: David Ahern @ 2017-04-20 14:25 UTC (permalink / raw)
To: Robert Shearman, davem; +Cc: netdev
In-Reply-To: <256c2549-b831-2e7e-b18b-69243bb33ce3@brocade.com>
On 4/20/17 7:01 AM, Robert Shearman wrote:
>> The cached dst on sockets is one known place that causes a hang on a
>> delete. Basically the delete stalls until the sockets are closed. I have
>> a patch for sk_rx_dst which is the one I chased down last week.
I got to the bottom on the sk_rx_dst caching -- it is only a problem on
older kernels (e.g., our 4.1 kernels) and only with multicast udp
sockets. The early demux code in 4.1 is matching a listen socket when it
should not. The early demux code for mcast had some changes unrelated to
false matching that fixed the problem.
^ permalink raw reply
* Re: [PATCH net-next v5 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-20 14:25 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, xiyou.wangcong, eric.dumazet, netdev
In-Reply-To: <20170420135915.GE1886@nanopsycho.orion>
On 17-04-20 09:59 AM, Jiri Pirko wrote:
> Thu, Apr 20, 2017 at 03:06:21PM CEST, jhs@mojatatu.com wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> When you dump hundreds of thousands of actions, getting only 32 per
>> dump batch even when the socket buffer and memory allocations allow
>> is inefficient.
>>
>> With this change, the user will get as many as possibly fitting
>> within the given constraints available to the kernel.
>>
>> A new top level TLV space is introduced. An attribute
>> TCAA_ACT_FLAGS is used to carry the flags indicating the user
>> is capable of processing these large dumps. Older user space which
>> doesn't set this flag doesn't get the large (than 32) batches.
>> The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
>> actions are put in a single batch. As such user space app knows how long
>> to iterate (independent of the type of action being dumped)
>> instead of hardcoded maximum of 32.
>>
>> Some results dumping 1.5M actions, first unpatched tc which the
>> kernel doesn't help:
>>
>> prompt$ time -p tc actions ls action gact | grep index | wc -l
>> 1500000
>> real 1388.43
>> user 2.07
>> sys 1386.79
>>
>> Now lets see a patched tc which sets the correct flags when requesting
>> a dump:
>>
>> prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
>> 1500000
>> real 178.13
>> user 2.02
>> sys 176.96
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> ---
>> include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++--
>> net/sched/act_api.c | 46 +++++++++++++++++++++++++++++++++---------
>> 2 files changed, 55 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>> index cce0613..d7d28ec 100644
>> --- a/include/uapi/linux/rtnetlink.h
>> +++ b/include/uapi/linux/rtnetlink.h
>> @@ -674,10 +674,27 @@ struct tcamsg {
>> unsigned char tca__pad1;
>> unsigned short tca__pad2;
>> };
>> +
>> +enum {
>> + TCAA_UNSPEC,
>
> TCAA stands for "traffic control action action". I don't get it :(
TC Action Attributes == TCAA.
> Prefix still sounds wrong to me, sorry :/
> Should be:
> TCA_SOMETHING_*
>
TCA_ATTR_XXX ?
I have another opportunity to make a change where we should close
the discussion on this. We cant drag it forever.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next v5 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jiri Pirko @ 2017-04-20 14:24 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: davem, xiyou.wangcong, eric.dumazet, netdev
In-Reply-To: <a0d70dc5-b0cb-c54e-7d98-93099b68c247@mojatatu.com>
Thu, Apr 20, 2017 at 04:18:50PM CEST, jhs@mojatatu.com wrote:
>On 17-04-20 09:59 AM, Jiri Pirko wrote:
>> Thu, Apr 20, 2017 at 03:06:21PM CEST, jhs@mojatatu.com wrote:
>> > From: Jamal Hadi Salim <jhs@mojatatu.com>
>> >
>> > When you dump hundreds of thousands of actions, getting only 32 per
>> > dump batch even when the socket buffer and memory allocations allow
>> > is inefficient.
>> >
>> > With this change, the user will get as many as possibly fitting
>> > within the given constraints available to the kernel.
>> >
>> > A new top level TLV space is introduced. An attribute
>> > TCAA_ACT_FLAGS is used to carry the flags indicating the user
>> > is capable of processing these large dumps. Older user space which
>> > doesn't set this flag doesn't get the large (than 32) batches.
>> > The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
>> > actions are put in a single batch. As such user space app knows how long
>> > to iterate (independent of the type of action being dumped)
>> > instead of hardcoded maximum of 32.
>> >
>> > Some results dumping 1.5M actions, first unpatched tc which the
>> > kernel doesn't help:
>> >
>> > prompt$ time -p tc actions ls action gact | grep index | wc -l
>> > 1500000
>> > real 1388.43
>> > user 2.07
>> > sys 1386.79
>> >
>> > Now lets see a patched tc which sets the correct flags when requesting
>> > a dump:
>> >
>> > prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
>> > 1500000
>> > real 178.13
>> > user 2.02
>> > sys 176.96
>> >
>> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> > ---
>> > include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++--
>> > net/sched/act_api.c | 46 +++++++++++++++++++++++++++++++++---------
>> > 2 files changed, 55 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>> > index cce0613..d7d28ec 100644
>> > --- a/include/uapi/linux/rtnetlink.h
>> > +++ b/include/uapi/linux/rtnetlink.h
>> > @@ -674,10 +674,27 @@ struct tcamsg {
>> > unsigned char tca__pad1;
>> > unsigned short tca__pad2;
>> > };
>> > +
>> > +enum {
>> > + TCAA_UNSPEC,
>>
>> TCAA stands for "traffic control action action". I don't get it :(
>> Prefix still sounds wrong to me, sorry :/
>> Should be:
>> TCA_SOMETHING_*
>>
>>
>> > + TCAA_ACT_TAB,
>> > +#define TCA_ACT_TAB TCAA_ACT_TAB
>> > + TCAA_ACT_FLAGS,
>> > + TCAA_ACT_COUNT,
>> > + __TCAA_MAX,
>> > +#define TCAA_MAX (__TCAA_MAX - 1)
>> > +};
>> > +
>> > #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>> > #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>> > -#define TCA_ACT_TAB 1 /* attr type must be >=1 */
>> > -#define TCAA_MAX 1
>> > +/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
>> > + *
>> > + * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
>> > + * actions in a dump. All dump responses will contain the number of actions
>> > + * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
>> > + *
>> > + */
>> > +#define ACT_LARGE_DUMP_ON BIT(0)
>>
>> Please put some prefix to the name, as I asked for in the previous
>> version.
>>
>
>Didnt mean to leave out but:
>I cant seem to find it. Do you recall what you said it should be?
TCA_SOMETHING_FLAGS_LARGE_DUMP_ON
>
>>
>> >
>> > /* New extended info filters for IFLA_EXT_MASK */
>> > #define RTEXT_FILTER_VF (1 << 0)
>> > diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> > index 82b1d48..f85b8fd 100644
>> > --- a/net/sched/act_api.c
>> > +++ b/net/sched/act_api.c
>> > @@ -83,6 +83,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
>> > struct netlink_callback *cb)
>> > {
>> > int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
>> > + unsigned short act_flags = cb->args[2];
>> > struct nlattr *nest;
>> >
>> > spin_lock_bh(&hinfo->lock);
>> > @@ -111,14 +112,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
>> > }
>> > nla_nest_end(skb, nest);
>> > n_i++;
>> > - if (n_i >= TCA_ACT_MAX_PRIO)
>> > + if (!(act_flags & ACT_LARGE_DUMP_ON) &&
>> > + n_i >= TCA_ACT_MAX_PRIO)
>> > goto done;
>> > }
>> > }
>> > done:
>> > spin_unlock_bh(&hinfo->lock);
>> > - if (n_i)
>> > + if (n_i) {
>> > cb->args[0] += n_i;
>> > + if (act_flags & ACT_LARGE_DUMP_ON)
>> > + cb->args[1] = n_i;
>> > + }
>> > return n_i;
>> >
>> > nla_put_failure:
>> > @@ -993,11 +998,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
>> > return tcf_add_notify(net, n, &actions, portid);
>> > }
>> >
>> > +static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
>> > + [TCAA_ACT_FLAGS] = { .type = NLA_U32 },
>> > +};
>> > +
>> > static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>> > struct netlink_ext_ack *extack)
>> > {
>> > struct net *net = sock_net(skb->sk);
>> > - struct nlattr *tca[TCA_ACT_MAX + 1];
>> > + struct nlattr *tca[TCAA_MAX + 1];
>> > u32 portid = skb ? NETLINK_CB(skb).portid : 0;
>> > int ret = 0, ovr = 0;
>> >
>> > @@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>> > !netlink_capable(skb, CAP_NET_ADMIN))
>> > return -EPERM;
>> >
>> > - ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
>> > + ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
>>
>> Please do this in a separate patch. It is an unrelated bug fix.
>>
>
>Ok, that is fair. Thanks. Ok, if this was patch 1/3 in next update.
Good.
>
>>
>>
>
>>
>> "tcaa" again, now as a variable :/
>> Just use "tb" as the rest of the code does.
>>
>
>Sure.
>
>>
>> >
>> > + if (tcaa[TCAA_ACT_FLAGS])
>> > + act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
>>
>> I still believe this is wrong. Should be a separate attr per flag.
>> For user experience breakage reasons:
>> 2 kernels should not behave differently on the exact same value passed
>> from userspace:
>> User passes 0x2. Now the kernel will ignore the set bit, the next kernel
>> will recognize it as a valid flag and do something.
>> Please let the discussion reach a consensus before pushing this again.
>>
>>
>
>Jiri - I dont agree. There is no such breakage. Refer to my previous
>email. Lets just move on.
Anyone else has opinion on this?
^ permalink raw reply
* Re: [PATCH v4 1/3] VSOCK: Add vsockmon tap functions
From: Stefan Hajnoczi @ 2017-04-20 14:24 UTC (permalink / raw)
To: Jorgen S. Hansen
Cc: netdev@vger.kernel.org, Zhu Yanjun, Michael S. Tsirkin,
Gerard Garcia
In-Reply-To: <F612F48C-8072-4588-B38A-2F366F53040A@vmware.com>
[-- Attachment #1: Type: text/plain, Size: 665 bytes --]
On Thu, Apr 20, 2017 at 12:27:37PM +0000, Jorgen S. Hansen wrote:
>
> > On Apr 13, 2017, at 6:18 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > +
> > +static void __vsock_deliver_tap(struct sk_buff *skb)
> > +{
> > + int ret;
> > + struct vsock_tap *tmp;
> > +
> > + list_for_each_entry_rcu(tmp, &vsock_tap_all, list) {
> > + ret = __vsock_deliver_tap_skb(skb, tmp->dev);
> > + if (unlikely(ret))
> > + break;
> > + }
> > +
> > + consume_skb(skb);
>
> It looks like the caller will allocate the skb regardless of whether vsock_tap_all is empty, so shouldn’t the skb be consumed in vsock_deliver_tap?
You are right. Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* Re: [PATCH v4 3/3] VSOCK: Add virtio vsock vsockmon hooks
From: Stefan Hajnoczi @ 2017-04-20 14:24 UTC (permalink / raw)
To: Jorgen S. Hansen
Cc: netdev@vger.kernel.org, Zhu Yanjun, Michael S. Tsirkin,
Gerard Garcia
In-Reply-To: <5E1B910C-0113-494A-A46D-41F9686BD525@vmware.com>
[-- Attachment #1: Type: text/plain, Size: 788 bytes --]
On Thu, Apr 20, 2017 at 12:35:40PM +0000, Jorgen S. Hansen wrote:
> >
> > +/* Packet capture */
> > +void virtio_transport_deliver_tap_pkt(struct virtio_vsock_pkt *pkt)
> > +{
> > + struct sk_buff *skb;
> > + struct af_vsockmon_hdr *hdr;
> > + unsigned char *t_hdr, *payload;
> > +
> > + skb = alloc_skb(sizeof(*hdr) + sizeof(pkt->hdr) + pkt->len,
> > + GFP_ATOMIC);
>
> So with the current API, an skb will always be allocated, even if there are no listeners? And we’ll copy the payload into the skb as well, if there is any. Would it make sense to introduce a check here (exposed as part of the vsock tap API), that skips all that in the case of no listeners? In the common case, there won’t be any listeners, so it would save some cycles.
Good point, will fix.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply
* Re: [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats
From: Saeed Mahameed @ 2017-04-20 14:23 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Gal Pressman, Linux Netdev List, Saeed Mahameed, Eric Dumazet,
Kernel Team
In-Reply-To: <20170420141526.maq6b6rgqfjcdyn6@kafai-mba.dhcp.thefacebook.com>
On Thu, Apr 20, 2017 at 5:15 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> On Thu, Apr 20, 2017 at 05:00:13PM +0300, Saeed Mahameed wrote:
>> On Thu, Apr 20, 2017 at 2:32 AM, Martin KaFai Lau <kafai@fb.com> wrote:
>> > We have observed a sudden spike in rx/tx_packets and rx/tx_bytes
>> > reported under /proc/net/dev. There is a race in mlx5e_update_stats()
>> > and some of the get-stats functions (the one that we hit is the
>> > mlx5e_get_stats() which is called by ndo_get_stats64()).
>> >
>> > In particular, the very first thing mlx5e_update_sw_counters()
>> > does is 'memset(s, 0, sizeof(*s))'. For example, if mlx5e_get_stats()
>> > is unlucky at one point, rx_bytes and rx_packets could be 0. One second
>> > later, a normal (and much bigger than 0) value will be reported.
>> >
>> > This patch is to use a 'struct mlx5e_sw_stats temp' to avoid
>> > a direct memset zero on priv->stats.sw.
>> >
>> > mlx5e_update_vport_counters() has a similar race. Hence, addressed
>> > together.
>> >
>> > I am lucky enough to catch this 0-reset in rx multicast:
>> > eth0: 41457665 76804 70 0 0 70 0 47085 15586634 87502 3 0 0 0 3 0
>> > eth0: 41459860 76815 70 0 0 70 0 47094 15588376 87516 3 0 0 0 3 0
>> > eth0: 41460577 76822 70 0 0 70 0 0 15589083 87521 3 0 0 0 3 0
>> > eth0: 41463293 76838 70 0 0 70 0 47108 15595872 87538 3 0 0 0 3 0
>> > eth0: 41463379 76839 70 0 0 70 0 47116 15596138 87539 3 0 0 0 3 0
>> >
>> > Cc: Saeed Mahameed <saeedm@mellanox.com>
>> > Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
>> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
>> > ---
>> > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++--
>> > 1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > index 66c133757a5e..246786bb861b 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> > @@ -174,7 +174,7 @@ static void mlx5e_tx_timeout_work(struct work_struct *work)
>> >
>> > static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
>> > {
>> > - struct mlx5e_sw_stats *s = &priv->stats.sw;
>> > + struct mlx5e_sw_stats temp, *s = &temp;
>> > struct mlx5e_rq_stats *rq_stats;
>> > struct mlx5e_sq_stats *sq_stats;
>> > u64 tx_offload_none = 0;
>> > @@ -229,12 +229,14 @@ static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
>> > s->link_down_events_phy = MLX5_GET(ppcnt_reg,
>> > priv->stats.pport.phy_counters,
>> > counter_set.phys_layer_cntrs.link_down_events);
>> > + memcpy(&priv->stats.sw, s, sizeof(*s));
>> > }
>> >
>> > static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
>> > {
>> > + struct mlx5e_vport_stats temp;
>> > int outlen = MLX5_ST_SZ_BYTES(query_vport_counter_out);
>> > - u32 *out = (u32 *)priv->stats.vport.query_vport_out;
>> > + u32 *out = (u32 *)temp.query_vport_out;
>> > u32 in[MLX5_ST_SZ_DW(query_vport_counter_in)] = {0};
>> > struct mlx5_core_dev *mdev = priv->mdev;
>> >
>> > @@ -245,6 +247,7 @@ static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
>> >
>> > memset(out, 0, outlen);
>>
>> Actually you don't need any temp here, it is safe to just remove this
>> redundant memset
>> and mlx5_cmd_exec will do the copy for you.
>>
>> > mlx5_cmd_exec(mdev, in, sizeof(in), out, outlen);
>> > + memcpy(priv->stats.vport.query_vport_out, out, outlen);
>> > }
>>
>> Anyway we still need a spin lock here, and also for all the counters
>> under priv->stats which are affected by this race as well.
>>
>> If you want I can accept this as a temporary fix for net and Gal will
>> work on a spin lock based mechanism to fix the memcpy race for all the
>> counters.
> A follow-up patch approach by Gal will be nice.
>
Ok, I will expect V3 with the removal of the memset from
mlx5e_update_vport_counters instead of the memcpy.
and Gal will work on the follow up patch
^ permalink raw reply
* Re: [PATCH] nl80211: fix enumeration type
From: Stefan Agner @ 2017-04-20 14:21 UTC (permalink / raw)
To: Johannes Berg
Cc: davem-fT/PcQaiUtIeIZ0/mPfg9Q,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1492672926.3109.5.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
On 2017-04-20 00:22, Johannes Berg wrote:
> On Wed, 2017-04-19 at 23:55 -0700, Stefan Agner wrote:
>> Use type enum nl80211_rate_info for bitrate information. This fixes
>> a warning when compiling with clang:
>> warning: implicit conversion from enumeration type 'enum
>> nl80211_rate_info'
>> to different enumeration type 'enum nl80211_attrs'
>
> I'm pretty sure I just applied exactly these two patches very recently.
>
> What made *two* people suddenly see this?
There has been renewed work on compiling Linux using clang... Sorry for
the clash.
--
Stefan
^ permalink raw reply
* Re: [PATCH net] ipv4: Avoid caching dsts when lookup skipped nh oif check
From: David Ahern @ 2017-04-20 14:21 UTC (permalink / raw)
To: Robert Shearman, davem; +Cc: netdev
In-Reply-To: <1492693132-4708-1-git-send-email-rshearma@brocade.com>
On 4/20/17 6:58 AM, Robert Shearman wrote:
> David reported that doing the following:
>
> ip li add red type vrf table 10
> ip link set dev eth1 vrf red
> ip addr add 127.0.0.1/8 dev red
> ip link set dev eth1 up
> ip li set red up
> ping -c1 -w1 -I red 127.0.0.1
> ip li del red
>
> results in a hang with this message:
>
> unregister_netdevice: waiting for red to become free. Usage count = 1
I think you misunderstood my comment. The above works fine today. There
is no bug with refcnts.
It breaks with your patches wanting to use a VRF device with the main
table (254).
^ permalink raw reply
* Re: [PATCH net-next v5 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-20 14:18 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, xiyou.wangcong, eric.dumazet, netdev
In-Reply-To: <20170420135915.GE1886@nanopsycho.orion>
On 17-04-20 09:59 AM, Jiri Pirko wrote:
> Thu, Apr 20, 2017 at 03:06:21PM CEST, jhs@mojatatu.com wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> When you dump hundreds of thousands of actions, getting only 32 per
>> dump batch even when the socket buffer and memory allocations allow
>> is inefficient.
>>
>> With this change, the user will get as many as possibly fitting
>> within the given constraints available to the kernel.
>>
>> A new top level TLV space is introduced. An attribute
>> TCAA_ACT_FLAGS is used to carry the flags indicating the user
>> is capable of processing these large dumps. Older user space which
>> doesn't set this flag doesn't get the large (than 32) batches.
>> The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
>> actions are put in a single batch. As such user space app knows how long
>> to iterate (independent of the type of action being dumped)
>> instead of hardcoded maximum of 32.
>>
>> Some results dumping 1.5M actions, first unpatched tc which the
>> kernel doesn't help:
>>
>> prompt$ time -p tc actions ls action gact | grep index | wc -l
>> 1500000
>> real 1388.43
>> user 2.07
>> sys 1386.79
>>
>> Now lets see a patched tc which sets the correct flags when requesting
>> a dump:
>>
>> prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
>> 1500000
>> real 178.13
>> user 2.02
>> sys 176.96
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> ---
>> include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++--
>> net/sched/act_api.c | 46 +++++++++++++++++++++++++++++++++---------
>> 2 files changed, 55 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>> index cce0613..d7d28ec 100644
>> --- a/include/uapi/linux/rtnetlink.h
>> +++ b/include/uapi/linux/rtnetlink.h
>> @@ -674,10 +674,27 @@ struct tcamsg {
>> unsigned char tca__pad1;
>> unsigned short tca__pad2;
>> };
>> +
>> +enum {
>> + TCAA_UNSPEC,
>
> TCAA stands for "traffic control action action". I don't get it :(
> Prefix still sounds wrong to me, sorry :/
> Should be:
> TCA_SOMETHING_*
>
>
>> + TCAA_ACT_TAB,
>> +#define TCA_ACT_TAB TCAA_ACT_TAB
>> + TCAA_ACT_FLAGS,
>> + TCAA_ACT_COUNT,
>> + __TCAA_MAX,
>> +#define TCAA_MAX (__TCAA_MAX - 1)
>> +};
>> +
>> #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>> -#define TCA_ACT_TAB 1 /* attr type must be >=1 */
>> -#define TCAA_MAX 1
>> +/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
>> + *
>> + * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
>> + * actions in a dump. All dump responses will contain the number of actions
>> + * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
>> + *
>> + */
>> +#define ACT_LARGE_DUMP_ON BIT(0)
>
> Please put some prefix to the name, as I asked for in the previous
> version.
>
Didnt mean to leave out but:
I cant seem to find it. Do you recall what you said it should be?
>
>>
>> /* New extended info filters for IFLA_EXT_MASK */
>> #define RTEXT_FILTER_VF (1 << 0)
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 82b1d48..f85b8fd 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -83,6 +83,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
>> struct netlink_callback *cb)
>> {
>> int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
>> + unsigned short act_flags = cb->args[2];
>> struct nlattr *nest;
>>
>> spin_lock_bh(&hinfo->lock);
>> @@ -111,14 +112,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
>> }
>> nla_nest_end(skb, nest);
>> n_i++;
>> - if (n_i >= TCA_ACT_MAX_PRIO)
>> + if (!(act_flags & ACT_LARGE_DUMP_ON) &&
>> + n_i >= TCA_ACT_MAX_PRIO)
>> goto done;
>> }
>> }
>> done:
>> spin_unlock_bh(&hinfo->lock);
>> - if (n_i)
>> + if (n_i) {
>> cb->args[0] += n_i;
>> + if (act_flags & ACT_LARGE_DUMP_ON)
>> + cb->args[1] = n_i;
>> + }
>> return n_i;
>>
>> nla_put_failure:
>> @@ -993,11 +998,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
>> return tcf_add_notify(net, n, &actions, portid);
>> }
>>
>> +static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
>> + [TCAA_ACT_FLAGS] = { .type = NLA_U32 },
>> +};
>> +
>> static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>> struct netlink_ext_ack *extack)
>> {
>> struct net *net = sock_net(skb->sk);
>> - struct nlattr *tca[TCA_ACT_MAX + 1];
>> + struct nlattr *tca[TCAA_MAX + 1];
>> u32 portid = skb ? NETLINK_CB(skb).portid : 0;
>> int ret = 0, ovr = 0;
>>
>> @@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
>> !netlink_capable(skb, CAP_NET_ADMIN))
>> return -EPERM;
>>
>> - ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
>> + ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
>
> Please do this in a separate patch. It is an unrelated bug fix.
>
Ok, that is fair. Thanks. Ok, if this was patch 1/3 in next update.
>
>
>
> "tcaa" again, now as a variable :/
> Just use "tb" as the rest of the code does.
>
Sure.
>
>>
>> + if (tcaa[TCAA_ACT_FLAGS])
>> + act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
>
> I still believe this is wrong. Should be a separate attr per flag.
> For user experience breakage reasons:
> 2 kernels should not behave differently on the exact same value passed
> from userspace:
> User passes 0x2. Now the kernel will ignore the set bit, the next kernel
> will recognize it as a valid flag and do something.
> Please let the discussion reach a consensus before pushing this again.
>
>
Jiri - I dont agree. There is no such breakage. Refer to my previous
email. Lets just move on.
cheers,
jamal
^ permalink raw reply
* Re: __sk_buff.data_end
From: Johannes Berg @ 2017-04-20 14:17 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev
In-Reply-To: <58F8C160.6010905@iogearbox.net>
On Thu, 2017-04-20 at 16:10 +0200, Daniel Borkmann wrote:
>
> I think this would be a rather more complex operation on the BPF
> side, it would need changes from LLVM (which assumes initial ctx sits
> in r1), verifier for tracking this ctx2, all the way down to JITs
> plus some way to handle 1 and 2 argument program calls generically.
> Much easier to pass additional meta data for the program via cb[],
> for example.
Yeah, it did seem very complex :)
> > Alternatively I can clear another pointer (u64) in the CB, store a
> > pointer there, and always emit code following that pointer - should
> > be possible right?
>
> What kind of pointer? If it's something like data_end as read-only,
> then this needs to be tracked in the verifier in addition, of course.
> Other option you could do (depending on what you want to achieve) is
> to have a bpf_probe_read() version as a helper for your prog type
> that would further walk that pointer/struct (similar to tracing)
> where this comes w/o any backward compat guarantees, though.
I meant something like this
struct wifi_cb {
struct wifi_data *wifi_data;
...
void *data_end; // with BUILD_BUG_ON to the right offset
};
Then struct wifi_data can contain extra data that doesn't fit into
wifi_cb, like the stuff I evicted for *data_end and *wifi_data. Let's
say one of those fields is "u64 boottime_ns;" (as I did in my patch
now), so we have
struct wifi_data {
u64 boottime_ns;
};
then I can still have
struct __wifi_sk_buff {
u32 len;
u32 data;
u32 data_end;
u32 boottime_ns; // this is strange but
// seems to be done this way?
};
And then when boottime_ns is accessed, I can have:
case offsetof(struct __wifi_sk_buff, boottime_ns):
off = si->off;
off -= offsetof(struct __wifi_sk_buff, boottime_ns);
off += offsetof(struct sk_buff, cb);
off += offsetof(struct wifi_cb, wifi_data);
*insn++ = BPF_LDX_MEM(BPF_SIZEOF(void *), si->dst_reg,
si->src_reg, off);
off = offsetof(struct wifi_data, boottime_ns);
*isns++ = BPF_LDX_MEM(BPF_SIZEOF(u64), si->dst_reg,
si->src_reg, off);
break;
no?
It seems to me this should work, and essentially emit code to follow
the pointer to inside struct wifi_data. Assuming
johannes
^ permalink raw reply
* Re: [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats
From: Martin KaFai Lau @ 2017-04-20 14:15 UTC (permalink / raw)
To: Saeed Mahameed
Cc: Gal Pressman, Linux Netdev List, Saeed Mahameed, Eric Dumazet,
Kernel Team
In-Reply-To: <CALzJLG_r4x2PvVTG8vdDRFfGBfRe-totyUaPtV8ehmqiL0dURA@mail.gmail.com>
On Thu, Apr 20, 2017 at 05:00:13PM +0300, Saeed Mahameed wrote:
> On Thu, Apr 20, 2017 at 2:32 AM, Martin KaFai Lau <kafai@fb.com> wrote:
> > We have observed a sudden spike in rx/tx_packets and rx/tx_bytes
> > reported under /proc/net/dev. There is a race in mlx5e_update_stats()
> > and some of the get-stats functions (the one that we hit is the
> > mlx5e_get_stats() which is called by ndo_get_stats64()).
> >
> > In particular, the very first thing mlx5e_update_sw_counters()
> > does is 'memset(s, 0, sizeof(*s))'. For example, if mlx5e_get_stats()
> > is unlucky at one point, rx_bytes and rx_packets could be 0. One second
> > later, a normal (and much bigger than 0) value will be reported.
> >
> > This patch is to use a 'struct mlx5e_sw_stats temp' to avoid
> > a direct memset zero on priv->stats.sw.
> >
> > mlx5e_update_vport_counters() has a similar race. Hence, addressed
> > together.
> >
> > I am lucky enough to catch this 0-reset in rx multicast:
> > eth0: 41457665 76804 70 0 0 70 0 47085 15586634 87502 3 0 0 0 3 0
> > eth0: 41459860 76815 70 0 0 70 0 47094 15588376 87516 3 0 0 0 3 0
> > eth0: 41460577 76822 70 0 0 70 0 0 15589083 87521 3 0 0 0 3 0
> > eth0: 41463293 76838 70 0 0 70 0 47108 15595872 87538 3 0 0 0 3 0
> > eth0: 41463379 76839 70 0 0 70 0 47116 15596138 87539 3 0 0 0 3 0
> >
> > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > index 66c133757a5e..246786bb861b 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> > @@ -174,7 +174,7 @@ static void mlx5e_tx_timeout_work(struct work_struct *work)
> >
> > static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
> > {
> > - struct mlx5e_sw_stats *s = &priv->stats.sw;
> > + struct mlx5e_sw_stats temp, *s = &temp;
> > struct mlx5e_rq_stats *rq_stats;
> > struct mlx5e_sq_stats *sq_stats;
> > u64 tx_offload_none = 0;
> > @@ -229,12 +229,14 @@ static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
> > s->link_down_events_phy = MLX5_GET(ppcnt_reg,
> > priv->stats.pport.phy_counters,
> > counter_set.phys_layer_cntrs.link_down_events);
> > + memcpy(&priv->stats.sw, s, sizeof(*s));
> > }
> >
> > static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
> > {
> > + struct mlx5e_vport_stats temp;
> > int outlen = MLX5_ST_SZ_BYTES(query_vport_counter_out);
> > - u32 *out = (u32 *)priv->stats.vport.query_vport_out;
> > + u32 *out = (u32 *)temp.query_vport_out;
> > u32 in[MLX5_ST_SZ_DW(query_vport_counter_in)] = {0};
> > struct mlx5_core_dev *mdev = priv->mdev;
> >
> > @@ -245,6 +247,7 @@ static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
> >
> > memset(out, 0, outlen);
>
> Actually you don't need any temp here, it is safe to just remove this
> redundant memset
> and mlx5_cmd_exec will do the copy for you.
>
> > mlx5_cmd_exec(mdev, in, sizeof(in), out, outlen);
> > + memcpy(priv->stats.vport.query_vport_out, out, outlen);
> > }
>
> Anyway we still need a spin lock here, and also for all the counters
> under priv->stats which are affected by this race as well.
>
> If you want I can accept this as a temporary fix for net and Gal will
> work on a spin lock based mechanism to fix the memcpy race for all the
> counters.
A follow-up patch approach by Gal will be nice.
>
> >
> > static void mlx5e_update_pport_counters(struct mlx5e_priv *priv)
> > --
> > 2.9.3
> >
^ permalink raw reply
* [PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.
From: Roger Quadros @ 2017-04-20 14:11 UTC (permalink / raw)
To: davem, Andrew Lunn, Florian Fainelli
Cc: tony, nsekhar, jsarha, netdev, linux-omap, linux-kernel, rogerq
In-Reply-To: <daa29a99-0a17-d316-57eb-3e6162938d8d@ti.com>
Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.
Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.
In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.
[1] - am572x-idk, am571x-idk, a437x-idk
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
v3:
- added more information in the DT binding document.
v2:
- add device tree binding document (mdio.txt)
- specify default reset delay in of_mdio.c instead of mdio_bus.c
Documentation/devicetree/bindings/net/mdio.txt | 33 ++++++++++++++++++++++++++
drivers/net/phy/mdio_bus.c | 22 +++++++++++++++++
drivers/of/of_mdio.c | 7 ++++++
include/linux/phy.h | 5 ++++
4 files changed, 67 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/mdio.txt
diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
new file mode 100644
index 0000000..3517369
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio.txt
@@ -0,0 +1,33 @@
+Common MDIO bus properties.
+
+These are generic properties that can apply to any MDIO bus.
+
+Optional properties:
+- reset-gpios: List of one or more GPIOs that control the RESET lines
+ of the PHYs on that MDIO bus.
+- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
+
+A list of child nodes, one per device on the bus is expected. These
+should follow the generic phy.txt, or a device specific binding document.
+
+Example :
+This example shows these optional properties, plus other properties
+required for the TI Davinci MDIO driver.
+
+ davinci_mdio: ethernet@0x5c030000 {
+ compatible = "ti,davinci_mdio";
+ reg = <0x5c030000 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+ reset-delay-us = <2>; /* PHY datasheet states 1uS min */
+
+ ethphy0: ethernet-phy@1 {
+ reg = <1>;
+ };
+
+ ethphy1: ethernet-phy@3 {
+ reg = <3>;
+ };
+ };
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..b353d99 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
#include <linux/init.h>
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/of_device.h>
#include <linux/of_mdio.h>
+#include <linux/of_gpio.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
#include <linux/skbuff.h>
@@ -307,6 +310,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
{
struct mdio_device *mdiodev;
int i, err;
+ struct gpio_desc *gpiod;
if (NULL == bus || NULL == bus->name ||
NULL == bus->read || NULL == bus->write)
@@ -333,6 +337,24 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner)
if (bus->reset)
bus->reset(bus);
+ /* de-assert bus level PHY GPIO resets */
+ for (i = 0; i < bus->num_reset_gpios; i++) {
+ gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+ GPIOD_OUT_LOW);
+ if (IS_ERR(gpiod)) {
+ err = PTR_ERR(gpiod);
+ if (err != -ENOENT) {
+ pr_err("mii_bus %s couldn't get reset GPIO\n",
+ bus->id);
+ return err;
+ }
+ } else {
+ gpiod_set_value_cansleep(gpiod, 1);
+ udelay(bus->reset_delay_us);
+ gpiod_set_value_cansleep(gpiod, 0);
+ }
+ }
+
for (i = 0; i < PHY_MAX_ADDR; i++) {
if ((bus->phy_mask & (1 << i)) == 0) {
struct phy_device *phydev;
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b29798..7e4c80f 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -22,6 +22,8 @@
#include <linux/of_net.h>
#include <linux/module.h>
+#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
+
MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
MODULE_LICENSE("GPL");
@@ -221,6 +223,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
mdio->dev.of_node = np;
+ /* Get bus level PHY reset GPIO details */
+ mdio->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+ of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
+ mdio->num_reset_gpios = of_gpio_named_count(np, "reset-gpios");
+
/* Register the MDIO bus */
rc = mdiobus_register(mdio);
if (rc)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 43a7748..80a6574 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -217,6 +217,11 @@ struct mii_bus {
* matching its address
*/
int irq[PHY_MAX_ADDR];
+
+ /* GPIO reset pulse width in uS */
+ int reset_delay_us;
+ /* Number of reset GPIOs */
+ int num_reset_gpios;
};
#define to_mii_bus(d) container_of(d, struct mii_bus, dev)
--
2.7.4
^ permalink raw reply related
* Re: __sk_buff.data_end
From: Daniel Borkmann @ 2017-04-20 14:10 UTC (permalink / raw)
To: Johannes Berg, Alexei Starovoitov; +Cc: netdev
In-Reply-To: <1492668065.3109.1.camel@sipsolutions.net>
On 04/20/2017 08:01 AM, Johannes Berg wrote:
> On Thu, 2017-04-20 at 02:01 +0200, Daniel Borkmann wrote:
>>
>> Yeah, should work as well for the 32 bit archs, on 64 bit we
>> have this effectively already:
>
> Right.
>
> [...]
>
>> Can you elaborate on why this works for mac80211? It uses cb
>> only up to that point from where you invoke the prog?
>
> No, it works because then I can move a u64 field to the same offset,
> and save/restore it across the BPF call :)
Right.
> But I don't have a *pointer* field to move there, and no space for the
> alignment anyway (already using all 48 bytes).
>
> Come to think of it - somebody had proposed extensions to this by
> passing an on-stack pointer in addition to the data in the cb.
>
> Perhaps we can extend BPF to have an optional second argument, and
> track a second context around the verifier, if applicable? Then we can
> solve all of this really easily, because it means we don't always have
> to go from the SKB context but could go from the other one (which could
> be that on-stack buffer).
I think this would be a rather more complex operation on the BPF side,
it would need changes from LLVM (which assumes initial ctx sits in r1),
verifier for tracking this ctx2, all the way down to JITs plus some way
to handle 1 and 2 argument program calls generically. Much easier to
pass additional meta data for the program via cb[], for example.
> Alternatively I can clear another pointer (u64) in the CB, store a
> pointer there, and always emit code following that pointer - should be
> possible right?
What kind of pointer? If it's something like data_end as read-only, then
this needs to be tracked in the verifier in addition, of course. Other
option you could do (depending on what you want to achieve) is to have
a bpf_probe_read() version as a helper for your prog type that would
further walk that pointer/struct (similar to tracing) where this comes
w/o any backward compat guarantees, though.
^ permalink raw reply
* Re: [PATCH net-next v2 5/5] virtio-net: keep tx interrupts disabled unless kick
From: Willem de Bruijn @ 2017-04-20 14:03 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, Michael S. Tsirkin, virtualization,
David Miller, Willem de Bruijn
In-Reply-To: <72551dd3-ba91-812e-9065-d153bf8a7024@redhat.com>
>> - if (!use_napi)
>> + if (use_napi) {
>> + if (kick)
>> + virtqueue_enable_cb_delayed(sq->vq);
>> + else
>> + virtqueue_disable_cb(sq->vq);
>
>
> Since virtqueue_disable_cb() do nothing for event idx. I wonder whether or
> not just calling enable_cb_dealyed() is ok here.
Good point.
> Btw, it does not disable interrupt at all, I propose a patch in the past
> which can do more than this:
>
> https://patchwork.kernel.org/patch/6472601/
Interesting. Yes, let me evaluate that variant.
Thanks for reviewing,
Willem
^ permalink raw reply
* Re: [PATCH net-next v4 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-20 14:03 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, netdev, eric.dumazet, xiyou.wangcong
In-Reply-To: <20170420113540.GC1886@nanopsycho.orion>
On 17-04-20 07:35 AM, Jiri Pirko wrote:
> Thu, Apr 20, 2017 at 12:42:55PM CEST, jhs@mojatatu.com wrote:
>> On 17-04-19 12:17 PM, Jiri Pirko wrote:
>
> Ha. So the current code is wrong, I see it now. Following is needed:
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 82b1d48..c432b22 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1005,7 +1005,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> !netlink_capable(skb, CAP_NET_ADMIN))
> return -EPERM;
>
> - ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
> + ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL,
> extack);
> if (ret < 0)
> return ret;
>
> You confused me squashing the change to this patch.
>
> Ok, so the name could be:
> TCA_ACTS_*
> ?
> I believe it is crucial to figure this out right now. TC UAPI is in deep
> mess already, no need to push it even more.
>
I could change that name. Look at the last series i sent, add any extra
comments to that and i will get to it by tomorrow.
>> They are not the same issue Jiri. We have used bitmasks fine on netlink
>
> Howcome they are not the same? I'm pretty certain they are.
>
They are not Jiri. I challenge you to point me to one netlink bitmask
representation used that has some bits that were not being used that
someone or some compiler is going to set some random values on.
Typically flags are u64/32/16
>
>> message for a millenia. Nobody sets garbage on a bitmask they are not
>> supposed to touch. The struct padding thing is a shame the way it
>> turned out - now netlink can no longer have a claim to be a (good)
>> wire protocol.
>> Other thing: I know you feel strongly about this but I dont agree that
>> everything has to be a TLV and in no way, as a matter of principle, you are
>> going to convince me (even when the cows come home) that I have to
>> use 64 bits to carry a single bit just so I can use TLVs.
>
> Then I guess we have to agree to disagree. I believe that your approach
> is wrong and will break users in future when you add even a single flag.
> Argument that "we are doing this for ages-therefore it is correct" has
> simply 0 value.
>
"doing these for 80 years" is not the main driving point.
I appreciate that data structures - even when fields are
annotated as "pads" there is no guarantee when i allocate one
the pads will be zeroes.
Bitmasks are atomic in nature and therefore used in whole - not complex
like structs. Thats the difference. I dont define this u32 bitmap that
has 8 bits that are "pads" because that concept doesnt make sense for
atomic fields.
This implies that 6 months down the road I dont regret and say "shit, I
need 2 more bits for flagbits but some idiot or compiler was setting
those fields so i cant use them now because the kernel wont tell the
difference between what s/he is doing and real values".
Allocations of data structures I can understand, unless we enforce
always reset to zeroes all the struct members as part of creation.
But lets agree to disagree if you are still not convinced.
On your "everything must be a TLV". Caveat is: there is a balance
between performance and flexibility. I should be able to pack aligned
complex data structures in a TLV for performance reasons.
One lesson is: in the future we can make these extra service header like
tcamsg very small and always use all their fields instead of defining
pads.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net v2] net/mlx5e: Fix race in mlx5e_sw_stats and mlx5e_vport_stats
From: Saeed Mahameed @ 2017-04-20 14:00 UTC (permalink / raw)
To: Martin KaFai Lau, Gal Pressman
Cc: Linux Netdev List, Saeed Mahameed, Eric Dumazet, Kernel Team
In-Reply-To: <20170419233259.2255806-1-kafai@fb.com>
On Thu, Apr 20, 2017 at 2:32 AM, Martin KaFai Lau <kafai@fb.com> wrote:
> We have observed a sudden spike in rx/tx_packets and rx/tx_bytes
> reported under /proc/net/dev. There is a race in mlx5e_update_stats()
> and some of the get-stats functions (the one that we hit is the
> mlx5e_get_stats() which is called by ndo_get_stats64()).
>
> In particular, the very first thing mlx5e_update_sw_counters()
> does is 'memset(s, 0, sizeof(*s))'. For example, if mlx5e_get_stats()
> is unlucky at one point, rx_bytes and rx_packets could be 0. One second
> later, a normal (and much bigger than 0) value will be reported.
>
> This patch is to use a 'struct mlx5e_sw_stats temp' to avoid
> a direct memset zero on priv->stats.sw.
>
> mlx5e_update_vport_counters() has a similar race. Hence, addressed
> together.
>
> I am lucky enough to catch this 0-reset in rx multicast:
> eth0: 41457665 76804 70 0 0 70 0 47085 15586634 87502 3 0 0 0 3 0
> eth0: 41459860 76815 70 0 0 70 0 47094 15588376 87516 3 0 0 0 3 0
> eth0: 41460577 76822 70 0 0 70 0 0 15589083 87521 3 0 0 0 3 0
> eth0: 41463293 76838 70 0 0 70 0 47108 15595872 87538 3 0 0 0 3 0
> eth0: 41463379 76839 70 0 0 70 0 47116 15596138 87539 3 0 0 0 3 0
>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index 66c133757a5e..246786bb861b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> @@ -174,7 +174,7 @@ static void mlx5e_tx_timeout_work(struct work_struct *work)
>
> static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
> {
> - struct mlx5e_sw_stats *s = &priv->stats.sw;
> + struct mlx5e_sw_stats temp, *s = &temp;
> struct mlx5e_rq_stats *rq_stats;
> struct mlx5e_sq_stats *sq_stats;
> u64 tx_offload_none = 0;
> @@ -229,12 +229,14 @@ static void mlx5e_update_sw_counters(struct mlx5e_priv *priv)
> s->link_down_events_phy = MLX5_GET(ppcnt_reg,
> priv->stats.pport.phy_counters,
> counter_set.phys_layer_cntrs.link_down_events);
> + memcpy(&priv->stats.sw, s, sizeof(*s));
> }
>
> static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
> {
> + struct mlx5e_vport_stats temp;
> int outlen = MLX5_ST_SZ_BYTES(query_vport_counter_out);
> - u32 *out = (u32 *)priv->stats.vport.query_vport_out;
> + u32 *out = (u32 *)temp.query_vport_out;
> u32 in[MLX5_ST_SZ_DW(query_vport_counter_in)] = {0};
> struct mlx5_core_dev *mdev = priv->mdev;
>
> @@ -245,6 +247,7 @@ static void mlx5e_update_vport_counters(struct mlx5e_priv *priv)
>
> memset(out, 0, outlen);
Actually you don't need any temp here, it is safe to just remove this
redundant memset
and mlx5_cmd_exec will do the copy for you.
> mlx5_cmd_exec(mdev, in, sizeof(in), out, outlen);
> + memcpy(priv->stats.vport.query_vport_out, out, outlen);
> }
Anyway we still need a spin lock here, and also for all the counters
under priv->stats which are affected by this race as well.
If you want I can accept this as a temporary fix for net and Gal will
work on a spin lock based mechanism to fix the memcpy race for all the
counters.
>
> static void mlx5e_update_pport_counters(struct mlx5e_priv *priv)
> --
> 2.9.3
>
^ permalink raw reply
* Re: [PATCH net-next v5 1/2] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jiri Pirko @ 2017-04-20 13:59 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: davem, xiyou.wangcong, eric.dumazet, netdev
In-Reply-To: <1492693582-26810-2-git-send-email-jhs@emojatatu.com>
Thu, Apr 20, 2017 at 03:06:21PM CEST, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>When you dump hundreds of thousands of actions, getting only 32 per
>dump batch even when the socket buffer and memory allocations allow
>is inefficient.
>
>With this change, the user will get as many as possibly fitting
>within the given constraints available to the kernel.
>
>A new top level TLV space is introduced. An attribute
>TCAA_ACT_FLAGS is used to carry the flags indicating the user
>is capable of processing these large dumps. Older user space which
>doesn't set this flag doesn't get the large (than 32) batches.
>The kernel uses the TCAA_ACT_COUNT attribute to tell the user how many
>actions are put in a single batch. As such user space app knows how long
>to iterate (independent of the type of action being dumped)
>instead of hardcoded maximum of 32.
>
>Some results dumping 1.5M actions, first unpatched tc which the
>kernel doesn't help:
>
>prompt$ time -p tc actions ls action gact | grep index | wc -l
>1500000
>real 1388.43
>user 2.07
>sys 1386.79
>
>Now lets see a patched tc which sets the correct flags when requesting
>a dump:
>
>prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
>1500000
>real 178.13
>user 2.02
>sys 176.96
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>---
> include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++--
> net/sched/act_api.c | 46 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 55 insertions(+), 12 deletions(-)
>
>diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>index cce0613..d7d28ec 100644
>--- a/include/uapi/linux/rtnetlink.h
>+++ b/include/uapi/linux/rtnetlink.h
>@@ -674,10 +674,27 @@ struct tcamsg {
> unsigned char tca__pad1;
> unsigned short tca__pad2;
> };
>+
>+enum {
>+ TCAA_UNSPEC,
TCAA stands for "traffic control action action". I don't get it :(
Prefix still sounds wrong to me, sorry :/
Should be:
TCA_SOMETHING_*
>+ TCAA_ACT_TAB,
>+#define TCA_ACT_TAB TCAA_ACT_TAB
>+ TCAA_ACT_FLAGS,
>+ TCAA_ACT_COUNT,
>+ __TCAA_MAX,
>+#define TCAA_MAX (__TCAA_MAX - 1)
>+};
>+
> #define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>-#define TCA_ACT_TAB 1 /* attr type must be >=1 */
>-#define TCAA_MAX 1
>+/* tcamsg flags stored in attribute TCAA_ACT_FLAGS
>+ *
>+ * ACT_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
>+ * actions in a dump. All dump responses will contain the number of actions
>+ * being dumped stored in for user app's consumption in TCAA_ACT_COUNT
>+ *
>+ */
>+#define ACT_LARGE_DUMP_ON BIT(0)
Please put some prefix to the name, as I asked for in the previous
version.
>
> /* New extended info filters for IFLA_EXT_MASK */
> #define RTEXT_FILTER_VF (1 << 0)
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index 82b1d48..f85b8fd 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -83,6 +83,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
> struct netlink_callback *cb)
> {
> int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
>+ unsigned short act_flags = cb->args[2];
> struct nlattr *nest;
>
> spin_lock_bh(&hinfo->lock);
>@@ -111,14 +112,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
> }
> nla_nest_end(skb, nest);
> n_i++;
>- if (n_i >= TCA_ACT_MAX_PRIO)
>+ if (!(act_flags & ACT_LARGE_DUMP_ON) &&
>+ n_i >= TCA_ACT_MAX_PRIO)
> goto done;
> }
> }
> done:
> spin_unlock_bh(&hinfo->lock);
>- if (n_i)
>+ if (n_i) {
> cb->args[0] += n_i;
>+ if (act_flags & ACT_LARGE_DUMP_ON)
>+ cb->args[1] = n_i;
>+ }
> return n_i;
>
> nla_put_failure:
>@@ -993,11 +998,15 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
> return tcf_add_notify(net, n, &actions, portid);
> }
>
>+static const struct nla_policy tcaa_policy[TCAA_MAX + 1] = {
>+ [TCAA_ACT_FLAGS] = { .type = NLA_U32 },
>+};
>+
> static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> struct netlink_ext_ack *extack)
> {
> struct net *net = sock_net(skb->sk);
>- struct nlattr *tca[TCA_ACT_MAX + 1];
>+ struct nlattr *tca[TCAA_MAX + 1];
> u32 portid = skb ? NETLINK_CB(skb).portid : 0;
> int ret = 0, ovr = 0;
>
>@@ -1005,7 +1014,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> !netlink_capable(skb, CAP_NET_ADMIN))
> return -EPERM;
>
>- ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
>+ ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, tcaa_policy,
Please do this in a separate patch. It is an unrelated bug fix.
> extack);
> if (ret < 0)
> return ret;
>@@ -1046,16 +1055,12 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> return ret;
> }
>
>-static struct nlattr *find_dump_kind(const struct nlmsghdr *n)
>+static struct nlattr *find_dump_kind(struct nlattr **nla)
> {
> struct nlattr *tb1, *tb2[TCA_ACT_MAX + 1];
> struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
>- struct nlattr *nla[TCAA_MAX + 1];
> struct nlattr *kind;
>
>- if (nlmsg_parse(n, sizeof(struct tcamsg), nla, TCAA_MAX,
>- NULL, NULL) < 0)
>- return NULL;
> tb1 = nla[TCA_ACT_TAB];
> if (tb1 == NULL)
> return NULL;
>@@ -1081,9 +1086,18 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> struct nlattr *nest;
> struct tc_action_ops *a_o;
> int ret = 0;
>+ struct nlattr *tcaa[TCAA_MAX + 1];
"tcaa" again, now as a variable :/
Just use "tb" as the rest of the code does.
> struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
>- struct nlattr *kind = find_dump_kind(cb->nlh);
>+ struct nlattr *kind = NULL;
>+ struct nlattr *count_attr = NULL;
>+ u32 act_flags = 0;
>+
>+ ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tcaa, TCAA_MAX,
>+ tcaa_policy, NULL);
>+ if (ret < 0)
>+ return ret;
>
>+ kind = find_dump_kind(tcaa);
> if (kind == NULL) {
> pr_info("tc_dump_action: action bad kind\n");
> return 0;
>@@ -1093,14 +1107,23 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> if (a_o == NULL)
> return 0;
>
>+ if (tcaa[TCAA_ACT_FLAGS])
>+ act_flags = nla_get_u32(tcaa[TCAA_ACT_FLAGS]);
I still believe this is wrong. Should be a separate attr per flag.
For user experience breakage reasons:
2 kernels should not behave differently on the exact same value passed
from userspace:
User passes 0x2. Now the kernel will ignore the set bit, the next kernel
will recognize it as a valid flag and do something.
Please let the discussion reach a consensus before pushing this again.
>+
> nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> cb->nlh->nlmsg_type, sizeof(*t), 0);
> if (!nlh)
> goto out_module_put;
>+
>+ cb->args[2] = act_flags;
>+
> t = nlmsg_data(nlh);
> t->tca_family = AF_UNSPEC;
> t->tca__pad1 = 0;
> t->tca__pad2 = 0;
>+ count_attr = nla_reserve(skb, TCAA_ACT_COUNT, sizeof(u32));
>+ if (!count_attr)
>+ goto out_module_put;
>
> nest = nla_nest_start(skb, TCA_ACT_TAB);
> if (nest == NULL)
>@@ -1113,6 +1136,8 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> if (ret > 0) {
> nla_nest_end(skb, nest);
> ret = skb->len;
>+ memcpy(nla_data(count_attr), &cb->args[1], sizeof(u32));
>+ cb->args[1] = 0;
> } else
> nlmsg_trim(skb, b);
>
>--
>1.9.1
>
^ permalink raw reply
* Re: [PATCH net-next v2 2/5] virtio-net: transmit napi
From: Willem de Bruijn @ 2017-04-20 13:58 UTC (permalink / raw)
To: Jason Wang
Cc: Network Development, Michael S. Tsirkin, virtualization,
David Miller, Willem de Bruijn
In-Reply-To: <2ae5889b-69d4-3041-d7fd-a7ea38e8c4e2@redhat.com>
On Thu, Apr 20, 2017 at 2:27 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2017年04月19日 04:21, Willem de Bruijn wrote:
>>
>> +static void virtnet_napi_tx_enable(struct virtnet_info *vi,
>> + struct virtqueue *vq,
>> + struct napi_struct *napi)
>> +{
>> + if (!napi->weight)
>> + return;
>> +
>> + if (!vi->affinity_hint_set) {
>> + napi->weight = 0;
>> + return;
>> + }
>> +
>> + return virtnet_napi_enable(vq, napi);
>> +}
>> +
>> static void refill_work(struct work_struct *work)
>
>
> Maybe I was wrong, but according to Michael's comment it looks like he want
> check affinity_hint_set just for speculative tx polling on rx napi instead
> of disabling it at all.
>
> And I'm not convinced this is really needed, driver only provide affinity
> hint instead of affinity, so it's not guaranteed that tx and rx interrupt
> are in the same vcpus.
You're right. I made the restriction broader than the request, to really err
on the side of caution for the initial merge of napi tx. And enabling
the optimization is always a win over keeping it off, even without irq
affinity.
The cycle cost is significant without affinity regardless of whether the
optimization is used. Though this is not limited to napi-tx, it is more
pronounced in that mode than without napi.
1x TCP_RR for affinity configuration {process, rx_irq, tx_irq}:
upstream:
1,1,1: 28985 Mbps, 278 Gcyc
1,0,2: 30067 Mbps, 402 Gcyc
napi tx:
1,1,1: 34492 Mbps, 269 Gcyc
1,0,2: 36527 Mbps, 537 Gcyc (!)
1,0,1: 36269 Mbps, 394 Gcyc
1,0,0: 34674 Mbps, 402 Gcyc
This is a particularly strong example. It is also representative
of most RR tests. It is less pronounced in other streaming tests.
10x TCP_RR, for instance:
upstream:
1,1,1: 42267 Mbps, 301 Gcyc
1,0,2: 40663 Mbps, 445 Gcyc
napi tx:
1,1,1: 42420 Mbps, 303 Gcyc
1,0,2: 42267 Mbps, 431 Gcyc
These numbers were obtained with the virtqueue_enable_cb_delayed
optimization after xmit_skb, btw. It turns out that moving that before
increases 1x TCP_RR further to ~39 Gbps, at the cost of reducing
100x TCP_RR a bit.
^ permalink raw reply
* RE: [net-next 04/14] i40e: dump VF information in debugfs
From: Mintz, Yuval @ 2017-04-20 13:57 UTC (permalink / raw)
To: Jiri Pirko
Cc: Or Gerlitz, Jeff Kirsher, Mitch Williams, David Miller,
Linux Netdev List, nhorman@redhat.com, sassmann@redhat.com,
jogreene@redhat.com
In-Reply-To: <20170420113942.GD1886@nanopsycho.orion>
> >> > Dump some internal state about VFs through debugfs. This provides
> >> > information not available with 'ip link show'.
> >>
> >> such as?
> >>
> >> donnwantobethedebugfspolice, but still, in the 2-3 times we tried to
> >> push debugfs to MLNX NIC drivers, Dave disallowed that, and lately
> >> the switch team even went further and deleted that portion of the
> >> mlxsw driver -- all to all, I don't see much point for these type of changes,
> thoughts?
> >
> >Don't want to hikjack your thread, but continuing this topic - Is there
> >some flat-out disapproval for debugfs in net-next now?
>
> It was mentioned by DaveM on the list multiple times.
> >
> >We're currently internally engaged with adding qed support for register
> >dumps [~equivalents for `ethtool -d' outputs] through debugfs, on
> >behalf of storage drivers [qedi/qedf] lacking the API for doing that.
>
> That sounds wrong. Either introduce a generic infra to expose the info you
> need or you don't do that.
I agree this surely isn't *our* convention, but for scsi using debugfs
[or sysfs] is a common practice for debug purposes.
Also, our HW debug capabilities are highly-customable, and I want to
have the ability to configure those on the fly [E.g., dynamically
configuring various HW events to be recorded].
Each such configuration involves multiple register writes and reads
according to user provided inputs.
I don't really see how to generalize the information collection in a way
that would benefit anyone else.
> For your inhouse debugging, you should have oot
> patch to expose whatever you need.
I don't want in-house debugging capabilities -
I want field debug capabilities.
^ permalink raw reply
* Re: [PATCH v2] mdio_bus: Issue GPIO RESET to PHYs.
From: Roger Quadros @ 2017-04-20 13:56 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem, Florian Fainelli, tony, nsekhar, jsarha, netdev,
linux-omap, linux-kernel
In-Reply-To: <20170420131305.GB6152@lunn.ch>
On 20/04/17 16:13, Andrew Lunn wrote:
> On Thu, Apr 20, 2017 at 11:39:20AM +0300, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY which manifests as PHY not being detected
>> or link not functional. To fix this, these PHYs need to be RESET
>> via a GPIO connected to the PHY's RESET pin.
>>
>> Some boards have a single GPIO controlling the PHY RESET pin of all
>> PHYs on the bus whereas some others have separate GPIOs controlling
>> individual PHY RESETs.
>>
>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>> as the PHY will not probe till its reset is de-asserted.
>> So do the RESET de-assertion in the MDIO bus driver.
>
> Hi Roger
>
> Networking patches should include in the subject line which tree they
> are for. In this case, net-next. So please make the Subject
>
> [patch v3 net-next] .....
>
>> [1] - am572x-idk, am571x-idk, a437x-idk
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>> v2:
>> - add device tree binding document (mdio.txt)
>> - specify default reset delay in of_mdio.c instead of mdio_bus.c
>>
>> Documentation/devicetree/bindings/net/mdio.txt | 20 ++++++++++++++++++++
>> drivers/net/phy/mdio_bus.c | 22 ++++++++++++++++++++++
>> drivers/of/of_mdio.c | 7 +++++++
>> include/linux/phy.h | 5 +++++
>> 4 files changed, 54 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/mdio.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
>> new file mode 100644
>> index 0000000..6e703d7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>> @@ -0,0 +1,20 @@
>> +Common MDIO bus properties.
>> +
>> +These are generic properties that can apply to any MDIO bus.
>> +
>> +Optional properties:
>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>> + of the PHYs on that MDIO bus.
>> +- reset-delay-us: RESET pulse width as per PHY datasheet.
>
> It would be good to explicitly say that it is in uS as part of the
> comment.
>
> Also, please document that we expect a list of child nodes, one per
> device on the bus. These should follow the generic phy.txt, or a
> device specific binding document.
>
>> +
>> +Example :
>> +
>
> It would be good to say something like:
>
> This example shows these optional properties, plus other properties
> required for the TI Davinci MDIO driver.
>
> Pointing this out may stop people cut/past the ti,hwmods property.
>
>> + davinci_mdio: ethernet@0x5c030000 {
>> + compatible = "ti,davinci_mdio";
>> + ti,hwmods = "davinci_mdio";
>> + reg = <0x5c030000 0x1000>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>> + reset-delay-us = <2>; /* PHY datasheet states 1uS min */
>> + };
>
> And please include at least one PHY on the bus.
>
> Sorry for asking for so much in the documentation. That is the problem
> with the documentation being missing to start with.
>
> The code looks good now.
>
OK Andrew. I'll post a v3 with your suggestions.
cheers,
-roger
^ permalink raw reply
* Re: sctp: deny peeloff operation on asocs with threads sleeping on it
From: Marcelo Ricardo Leitner @ 2017-04-20 13:54 UTC (permalink / raw)
To: Salvatore Bonaccorso, netdev
Cc: Alexander Popov, Ben Hutchings, Xin Long, David S. Miller,
Greg Kroah-Hartman
In-Reply-To: <20170420123257.GA11798@lorien.valinor.li>
Em 20-04-2017 09:32, Salvatore Bonaccorso escreveu:
> Hi
>
> According to the documentation I should have sent this to
> netdev@vger.kernel.org rather than stable@vger.kernel.org.
>
> Rationale: Whilst 00eff2ebbd229758e90659907724c14dd5a18339 went to
> stable, the dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 commit is
> missing.
Cc'ing Greg too.
This is probably because I noticed this commit was missing when Greg
posted the commits being considered for backports, then I raised the
concern and Greg decided to pull it too before David replied.
Not sure if this caused some out-of-the-process stuff.
>
> Full quoting, my original misleaded mail:
>
> On Thu, Apr 20, 2017 at 01:44:05PM +0200, Salvatore Bonaccorso wrote:
>> Hi
>>
>> Apparently the following commit
>>
>> dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 (sctp: deny peeloff operation
>> on asocs with threads sleeping on it)
>>
>> (was not CC'ed stable@vger.kernel.org, but was already applied in
>> 3.2.87 and 3.16.42) is missing from 4.9:
>>
>> 2dcab598484185dea7ec22219c76dcdd59e3cb90 was included in 4.9.11 via
>> 00eff2ebbd229758e90659907724c14dd5a18339 . But
>> dfcb9f4f99f1e9a49e43398a7bfbf56927544af1 is missing from 4.9.
>>
>> This was assigned CVE-2017-6353.
>>
>> Reference:
>> https://marc.info/?l=linux-netdev&m=148785309416337&w=2
>> http://www.openwall.com/lists/oss-security/2017/02/27/2
>>
>> Regards,
>> Salvatore
>
> Regards,
> Salvatore
>
^ 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