* Re: [PATCH net-next 04/11] net: dsa: mv88e6xxx: add port FID accessors
From: Vivien Didelot @ 2016-11-02 1:01 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli
In-Reply-To: <20161101093819.GA2802@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
>> --- a/drivers/net/dsa/mv88e6xxx/port.c
>> +++ b/drivers/net/dsa/mv88e6xxx/port.c
>> @@ -61,6 +61,8 @@ int mv88e6xxx_port_set_state(struct mv88e6xxx_chip *chip, int port, u8 state)
>> return 0;
>> }
>>
>> +/* Offset 0x05: Port Control 1 */
>> +
>> /* Offset 0x06: Port Based VLAN Map */
>>
>> int mv88e6xxx_port_set_vlan_map(struct mv88e6xxx_chip *chip, int port, u16 map)
>
> This comment seems in an odd place.
It isn't. Port's default FID uses register 0x05 and I document them
above the related functions.
Thanks,
Vivien
^ permalink raw reply
* [PATCH 1/1] xen-netfront: cast grant table reference first to type int
From: Dongli Zhang @ 2016-11-02 1:04 UTC (permalink / raw)
To: netdev, linux-kernel, xen-devel; +Cc: davem
IS_ERR_VALUE() in commit 87557efc27f6a50140fb20df06a917f368ce3c66
("xen-netfront: do not cast grant table reference to signed short") would
not return true for error code unless we cast ref first to type int.
Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com>
---
drivers/net/xen-netfront.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 189a28d..bf2744e 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -304,7 +304,7 @@ static void xennet_alloc_rx_buffers(struct netfront_queue *queue)
queue->rx_skbs[id] = skb;
ref = gnttab_claim_grant_reference(&queue->gref_rx_head);
- WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));
+ WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)(int)ref));
queue->grant_rx_ref[id] = ref;
page = skb_frag_page(&skb_shinfo(skb)->frags[0]);
@@ -428,7 +428,7 @@ static void xennet_tx_setup_grant(unsigned long gfn, unsigned int offset,
id = get_id_from_freelist(&queue->tx_skb_freelist, queue->tx_skbs);
tx = RING_GET_REQUEST(&queue->tx, queue->tx.req_prod_pvt++);
ref = gnttab_claim_grant_reference(&queue->gref_tx_head);
- WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)ref));
+ WARN_ON_ONCE(IS_ERR_VALUE((unsigned long)(int)ref));
gnttab_grant_foreign_access_ref(ref, queue->info->xbdev->otherend_id,
gfn, GNTMAP_readonly);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next 07/11] net: dsa: mv88e6xxx: add port link setter
From: Vivien Didelot @ 2016-11-02 1:07 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli
In-Reply-To: <20161101094313.GB2802@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
>> +#define LINK_UNKNOWN -1
>> +
>> + /* Port's MAC link state
>> + * LINK_UNKNOWN for normal link detection, 0 to force link down,
>> + * otherwise force link up.
>> + */
>> + int (*port_set_link)(struct mv88e6xxx_chip *chip, int port, int link);
>
> Maybe LINK_AUTO would be better than UNKNOWN? Or LINK_UNFORCED.
I used LINK_UNKNOWN to be consistent with the supported SPEED_UNKNOWN
and DUPLEX_UNKNOWN values of PHY devices.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next 08/11] net: dsa: mv88e6xxx: add port duplex setter
From: Vivien Didelot @ 2016-11-02 1:12 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev, linux-kernel, kernel, David S. Miller, Florian Fainelli
In-Reply-To: <20161101094440.GC2802@lunn.ch>
Hi Andrew,
Andrew Lunn <andrew@lunn.ch> writes:
>> int (*port_set_link)(struct mv88e6xxx_chip *chip, int port, int link);
>> +
>> + /* Port's MAC duplex mode
>> + *
>> + * Use DUPLEX_HALF or DUPLEX_FULL to force half or full duplex, or
>> + * DUPLEX_UNKNOWN for normal duplex detection.
>> + */
>> + int (*port_set_duplex)(struct mv88e6xxx_chip *chip, int port, int dup);
>
> Again, i think DUPLEX_UNFORCED, or DUPLEX_AUTO would be better.
DUPLEX_UNKNOWN and SPEED_UNKNOWN are existing values used by the
phy/ethtool interface for the phy_device. I prefered to use them when
possible instead of creating new ones.
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH] rtl8xxxu: Fix for agressive power saving by rtl8723bu wireless IC
From: John Heenan @ 2016-11-02 1:13 UTC (permalink / raw)
To: Jes Sorensen, Barry Day, Rafał Miłecki
Cc: Kalle Valo, linux-wireless, netdev, linux-kernel
In-Reply-To: <CAAye0QNjYY5gBzSdc+0_rhy1OHk7iQ-CVp0Svph1DR7+mgVWGg@mail.gmail.com>
Barry Day has submitted real world reports for the 8192eu and 8192cu.
This needs to be acknowledged. I have submitted real world reports for
the 8723bu.
When it comes down to it, it looks like the kernel code changes are
really going to be very trivial to fix this problem and we need to
take the focus off dramatic outbursts over style issues to a strategy
for getting usable results from real world testing.
Addressing style issues in a dramatic manner to me looks like a mean
sport for maintainers who line up to easy target first time
contributors. This mean attitude comes from the top with a well known
comment about "publicly making fun of people". The polite comments
over style from Joe Perches and Rafał Miłecki are welcomed.
An effective strategy would be to insert some printk statements to
trace what init steps vendor derived drivers do each time
wpa_supplicant is called and ask real world testers to report their
results. This is a lot more productive and less error prone than
laboriously pouring over vendor source code. Alternative drivers that
use vendor code from Realtek is enormously complicated and a huge pain
to make sense of.
Joe Sorensen's driver code is far easier to make sense of and it is a
shame Realtek don't come to the party. Joe Sorensens's code take takes
advantage of the excellent work of kernel contributors to the mac80211
driver.
Previous comments I made about enable_rf, rtl8xxxu_start,
rtl8xxxu_init_device etc should be clarified. I will leave it for the
moment as it currently serves no direct useful purpose.
John Heenan
On 1 November 2016 at 17:24, John Heenan <john@zgus.com> wrote:
> I have a prepared another patch that is not USB VID/PID dependent for
> rtl8723bu devices. It is more elegant. I will send it after this
> email.
>
> If I have more patches is it preferable I just put them on github only
> and notify a link address until there might be some resolution?
>
> What I meant below about not finding a matching function is that I
> cannot find matching actions to take on any function calls in
> rtl8xxxu_stop as for rtl8xxxu_start.
>
> The function that is called later and potentially more than once for
> rtl8723bu devices is rtl8xxxu_init_device. There is no corresponding
> rtl8xxxu_deinit_device function.
>
> enable_rf is called in rtl8xxxu_start, not in the delayed call to
> rtl8xxxu_init_device. The corresponding disable_rf is called in
> rtl8xxxu_stop. So no matching issue here. However please see below for
> another potential issue.
>
> power_on is called in rtl8xxxu_init_device. power_off is called in
> rtl8xxxu_disconnect. Does not appear to be an issue if calling
> power_on has no real effect if already on.
>
> The following should be looked at though. For all devices enable_rf is
> only called once. For the proposed patch the first call to
> rtl8xxxu_init_device is called after the single call to enable_rf for
> rtl8723bu devices. Without the patch enable_rf is called after
> rtl8xxxu_init_device for all devices
>
> Perhaps enable_rf just configures RF modes to start up when RF is
> powered on or if called after power_on then enters requested RF modes.
>
> So I cannot see appropriate additional matching action to take.
>
> Below is some background for anyone interested
>
> rtl8xxxu_start and rtl8xxxu_stop are assigned to rtl8xxxu_ops.
>
> rtl8xxxu_probe assigns rtl8xxxu_ops to its driver layer with
> ieee80211_register_hw.
>
> rtl8xxxu_disconnect unassigns rtl8xxxu_ops from its driver layer with
> ieee80211_unregister_hw.
>
> rtl8xxxu_probe and rtl8xxxu_disconnect are USB driver functions
>
> John Heenan
>
>
> On 1 November 2016 at 08:15, John Heenan <john@zgus.com> wrote:
>> On 1 November 2016 at 07:25, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
>>> John Heenan <john@zgus.com> writes:
>>
>>>
>>>> @@ -5776,9 +5781,17 @@ static int rtl8xxxu_start(struct ieee80211_hw *hw)
>>>> struct rtl8xxxu_tx_urb *tx_urb;
>>>> unsigned long flags;
>>>> int ret, i;
>>>> + struct usb_device_descriptor *udesc = &priv->udev->descriptor;
>>>>
>>>> ret = 0;
>>>>
>>>> + if(udesc->idVendor == USB_VENDOR_ID_REALTEK
>>>> + && udesc->idProduct == USB_PRODUCT_ID_RTL8723BU) {
>>>> + ret = rtl8xxxu_init_device(hw);
>>>> + if (ret)
>>>> + goto error_out;
>>>> + }
>>>> +
>>>
>>> As mentioned previously, if this is to be changed here, it has to be
>>> matched in the _stop section too.
>>
>> I looked at this and could not find a matching function. I will have a
>> look again.
>>
^ permalink raw reply
* Re: [PATCH v7 5/6] net: ipv4, ipv6: run cgroup eBPF egress programs
From: Daniel Borkmann @ 2016-11-02 1:17 UTC (permalink / raw)
To: David Miller, daniel-cYrQPVfZoowdnm+yROfE0A
Cc: htejun-b10kYP2dOMg, ast-b10kYP2dOMg, kafai-b10kYP2dOMg,
fw-HFFVJYpyMKqzQB+pC5nmwQ, pablo-Cap9r6Oaw4JrovVCs/uTlw,
harald-H+wXaHxf7aLQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
sargun-GaZTRHToo+CzQB+pC5nmwQ, cgroups-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161031.124003.1361406552151798940.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On 10/31/2016 05:40 PM, David Miller wrote:
> From: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
> Date: Tue, 25 Oct 2016 12:14:13 +0200
>
>> @@ -312,6 +314,13 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>> skb->dev = dev;
>> skb->protocol = htons(ETH_P_IP);
>>
>> + ret = cgroup_bpf_run_filter(sk_to_full_sk(sk), skb,
>> + BPF_CGROUP_INET_EGRESS);
>> + if (ret) {
>> + kfree_skb(skb);
>> + return ret;
>> + }
>> +
>> /*
>> * Multicasts are looped back for other local users
>> */
>> @@ -364,12 +373,20 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>> int ip_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>> {
>> struct net_device *dev = skb_dst(skb)->dev;
>> + int ret;
>>
>> IP_UPD_PO_STATS(net, IPSTATS_MIB_OUT, skb->len);
>>
>> skb->dev = dev;
>> skb->protocol = htons(ETH_P_IP);
>>
>> + ret = cgroup_bpf_run_filter(sk_to_full_sk(sk), skb,
>> + BPF_CGROUP_INET_EGRESS);
>> + if (ret) {
>> + kfree_skb(skb);
>> + return ret;
>> + }
>> +
>> return NF_HOOK_COND(NFPROTO_IPV4, NF_INET_POST_ROUTING,
>> net, sk, skb, NULL, dev,
>> ip_finish_output,
>
> The "sk" here is not necessarily the application socket. It could be
> a UDP tunnel socket or similar encapsulation object.
>
> "skb->sk" is always the application socket, so is probably what you
> need to pass down into the cgroup bpf run filter hook.
Wouldn't that mean however, when you go through stacked devices that
you'd run the same eBPF cgroup program for skb->sk multiple times?
(Except for things like crossing netns due to the skb_orphan() there.)
For example, when you're doing accounting through this facility, then
TX part is counted X times instead of ideally just once, and for RX
afaik, we seem to short-cut sk_filter_trim_cap() on the encaps anyway.
I guess one could work around this by marking the skb with a verdict
once it was accounted for the first time, either from within eBPF prog
or from stack side. For eBPF we'd need to implement our own .is_valid_access()
callback for the verifier to allow writing into skb meta data like skb->mark
(sk filter can only write into cb[] for passing data between tail calls)
plus making sure in either case that no other user is mangling that flag
further on; hmm, probably rather unintuitive and fragile.
Unless I'm missing something obvious, I would currently think that just
passing "sk" might be more correct.
^ permalink raw reply
* Re: [Patch net] inet: fix sleeping inside inet_wait_for_connect()
From: Eric Dumazet @ 2016-11-02 1:54 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, Andrey Konovalov, Peter Zijlstra
In-Reply-To: <1478041476-5568-1-git-send-email-xiyou.wangcong@gmail.com>
On Tue, 2016-11-01 at 16:04 -0700, Cong Wang wrote:
> Andrey reported this kernel warning:
> Unlike commit 26cabd31259ba43f68026ce3f62b78094124333f
> ("sched, net: Clean up sk_wait_event() vs. might_sleep()"), the
> sleeping function is called before schedule_timeout(), this is indeed
> a bug. Fix this by moving the wait logic to the new API, it is similar
> to commit ff960a731788a7408b6f66ec4fd772ff18833211
> ("netdev, sched/wait: Fix sleeping inside wait event").
>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
Excellent.
I guess we could also define sk_wait_event_woken()
and use it instead of sk_wait_event(), and also in
inet_wait_for_connect()
+#define sk_wait_event_woken(__sk, __timeo, __condition, __wait) \
+ ({ int __rc; \
+ release_sock(__sk); \
+ __rc = __condition; \
+ if (!__rc) { \
+ *(__timeo) = wait_woken(__wait, TASK_INTERRUPTIBLE, \
+ *(__timeo)); \
+ } \
+ lock_sock(__sk); \
+ __rc = __condition; \
+ __rc; \
+ })
sk_wait_data() would need :
@@ -2078,14 +2080,14 @@ void __sk_flush_backlog(struct sock *sk)
*/
int sk_wait_data(struct sock *sk, long *timeo, const struct sk_buff *skb)
{
+ DEFINE_WAIT_FUNC(wait, woken_wake_function);
int rc;
- DEFINE_WAIT(wait);
- prepare_to_wait(sk_sleep(sk), &wait, TASK_INTERRUPTIBLE);
+ add_wait_queue(sk_sleep(sk), &wait);
sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
- rc = sk_wait_event(sk, timeo, skb_peek_tail(&sk->sk_receive_queue) != skb);
+ rc = sk_wait_event_woken(sk, timeo, skb_peek_tail(&sk->sk_receive_queue) != skb, &wait);
sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
- finish_wait(sk_sleep(sk), &wait);
+ remove_wait_queue(sk_sleep(sk), &wait);
return rc;
}
EXPORT_SYMBOL(sk_wait_data);
^ permalink raw reply
* Re: [PATCH net-next v2] ipv4: fib: Replay events when registering FIB notifier
From: Roopa Prabhu @ 2016-11-02 2:13 UTC (permalink / raw)
To: Ido Schimmel
Cc: Eric Dumazet, netdev, davem, jiri, mlxsw, dsa, nikolay, andy,
vivien.didelot, andrew, f.fainelli, alexander.h.duyck, kuznet,
jmorris, yoshfuji, kaber, Ido Schimmel
In-Reply-To: <20161101170345.pq2ewecw35mrurkp@splinter>
On 11/1/16, 10:03 AM, Ido Schimmel wrote:
> Hi Roopa,
>
> On Tue, Nov 01, 2016 at 08:14:14AM -0700, Roopa Prabhu wrote:
>>
[snip]
>> I have the same concern as Eric here.
>>
>> I understand why you need it, but can the driver request for an initial dump and that
>> dump be made more efficient somehow ie not hold rtnl for the whole dump ?.
>> instead of making the fib notifier registration code doing it.
> We can do what we suggested in the last bi-weekly meeting, which is
> still holding rtnl, but moving the hardware operation to delayed work.
> This is possible because upper layers always assume operation was
> successful and driver is responsible for invoking its abort mechanism in
> case of failure.
>
>> these routing table sizes can be huge and an analogy for this in user-space:
>> We do request a netlink dump of routing tables at initialization (on driver starts or resets)...
>> but, existing netlink routing table dumps for that scale don't hold rtnl for the whole dump.
>> The dump is split into multiple responses to the user and hence it does not starve other rtnl users.
> In my reply to Eric I mentioned that when we register and unregister
> from this chain the tables aren't really huge, but instead quite small.
> I understand your concerns, but I don't wish to make things more
> complicated than they should be only to address concerns that aren't
> really realistic.
I understand..but, if you are adding some core infrastructure for switchdev ..it cannot be
based on the number of simple use-cases or data you have today.
I won't be surprised if tomorrow other switch drivers have a case where they need to
reset the hw routing table state and reprogram all routes again. Re-registering the notifier to just
get the routing state of the kernel will not scale. For the long term, since the driver does not maintain a cache,
a pull api with efficient use of rtnl will be useful for other such cases as well.
If you don't want to get to the complexity of a new api right away because of the
simple case of management interface routes you have, Can your driver register the notifier early ?
(I am sure you have probably already thought about this)
>
> I believe current patch is quite simple and also consistent with other
> notification chains in the kernel, such as the netdevice, where rtnl is
> held during replay of events.
> http://lxr.free-electrons.com/source/net/core/dev.c#L1535
as you know, netdev and routing scale are not the same thing.
Looking at the current code for netdevices (replay and rollback on failure),
a pull api (equivalent to the netlink dump api) may end up being less complex...with an
ability to batch in the future.
^ permalink raw reply
* Re: Let's do P4
From: Daniel Borkmann @ 2016-11-02 2:29 UTC (permalink / raw)
To: Jiri Pirko, Alexei Starovoitov
Cc: Thomas Graf, John Fastabend, Jakub Kicinski, netdev, davem, jhs,
roopa, simon.horman, ast, prem, hannes, jbenc, tom, mattyk,
idosch, eladr, yotamg, nogahf, ogerlitz, linville, andy,
f.fainelli, dsa, vivien.didelot, andrew, ivecera,
Maciej Żenczykowski
In-Reply-To: <20161031093922.GA2895@nanopsycho.orion>
On 10/31/2016 10:39 AM, Jiri Pirko wrote:
> Sun, Oct 30, 2016 at 11:39:05PM CET, alexei.starovoitov@gmail.com wrote:
>> On Sun, Oct 30, 2016 at 05:38:36PM +0100, Jiri Pirko wrote:
>>> Sun, Oct 30, 2016 at 11:26:49AM CET, tgraf@suug.ch wrote:
>>>> On 10/30/16 at 08:44am, Jiri Pirko wrote:
>>>>> Sat, Oct 29, 2016 at 06:46:21PM CEST, john.fastabend@gmail.com wrote:
>>>>>> On 16-10-29 07:49 AM, Jakub Kicinski wrote:
>>>>>>> On Sat, 29 Oct 2016 09:53:28 +0200, Jiri Pirko wrote:
>>>>>>>> Hi all.
>>
>> sorry for delay. travelling to KS, so probably missed something in
>> this thread and comments can be totally off...
>>
>> the subject "let's do P4" is imo misleading, since it reads like
>> we don't do P4 at the moment, whereas the opposite is true.
>> Several p4->bpf compilers is a proof.
>
> We don't do p4 in kernel now, we don't do p4 offloading now. That is
> the reason I started this discussion.
>
>>> The network world is divided into 2 general types of hw:
>>> 1) network ASICs - network specific silicon, containing things like TCAM
>>> These ASICs are suitable to be programmed by P4.
>>
>> i think the opposite is the case in case of P4.
>> when hw asic has tcam it's still far far away from being usable with P4
>> which requires fully programmable protocol parser, arbitrary tables and so on.
>> P4 doesn't even define TCAM as a table type. The p4 program can declare
>> a desired algorithm of search in the table and compiler has to figure out
>> what HW resources to use to satisfy such p4 program.
>>
>>> 2) network processors - basically a general purpose CPUs
>>> These processors are suitable to be programmed by eBPF.
>>
>> I think this statement is also misleading, since it positions
>> p4 and bpf as competitors whereas that's not the case.
>> p4 is the language. bpf is an instruction set.
>
> I wanted to say that we are having 2 approaches in silicon, 2 different
> paradigms. Sure you can do p4>bpf. But hard to do it the opposite way.
>
>>> Exactly. Following drawing shows p4 pipeline setup for SW and Hw:
>>>
>>> |
>>> | +--> ebpf engine
>>> | |
>>> | |
>>> | compilerB
>>> | ^
>>> | |
>>> p4src --> compilerA --> p4ast --TCNL--> cls_p4 --+-> driver -> compilerC -> HW
>>> |
>>> userspace | kernel
>>> |
Sorry for jumping into the middle and the delay (plumbers this week). My
question would be, if the main target is for p4 *offloading* anyway, who
would use this sw fallback path? Mostly for testing purposes?
I'm not sure about compilerB here and the complexity that needs to be
pushed into the kernel along with it. I would assume this would result
in slower code than what the existing P4 -> eBPF front ends for LLVM
would generate since it could perform all kind of optimizations there,
that might not be feasible for doing inside the kernel. Thus, if I'd want
to do that in sw, I'd just use the existing LLVM facilities instead and
go via cls_bpf in that case.
What is your compilerA? Is that part of tc in user space? Maybe linked
against LLVM lib, for example? If you really want some sw path, can't tc
do this transparently from user space instead when it gets a netlink error
that it cannot get offloaded (and thus switch internally to f_bpf's loader)?
^ permalink raw reply
* Re: [PATCH net-next 0/3] tools lib bpf: Synchronize implementations
From: Daniel Borkmann @ 2016-11-02 2:46 UTC (permalink / raw)
To: Joe Stringer, David Ahern; +Cc: netdev, wangnan0, ast
In-Reply-To: <CAPWQB7HtmX7+v37q9rH_3PD+QfSVg_uRiP_=5ZoKnwsF8gpyKg@mail.gmail.com>
On 11/01/2016 11:17 PM, Joe Stringer wrote:
> On 1 November 2016 at 13:51, David Ahern <dsa@cumulusnetworks.com> wrote:
>> On 10/31/16 12:39 PM, Joe Stringer wrote:
>>> Update tools/lib/bpf to provide more functionality and improve interoperation
>>> with other tools that generate and use eBPF code.
>>>
>>> The kernel uapi headers are a bit newer than the version in the tools/
>>> directory; synchronize those.
>>>
>>> samples/bpf/libbpf* has a bit more functionality than tools/lib/bpf, so extend
>>> tools/lib/bpf/bpf* with these functions to bring them into parity.
>>>
>>> tools/lib/bpf cannot read ELFs that tc can read, and vice versa. Update the
>>> map definition to be the same as in tc so the ELFs may be interchangeable
>>> (at least for now; I don't have a long-term plan in mind to ensure this always
>>> works).
>>
>> can samples/bpf be converted to use tools/lib/bpf/libbpf.a?
>
> I have a few other patches sitting around that need this series,
> including an attempt at this.
That sounds great, looking forward!
^ permalink raw reply
* Re: [PATCH net-next 2/3] tools lib bpf: Sync with samples/bpf/libbpf
From: Daniel Borkmann @ 2016-11-02 2:52 UTC (permalink / raw)
To: Joe Stringer, netdev; +Cc: wangnan0, ast
In-Reply-To: <20161031183917.9938-3-joe@ovn.org>
On 10/31/2016 07:39 PM, Joe Stringer wrote:
> Extend the tools/ version of libbpf to include all of the functionality
> provided in the samples/bpf version.
>
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
> tools/lib/bpf/bpf.c | 139 +++++++++++++++++++++++++++------
> tools/lib/bpf/bpf.h | 208 +++++++++++++++++++++++++++++++++++++++++++++++--
> tools/lib/bpf/libbpf.c | 3 +-
> 3 files changed, 317 insertions(+), 33 deletions(-)
>
[...]
> +int open_raw_sock(const char *name)
> +{
> + struct sockaddr_ll sll;
> + int sock;
> +
> + sock = socket(PF_PACKET, SOCK_RAW | SOCK_NONBLOCK | SOCK_CLOEXEC,
> + htons(ETH_P_ALL));
> + if (sock < 0) {
> + printf("cannot create raw socket\n");
> + return -1;
> + }
> +
> + memset(&sll, 0, sizeof(sll));
> + sll.sll_family = AF_PACKET;
> + sll.sll_ifindex = if_nametoindex(name);
> + sll.sll_protocol = htons(ETH_P_ALL);
> + if (bind(sock, (struct sockaddr *)&sll, sizeof(sll)) < 0) {
> + printf("bind to %s: %s\n", name, strerror(errno));
> + close(sock);
> + return -1;
> + }
> +
> + return sock;
> +}
> +
> +int perf_event_open(struct perf_event_attr *attr, int pid, int cpu,
> + int group_fd, unsigned long flags)
> +{
> + return syscall(__NR_perf_event_open, attr, pid, cpu,
> + group_fd, flags);
> }
I'm actually wondering, above bits are not really libbpf related. Maybe
these should go elsewhere into some other misc header file just for the
samples to use?
> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> index e8ba54087497..227edb23c022 100644
> --- a/tools/lib/bpf/bpf.h
> +++ b/tools/lib/bpf/bpf.h
> @@ -23,16 +23,208 @@
>
> #include <linux/bpf.h>
>
> +struct bpf_insn;
> +
Isn't that already defined in the above uapi bpf.h anyway?
> int bpf_create_map(enum bpf_map_type map_type, int key_size, int value_size,
> - int max_entries);
> + int max_entries, int map_flags);
> +int bpf_update_elem(int fd, void *key, void *value, unsigned long long flags);
> +int bpf_lookup_elem(int fd, void *key, void *value);
> +int bpf_delete_elem(int fd, void *key);
> +int bpf_get_next_key(int fd, void *key, void *next_key);
> +
> +int bpf_load_program(enum bpf_prog_type prog_type,
> + const struct bpf_insn *insns, int insn_len,
> + const char *license, int kern_version,
> + char *log_buf, size_t log_buf_sz);
> +
> +int bpf_obj_pin(int fd, const char *pathname);
> +int bpf_obj_get(const char *pathname);
^ permalink raw reply
* Re: [PATCH net-next 3/3] tools lib bpf: Sync bpf_map_def with tc
From: Daniel Borkmann @ 2016-11-02 3:09 UTC (permalink / raw)
To: Joe Stringer, netdev; +Cc: wangnan0, ast
In-Reply-To: <20161031183917.9938-4-joe@ovn.org>
On 10/31/2016 07:39 PM, Joe Stringer wrote:
> TC uses a slightly different map layout in its ELFs. Update libbpf to
> use the same definition so that ELFs may be built using libbpf and
> loaded using tc.
>
> Signed-off-by: Joe Stringer <joe@ovn.org>
> ---
> tools/lib/bpf/libbpf.h | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index dd7a513efb10..ea70c2744f8c 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -181,10 +181,13 @@ bool bpf_program__is_kprobe(struct bpf_program *prog);
> * and will be treated as an error due to -Werror.
> */
> struct bpf_map_def {
> - unsigned int type;
> - unsigned int key_size;
> - unsigned int value_size;
> - unsigned int max_entries;
> + uint32_t type;
> + uint32_t key_size;
> + uint32_t value_size;
> + uint32_t max_entries;
> + uint32_t flags;
> + uint32_t id;
> + uint32_t pinning;
> };
>
> /*
I think the problem is that this would break existing obj files that have
been compiled with the current struct bpf_map_def (besides libbpf not having
a use for the last two members right now).
For tc, we have a refactoring of the tc_bpf.c bits that generalizes them so
we can move these bits into iproute2 lib part and add new BPF types really
easily. What I did along with that is to implement a map compat mode, where
it detects the size of struct bpf_elf_map (or however you want to name it)
from the obj file and fixes up the missing members with some reasonable default,
so these programs can still be loaded. Thus, the sample code using the current
struct bpf_map_def will then work with tc as well. (I'll post the iproute2
patch early next week.)
^ permalink raw reply
* Re: [PATCH net-next 2/3] tools lib bpf: Sync with samples/bpf/libbpf
From: Joe Stringer @ 2016-11-02 3:50 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, wangnan0, ast
In-Reply-To: <58195506.8040205@iogearbox.net>
On 1 November 2016 at 19:52, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 10/31/2016 07:39 PM, Joe Stringer wrote:
>>
>> Extend the tools/ version of libbpf to include all of the functionality
>> provided in the samples/bpf version.
>>
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>> ---
>> tools/lib/bpf/bpf.c | 139 +++++++++++++++++++++++++++------
>> tools/lib/bpf/bpf.h | 208
>> +++++++++++++++++++++++++++++++++++++++++++++++--
>> tools/lib/bpf/libbpf.c | 3 +-
>> 3 files changed, 317 insertions(+), 33 deletions(-)
>>
> [...]
>
>> +int open_raw_sock(const char *name)
>> +{
>> + struct sockaddr_ll sll;
>> + int sock;
>> +
>> + sock = socket(PF_PACKET, SOCK_RAW | SOCK_NONBLOCK | SOCK_CLOEXEC,
>> + htons(ETH_P_ALL));
>> + if (sock < 0) {
>> + printf("cannot create raw socket\n");
>> + return -1;
>> + }
>> +
>> + memset(&sll, 0, sizeof(sll));
>> + sll.sll_family = AF_PACKET;
>> + sll.sll_ifindex = if_nametoindex(name);
>> + sll.sll_protocol = htons(ETH_P_ALL);
>> + if (bind(sock, (struct sockaddr *)&sll, sizeof(sll)) < 0) {
>> + printf("bind to %s: %s\n", name, strerror(errno));
>> + close(sock);
>> + return -1;
>> + }
>> +
>> + return sock;
>> +}
>> +
>> +int perf_event_open(struct perf_event_attr *attr, int pid, int cpu,
>> + int group_fd, unsigned long flags)
>> +{
>> + return syscall(__NR_perf_event_open, attr, pid, cpu,
>> + group_fd, flags);
>> }
>
>
> I'm actually wondering, above bits are not really libbpf related. Maybe
> these should go elsewhere into some other misc header file just for the
> samples to use?
True, I started this patch by just copying the whole file over to
tools/lib/bpf so this came with it. I can bring it back to the samples
directory in v2.
>> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
>> index e8ba54087497..227edb23c022 100644
>> --- a/tools/lib/bpf/bpf.h
>> +++ b/tools/lib/bpf/bpf.h
>> @@ -23,16 +23,208 @@
>>
>> #include <linux/bpf.h>
>>
>> +struct bpf_insn;
>> +
>
>
> Isn't that already defined in the above uapi bpf.h anyway?
Yup, I'll drop this bit.
^ permalink raw reply
* Re: [PATCH net-next 3/3] tools lib bpf: Sync bpf_map_def with tc
From: Joe Stringer @ 2016-11-02 4:09 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, wangnan0, ast
In-Reply-To: <581958D1.3010209@iogearbox.net>
On 1 November 2016 at 20:09, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 10/31/2016 07:39 PM, Joe Stringer wrote:
>>
>> TC uses a slightly different map layout in its ELFs. Update libbpf to
>> use the same definition so that ELFs may be built using libbpf and
>> loaded using tc.
>>
>> Signed-off-by: Joe Stringer <joe@ovn.org>
>> ---
>> tools/lib/bpf/libbpf.h | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index dd7a513efb10..ea70c2744f8c 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -181,10 +181,13 @@ bool bpf_program__is_kprobe(struct bpf_program
>> *prog);
>> * and will be treated as an error due to -Werror.
>> */
>> struct bpf_map_def {
>> - unsigned int type;
>> - unsigned int key_size;
>> - unsigned int value_size;
>> - unsigned int max_entries;
>> + uint32_t type;
>> + uint32_t key_size;
>> + uint32_t value_size;
>> + uint32_t max_entries;
>> + uint32_t flags;
>> + uint32_t id;
>> + uint32_t pinning;
>> };
>>
>> /*
>
>
> I think the problem is that this would break existing obj files that have
> been compiled with the current struct bpf_map_def (besides libbpf not having
> a use for the last two members right now).
Right, this is a problem. I wasn't sure whether libbpf was yet at a
stage where it tries to retain compatibility with binaries compiled
against older kernels.
> For tc, we have a refactoring of the tc_bpf.c bits that generalizes them so
> we can move these bits into iproute2 lib part and add new BPF types really
> easily. What I did along with that is to implement a map compat mode, where
> it detects the size of struct bpf_elf_map (or however you want to name it)
> from the obj file and fixes up the missing members with some reasonable
> default,
> so these programs can still be loaded. Thus, the sample code using the
> current
> struct bpf_map_def will then work with tc as well. (I'll post the iproute2
> patch early next week.)
Are you encoding the number of maps into the start of the maps section
in the ELF then using that to divide out and determine the size?
I look forward to your patches. Maybe if TC is more tolerant of other
map definition sizes then this patch is less relevant.
^ permalink raw reply
* Re: Let's do P4
From: Maciej Żenczykowski @ 2016-11-02 5:06 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jiri Pirko, Alexei Starovoitov, Thomas Graf, John Fastabend,
Jakub Kicinski, Linux NetDev, David Miller, Jamal Hadi Salim,
roopa, simon.horman, ast, prem, Hannes Frederic Sowa, Jiri Benc,
Tom Herbert, mattyk, idosch, eladr, yotamg, nogahf, ogerlitz,
John W. Linville, Andy Gospodarek, Florian Fainelli
In-Reply-To: <58194F83.9020205@iogearbox.net>
> Sorry for jumping into the middle and the delay (plumbers this week). My
> question would be, if the main target is for p4 *offloading* anyway, who
> would use this sw fallback path? Mostly for testing purposes?
>
> I'm not sure about compilerB here and the complexity that needs to be
> pushed into the kernel along with it. I would assume this would result
> in slower code than what the existing P4 -> eBPF front ends for LLVM
> would generate since it could perform all kind of optimizations there,
> that might not be feasible for doing inside the kernel. Thus, if I'd want
> to do that in sw, I'd just use the existing LLVM facilities instead and
> go via cls_bpf in that case.
>
> What is your compilerA? Is that part of tc in user space? Maybe linked
> against LLVM lib, for example? If you really want some sw path, can't tc
> do this transparently from user space instead when it gets a netlink error
> that it cannot get offloaded (and thus switch internally to f_bpf's loader)?
Since we're jumping in the middle ;-)
Ideally we'd have an interface where some generic like program is
loaded into the kernel,
and the kernel core fetches some sort of generic description of the
hardware capabilities,
translates the program and fits as much of it as it can into the hardware,
possibly all of it, and emulates/executes the rest in software.
ie. if hardware can only match on 5 different 10 byte headers, but we
need to match on 7 different 12 byte headers,
we can still use the hardware to help us dispatch straight into 'check
the last 2 bytes, then the last 2 headers' software emulation code.
or maybe hardware can match, but can't count packets... so we need to
implement counting in sw.
or it can't do all types of encap/decap, so we need to sw encap in
certain cases...
Doing this via extracting such information out of a bpf program seems
pretty hard.
Or maybe I'm overestimating the true difficulty of taking a bpf
program and extracting it into a TCAM...
Maybe if the bpf program has a more 'standard' layout
(ie. a tree doing packet parsing/matching, with 'actions' in the
leaves) then it's not so hard?...
Obviously real hardware has significantly more capabilities then just
a tcam at the front of the pipeline...
I'm afraid I lack the knowledge of what the real capabilities of
current (and future...) hardware are...
But maybe we could come up with some sufficiently generic description
of *what* we want accomplished
instead of the precise specifics of how.
^ permalink raw reply
* [PATCH net] cxgb4: correct device ID of T6 adapter
From: Hariprasad Shenai @ 2016-11-02 5:22 UTC (permalink / raw)
To: netdev; +Cc: davem, leedom, nirranjan, Hariprasad Shenai
Signed-off-by: Hariprasad Shenai <hariprasad@chelsio.com>
---
drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h b/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
index 50812a1d67bd..df1573c4a659 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/t4_pci_id_tbl.h
@@ -178,9 +178,9 @@ CH_PCI_DEVICE_ID_TABLE_DEFINE_BEGIN
CH_PCI_ID_TABLE_FENTRY(0x6005),
CH_PCI_ID_TABLE_FENTRY(0x6006),
CH_PCI_ID_TABLE_FENTRY(0x6007),
+ CH_PCI_ID_TABLE_FENTRY(0x6008),
CH_PCI_ID_TABLE_FENTRY(0x6009),
CH_PCI_ID_TABLE_FENTRY(0x600d),
- CH_PCI_ID_TABLE_FENTRY(0x6010),
CH_PCI_ID_TABLE_FENTRY(0x6011),
CH_PCI_ID_TABLE_FENTRY(0x6014),
CH_PCI_ID_TABLE_FENTRY(0x6015),
--
2.3.4
^ permalink raw reply related
* [PATCH net] net: Check for fullsock in sock_i_uid()
From: Subash Abhinov Kasiviswanathan @ 2016-11-02 5:27 UTC (permalink / raw)
To: netdev; +Cc: Subash Abhinov Kasiviswanathan, Eric Dumazet
sock_i_uid() acquires the sk_callback_lock which does not exist
for sockets in TCP_NEW_SYN_RECV state. This results in errors
showing up as spinlock bad magic.
Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Cc: Eric Dumazet <edumazet@google.com>
---
net/core/sock.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/core/sock.c b/net/core/sock.c
index c73e28f..af15ef0 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1727,7 +1727,10 @@ void sock_efree(struct sk_buff *skb)
kuid_t sock_i_uid(struct sock *sk)
{
- kuid_t uid;
+ kuid_t uid = GLOBAL_ROOT_UID;
+
+ if (!sk_fullsock(sk))
+ return uid;
read_lock_bh(&sk->sk_callback_lock);
uid = sk->sk_socket ? SOCK_INODE(sk->sk_socket)->i_uid : GLOBAL_ROOT_UID;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH (net.git) 2/3] stmmac: fix PTP support for GMAC4
From: Giuseppe CAVALLARO @ 2016-11-02 6:34 UTC (permalink / raw)
To: Rayagond Kokatanur
Cc: netdev, alexandre.torgue, Richard Cochran, linux-kernel,
seraphin.bonnaffe
In-Reply-To: <CAJ3bTp7YON-+EiXCY+eaBP6NhtGJTngh2pLaKXdp2YPP1i+C6w@mail.gmail.com>
Hello Rayagond
if patches are ok, can we consider you Acked-by ?
Thx
Peppe
On 10/27/2016 12:51 PM, Rayagond Kokatanur wrote:
> On Thu, Oct 27, 2016 at 4:02 PM, Giuseppe CAVALLARO
> <peppe.cavallaro@st.com> wrote:
>> Hello Rayagond !
>>
>> On 10/27/2016 12:25 PM, Rayagond Kokatanur wrote:
>>>>
>>>> +static int dwmac4_wrback_get_rx_timestamp_status(void *desc, u32 ats)
>>>>> {
>>>>> struct dma_desc *p = (struct dma_desc *)desc;
>>>>> + int ret = -EINVAL;
>>>>> +
>>>>> + /* Get the status from normal w/b descriptor */
>>>>> + if (likely(p->des3 & TDES3_RS1V)) {
>>>>> + if (likely(p->des1 & RDES1_TIMESTAMP_AVAILABLE)) {
>>>>> + int i = 0;
>>>>> +
>>>>> + /* Check if timestamp is OK from context
>>>>> descriptor */
>>>>> + do {
>>>>> + ret = dwmac4_rx_check_timestamp(desc);
>>>
>>> Here, "desc" is not pointing to next descriptor (ie context
>>> descriptor). Driver should check the context descriptor.
>>
>>
>> you are right and this is done by the caller: stmmac_get_rx_hwtstamp
>
> Yes.
>
>>
>> Cheers
>> peppe
>>
>
>
>
^ permalink raw reply
* Re: [net-next PATCH 0/7] stmmac: dwmac-sti refactor+cleanup
From: Giuseppe CAVALLARO @ 2016-11-02 6:39 UTC (permalink / raw)
To: Joachim Eastwood, davem; +Cc: alexandre.torgue, netdev
In-Reply-To: <20161030200507.851-1-manabian@gmail.com>
Hello Joachim
thx for this series, I will review them and test the changes
on my board asap ... I let you known.
Peppe
On 10/30/2016 9:05 PM, Joachim Eastwood wrote:
> This patch set aims to remove the init/exit callbacks from the
> dwmac-sti driver and instead use standard PM callbacks. Doing this
> will also allow us to cleanup the driver.
>
> Eventually the init/exit callbacks will be deprecated and removed
> from all drivers dwmac-* except for dwmac-generic. Drivers will be
> refactored to use standard PM and remove callbacks.
>
> Note that this patch set has only been test compiled and no functional
> change is intended.
>
> Joachim Eastwood (7):
> stmmac: dwmac-sti: remove useless of_node check
> stmmac: dwmac-sti: remove clk NULL checks
> stmmac: dwmac-sti: add PM ops and resume function
> stmmac: dwmac-sti: move st,gmac_en parsing to sti_dwmac_parse_data
> stmmac: dwmac-sti: move clk_prepare_enable out of init and add error handling
> stmmac: dwmac-sti: clean up and rename sti_dwmac_init
> stmmac: dwmac-sti: remove unused priv dev member
>
> drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 86 ++++++++++++++++---------
> 1 file changed, 57 insertions(+), 29 deletions(-)
>
^ permalink raw reply
* Re: [PATCH (net.git) 0/3] stmmac: fix PTP support
From: Rayagond Kokatanur @ 2016-11-02 7:03 UTC (permalink / raw)
To: Richard Cochran
Cc: Giuseppe Cavallaro, netdev, alexandre.torgue, linux-kernel,
Seraphin Bonnaffe
In-Reply-To: <20161026085812.GD11974@localhost.localdomain>
On Wed, Oct 26, 2016 at 2:28 PM, Richard Cochran
<richardcochran@gmail.com> wrote:
> On Wed, Oct 26, 2016 at 08:56:01AM +0200, Giuseppe Cavallaro wrote:
>> This subset of patches aim to fix the PTP support
>> for the stmmac and especially for 4.x chip series.
>> While setting PTP on an ST box with 4.00a Ethernet
>> core, the kernel panics due to a broken settings
>> of the descriptors. The patches review the
>> register configuration, the algo used for configuring
>> the protocol, the way to get the timestamp inside
>> the RX/TX descriptors and, in the end, the statistics
>> displayed by ethtool.
>>
>> Giuseppe Cavallaro (3):
>> stmmac: update the PTP header file
>> stmmac: fix PTP support for GMAC4
>> stmmac: fix PTP type ethtool stats
>
> Acked-by: Richard Cochran <richardcochran@gmail.com>
Acked-by: Rayagond Kokatanur <rayagond@vayavyalabs.com>
--
wwr
Rayagond
^ permalink raw reply
* Re: [PATCH (net.git) 2/3] stmmac: fix PTP support for GMAC4
From: Rayagond Kokatanur @ 2016-11-02 7:04 UTC (permalink / raw)
To: Giuseppe CAVALLARO
Cc: netdev, alexandre.torgue, Richard Cochran, linux-kernel,
Seraphin Bonnaffe
In-Reply-To: <a0a8b469-2b5a-3a32-9896-e531ed281a6c@st.com>
On Wed, Nov 2, 2016 at 12:04 PM, Giuseppe CAVALLARO
<peppe.cavallaro@st.com> wrote:
> Hello Rayagond
>
> if patches are ok, can we consider you Acked-by ?
Yes.
>
> Thx
> Peppe
>
>
> On 10/27/2016 12:51 PM, Rayagond Kokatanur wrote:
>>
>> On Thu, Oct 27, 2016 at 4:02 PM, Giuseppe CAVALLARO
>> <peppe.cavallaro@st.com> wrote:
>>>
>>> Hello Rayagond !
>>>
>>> On 10/27/2016 12:25 PM, Rayagond Kokatanur wrote:
>>>>>
>>>>>
>>>>> +static int dwmac4_wrback_get_rx_timestamp_status(void *desc, u32 ats)
>>>>>>
>>>>>> {
>>>>>> struct dma_desc *p = (struct dma_desc *)desc;
>>>>>> + int ret = -EINVAL;
>>>>>> +
>>>>>> + /* Get the status from normal w/b descriptor */
>>>>>> + if (likely(p->des3 & TDES3_RS1V)) {
>>>>>> + if (likely(p->des1 & RDES1_TIMESTAMP_AVAILABLE)) {
>>>>>> + int i = 0;
>>>>>> +
>>>>>> + /* Check if timestamp is OK from context
>>>>>> descriptor */
>>>>>> + do {
>>>>>> + ret = dwmac4_rx_check_timestamp(desc);
>>>>
>>>>
>>>> Here, "desc" is not pointing to next descriptor (ie context
>>>> descriptor). Driver should check the context descriptor.
>>>
>>>
>>>
>>> you are right and this is done by the caller: stmmac_get_rx_hwtstamp
>>
>>
>> Yes.
>>
>>>
>>> Cheers
>>> peppe
>>>
>>
>>
>>
>
--
wwr
Rayagond
^ permalink raw reply
* [PATCH] igb/e1000: correct register comments
From: Cao jin @ 2016-11-02 7:20 UTC (permalink / raw)
To: linux-kernel, netdev; +Cc: intel-wired-lan, jeffrey.t.kirsher
Signed-off-by: Cao jin <caoj.fnst@cn.fujitsu.com>
---
drivers/net/ethernet/intel/igb/e1000_regs.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/e1000_regs.h b/drivers/net/ethernet/intel/igb/e1000_regs.h
index d84afdd..58adbf2 100644
--- a/drivers/net/ethernet/intel/igb/e1000_regs.h
+++ b/drivers/net/ethernet/intel/igb/e1000_regs.h
@@ -320,7 +320,7 @@
#define E1000_VT_CTL 0x0581C /* VMDq Control - RW */
#define E1000_WUC 0x05800 /* Wakeup Control - RW */
#define E1000_WUFC 0x05808 /* Wakeup Filter Control - RW */
-#define E1000_WUS 0x05810 /* Wakeup Status - RO */
+#define E1000_WUS 0x05810 /* Wakeup Status - R/W1C */
#define E1000_MANC 0x05820 /* Management Control - RW */
#define E1000_IPAV 0x05838 /* IP Address Valid - RW */
#define E1000_WUPL 0x05900 /* Wakeup Packet Length - RW */
--
2.1.0
^ permalink raw reply related
* Re: [PATCH net-next v2] ipv4: fib: Replay events when registering FIB notifier
From: Jiri Pirko @ 2016-11-02 7:20 UTC (permalink / raw)
To: Roopa Prabhu
Cc: Ido Schimmel, Eric Dumazet, netdev, davem, jiri, mlxsw, dsa,
nikolay, andy, vivien.didelot, andrew, f.fainelli,
alexander.h.duyck, kuznet, jmorris, yoshfuji, kaber, Ido Schimmel
In-Reply-To: <58194BD6.5040406@cumulusnetworks.com>
Wed, Nov 02, 2016 at 03:13:42AM CET, roopa@cumulusnetworks.com wrote:
>On 11/1/16, 10:03 AM, Ido Schimmel wrote:
>> Hi Roopa,
>>
>> On Tue, Nov 01, 2016 at 08:14:14AM -0700, Roopa Prabhu wrote:
>>>
>[snip]
>>> I have the same concern as Eric here.
>>>
>>> I understand why you need it, but can the driver request for an initial dump and that
>>> dump be made more efficient somehow ie not hold rtnl for the whole dump ?.
>>> instead of making the fib notifier registration code doing it.
>> We can do what we suggested in the last bi-weekly meeting, which is
>> still holding rtnl, but moving the hardware operation to delayed work.
>> This is possible because upper layers always assume operation was
>> successful and driver is responsible for invoking its abort mechanism in
>> case of failure.
>>
>>> these routing table sizes can be huge and an analogy for this in user-space:
>>> We do request a netlink dump of routing tables at initialization (on driver starts or resets)...
>>> but, existing netlink routing table dumps for that scale don't hold rtnl for the whole dump.
>>> The dump is split into multiple responses to the user and hence it does not starve other rtnl users.
>> In my reply to Eric I mentioned that when we register and unregister
>> from this chain the tables aren't really huge, but instead quite small.
>> I understand your concerns, but I don't wish to make things more
>> complicated than they should be only to address concerns that aren't
>> really realistic.
>
>I understand..but, if you are adding some core infrastructure for switchdev ..it cannot be
>based on the number of simple use-cases or data you have today.
>
>I won't be surprised if tomorrow other switch drivers have a case where they need to
>reset the hw routing table state and reprogram all routes again. Re-registering the notifier to just
>get the routing state of the kernel will not scale. For the long term, since the driver does not maintain a cache,
Driver (mlxsw, rocker) maintain a cache. So I'm not sure why you say
otherwise.
>a pull api with efficient use of rtnl will be useful for other such cases as well.
How do you imagine this "pull API" should look like?
>
>
>If you don't want to get to the complexity of a new api right away because of the
>simple case of management interface routes you have, Can your driver register the notifier early ?
>(I am sure you have probably already thought about this)
Register early? What it would resolve? I must be missing something. We
register as early as possible. But the thing is, we cannot register
in a past. And that is what this patch resolves.
>
>>
>> I believe current patch is quite simple and also consistent with other
>> notification chains in the kernel, such as the netdevice, where rtnl is
>> held during replay of events.
>> http://lxr.free-electrons.com/source/net/core/dev.c#L1535
>as you know, netdev and routing scale are not the same thing.
>Looking at the current code for netdevices (replay and rollback on failure),
>a pull api (equivalent to the netlink dump api) may end up being less complex...with an
>ability to batch in the future.
>
>
>
>
>
>
>
^ permalink raw reply
* Re: [PATCH net-next v2] ipv4: fib: Replay events when registering FIB notifier
From: Jiri Pirko @ 2016-11-02 7:35 UTC (permalink / raw)
To: David Miller
Cc: roopa, eric.dumazet, idosch, netdev, jiri, mlxsw, dsa, nikolay,
andy, vivien.didelot, andrew, f.fainelli, alexander.h.duyck,
kuznet, jmorris, yoshfuji, kaber, idosch
In-Reply-To: <20161101.113650.140429913221385583.davem@davemloft.net>
Tue, Nov 01, 2016 at 04:36:50PM CET, davem@davemloft.net wrote:
>From: Roopa Prabhu <roopa@cumulusnetworks.com>
>Date: Tue, 01 Nov 2016 08:14:14 -0700
>
>> On 11/1/16, 7:19 AM, Eric Dumazet wrote:
>>> On Tue, 2016-11-01 at 00:57 +0200, Ido Schimmel wrote:
>>>> On Mon, Oct 31, 2016 at 02:24:06PM -0700, Eric Dumazet wrote:
>>>>> How well will this work for large FIB tables ?
>>>>>
>>>>> Holding rtnl while sending thousands of skb will prevent consumers to
>>>>> make progress ?
>>>> Can you please clarify what do you mean by "while sending thousands of
>>>> skb"? This patch doesn't generate notifications to user space, but
>>>> instead invokes notification routines inside the kernel. I probably
>>>> misunderstood you.
>>>>
>>>> Are you suggesting this be done using RCU instead? Well, there are a
>>>> couple of reasons why I took RTNL here:
>>>>
>>> No, I do not believe RCU is wanted here, in control path where we might
>>> sleep anyway.
>>>
>>>> 1) The FIB notification chain is blocking, so listeners are expected to
>>>> be able to sleep. This isn't possible if we use RCU. Note that this
>>>> chain is mainly useful for drivers that reflect the FIB table into a
>>>> capable device and hardware operations usually involve sleeping.
>>>>
>>>> 2) The insertion of a single route is done with RTNL held. I didn't want
>>>> to differentiate between both cases. This property is really useful for
>>>> listeners, as they don't need to worry about locking in writer-side.
>>>> Access to data structs is serialized by RTNL.
>>> My concern was that for large iterations, you might hold RTNL and/or
>>> current cpu for hundred of ms or even seconds...
>>>
>> I have the same concern as Eric here.
>>
>> I understand why you need it, but can the driver request for an initial dump and that
>> dump be made more efficient somehow ie not hold rtnl for the whole dump ?.
>> instead of making the fib notifier registration code doing it.
>>
>> these routing table sizes can be huge and an analogy for this in user-space:
>> We do request a netlink dump of routing tables at initialization (on driver starts or resets)...
>> but, existing netlink routing table dumps for that scale don't hold rtnl for the whole dump.
>> The dump is split into multiple responses to the user and hence it does not starve other rtnl users.
>>
>> In-fact I don't think netlink routing table dumps from user-space hold rtnl_lock for the whole dump.
>> IIRC this was done to allow route add/dels to be allowed in parallel for performance reasons.
>> (I will need to double check to confirm this).
>
>I've always had some reservations about using notifiers for getting
>the FIB entries down to the offloaded device.
Yeah, me too. But there is no really other way to do it. I thought about
it quite long. But maybe I missed something.
>
>And this problem is just another symptom that it is the wrong
>mechanism for propagating this information.
>
>As suggested by Roopa here, perhaps we're looking at the problem from
>the wrong direction. We tend to design NDO ops and notifiers, to
>"push" things to the driver, but maybe something more like a push+pull
>model is better.
How do you imagine this mode should looks like? Could you draw me some
example?
Thanks!
^ permalink raw reply
* Re: Let's do P4
From: Jiri Pirko @ 2016-11-02 8:07 UTC (permalink / raw)
To: John Fastabend
Cc: Alexei Starovoitov, Thomas Graf, Jakub Kicinski, netdev, davem,
jhs, roopa, simon.horman, ast, daniel, prem, hannes, jbenc, tom,
mattyk, idosch, eladr, yotamg, nogahf, ogerlitz, linville, andy,
f.fainelli, dsa, vivien.didelot, andrew, ivecera,
Maciej Żenczykowski
In-Reply-To: <5818B11C.2040004@gmail.com>
Tue, Nov 01, 2016 at 04:13:32PM CET, john.fastabend@gmail.com wrote:
>[...]
>
>>>> P4 is ment to program programable hw, not fixed pipeline.
>>>>
>>>
>>> I'm guessing there are no upstream drivers at the moment that support
>>> this though right? The rocker universe bits though could leverage this.
>>
>> mlxsw. But this is naturaly not implemented yet, as there is no
>> infrastructure.
>
>Really? What is re-programmable?
>
>Can the parse graph support arbitrary parse graph?
>Can the table topology be reconfigured?
>Can new tables be created?
>What about "new" actions being defined at configuration time?
>
>Or is this just the normal TCAM configuration of defining key widths and
>fields.
At this point TCAM configuration.
>
>>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> since I cannot see how one can put the whole p4 language compiler
>>>>>>> into the driver, so this last step of p4ast->hw, I presume, will be
>>>>>>> done by firmware, which will be running full compiler in an embedded cpu
>>>>>>
>>>>>> In case of mlxsw, that compiler would be in driver.
>>>>>>
>>>>>>
>>>>>>> on the switch. To me that's precisely the kernel bypass, since we won't
>>>>>>> have a clue what HW capabilities actually are and won't be able to fine
>>>>>>> grain control them.
>>>>>>> Please correct me if I'm wrong.
>>>>>>
>>>>>> You are wrong. By your definition, everything has to be figured out in
>>>>>> driver and FW does nothing. Otherwise it could do "something else" and
>>>>>> that would be a bypass? Does not make any sense to me whatsoever.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>> Plus the thing I cannot imagine in the model you propose is table fillup.
>>>>>>>> For ebpf, you use maps. For p4 you would have to have a separate HW-only
>>>>>>>> API. This is very similar to the original John's Flow-API. And therefore
>>>>>>>> a kernel bypass.
>>>>>>>
>>>>>>> I think John's flow api is a better way to expose mellanox switch capabilities.
>>>>>>
>>>>>> We are under impression that p4 suits us nicely. But it is not about
>>>>>> us, it is about finding the common way to do this.
>>>>>>
>>>>>
>>>>> I'll just poke at my FlowAPI question again. For fixed ASICS what is
>>>>> the Flow-API missing. We have a few proof points that show it is both
>>>>> sufficient and usable for the handful of use cases we care about.
>>>>
>>>> Yeah, it is most probably fine. Even for flex ASICs to some point. The
>>>> question is how it stands comparing to other alternatives, like p4
>>>>
>>>
>>> Just to be clear the Flow-API _was_ generated from the initial P4 spec.
>>> The header files and tools used with it were autogenerated ("compiled"
>>> in a loose sense) from the P4 program. The piece I never exposed
>>> was the set_* operations to reconfigure running systems. I'm not sure
>>> how valuable this is in practice though.
>>>
>>> Also there is a P4-16 spec that will be released shortly that is more
>>> flexible and also more complex.
>>
>> Would it be able to easily extend the Flow-API to include the changes?
>>
>
>P4-16 will allow externs, "functions" to execute in the control flow and
>possibly inside the parse graph. None of this was considered in the
>Flow-API. So none of this is supported.
>
>I still have the question are you trying to push the "programming" of
>the device via 'tc' or just the runtime configuration of tables? If it
>is just runtime Flow-API is sufficient IMO. If its programming the
>device using the complete P4-16 spec than no its not sufficient. But
Sure we need both.
>I don't believe vendors will expose the complete programmability of the
>device in the driver, this is going to look more like a fw update than
>a runtime change at least on the devices I'm aware of.
Depends on driver. I think it is fine if driver processed it into come
hw configuration sequence or it simply pushed the program down to fw.
Both usecases are legit.
>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>> I also think it's not fair to call it 'bypass'. I see nothing in it
>>>>>>> that justify such 'swear word' ;)
>>>>>>
>>>>>> John's Flow-API was a kernel bypass. Why? It was a API specifically
>>>>>> designed to directly work with HW tables, without kernel being involved.
>>>>>
>>>>> I don't think that is a fair definition of HW bypass. The SKIP_SW flag
>>>>> does exactly that for 'tc' based offloads and it was not rejected.
>>>>
>>>> No, no, no. You still have possibility to do the same thing in kernel,
>>>> same functionality, with the same API. That is a big difference.
>>>>
>>>>
>>>>>
>>>>> The _real_ reason that seems to have fallen out of this and other
>>>>> discussion is the Flow-API didn't provide an in-kernel translation into
>>>>> an emulated patch. Note we always had a usermode translation to eBPF.
>>>>> A secondary reason appears to be overhead of adding yet another netlink
>>>>> family.
>>>>
>>>> Yeah. Maybe you remember, back then when Flow-API was being discussed,
>>>> I suggested to wrap it under TC as cls_xflows and cls_xflowsaction of
>>>> some sort and do in-kernel datapath implementation. I believe that after
>>>> that, it would be acceptable.
>>>>
>>>
>>> As I understand the thread here that is exactly the proposal here right?
>>> With a discussion around if the structures/etc are sufficient or any
>>> alternative representations exist.
>>
>> Might be the way, yes. But I fear that with other p4 extensions this
>> might not be easy to align with. Therefore I though about something more
>> generic, like the p4ast.
>>
>
>Same question as above are we _really_ talking about pushing the entire
>programmability of the device via 'tc'. If so we need to have a vendor
>say they will support and implement this?
We need some API, and I believe that TC is perfectly suitable for that.
Why do you think it's a problem?
>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> The goal of flow api was to expose HW features to user space, so that
>>>>>>> user space can program it. For something simple as mellanox switch
>>>>>>> asic it fits perfectly well.
>>>>>>
>>>>>> Again, this is not mlx-asic-specific. And again, that is a kernel bypass.
>>>>>>
>>>>>>
>>>>>>> Unless I misunderstand the bigger goal of this discussion and it's
>>>>>>> about programming ezchip devices.
>>>>>>
>>>>>> No. For network processors, I believe that BPF is nicely offloadable, no
>>>>>> need to do the excercise for that.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> If the goal is to model hw tcam in the linux kernel then just introduce
>>>>>>> tcam bpf map type. It will be dog slow in user space, but it will
>>>>>>> match exactly what is happnening in the HW and user space can make
>>>>>>> sensible trade-offs.
>>>>>>
>>>>>> No, you got me completely wrong. This is not about the TCAM. This is
>>>>>> about differences in the 2 words (p4/bpf).
>>>>>> Again, for "p4-ish" devices, you have to translate BPF. And as you
>>>>>> noted, it's an instruction set. Very hard if not impossible to parse in
>>>>>> order to get back the original semantics.
>>>>>>
>>>>>
>>>>> I think in this discussion "p4-ish" devices means devices with multiple
>>>>> tables in a pipeline? Not devices that have programmable/configurable
>>>>> pipelines right? And if we get to talking about reconfigurable devices
>>>>> I believe this should be done out of band as it typically means
>>>>> reloading some ucode, etc.
>>>>
>>>> I'm talking about both. But I think we should focus on reconfigurable
>>>> ones, as we probably won't see that much fixed ones in the future.
>>>>
>>>
>>> hmm maybe but the 10/40/100Gbps devices are going to be around for some
>>> time. So we need to ensure these work well.
>>
>> Yes, but I would like to emphasize, if we are defining new api
>> the primary focus should be on new devices.
>>
>>
>
>What device though. Back to mlxsw question about actually supporting
>this stuff.
>
^ 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