* Re: [PATCH RFC] cfg80211: add new command for reporting wiphy crashes
From: Johannes Berg @ 2019-09-26 12:04 UTC (permalink / raw)
To: Rafał Miłecki, Jouni Malinen
Cc: David S . Miller, linux-wireless, netdev, linux-kernel, hostap,
openwrt-devel
In-Reply-To: <09503390-91f0-3789-496a-6e9891156c5e@gmail.com>
On Thu, 2019-09-26 at 14:00 +0200, Rafał Miłecki wrote:
> > You can't seriously be suggesting that the driver doesn't *have* enough
> > information - everything passed through it :)
>
> Precisely: it doesn't store (cache) enough info.
But nothing stops it (the driver) from doing so!
> In brcmfmac on .add_virtual_intf() we:
> 1) Send request to the FullMAC firmware
> 2) Wait for a reply
> 3) On success we create interface
> 4) We wake up .add_virtual_intf() handler
>
> I'll need to find a way to skip step 3 in recovery path since interface
> on host side already exists.
Sure, we skip lots of things in all drivers, look at iwlwifi for example
with IWL_MVM_STATUS_IN_HW_RESTART.
> OK, so basically I need to work on caching setup data. Should I try
> doing that at my selected driver level (brcmfmac)? Or should I focus on
> generic solution (cfg80211) and consider offloading mac80211 if
> possible?
I think doing it generically will not work well, you have different code
paths and different formats, different data that you need etc.
Sometimes there's information cfg80211 doesn't even *have*, because the
driver is responsible for things (e.g. elements). I guess you can try,
but my gut feeling is that it'd simpler in the driver.
Now you can argue that everything passes through cfg80211 so it must
have enough data too (just like I'm arguing the driver certainly has
enough data), but ... it seems to me the cfg80211 is usually more
action-based, where the restore flow needs to keep the *state*, not
replay the series of actions that happened.
johannes
^ permalink raw reply
* Re: [PATCH] net: phy: micrel: add Asym Pause workaround for KSZ9021
From: Andrew Lunn @ 2019-09-26 12:09 UTC (permalink / raw)
To: Hans Andersson
Cc: f.fainelli, hkallweit1, davem, netdev, linux-kernel,
antoine.tenart, Hans Andersson
In-Reply-To: <20190926075437.18088-1-haan@cellavision.se>
On Thu, Sep 26, 2019 at 09:54:37AM +0200, Hans Andersson wrote:
> From: Hans Andersson <hans.andersson@cellavision.se>
>
> The Micrel KSZ9031 PHY may fail to establish a link when the Asymmetric
> Pause capability is set. This issue is described in a Silicon Errata
> (DS80000691D or DS80000692D), which advises to always disable the
> capability.
>
> Micrel KSZ9021 has no errata, but has the same issue with Asymmetric Pause.
> This patch apply the same workaround as the one for KSZ9031.
>
> Signed-off-by: Hans Andersson <hans.andersson@cellavision.se>
Fixes: 3aed3e2a143c ("net: phy: micrel: add Asym Pause workaround")
This is not the best fixes: tag, since it was not that change which
broke it. But going further back will be hard.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* RE: [PATCH] net: ena: clean up indentation issue
From: Kiyanovski, Arthur @ 2019-09-26 12:17 UTC (permalink / raw)
To: Colin King, Belgazal, Netanel, Bshara, Saeed, Machulsky, Zorik,
David S . Miller, Jubran, Samih, netdev@vger.kernel.org
Cc: kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190926112252.21498-1-colin.king@canonical.com>
> -----Original Message-----
> From: Colin King <colin.king@canonical.com>
> Sent: Thursday, September 26, 2019 2:23 PM
> To: Belgazal, Netanel <netanel@amazon.com>; Bshara, Saeed
> <saeedb@amazon.com>; Machulsky, Zorik <zorik@amazon.com>; David S .
> Miller <davem@davemloft.net>; Kiyanovski, Arthur <akiyano@amazon.com>;
> Jubran, Samih <sameehj@amazon.com>; netdev@vger.kernel.org
> Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] net: ena: clean up indentation issue
>
> From: Colin Ian King <colin.king@canonical.com>
>
> There memset is indented incorrectly, remove the extraneous tabs.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/ethernet/amazon/ena/ena_eth_com.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/amazon/ena/ena_eth_com.c
> b/drivers/net/ethernet/amazon/ena/ena_eth_com.c
> index 38046bf0ff44..2845ac277724 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_eth_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_eth_com.c
> @@ -211,8 +211,8 @@ static int ena_com_sq_update_llq_tail(struct
> ena_com_io_sq *io_sq)
>
> pkt_ctrl->curr_bounce_buf =
> ena_com_get_next_bounce_buffer(&io_sq-
> >bounce_buf_ctrl);
> - memset(io_sq->llq_buf_ctrl.curr_bounce_buf,
> - 0x0, llq_info->desc_list_entry_size);
> + memset(io_sq->llq_buf_ctrl.curr_bounce_buf,
> + 0x0, llq_info->desc_list_entry_size);
>
> pkt_ctrl->idx = 0;
> if (unlikely(llq_info->desc_stride_ctrl ==
> ENA_ADMIN_SINGLE_DESC_PER_ENTRY))
> --
> 2.20.1
LGTM Thanks!
^ permalink raw reply
* Re: [PATCH net] devlink: Fix error handling in param and info_get dumpit cb
From: Andrew Lunn @ 2019-09-26 12:27 UTC (permalink / raw)
To: Vasundhara Volam; +Cc: davem, netdev, Jiri Pirko, Michael Chan
In-Reply-To: <1569490554-21238-1-git-send-email-vasundhara-v.volam@broadcom.com>
On Thu, Sep 26, 2019 at 03:05:54PM +0530, Vasundhara Volam wrote:
> If any of the param or info_get op returns error, dumpit cb is
> skipping to dump remaining params or info_get ops for all the
> drivers.
>
> Instead skip only for the param/info_get op which returned error
> and continue to dump remaining information, except if the return
> code is EMSGSIZE.
Hi Vasundhara
How do we get to see something did fail? If it failed, it failed for a
reason, and we want to know.
What is your real use case here? What is failing, and why are you
O.K. to skip this failure?
Andrew
^ permalink raw reply
* Re: [PATCH net-next] dt-bindings: net: ravb: Add support for r8a774b1 SoC
From: Geert Uytterhoeven @ 2019-09-26 12:39 UTC (permalink / raw)
To: Biju Das
Cc: Rob Herring, Mark Rutland, Sergei Shtylyov, David S. Miller,
Simon Horman, Fabrizio Castro, netdev, Linux-Renesas,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Simon Horman, Chris Paterson
In-Reply-To: <1569245566-9987-1-git-send-email-biju.das@bp.renesas.com>
On Mon, Sep 23, 2019 at 3:33 PM Biju Das <biju.das@bp.renesas.com> wrote:
> Document RZ/G2N (R8A774B1) SoC bindings.
>
> Signed-off-by: Biju Das <biju.das@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH bpf] libbpf: teach btf_dumper to emit stand-alone anonymous enum definitions
From: Daniel Borkmann @ 2019-09-26 12:43 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, netdev, ast, andrii.nakryiko, kernel-team
In-Reply-To: <20190925203745.3173184-1-andriin@fb.com>
On Wed, Sep 25, 2019 at 01:37:45PM -0700, Andrii Nakryiko wrote:
> BTF-to-C converter previously skipped anonymous enums in an assumption
> that those are embedded in struct's field definitions. This is not
> always the case and a lot of kernel constants are defined as part of
> anonymous enums. This change fixes the logic by eagerly marking all
> types as either referenced by any other type or not. This is enough to
> distinguish two classes of anonymous enums and emit previously omitted
> enum definitions.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Applied, thanks!
^ permalink raw reply
* Re: Are BPF tail calls only supposed to work with pinned maps?
From: Daniel Borkmann @ 2019-09-26 12:53 UTC (permalink / raw)
To: Toke Høiland-Jørgensen; +Cc: netdev, bpf
In-Reply-To: <874l0z2tdx.fsf@toke.dk>
Hi Toke,
On Thu, Sep 26, 2019 at 01:23:38PM +0200, Toke Høiland-Jørgensen wrote:
[...]
> While working on a prototype of the XDP chain call feature, I ran into
> some strange behaviour with tail calls: If I create a userspace program
> that loads two XDP programs, one of which tail calls the other, the tail
> call map would appear to be empty even though the userspace program
> populates it as part of the program loading.
>
> I eventually tracked this down to this commit:
> c9da161c6517 ("bpf: fix clearing on persistent program array maps")
Correct.
> Which clears PROG_ARRAY maps whenever the last uref to it disappears
> (which it does when my loader exits after attaching the XDP program).
>
> This effectively means that tail calls only work if the PROG_ARRAY map
> is pinned (or the process creating it keeps running). And as far as I
> can tell, the inner_map reference in bpf_map_fd_get_ptr() doesn't bump
> the uref either, so presumably if one were to create a map-in-map
> construct with tail call pointer in the inner map(s), each inner map
> would also need to be pinned (haven't tested this case)?
There is no map in map support for tail calls today.
> Is this really how things are supposed to work? From an XDP use case PoV
> this seems somewhat surprising...
>
> Or am I missing something obvious here?
The way it was done like this back then was in order to break up cyclic
dependencies as otherwise the programs and maps involved would never get
freed as they reference themselves and live on in the kernel forever
consuming potentially large amount of resources, so orchestration tools
like Cilium typically just pin the maps in bpf fs (like most other maps
it uses and accesses from agent side) in order to up/downgrade the agent
while keeping BPF datapath intact.
Thanks,
Daniel
^ permalink raw reply
* pull-request: wireless-drivers 2019-09-26
From: Kalle Valo @ 2019-09-26 12:57 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, linux-kernel
Hi Dave,
here's a pull request to net tree for v5.4. Please let me know if there
are any problems.
Kalle
The following changes since commit 280ceaed79f18db930c0cc8bb21f6493490bf29c:
usbnet: sanity checking of packet sizes and device mtu (2019-09-19 13:27:11 +0200)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git tags/wireless-drivers-for-davem-2019-09-26
for you to fetch changes up to 2b481835cf4e7384b80d7762074b32a45b792d99:
wil6210: use after free in wil_netif_rx_any() (2019-09-25 09:12:20 +0300)
----------------------------------------------------------------
wireless-drivers fixes for 5.4
First set of fixes for 5.4 sent during the merge window. Most are
regressions fixes but the mt7615 problem has been since it was merged.
iwlwifi
* fix a build regression related CONFIG_THERMAL
* avoid using GEO_TX_POWER_LIMIT command on certain firmware versions
rtw88
* fixes for skb leaks
zd1211rw
* fix a compiler warning on 32 bit
mt76
* fix the firmware paths for mt7615 to match with linux-firmware
wil6210
* fix use of skb after free
----------------------------------------------------------------
Dan Carpenter (1):
wil6210: use after free in wil_netif_rx_any()
Geert Uytterhoeven (1):
zd1211rw: zd_usb: Use "%zu" to format size_t
Johannes Berg (1):
iwlwifi: mvm: fix build w/o CONFIG_THERMAL
Lorenzo Bianconi (1):
mt76: mt7615: fix mt7615 firmware path definitions
Luca Coelho (1):
iwlwifi: fw: don't send GEO_TX_POWER_LIMIT command to FW version 36
Yan-Hsuan Chuang (3):
rtw88: pci: extract skbs free routine for trx rings
rtw88: pci: release tx skbs DMAed when stop
rtw88: configure firmware after HCI started
drivers/net/wireless/ath/wil6210/txrx.c | 2 +-
drivers/net/wireless/intel/iwlwifi/mvm/fw.c | 8 ++--
drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 9 +++-
drivers/net/wireless/mediatek/mt76/mt7615/mcu.c | 11 ++---
drivers/net/wireless/mediatek/mt76/mt7615/mt7615.h | 6 +--
drivers/net/wireless/realtek/rtw88/mac.c | 3 --
drivers/net/wireless/realtek/rtw88/main.c | 4 ++
drivers/net/wireless/realtek/rtw88/pci.c | 48 +++++++++++++++++-----
drivers/net/wireless/zydas/zd1211rw/zd_usb.c | 2 +-
9 files changed, 63 insertions(+), 30 deletions(-)
^ permalink raw reply
* Re: [RFC PATCH v4 2/5] ptp: Reorganize ptp_kvm modules to make it arch-independent.
From: Suzuki K Poulose @ 2019-09-26 13:06 UTC (permalink / raw)
To: Jianyong Wu, netdev, yangbo.lu, john.stultz, tglx, pbonzini,
sean.j.christopherson, maz, richardcochran, Mark.Rutland,
Will.Deacon
Cc: linux-kernel, linux-arm-kernel, kvmarm, kvm, Steve.Capper,
Kaly.Xin, justin.he, nd
In-Reply-To: <20190926114212.5322-3-jianyong.wu@arm.com>
Hi Jianyong,
On 26/09/2019 12:42, Jianyong Wu wrote:
> Currently, ptp_kvm modules implementation is only for x86 which includs
> large part of arch-specific code. This patch move all of those code
> into new arch related file in the same directory.
>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
> ---
> drivers/ptp/Makefile | 1 +
> drivers/ptp/{ptp_kvm.c => kvm_ptp.c} | 77 ++++++------------------
> drivers/ptp/ptp_kvm_x86.c | 87 ++++++++++++++++++++++++++++
> include/asm-generic/ptp_kvm.h | 12 ++++
> 4 files changed, 118 insertions(+), 59 deletions(-)
> rename drivers/ptp/{ptp_kvm.c => kvm_ptp.c} (63%)
minor nit: Could we not skip renaming the file ? Given
you are following the ptp_kvm_* for the arch specific
files and the header files, wouldn't it be good to
keep ptp_kvm.c ?
Rest looks fine.
Cheers
Suzuki
^ permalink raw reply
* Re: [PATCH RFC] cfg80211: add new command for reporting wiphy crashes
From: Rafał Miłecki @ 2019-09-26 11:52 UTC (permalink / raw)
To: Jouni Malinen, Rafał Miłecki
Cc: Johannes Berg, David S . Miller, linux-wireless, netdev,
linux-kernel, hostap, openwrt-devel
In-Reply-To: <20190920140143.GA30514@w1.fi>
On 20.09.2019 16:01, Jouni Malinen wrote:
> On Fri, Sep 20, 2019 at 03:37:08PM +0200, Rafał Miłecki wrote:
>> Hardware or firmware instability may result in unusable wiphy. In such
>> cases usually a hardware reset is needed. To allow a full recovery
>> kernel has to indicate problem to the user space.
>
> Why? Shouldn't the driver be able to handle this on its own since all
> the previous configuration was done through the driver anyway. As far as
> I know, there are drivers that do indeed try to do this and handle it
> successfully at least for station mode. AP mode may be more complex, but
> for that one, I guess it would be fine to drop all associations (and
> provide indication of that to user space) and just restart the BSS.
Indeed my main concert is AP mode. I'm afraid that cfg80211 doesn't
cache all settings, consider e.g. nl80211_start_ap(). It builds
struct cfg80211_ap_settings using info from nl80211 message and
passes it to the driver (rdev_start_ap()). Once it's done it
caches only a small subset of all setup data.
In other words driver doesn't have enough info to recover interfaces
setup.
>> This new nl80211 command lets user space known wiphy has crashed and has
>> been just recovered. When applicable it should result in supplicant or
>> authenticator reconfiguring all interfaces.
>
> For me, that is not really "recovered" if some additional
> reconfiguration steps are needed.. I'd like to get a more detailed view
> on what exactly might need to be reconfigured and how would user space
> know what exactly to do. Or would the plan here be that the driver would
> not even indicate this crash if it is actually able to internally
> recover fully from the firmware restart?
I meant that hardware has been recovered & is operational again (driver
can talk to it). I expected user space to reconfigure all interfaces
using the same settings that were used on previous run.
If driver were able to recover interfaces setup on its own (with a help
of cfg80211) then user space wouldn't need to be involved.
>> I'd like to use this new cfg80211_crash_report() in brcmfmac after a
>> successful recovery from a FullMAC firmware crash.
>>
>> Later on I'd like to modify hostapd to reconfigure wiphy using a
>> previously used setup.
>
> So this implies that at least something would need to happen in AP mode.
> Do you have a list of items that the driver cannot do on its own and why
> it would be better to do them from user space?
First of all I was wondering how to handle interfaces creation. After a
firmware crash we have:
1) Interfaces created in Linux
2) No corresponsing interfaces in firmware
Syncing that (re-creating in-firmware firmwares) may be a bit tricky
depending on a driver and hardware. For some cases it could be easier to
delete all interfaces and ask user space to setup wiphy (create required
interfaces) again. I'm not sure if that's acceptable though?
If we agree interfaces should stay and driver simply should configure
firmware properly, then we need all data as explained earlier. struct
cfg80211_ap_settings is not available during runtime. How should we
handle that problem?
>> I'm OpenWrt developer & user and I got annoyed by my devices not auto
>> recovering after various failures. There are things I cannot fix (hw
>> failures or closed fw crashes) but I still expect my devices to get
>> back to operational state as soon as possible on their own.
>
> I fully agree with the auto recovery being important thing to cover for
> this, but I'm not yet convinced that this needs user space action. Or if
> it does, there would need to be more detailed way of indicating what
> exactly is needed for user space to do. The proposed change here is just
> saying "hey, I crashed and did something to get the hardware/firmware
> responding again" which does not really tell much to user space other
> than potentially requiring full disable + re-enable for the related
> interfaces. And that is something that should not actually be done in
> all cases of firmware crashes since there are drivers that handle
> recovery in a manner that is in practice completely transparent to user
> space.
I was aiming for a brutal force solution: just make user space
interfaces need a full setup just at they were just created.
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Edward Cree @ 2019-09-26 13:09 UTC (permalink / raw)
To: Paul Blakey, Jakub Kicinski
Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller,
netdev@vger.kernel.org, Jiri Pirko, Cong Wang, Jamal Hadi Salim,
Simon Horman, Or Gerlitz
In-Reply-To: <541fde6d-01ce-edf3-84e4-153756aba00f@mellanox.com>
On 26/09/2019 08:30, Paul Blakey wrote:
> Ok, I thought you meant merging the rules because we do want to support
> those modifications use-cases.
I think the point is that your use-case is sufficiently weird and
obscure that code in the core to support it needs to be unintrusive;
and this clearly wasn't (you managed to piss off Linus...) so it
should be reverted, and held off until a more palatable solution can
be produced. I agree with Alexei on this.
Neither currently-supported-by-drivers cases nor the first step that's
likely to be added (the simple conntrack with modifications only at
the end) needs this, it's for capabilities that are farther in the
future, so there's really no need for it to be in the tree when it's
not ready, which appears to be the case at present.
> In nat scenarios the packet will be modified, and then there can be a miss:
>
> -trk .... CT(zone X, Restore NAT),goto chain 1
>
> +trk+est, match on ipv4, CT(zone Y), goto chain 2
>
> +trk+est, output..
I'm confused, I thought the usual nat scenario looked more like
0: -trk ... action ct(zone x), goto chain 1
1: +trk+new ... action ct(commit, nat=foo) # sw only
1: +trk+est ... action ct(nat), mirred eth1
i.e. the NAT only happens after conntrack has matched (and thus provided
the saved NAT metadata), at the end of the pipe. I don't see how you
can NAT a -trk packet.
> Also, there are stats issues if we already accounted for some actions in
> hardware.
AFAICT only 'deliverish' actions (i.e. mirred and drop) in TC have stats.
So stats are unlikely to be a problem unless you've got (say) a mirred
mirror before you send to ct and goto chain, in which case the extra
copy of the packet is a rather bigger problem for idempotency than mere
stats ;-)
^ permalink raw reply
* Re: Are BPF tail calls only supposed to work with pinned maps?
From: Toke Høiland-Jørgensen @ 2019-09-26 13:12 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, bpf
In-Reply-To: <20190926125347.GB6563@pc-63.home>
Daniel Borkmann <daniel@iogearbox.net> writes:
> Hi Toke,
>
> On Thu, Sep 26, 2019 at 01:23:38PM +0200, Toke Høiland-Jørgensen wrote:
> [...]
>> While working on a prototype of the XDP chain call feature, I ran into
>> some strange behaviour with tail calls: If I create a userspace program
>> that loads two XDP programs, one of which tail calls the other, the tail
>> call map would appear to be empty even though the userspace program
>> populates it as part of the program loading.
>>
>> I eventually tracked this down to this commit:
>> c9da161c6517 ("bpf: fix clearing on persistent program array maps")
>
> Correct.
>
>> Which clears PROG_ARRAY maps whenever the last uref to it disappears
>> (which it does when my loader exits after attaching the XDP program).
>>
>> This effectively means that tail calls only work if the PROG_ARRAY map
>> is pinned (or the process creating it keeps running). And as far as I
>> can tell, the inner_map reference in bpf_map_fd_get_ptr() doesn't bump
>> the uref either, so presumably if one were to create a map-in-map
>> construct with tail call pointer in the inner map(s), each inner map
>> would also need to be pinned (haven't tested this case)?
>
> There is no map in map support for tail calls today.
Not directly, but can't a program do:
tail_call_map = bpf_map_lookup(outer_map, key);
bpf_tail_call(tail_call_map, idx);
>> Is this really how things are supposed to work? From an XDP use case PoV
>> this seems somewhat surprising...
>>
>> Or am I missing something obvious here?
>
> The way it was done like this back then was in order to break up cyclic
> dependencies as otherwise the programs and maps involved would never get
> freed as they reference themselves and live on in the kernel forever
> consuming potentially large amount of resources, so orchestration tools
> like Cilium typically just pin the maps in bpf fs (like most other maps
> it uses and accesses from agent side) in order to up/downgrade the agent
> while keeping BPF datapath intact.
Right. I can see how the cyclic reference thing gets thorny otherwise.
However, the behaviour was somewhat surprising to me; is it documented
anywhere?
I think I'll probably end up creating a new map type for chaining
programs anyway, so this is not a huge show-stopper for me; but it had
me scratching my head for a while there... ;)
-Toke
^ permalink raw reply
* Re: [PATCH] vhost: introduce mdev based hardware backend
From: Tiwei Bie @ 2019-09-26 13:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jasowang, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang,
lingshan.zhu
In-Reply-To: <20190926042156-mutt-send-email-mst@kernel.org>
On Thu, Sep 26, 2019 at 04:35:18AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 26, 2019 at 12:54:27PM +0800, Tiwei Bie wrote:
[...]
> > diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> > index 40d028eed645..5afbc2f08fa3 100644
> > --- a/include/uapi/linux/vhost.h
> > +++ b/include/uapi/linux/vhost.h
> > @@ -116,4 +116,12 @@
> > #define VHOST_VSOCK_SET_GUEST_CID _IOW(VHOST_VIRTIO, 0x60, __u64)
> > #define VHOST_VSOCK_SET_RUNNING _IOW(VHOST_VIRTIO, 0x61, int)
> >
> > +/* VHOST_MDEV specific defines */
> > +
> > +#define VHOST_MDEV_SET_STATE _IOW(VHOST_VIRTIO, 0x70, __u64)
> > +
> > +#define VHOST_MDEV_S_STOPPED 0
> > +#define VHOST_MDEV_S_RUNNING 1
> > +#define VHOST_MDEV_S_MAX 2
> > +
> > #endif
>
> So assuming we have an underlying device that behaves like virtio:
I think they are really good questions/suggestions. Thanks!
>
> 1. Should we use SET_STATUS maybe?
I like this idea. I will give it a try.
> 2. Do we want a reset ioctl?
I think it is helpful. If we use SET_STATUS, maybe we
can use it to support the reset.
> 3. Do we want ability to enable rings individually?
I will make it possible at least in the vhost layer.
> 4. Does device need to limit max ring size?
> 5. Does device need to limit max number of queues?
I think so. It's helpful to have ioctls to report the max
ring size and max number of queues.
Thanks!
Tiwei
>
> --
> MST
^ permalink raw reply
* Re: [PATCH net] net: axienet: fix a signedness bug in probe
From: Dan Carpenter @ 2019-09-26 13:18 UTC (permalink / raw)
To: Alvaro G. M
Cc: Radhey Shyam Pandey, David S. Miller, Michal Simek, Russell King,
netdev, kernel-janitors, Andrew Lunn
In-Reply-To: <20190925110542.GA21923@salem.gmr.ssr.upm.es>
On Wed, Sep 25, 2019 at 01:05:43PM +0200, Alvaro G. M wrote:
> Hi, Dan
>
> On Wed, Sep 25, 2019 at 01:59:11PM +0300, Dan Carpenter wrote:
> > The "lp->phy_mode" is an enum but in this context GCC treats it as an
> > unsigned int so the error handling is never triggered.
> >
> > lp->phy_mode = of_get_phy_mode(pdev->dev.of_node);
> > - if (lp->phy_mode < 0) {
> > + if ((int)lp->phy_mode < 0) {
>
> This (almost) exact code appears in a lot of different drivers too,
> so maybe it'd be nice to review them all and apply the same cast if needed?
>
This is a new warning in Smatch. I did send patches for the whole
kernel. We won't get these bugs in the future because people run Smatch
on the kernel and will find the bugs. All the bugs were from 2017 or
later which suggests that someone cleared these out two years ago but
soon the 0-day bot will warn about issues so they will get fixed
quicker.
I'm sort of out of it today...
The get_phy_mode() function seem like they lend themselves to creating
these bugs. The ->phy_mode variables tend to be declared in the driver
so it would require quite a few patches to make them all int and I'm not
sure that's more beautiful. Andrew Lunn's idea to update the API would
probably be a good idea.
I'm going back to bed for now and I'll think about this some more.
regards,
dan carpenter
^ permalink raw reply
* Re: DSA driver kernel extension for dsa mv88e6190 switch
From: Zoran Stojsavljevic @ 2019-09-26 13:23 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev
In-Reply-To: <20190923191713.GB28770@lunn.ch>
Hello Andrew,
I would like to thank you for the reply.
I do not know if this is the right place to post such the questions,
but my best guess is: yes.
Since till now I did not make any success to make (using DSA driver)
make mv88e6190 single switch to work with any kernel.org. :-(
I did ugly workaround as kernel dsa patch, which allowed me to
introduce TXC and RXC clock skews between I.MX6 and mv88e6190 (MAC to
MAC layer over rgmii).
And, yes, switch is working in dummy state (as you correctly described
it), passing traffic everywhere (flooding).
i.MX6 has a silicon bug, which does not allow skew configuration on
its side. PCB is out of consideration, so this ugly patch makes switch
to apply these two delays.Then, in dummy state, everything works.
_______
My DTS mv88e6190 configuration, which I adopted for the custom board I
am working on, could be seen here:
https://pastebin.com/xpXQYNRX
But on another note... I am wondering if I am setting correct kernel
configuration for it?!
Here is the part of the configuration I made while going through maze
of posts from google search results:
Switch (and switch-ish) device support @ Networking
support->Networking options
Distributed Switch Architecture @ Networking support->Networking options
Tag driver for Marvell switches using DSA headers @ Networking
support->Networking options->Distributed Switch Architecture
Tag driver for Marvell switches using EtherType DSA headers @
Networking support->Networking options->Distributed Switch
Architecture
Marvell 88E6xxx Ethernet switch fabric support @ Device
Drivers->Network device support->Distributed Switch Architecture
drivers
Switch Global 2 Registers support @ Device Drivers->Network
device support->Distributed Switch Architecture drivers->Marvell
88E6xxx Ethernet switch fabric support
Freescale devices @ Device Drivers->Network device
support->Ethernet driver support
FEC ethernet controller (of ColdFire and some i.MX CPUs) @
Device Drivers->Network device support->Ethernet driver
support->Freescale devices
Marvell devices @ Device Drivers->Network device
support->Ethernet driver support
Marvell MDIO interface support @ Device Drivers->Network device
support->Ethernet driver support->Marvell devices
MDIO Bus/PHY emulation with fixed speed/link PHYs @ Device
Drivers->Network device support->PHY Device support and infrastructure
(Do we need Marvell PHYs option as =y ? I do not think so - should be:
is not set)
What possibly I made wrong here (this does not work - I could not get
through the switch, and seems that MDIO works (from the logic
analyzer), but addresses some 0x1B/0x1C ports, which should NOT be
addressed, according to the the DTS configuration shown)?
Thank you,
Zoran
_______
On Thu, Sep 26, 2019 at 1:10 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > We have the configuration problem with the Marvell 88E6190 switch.
> > What the our problem is... Is the switch is NOT configured with the
> > EEPROM (24C512), which does not exist on the board.
>
> That is pretty normal. If there is a EEPROM, i generally recommend it
> is left empty. We want Linux to configure the switch, and if it finds
> it already partially configured, things can get confused.
>
> > It is put in autoconfig by HW straps (NOCPU mode).
>
> So dumb switch mode. All ports are switched between each other.
>
> > Once the MDIO command, issued to
> > probe the switch and read the make of it, the switch jumps out of the
> > autoconfig mode.
>
> Correct. Dumb switch mode is dangerous. There is no STP, etc.
> Depending on what you have in device tree, the ports are either
> configured down, or separated.
>
> > There are some commands issued from the DSA to
> > configure the switch (to apply to switch TXC and RXC RGMII delays -
> > RGMII-ID mode), but this is not enough to make it work properly.
>
> Define 'work properly'. How are you configuring the interfaces? Do
> you remember to bring the master interface up? Are you adding the
> interfaces to a bridge?
>
> Andrew
^ permalink raw reply
* Re: [PATCH net] net: axienet: fix a signedness bug in probe
From: Andrew Lunn @ 2019-09-26 13:24 UTC (permalink / raw)
To: Dan Carpenter
Cc: Alvaro G. M, Radhey Shyam Pandey, David S. Miller, Michal Simek,
Russell King, netdev, kernel-janitors
In-Reply-To: <20190926131811.GG29696@kadam>
> The get_phy_mode() function seem like they lend themselves to creating
> these bugs. The ->phy_mode variables tend to be declared in the driver
> so it would require quite a few patches to make them all int and I'm not
> sure that's more beautiful. Andrew Lunn's idea to update the API would
> probably be a good idea.
Hi Dan
I started on it. Once net-next has opened, and 0-day has compile
tested my changes, i will post the code for review.
Andrew
^ permalink raw reply
* Re: [PATCH] vhost: introduce mdev based hardware backend
From: Michael S. Tsirkin @ 2019-09-26 13:26 UTC (permalink / raw)
To: Tiwei Bie
Cc: jasowang, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang,
lingshan.zhu
In-Reply-To: <20190926131439.GA11652@___>
On Thu, Sep 26, 2019 at 09:14:39PM +0800, Tiwei Bie wrote:
> > 4. Does device need to limit max ring size?
> > 5. Does device need to limit max number of queues?
>
> I think so. It's helpful to have ioctls to report the max
> ring size and max number of queues.
Also, let's not repeat the vhost net mistakes, let's lock
everything to the order required by the virtio spec,
checking status bits at each step.
E.g.:
set backend features
set features
detect and program vqs
enable vqs
enable driver
and check status at each step to force the correct order.
e.g. don't allow enabling vqs after driver ok, etc
--
MST
^ permalink raw reply
* Re: [PATCH] vhost: introduce mdev based hardware backend
From: Tiwei Bie @ 2019-09-26 13:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: jasowang, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang,
lingshan.zhu
In-Reply-To: <20190926091945-mutt-send-email-mst@kernel.org>
On Thu, Sep 26, 2019 at 09:26:22AM -0400, Michael S. Tsirkin wrote:
> On Thu, Sep 26, 2019 at 09:14:39PM +0800, Tiwei Bie wrote:
> > > 4. Does device need to limit max ring size?
> > > 5. Does device need to limit max number of queues?
> >
> > I think so. It's helpful to have ioctls to report the max
> > ring size and max number of queues.
>
> Also, let's not repeat the vhost net mistakes, let's lock
> everything to the order required by the virtio spec,
> checking status bits at each step.
> E.g.:
> set backend features
> set features
> detect and program vqs
> enable vqs
> enable driver
>
> and check status at each step to force the correct order.
> e.g. don't allow enabling vqs after driver ok, etc
Got it. Thanks a lot!
Regards,
Tiwei
>
> --
> MST
^ permalink raw reply
* Re: DSA driver kernel extension for dsa mv88e6190 switch
From: Andrew Lunn @ 2019-09-26 13:38 UTC (permalink / raw)
To: Zoran Stojsavljevic; +Cc: netdev
In-Reply-To: <CAGAf8LyQpi_R-A2Zx72bJhSBqnFo-r=KCnfVCTD9N8cNNtbhrQ@mail.gmail.com>
On Thu, Sep 26, 2019 at 03:23:48PM +0200, Zoran Stojsavljevic wrote:
> Hello Andrew,
>
> I would like to thank you for the reply.
>
> I do not know if this is the right place to post such the questions,
> but my best guess is: yes.
>
> Since till now I did not make any success to make (using DSA driver)
> make mv88e6190 single switch to work with any kernel.org. :-(
>
> I did ugly workaround as kernel dsa patch, which allowed me to
> introduce TXC and RXC clock skews between I.MX6 and mv88e6190 (MAC to
> MAC layer over rgmii).
You should not need any kernel patches for switch side RGMII
delays. rgmii-id in the DT for the switch CPU port should be enough.
Some of the vf610-zii platforms use it.
> My DTS mv88e6190 configuration, which I adopted for the custom board I
> am working on, could be seen here:
> https://pastebin.com/xpXQYNRX
So you have the FEC using rgmii-id. Which you say does not work? So
why not use plain rgmii. What you have in port@0 looks correct.
gpios = <&gpio1 29 GPIO_ACTIVE_HIGH>; is wrong. It probably should be
reset-gpios. The rest looks O.K.
> But on another note... I am wondering if I am setting correct kernel
> configuration for it?!
>
> Here is the part of the configuration I made while going through maze
> of posts from google search results:
>
> Switch (and switch-ish) device support @ Networking
> support->Networking options
> Distributed Switch Architecture @ Networking support->Networking options
> Tag driver for Marvell switches using DSA headers @ Networking
> support->Networking options->Distributed Switch Architecture
> Tag driver for Marvell switches using EtherType DSA headers @
> Networking support->Networking options->Distributed Switch
> Architecture
> Marvell 88E6xxx Ethernet switch fabric support @ Device
> Drivers->Network device support->Distributed Switch Architecture
> drivers
> Switch Global 2 Registers support @ Device Drivers->Network
> device support->Distributed Switch Architecture drivers->Marvell
> 88E6xxx Ethernet switch fabric support
> Freescale devices @ Device Drivers->Network device
> support->Ethernet driver support
> FEC ethernet controller (of ColdFire and some i.MX CPUs) @
> Device Drivers->Network device support->Ethernet driver
> support->Freescale devices
> Marvell devices @ Device Drivers->Network device
> support->Ethernet driver support
> Marvell MDIO interface support @ Device Drivers->Network device
> support->Ethernet driver support->Marvell devices
> MDIO Bus/PHY emulation with fixed speed/link PHYs @ Device
> Drivers->Network device support->PHY Device support and infrastructure
>
> (Do we need Marvell PHYs option as =y ? I do not think so - should be:
> is not set)
Yes you do. The PHYs inside the switch are Marvell.
> What possibly I made wrong here (this does not work - I could not get
> through the switch, and seems that MDIO works (from the logic
> analyzer), but addresses some 0x1B/0x1C ports, which should NOT be
> addressed, according to the the DTS configuration shown)?
0x1b is global1, and 0x1c is global2. These are registers shared by
all ports.
Please show me the configuration steps you are doing? How are you
configuring the FEC and the switch interfaces?
Andrew
^ permalink raw reply
* Re: [PATCH V2] net: dsa: microchip: Always set regmap stride to 1
From: George McCollister @ 2019-09-26 13:52 UTC (permalink / raw)
To: Marek Vasut
Cc: netdev, Andrew Lunn, David S . Miller, Florian Fainelli,
Tristram Ha, Vivien Didelot, Woojung Huh
In-Reply-To: <20190925220842.4301-1-marex@denx.de>
On Wed, Sep 25, 2019 at 5:08 PM Marek Vasut <marex@denx.de> wrote:
>
> The regmap stride is set to 1 for regmap describing 8bit registers already.
> However, for 16/32/64bit registers, the stride is 2/4/8 respectively. This
> is not correct, as the switch protocol supports unaligned register reads
> and writes and the KSZ87xx even uses such unaligned register accesses to
> read e.g. MIB counter.
>
> This patch fixes MIB counter access on KSZ87xx.
After looking through a couple hundred pages of register documentation
for KSZ9477 and KSZ9567 I find only registers that are aligned to
their width. In my testing the KSZ9567 works fine with and without the
patch. The only downside is that all of the unaligned registers
needlessly show up in the debugfs regmap, this doesn't really matter
though. As long as it fixes the issues on KSZ87xx this looks fine to
me.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: George McCollister <george.mccollister@gmail.com>
> Cc: Tristram Ha <Tristram.Ha@microchip.com>
> Cc: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> Cc: Woojung Huh <woojung.huh@microchip.com>
> Fixes: 46558d601cb6 ("net: dsa: microchip: Initial SPI regmap support")
> Fixes: 255b59ad0db2 ("net: dsa: microchip: Factor out regmap config generation into common header")
> ---
> V2: Add Fixes: tags
> ---
> drivers/net/dsa/microchip/ksz_common.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index a24d8e61fbe7..dd60d0837fc6 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -303,7 +303,7 @@ static inline void ksz_pwrite32(struct ksz_device *dev, int port, int offset,
> { \
> .name = #width, \
> .val_bits = (width), \
> - .reg_stride = (width) / 8, \
> + .reg_stride = 1, \
> .reg_bits = (regbits) + (regalign), \
> .pad_bits = (regpad), \
> .max_register = BIT(regbits) - 1, \
> --
> 2.23.0
>
Reviewed-by: George McCollister <george.mccollister@gmail.com>
I tested the patch on the KSZ9567, not anything else FWIW:
Tested-by: George McCollister <george.mccollister@gmail.com>
^ permalink raw reply
* RE: chapoly acceleration hardware [Was: Re: [RFC PATCH 00/18] crypto: wireguard using the existing crypto API]
From: Pascal Van Leeuwen @ 2019-09-26 13:52 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Ard Biesheuvel, Linux Crypto Mailing List, linux-arm-kernel,
Herbert Xu, David Miller, Greg KH, Linus Torvalds, Samuel Neves,
Dan Carpenter, Arnd Bergmann, Eric Biggers, Andy Lutomirski,
Will Deacon, Marc Zyngier, Catalin Marinas, Willy Tarreau, Netdev,
Toke Høiland-Jørgensen, Dave Taht
In-Reply-To: <CAHmME9r5m7D-oMU6Lv_ZhEyWmrNscMr5HokzdK0wg2Ayzzbeow@mail.gmail.com>
> -----Original Message-----
> From: Jason A. Donenfeld <Jason@zx2c4.com>
> Sent: Thursday, September 26, 2019 1:07 PM
> To: Pascal Van Leeuwen <pvanleeuwen@verimatrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Linux Crypto Mailing List <linux-
> crypto@vger.kernel.org>; linux-arm-kernel <linux-arm-kernel@lists.infradead.org>;
> Herbert Xu <herbert@gondor.apana.org.au>; David Miller <davem@davemloft.net>; Greg KH
> <gregkh@linuxfoundation.org>; Linus Torvalds <torvalds@linux-foundation.org>; Samuel
> Neves <sneves@dei.uc.pt>; Dan Carpenter <dan.carpenter@oracle.com>; Arnd Bergmann
> <arnd@arndb.de>; Eric Biggers <ebiggers@google.com>; Andy Lutomirski <luto@kernel.org>;
> Will Deacon <will@kernel.org>; Marc Zyngier <maz@kernel.org>; Catalin Marinas
> <catalin.marinas@arm.com>; Willy Tarreau <w@1wt.eu>; Netdev <netdev@vger.kernel.org>;
> Toke Høiland-Jørgensen <toke@toke.dk>; Dave Taht <dave.taht@gmail.com>
> Subject: chapoly acceleration hardware [Was: Re: [RFC PATCH 00/18] crypto: wireguard
> using the existing crypto API]
>
> [CC +willy, toke, dave, netdev]
>
> Hi Pascal
>
> On Thu, Sep 26, 2019 at 12:19 PM Pascal Van Leeuwen
> <pvanleeuwen@verimatrix.com> wrote:
> > Actually, that assumption is factually wrong. I don't know if anything
> > is *publicly* available, but I can assure you the silicon is running in
> > labs already. And something will be publicly available early next year
> > at the latest. Which could nicely coincide with having Wireguard support
> > in the kernel (which I would also like to see happen BTW) ...
> >
> > Not "at some point". It will. Very soon. Maybe not in consumer or server
> > CPUs, but definitely in the embedded (networking) space.
> > And it *will* be much faster than the embedded CPU next to it, so it will
> > be worth using it for something like bulk packet encryption.
>
> Super! I was wondering if you could speak a bit more about the
> interface. My biggest questions surround latency. Will it be
> synchronous or asynchronous?
>
The hardware being external to the CPU and running in parallel with it,
obviously asynchronous.
> If the latter, why?
>
Because, as you probably already guessed, the round-trip latency is way
longer than the actual processing time, at least for small packets.
Partly because the only way to communicate between the CPU and the HW
accelerator (whether that is crypto, a GPU, a NIC, etc.) that doesn't
keep the CPU busy moving data is through memory, with the HW doing DMA.
And, as any programmer should now, round trip times to memory are huge
relative to the processing speed.
And partly because these accelerators are very similar to CPU's in
terms of architecture, doing pipelined processing and having multiple
of such pipelines in parallel. Except that these pipelines are not
working on low-level instructions but on full packets/blocks. So they
need to have many packets in flight to keep those pipelines fully
occupied. And packets need to move through the various pipeline stages,
so they incur the time needed to process them multiple times. (just
like e.g. a multiply instruction with a throughput of 1 per cycle
actually may need 4 or more cycles to actually provide its result)
Could you do that from a synchronous interface? In theory, probably,
if you would spawn a new thread for every new packet arriving and
rely on the scheduler to preempt the waiting threads. But you'd need
as many threads as the HW accelerator can have packets in flight,
while an async would need only 2 threads: one to handle the input to
the accelerator and one to handle the output (or at most one thread
per CPU, if you want to divide the workload)
Such a many-thread approach seems very inefficient to me.
> What will its latencies be?
>
Depends very much on the specific integration scenario (i.e. bus
speed, bus hierarchy, cache hierarchy, memory speed, etc.) but on
the order of a few thousand CPU clocks is not unheard of.
Which is an eternity for the CPU, but still only a few uSec in
human time. Not a problem unless you're a high-frequency trader and
every ns counts ...
It's not like the CPU would process those packets in zero time.
> How deep will its buffers be?
>
That of course depends on the specific accelerator implementation,
but possibly dozens of small packets in our case, as you'd need
at least width x depth packets in there to keep the pipes busy.
Just like a modern CPU needs hundreds of instructions in flight
to keep all its resources busy.
> The reason I ask is that a
> lot of crypto acceleration hardware of the past has been fast and
> having very deep buffers, but at great expense of latency.
>
Define "great expense". Everything is relative. The latency is very
high compared to per-packet processing time but at the same time it's
only on the order of a few uSec. Which may not even be significant on
the total time it takes for the packet to travel from input MAC to
output MAC, considering the CPU will still need to parse and classify
it and do pre- and postprocessing on it.
> In the networking context, keeping latency low is pretty important.
>
I've been doing this for IPsec for nearly 20 years now and I've never
heard anyone complain about our latency, so it must be OK.
We're also doing (fully inline, no CPU involved) MACsec cores, which
operate at layer 2 and I know it's a concern there for very specific
use cases (high frequency trading, precision time protocol, ...).
For "normal" VPN's though, a few uSec more or less should be a non-issue.
> Already
> WireGuard is multi-threaded which isn't super great all the time for
> latency (improvements are a work in progress). If you're involved with
> the design of the hardware, perhaps this is something you can help
> ensure winds up working well? For example, AES-NI is straightforward
> and good, but Intel can do that because they are the CPU. It sounds
> like your silicon will be adjacent. How do you envision this working
> in a low latency environment?
>
Depends on how low low-latency is. If you really need minimal latency,
you need an inline implementation. Which we can also provide, BTW :-)
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Paul Blakey @ 2019-09-26 13:56 UTC (permalink / raw)
To: Edward Cree, Jakub Kicinski
Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller,
netdev@vger.kernel.org, Jiri Pirko, Cong Wang, Jamal Hadi Salim,
Simon Horman, Or Gerlitz
In-Reply-To: <08f58572-26ed-e947-5b0c-73732ef7eb35@solarflare.com>
On 9/26/2019 4:09 PM, Edward Cree wrote:
> On 26/09/2019 08:30, Paul Blakey wrote:
>> Ok, I thought you meant merging the rules because we do want to support
>> those modifications use-cases.
> I think the point is that your use-case is sufficiently weird and
> obscure that code in the core to support it needs to be unintrusive;
> and this clearly wasn't (you managed to piss off Linus...) so it
> should be reverted, and held off until a more palatable solution can
> be produced. I agree with Alexei on this.
> Neither currently-supported-by-drivers cases nor the first step that's
> likely to be added (the simple conntrack with modifications only at
> the end) needs this, it's for capabilities that are farther in the
> future, so there's really no need for it to be in the tree when it's
> not ready, which appears to be the case at present.
The use-case isn't weird, as I've just shown, even nat needs that. See
below that even OvS has selftests that test that.
That's normal OvS nat, which you can see in many use-cases. You can take
a look at different OvS controllers
and you would see the same behavior.
This is the first step that was required to support offloading
connection tracking from OvS.
We are in progress of submitting the userspace patches, and would have
done it today if it weren't'
for this CONFIG discussion. It was already submitted as RFC already.
Our driver implementation is ready and we will submit it once userspace
is accepted.
>> In nat scenarios the packet will be modified, and then there can be a miss:
>>
>> -trk .... CT(zone X, Restore NAT),goto chain 1
>>
>> +trk+est, match on ipv4, CT(zone Y), goto chain 2
>>
>> +trk+est, output..
> I'm confused, I thought the usual nat scenario looked more like
> 0: -trk ... action ct(zone x), goto chain 1
> 1: +trk+new ... action ct(commit, nat=foo) # sw only
> 1: +trk+est ... action ct(nat), mirred eth1
> i.e. the NAT only happens after conntrack has matched (and thus provided
> the saved NAT metadata), at the end of the pipe. I don't see how you
> can NAT a -trk packet.
Both are valid, Nat in the first hop, executes the nat stored on the
connection if available (configured by commit).
You can see it in multiple uses in ovs tests: git grep -pni
"table=0.*ct.*nat.*" tests/system-traffic.at
Even after you restore nat, you can jump to different zones, or even
again to another table....
I've seen much deeper re-circulation chains (which usually means CT())
action.
See ovs self-tests as reference.
>
>> Also, there are stats issues if we already accounted for some actions in
>> hardware.
> AFAICT only 'deliverish' actions (i.e. mirred and drop) in TC have stats.
> So stats are unlikely to be a problem unless you've got (say) a mirred
> mirror before you send to ct and goto chain, in which case the extra
> copy of the packet is a rather bigger problem for idempotency than mere
> stats ;-)
All tc actions have software stats, and at least one (goto, mirred,
drop) per OvS generated rule will have hardware stats.
All OvS datapath rules have stats, and in turn the translated TC rules
all have stats. OvS ages each rule independently.
^ permalink raw reply
* Re: [PATCH] vhost: introduce mdev based hardware backend
From: kbuild test robot @ 2019-09-26 14:05 UTC (permalink / raw)
To: Tiwei Bie
Cc: kbuild-all, mst, jasowang, alex.williamson, maxime.coquelin,
linux-kernel, kvm, virtualization, netdev, dan.daly,
cunming.liang, zhihong.wang, lingshan.zhu, tiwei.bie
In-Reply-To: <20190926045427.4973-1-tiwei.bie@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]
Hi Tiwei,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on vhost/linux-next]
[cannot apply to v5.3 next-20190925]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Tiwei-Bie/vhost-introduce-mdev-based-hardware-backend/20190926-125932
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-13) 7.4.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/vhost/mdev.c:13:10: fatal error: linux/virtio_mdev.h: No such file or directory
#include <linux/virtio_mdev.h>
^~~~~~~~~~~~~~~~~~~~~
compilation terminated.
vim +13 drivers/vhost/mdev.c
> 13 #include <linux/virtio_mdev.h>
14
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69514 bytes --]
^ permalink raw reply
* [PATCH net] sk_buff: drop all skb extensions on free and skb scrubbing
From: Florian Westphal @ 2019-09-26 14:18 UTC (permalink / raw)
To: netdev; +Cc: steffen.klassert, paulb, vladbu, Florian Westphal
Now that we have a 3rd extension, add a new helper that drops the
extension space and use it when we need to scrub an sk_buff.
At this time, scrubbing clears secpath and bridge netfilter data, but
retains the tc skb extension, after this patch all three get cleared.
NAPI reuse/free assumes we can only have a secpath attached to skb, but
it seems better to clear all extensions there as well.
Fixes: 95a7233c452a ("net: openvswitch: Set OvS recirc_id from tc chain index")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
include/linux/skbuff.h | 9 +++++++++
net/core/dev.c | 4 ++--
net/core/skbuff.c | 2 +-
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 907209c0794e..4debdd58a0ce 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4144,8 +4144,17 @@ static inline void *skb_ext_find(const struct sk_buff *skb, enum skb_ext_id id)
return NULL;
}
+
+static inline void skb_ext_reset(struct sk_buff *skb)
+{
+ if (skb->active_extensions) {
+ __skb_ext_put(skb->extensions);
+ skb->active_extensions = 0;
+ }
+}
#else
static inline void skb_ext_put(struct sk_buff *skb) {}
+static inline void skb_ext_reset(struct sk_buff *skb) {}
static inline void skb_ext_del(struct sk_buff *skb, int unused) {}
static inline void __skb_ext_copy(struct sk_buff *d, const struct sk_buff *s) {}
static inline void skb_ext_copy(struct sk_buff *dst, const struct sk_buff *s) {}
diff --git a/net/core/dev.c b/net/core/dev.c
index 71b18e80389f..bf3ed413abaf 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5666,7 +5666,7 @@ EXPORT_SYMBOL(gro_find_complete_by_type);
static void napi_skb_free_stolen_head(struct sk_buff *skb)
{
skb_dst_drop(skb);
- secpath_reset(skb);
+ skb_ext_put(skb);
kmem_cache_free(skbuff_head_cache, skb);
}
@@ -5733,7 +5733,7 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
skb->encapsulation = 0;
skb_shinfo(skb)->gso_type = 0;
skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
- secpath_reset(skb);
+ skb_ext_reset(skb);
napi->skb = skb;
}
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f12e8a050edb..01d65206f4fb 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5119,7 +5119,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet)
skb->skb_iif = 0;
skb->ignore_df = 0;
skb_dst_drop(skb);
- secpath_reset(skb);
+ skb_ext_reset(skb);
nf_reset(skb);
nf_reset_trace(skb);
--
2.21.0
^ permalink raw reply related
* Re: Fwd: [PATCH] bonding/802.3ad: fix slave initialization states race
From: Aleksei Zakharov @ 2019-09-26 14:25 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev, zhangsha (A)
In-Reply-To: <15507.1569472734@nyx>
чт, 26 сент. 2019 г. в 07:38, Jay Vosburgh <jay.vosburgh@canonical.com>:
>
> Aleksei Zakharov <zaharov@selectel.ru> wrote:
>
> >ср, 25 сент. 2019 г. в 03:31, Jay Vosburgh <jay.vosburgh@canonical.com>:
> >>
> >> Алексей Захаров wrote:
> >> [...]
> >> >Right after reboot one of the slaves hangs with actor port state 71
> >> >and partner port state 1.
> >> >It doesn't send lacpdu and seems to be broken.
> >> >Setting link down and up again fixes slave state.
> >> [...]
> >>
> >> I think I see what failed in the first patch, could you test the
> >> following patch? This one is for net-next, so you'd need to again swap
> >> slave_err / netdev_err for the Ubuntu 4.15 kernel.
> >>
> >I've tested new patch. It seems to work. I can't reproduce the bug
> >with this patch.
> >There are two types of messages when link becomes up:
> >First:
> >bond-san: EVENT 1 llu 4294895911 slave eth2
> >8021q: adding VLAN 0 to HW filter on device eth2
> >bond-san: link status definitely down for interface eth2, disabling it
> >mlx4_en: eth2: Link Up
> >bond-san: EVENT 4 llu 4294895911 slave eth2
> >bond-san: link status up for interface eth2, enabling it in 500 ms
> >bond-san: invalid new link 3 on slave eth2
> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> >Second:
> >bond-san: EVENT 1 llu 4295147594 slave eth2
> >8021q: adding VLAN 0 to HW filter on device eth2
> >mlx4_en: eth2: Link Up
> >bond-san: EVENT 4 llu 4295147594 slave eth2
> >bond-san: link status up again after 0 ms for interface eth2
> >bond-san: link status definitely up for interface eth2, 10000 Mbps full duplex
> > [...]
>
> The "invalid new link" is appearing because bond_miimon_commit
> is being asked to commit a new state that isn't UP or DOWN (3 is
> BOND_LINK_BACK). I looked through the patched code today, and I don't
> see a way to get to that message with the new link set to 3, so I'll add
> some instrumentation and send out another patch to figure out what's
> going on, as that shouldn't happen.
>
> I don't see the "invalid" message testing locally, I think
> because my network device doesn't transition to carrier up as quickly as
> yours. I thought you were getting BOND_LINK_BACK passed through from
> bond_enslave (which calls bond_set_slave_link_state, which will set
> link_new_link to BOND_LINK_BACK and leave it there), but the
> link_new_link is reset first thing in bond_miimon_inspect, so I'm not
> sure how it gets into bond_miimon_commit (I'm thinking perhaps a
> concurrent commit triggered by another slave, which then picks up this
> proposed link state change by happenstance).
I assume that "invalid new link" happens in this way:
Interface goes up
NETDEV_CHANGE event occurs
bond_update_speed_duplex fails
and slave->last_link_up returns true
slave->link becomes BOND_LINK_FAIL
bond_check_dev_link returns 0
miimon proposes slave->link_new_state BOND_LINK_DOWN
NETDEV_UP event occurs
miimon sets commit++
miimon proposes slave->link_new_state BOND_LINK_BACK
miimon sets slave->link to BOND_LINK_BACK
we have updelay configured, so it doesn't set BOND_LINK_UP in the next
case section
miimon says "Invalid new link" and sets link state UP during next
inspection(after updelay, i suppose)
For the second type of messages it looks like this:
Interface goes up
NETDEV_CHANGE event occurs
bond_update_speed_duplex fails
and slave->last_link_up returns true
slave->link becomes BOND_LINK_FAIL
NETDEV_UP event occurs
bond_check_dev_link returns 1
miimon proposes slave->link_new_state BOND_LINK_UP and says "link
status up again"
My first patch changed slave->last_link_up check to (slave->link ==
BOND_LINK_UP).
This check looks more consistent for me, but I might be wrong here.
As a result if link was in BOND_LINK_FAIL or BOND_LINK_BACK when
CHANGE or UP event,
it became BOND_LINK_DOWN.
But if it was initially UP and bond_update_speed_duplex was unable to
get speed/duplex,
link became BOND_LINK_FAIL.
I don't understand a few things here:
How could a link be in a different state from time to time during the
first NETDEV_* event?
And why slave->last_link_up is set when the first NETDEV event occurs?
I hope I didn't messed things up too much here.
--
Best Regards,
Aleksei Zakharov
^ 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