* Re: [PATCH] dt-bindings: net: dwmac: fix 'mac-mode' type
From: Florian Fainelli @ 2019-09-21 3:02 UTC (permalink / raw)
To: Jakub Kicinski, Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, peppe.cavallaro,
alexandre.torgue, andrew
In-Reply-To: <20190920181141.52cfee67@cakuba.netronome.com>
On 9/20/2019 6:11 PM, Jakub Kicinski wrote:
> On Tue, 17 Sep 2019 13:30:52 +0300, Alexandru Ardelean wrote:
>> The 'mac-mode' property is similar to 'phy-mode' and 'phy-connection-type',
>> which are enums of mode strings.
>>
>> The 'dwmac' driver supports almost all modes declared in the 'phy-mode'
>> enum (except for 1 or 2). But in general, there may be a case where
>> 'mac-mode' becomes more generic and is moved as part of phylib or netdev.
>>
>> In any case, the 'mac-mode' field should be made an enum, and it also makes
>> sense to just reference the 'phy-connection-type' from
>> 'ethernet-controller.yaml'. That will also make it more future-proof for new
>> modes.
>>
>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Applied, thank you!
>
> FWIW I had to add the Fixes tag by hand, either ozlabs patchwork or my
> git-pw doesn't have the automagic handling there, yet.
AFAICT the ozlabs patchwork instance does not do it, but if you have
patchwork administrative rights (the jango administration panel I mean)
then it is simple to add the regular expression to the list of tags that
patchwork already recognized. Had tried getting that included by
default, but it also counted all of those tags and therefore was not
particularly fine grained:
https://lists.ozlabs.org/pipermail/patchwork/2017-January/003910.html
--
Florian
^ permalink raw reply
* Re: [PATCH net] ipv6: fix a typo in fib6_rule_lookup()
From: Jakub Kicinski @ 2019-09-21 2:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, Wei Wang, Yi Ren
In-Reply-To: <20190919171236.111294-1-edumazet@google.com>
On Thu, 19 Sep 2019 10:12:36 -0700, Eric Dumazet wrote:
> Yi Ren reported an issue discovered by syzkaller, and bisected
> to the cited commit.
>
> Many thanks to Yi, this trivial patch does not reflect the patient
> work that has been done.
>
> Fixes: d64a1f574a29 ("ipv6: honor RT6_LOOKUP_F_DST_NOREF in rule lookup logic")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Wei Wang <weiwan@google.com>
> Bisected-and-Reported-by: Yi Ren <c4tren@gmail.com>
Apparently:
WARNING: 'Bisected-and-reported-by:' is the preferred signature form
#15:
Bisected-and-Reported-by: Yi Ren <c4tren@gmail.com>
total: 0 errors, 1 warnings, 0 checks, 8 lines checked
So I lower-cased that R, hope that's okay.
Applied, queued, thank you!
^ permalink raw reply
* Re: [PATCH net] sch_netem: fix a divide by zero in tabledist()
From: Jakub Kicinski @ 2019-09-21 2:15 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David S . Miller, netdev, Eric Dumazet, syzbot
In-Reply-To: <20190918150539.135042-1-edumazet@google.com>
On Wed, 18 Sep 2019 08:05:39 -0700, Eric Dumazet wrote:
> syzbot managed to crash the kernel in tabledist() loading
> an empty distribution table.
>
> t = dist->table[rnd % dist->size];
>
> Simply return an error when such load is attempted.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
Applied, queued, thank you!
^ permalink raw reply
* Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize
From: Jakub Kicinski @ 2019-09-21 2:03 UTC (permalink / raw)
To: Bjørn Mork; +Cc: netdev, linux-usb, Oliver Neukum
In-Reply-To: <20190918121738.6343-1-bjorn@mork.no>
On Wed, 18 Sep 2019 14:17:38 +0200, Bjørn Mork wrote:
> Endpoints with zero wMaxPacketSize are not usable for transferring
> data. Ignore such endpoints when looking for valid in, out and
> status pipes, to make the drivers more robust against invalid and
> meaningless descriptors.
>
> The wMaxPacketSize of these endpoints are used for memory allocations
> and as divisors in many usbnet minidrivers. Avoiding zero is therefore
> critical.
>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
Fixes tag would be useful. I'm not sure how far into stable we should
backport this.
Is this something that occurs on real devices or protection from
malicious ones?
^ permalink raw reply
* Re: [PATCH] dt-bindings: net: remove un-implemented property
From: Jakub Kicinski @ 2019-09-21 1:58 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: netdev, devicetree, linux-kernel, davem, robh+dt
In-Reply-To: <20190918111447.3084-1-alexandru.ardelean@analog.com>
On Wed, 18 Sep 2019 14:14:47 +0300, Alexandru Ardelean wrote:
> The `adi,disable-energy-detect` property was implemented in an initial
> version of the `adin` driver series, but after a review it was discarded in
> favor of implementing the ETHTOOL_PHY_EDPD phy-tunable option.
>
> With the ETHTOOL_PHY_EDPD control, it's possible to disable/enable
> Energy-Detect-Power-Down for the `adin` PHY, so this device-tree is not
> needed.
>
> Fixes: 767078132ff9 ("dt-bindings: net: add bindings for ADIN PHY driver")
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied, thank you!
^ permalink raw reply
* Re: [PATCH] drivers/net: release skb on failure
From: Jakub Kicinski @ 2019-09-21 1:49 UTC (permalink / raw)
To: Navid Emamdoost
Cc: emamd001, smccaman, kjlu, Manish Chopra, GR-Linux-NIC-Dev,
David S. Miller, netdev, linux-kernel
In-Reply-To: <20190918044521.14953-1-navid.emamdoost@gmail.com>
On Tue, 17 Sep 2019 23:45:21 -0500, Navid Emamdoost wrote:
> In ql_run_loopback_test, ql_lb_send does not release skb when fails. So
> it must be released before returning.
>
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c | 4 +++-
Thanks for the patch, this driver has been moved, please see
commit 955315b0dc8c8641311430f40fbe53990ba40e33
Author: Benjamin Poirier <bpoirier@suse.com>
Date: Tue Jul 23 15:14:13 2019 +0900
qlge: Move drivers/net/ethernet/qlogic/qlge/ to
drivers/staging/qlge/
The hardware has been declared EOL by the vendor more than 5 years
ago. What's more relevant to the Linux kernel is that the quality
of this driver is not on par with many other mainline drivers.
Cc: Manish Chopra <manishc@marvell.com>
Message-id: <20190617074858.32467-1-bpoirier@suse.com>
Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Could you rebase, and send the new version to GregKH as he is the
stable maintainer?
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> index a6886cc5654c..d539b71b2a5c 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> @@ -544,8 +544,10 @@ static int ql_run_loopback_test(struct ql_adapter *qdev)
> skb_put(skb, size);
> ql_create_lb_frame(skb, size);
> rc = ql_lb_send(skb, qdev->ndev);
> - if (rc != NETDEV_TX_OK)
> + if (rc != NETDEV_TX_OK) {
> + dev_kfree_skb_any(skb);
> return -EPIPE;
> + }
> atomic_inc(&qdev->lb_count);
> }
> /* Give queue time to settle before testing results. */
^ permalink raw reply
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
From: Thierry Reding @ 2019-09-21 1:35 UTC (permalink / raw)
To: Florian Fainelli
Cc: David S . Miller, Giuseppe Cavallaro, Alexandre Torgue,
Jose Abreu, Jon Hunter, Bitan Biswas, netdev, linux-tegra
In-Reply-To: <fa2fafac-f193-3cef-666a-767859d41f91@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]
On Fri, Sep 20, 2019 at 10:02:28AM -0700, Florian Fainelli wrote:
> On 9/20/19 10:00 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > The DWMAC 4.10 supports the same enhanced addressing mode as later
> > generations. Parse this capability from the hardware feature registers
> > and set the EAME (Enhanced Addressing Mode Enable) bit when necessary.
>
> Do you think these two patches should have companion Fixes: tag? They
> are definitively bug fixes, but maybe you would also want those to be
> back ported to -stable trees?
I wouldn't really consider these bug fixes. They're more along the lines
of feature additions. The driver previously didn't support EAME and that
was fine. The fact that it never worked under specific circumstances is
not a bug or regression, really.
Thierry
> > Thierry Reding (2):
> > net: stmmac: Only enable enhanced addressing mode when needed
> > net: stmmac: Support enhanced addressing mode for DWMAC 4.10
> >
> > drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 1 +
> > .../ethernet/stmicro/stmmac/dwmac4_descs.c | 4 ++--
> > .../net/ethernet/stmicro/stmmac/dwmac4_dma.c | 22 +++++++++++++++++++
> > .../net/ethernet/stmicro/stmmac/dwmac4_dma.h | 3 +++
> > .../ethernet/stmicro/stmmac/dwxgmac2_dma.c | 5 ++++-
> > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 6 +++++
> > include/linux/stmmac.h | 1 +
> > 7 files changed, 39 insertions(+), 3 deletions(-)
> >
>
>
> --
> Florian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net] ipv4: Revert removal of rt_uses_gateway
From: David Ahern @ 2019-09-21 1:32 UTC (permalink / raw)
To: Jakub Kicinski, David Ahern; +Cc: davem, netdev, ja
In-Reply-To: <20190920183041.7c57b973@cakuba.netronome.com>
On 9/20/19 7:30 PM, Jakub Kicinski wrote:
> I'm assuming the mix of u8 and __u8 is intentional, since this is a partial revert :)
original patch for rt_uses_gateway used __u8 so yes put that back.
not sure why I used u8 for the new field, but left it on the move.
^ permalink raw reply
* Re: [PATCH net] ipv4: Revert removal of rt_uses_gateway
From: Jakub Kicinski @ 2019-09-21 1:30 UTC (permalink / raw)
To: David Ahern; +Cc: davem, netdev, ja, David Ahern
In-Reply-To: <20190917173949.19982-1-dsahern@kernel.org>
On Tue, 17 Sep 2019 10:39:49 -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Julian noted that rt_uses_gateway has a more subtle use than 'is gateway
> set':
> https://lore.kernel.org/netdev/alpine.LFD.2.21.1909151104060.2546@ja.home.ssi.bg/
>
> Revert that part of the commit referenced in the Fixes tag.
>
> Currently, there are no u8 holes in 'struct rtable'. There is a 4-byte hole
> in the second cacheline which contains the gateway declaration. So move
> rt_gw_family down to the gateway declarations since they are always used
> together, and then re-use that u8 for rt_uses_gateway. End result is that
> rtable size is unchanged.
>
> Fixes: 1550c171935d ("ipv4: Prepare rtable for IPv6 gateway")
> Reported-by: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: David Ahern <dsahern@gmail.com>
I'm assuming the mix of u8 and __u8 is intentional, since this is a partial revert :)
Applied, queued, thanks!
^ permalink raw reply
* Re: [PATCH] selftests: Update fib_nexthop_multiprefix to handle missing ping6
From: Jakub Kicinski @ 2019-09-21 1:22 UTC (permalink / raw)
To: David Ahern; +Cc: davem, netdev, David Ahern
In-Reply-To: <20190917173035.19753-1-dsahern@kernel.org>
On Tue, 17 Sep 2019 10:30:35 -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Some distributions (e.g., debian buster) do not install ping6. Re-use
> the hook in pmtu.sh to detect this and fallback to ping.
>
> Fixes: 735ab2f65dce ("selftests: Add test with multiple prefixes using single nexthop")
> Signed-off-by: David Ahern <dsahern@gmail.com>
Applied, queued, thanks!
^ permalink raw reply
* Re: [PATCH net] selftests: Update fib_tests to handle missing ping6
From: Jakub Kicinski @ 2019-09-21 1:17 UTC (permalink / raw)
To: David Ahern; +Cc: davem, netdev, David Ahern
In-Reply-To: <20190917173021.19705-1-dsahern@kernel.org>
On Tue, 17 Sep 2019 10:30:21 -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Some distributions (e.g., debian buster) do not install ping6. Re-use
> the hook in pmtu.sh to detect this and fallback to ping.
>
> Fixes: a0e11da78f48 ("fib_tests: Add tests for metrics on routes")
> Signed-off-by: David Ahern <dsahern@gmail.com>
Applied, queued, thanks!
^ permalink raw reply
* Re: [PATCH net] net/sched: act_sample: don't push mac header on ip6gre ingress
From: Jakub Kicinski @ 2019-09-21 1:12 UTC (permalink / raw)
To: Davide Caratti
Cc: Jiri Pirko, Cong Wang, Jamal Hadi Salim, David S. Miller, netdev,
Yotam Gigi
In-Reply-To: <c359067b4a84342ff24c6a3d089171de68489fcd.1568709449.git.dcaratti@redhat.com>
On Tue, 17 Sep 2019 11:30:55 +0200, Davide Caratti wrote:
> current 'sample' action doesn't push the mac header of ingress packets if
> they are received by a layer 3 tunnel (like gre or sit); but it forgot to
> check for gre over ipv6, so the following script:
>
> # tc q a dev $d clsact
> # tc f a dev $d ingress protocol ip flower ip_proto icmp action sample \
> > group 100 rate 1
> # psample -v -g 100
>
> dumps everything, including outer header and mac, when $d is a gre tunnel
> over ipv6. Fix this adding a missing label for ARPHRD_IP6GRE devices.
>
> Fixes: 5c5670fae430 ("net/sched: Introduce sample tc action")
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
Applied, queued for stable, thank you!
^ permalink raw reply
* Re: [PATCH] dt-bindings: net: dwmac: fix 'mac-mode' type
From: Jakub Kicinski @ 2019-09-21 1:11 UTC (permalink / raw)
To: Alexandru Ardelean
Cc: netdev, devicetree, linux-kernel, davem, robh+dt, peppe.cavallaro,
alexandre.torgue, andrew, f.fainelli
In-Reply-To: <20190917103052.13456-1-alexandru.ardelean@analog.com>
On Tue, 17 Sep 2019 13:30:52 +0300, Alexandru Ardelean wrote:
> The 'mac-mode' property is similar to 'phy-mode' and 'phy-connection-type',
> which are enums of mode strings.
>
> The 'dwmac' driver supports almost all modes declared in the 'phy-mode'
> enum (except for 1 or 2). But in general, there may be a case where
> 'mac-mode' becomes more generic and is moved as part of phylib or netdev.
>
> In any case, the 'mac-mode' field should be made an enum, and it also makes
> sense to just reference the 'phy-connection-type' from
> 'ethernet-controller.yaml'. That will also make it more future-proof for new
> modes.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied, thank you!
FWIW I had to add the Fixes tag by hand, either ozlabs patchwork or my
git-pw doesn't have the automagic handling there, yet.
^ permalink raw reply
* Re: [PATCH net] net/rds: Check laddr_check before calling it
From: Jakub Kicinski @ 2019-09-21 1:09 UTC (permalink / raw)
To: Ka-Cheong Poon; +Cc: netdev, santosh.shilimkar, davem, rds-devel
In-Reply-To: <1568734158-18021-1-git-send-email-ka-cheong.poon@oracle.com>
On Tue, 17 Sep 2019 08:29:18 -0700, Ka-Cheong Poon wrote:
> In rds_bind(), laddr_check is called without checking if it is NULL or
> not. And rs_transport should be reset if rds_add_bound() fails.
>
> Reported-by: syzbot+fae39afd2101a17ec624@syzkaller.appspotmail.com
> Signed-off-by: Ka-Cheong Poon <ka-cheong.poon@oracle.com>
Looks good, but could you please provide a fixes tag?
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Daniel Borkmann @ 2019-09-21 0:06 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vlad Buslov, David Miller, Paul Blakey, netdev@vger.kernel.org,
Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar,
Simon Horman, Alexei Starovoitov, Or Gerlitz
In-Reply-To: <20190920155605.7c81c2af@cakuba.netronome.com>
On 9/21/19 12:56 AM, Jakub Kicinski wrote:
[...]
>> I thought idea of stuffing things into skb extensions are only justified if
>> it's not enabled by default for everyone. :(
>>
>> [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
>
> The skb ext allocation is only done with GOTO_CHAIN, which AFAIK only
> has practical use for offload. We could perhaps add another static
> branch there or move the OvS static branch out of the OvS module so
> there are no linking issues?
>
> I personally have little sympathy for this piece of code, it is perhaps
> the purest form of a wobbly narrow-use construct pushed into TC for HW
> offload.
>
> Any suggestions on the way forward? :(
Presumably there are no clean solutions here, but on the top of my head for
this use case, you'd need to /own/ the underlying datapath anyway, so couldn't
you program the OVS key->recirc_id based on skb->mark (or alternatively via
skb->tc_index) which was previously set by tc ingress?
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH net v3 00/11] net: fix nested device bugs
From: Jakub Kicinski @ 2019-09-20 23:59 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, netdev, j.vosburgh, vfalico, andy, jiri, sd, roopa, saeedm,
manishc, rahulv, kys, haiyangz, stephen, sashal, hare, varun,
ubraun, kgraul, jay.vosburgh
In-Reply-To: <20190916134802.8252-1-ap420073@gmail.com>
On Mon, 16 Sep 2019 22:47:51 +0900, Taehee Yoo wrote:
> This patchset fixes several bugs that are related to nesting
> device infrastructure.
> Current nesting infrastructure code doesn't limit the depth level of
> devices. nested devices could be handled recursively. at that moment,
> it needs huge memory and stack overflow could occur.
> Below devices type have same bug.
> VLAN, BONDING, TEAM, MACSEC, MACVLAN and VXLAN.
Is this list exhaustive? Looks like qmi_wwan.c perhaps could also have
similar problem? Or virt_wifi? Perhaps worth CCing the authors of those
drivers and mentioning that?
Thank you for working on this!
^ permalink raw reply
* Re: [PATCH net v3 09/11] net: core: add ignore flag to netdev_adjacent structure
From: Jakub Kicinski @ 2019-09-20 23:55 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, netdev, j.vosburgh, vfalico, andy, jiri, sd, roopa, saeedm,
manishc, rahulv, kys, haiyangz, stephen, sashal, hare, varun,
ubraun, kgraul, jay.vosburgh
In-Reply-To: <20190916134802.8252-10-ap420073@gmail.com>
On Mon, 16 Sep 2019 22:48:00 +0900, Taehee Yoo wrote:
> In order to link an adjacent node, netdev_upper_dev_link() is used
> and in order to unlink an adjacent node, netdev_upper_dev_unlink() is used.
> unlink operation does not fail, but link operation can fail.
>
> In order to exchange adjacent nodes, we should unlink an old adjacent
> node first. then, link a new adjacent node.
> If link operation is failed, we should link an old adjacent node again.
> But this link operation can fail too.
> It eventually breaks the adjacent link relationship.
>
> This patch adds an ignore flag into the netdev_adjacent structure.
> If this flag is set, netdev_upper_dev_link() ignores an old adjacent
> node for a moment.
> So we can skip unlink operation before link operation.
>
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
Could this perhaps be achieved by creating prepare, commit, and abort
helpers? That would make the API look slightly more canonical.
netdev_adjacent_change_prepare(old, new, dev)
netdev_adjacent_change_commit(old, new, dev)
netdev_adjacent_change_abort(old, new, dev)
The current naming makes the operation a little harder to follow if one
is just reading the vxlan code.
Please let me know if I didn't read the code closely enough to
understand why that's not fitting here.
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 5bb5756129af..4506810c301b 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -4319,6 +4319,10 @@ int netdev_master_upper_dev_link(struct net_device *dev,
> struct netlink_ext_ack *extack);
> void netdev_upper_dev_unlink(struct net_device *dev,
> struct net_device *upper_dev);
> +void netdev_adjacent_dev_disable(struct net_device *upper_dev,
> + struct net_device *lower_dev);
> +void netdev_adjacent_dev_enable(struct net_device *upper_dev,
> + struct net_device *lower_dev);
> void netdev_adjacent_rename_links(struct net_device *dev, char *oldname);
> void *netdev_lower_dev_get_private(struct net_device *dev,
> struct net_device *lower_dev);
^ permalink raw reply
* Re: [PATCH net v3 10/11] vxlan: add adjacent link to limit depth level
From: Jakub Kicinski @ 2019-09-20 23:48 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, netdev, j.vosburgh, vfalico, andy, jiri, sd, roopa, saeedm,
manishc, rahulv, kys, haiyangz, stephen, sashal, hare, varun,
ubraun, kgraul, jay.vosburgh
In-Reply-To: <20190916134802.8252-11-ap420073@gmail.com>
On Mon, 16 Sep 2019 22:48:01 +0900, Taehee Yoo wrote:
> Current vxlan code doesn't limit the number of nested devices.
> Nested devices would be handled recursively and this routine needs
> huge stack memory. So, unlimited nested devices could make
> stack overflow.
>
> In order to fix this issue, this patch adds adjacent links.
> The adjacent link APIs internally check the depth level.
> Fixes: acaf4e70997f ("net: vxlan: when lower dev unregisters remove vxlan dev as well")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
Minor nit picks here, I hope you don't mind.
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 3d9bcc957f7d..0d5c8d22d8a4 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -3567,6 +3567,8 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> struct vxlan_net *vn = net_generic(net, vxlan_net_id);
> struct vxlan_dev *vxlan = netdev_priv(dev);
> struct vxlan_fdb *f = NULL;
> + struct net_device *remote_dev = NULL;
> + struct vxlan_rdst *dst = &vxlan->default_dst;
Especially in places where reverse christmas tree variable ordering is
adhered to, could you please preserve it? That'd mean:
struct vxlan_net *vn = net_generic(net, vxlan_net_id);
struct vxlan_dev *vxlan = netdev_priv(dev);
struct net_device *remote_dev = NULL;
struct vxlan_fdb *f = NULL;
bool unregister = false;
struct vxlan_rdst *dst;
int err;
dst = &vxlan->default_dst;
here.
> bool unregister = false;
> int err;
>
> @@ -3577,14 +3579,14 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> dev->ethtool_ops = &vxlan_ethtool_ops;
>
> /* create an fdb entry for a valid default destination */
> - if (!vxlan_addr_any(&vxlan->default_dst.remote_ip)) {
> + if (!vxlan_addr_any(&dst->remote_ip)) {
> err = vxlan_fdb_create(vxlan, all_zeros_mac,
> - &vxlan->default_dst.remote_ip,
> + &dst->remote_ip,
> NUD_REACHABLE | NUD_PERMANENT,
> vxlan->cfg.dst_port,
> - vxlan->default_dst.remote_vni,
> - vxlan->default_dst.remote_vni,
> - vxlan->default_dst.remote_ifindex,
> + dst->remote_vni,
> + dst->remote_vni,
> + dst->remote_ifindex,
> NTF_SELF, &f);
> if (err)
> return err;
> @@ -3595,26 +3597,43 @@ static int __vxlan_dev_create(struct net *net, struct net_device *dev,
> goto errout;
> unregister = true;
>
> + if (dst->remote_ifindex) {
> + remote_dev = __dev_get_by_index(net, dst->remote_ifindex);
> + if (!remote_dev)
> + goto errout;
> +
> + err = netdev_upper_dev_link(remote_dev, dev, extack);
> + if (err)
> + goto errout;
> + }
> +
> err = rtnl_configure_link(dev, NULL);
> if (err)
> - goto errout;
> + goto unlink;
>
> if (f) {
> - vxlan_fdb_insert(vxlan, all_zeros_mac,
> - vxlan->default_dst.remote_vni, f);
> + vxlan_fdb_insert(vxlan, all_zeros_mac, dst->remote_vni, f);
>
> /* notify default fdb entry */
> err = vxlan_fdb_notify(vxlan, f, first_remote_rtnl(f),
> RTM_NEWNEIGH, true, extack);
> if (err) {
> vxlan_fdb_destroy(vxlan, f, false, false);
> + if (remote_dev)
> + netdev_upper_dev_unlink(remote_dev, dev);
> goto unregister;
> }
> }
>
> list_add(&vxlan->next, &vn->vxlan_list);
> + if (remote_dev) {
> + dst->remote_dev = remote_dev;
> + dev_hold(remote_dev);
> + }
> return 0;
> -
> +unlink:
> + if (remote_dev)
> + netdev_upper_dev_unlink(remote_dev, dev);
> errout:
> /* unregister_netdevice() destroys the default FDB entry with deletion
> * notification. But the addition notification was not sent yet, so
> @@ -3936,6 +3955,8 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> struct net_device *lowerdev;
> struct vxlan_config conf;
> int err;
> + bool linked = false;
> + bool disabled = false;
Same here.
> err = vxlan_nl2conf(tb, data, dev, &conf, true, extack);
> if (err)
> @@ -3946,6 +3967,16 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> if (err)
> return err;
>
> + if (lowerdev) {
> + if (dst->remote_dev && lowerdev != dst->remote_dev) {
> + netdev_adjacent_dev_disable(dst->remote_dev, dev);
> + disabled = true;
> + }
> + err = netdev_upper_dev_link(lowerdev, dev, extack);
> + if (err)
> + goto err;
would you mind naming the label errout? there is an err variable, and
other places in this file use errout
> + linked = true;
> + }
> /* handle default dst entry */
> if (!vxlan_addr_equal(&conf.remote_ip, &dst->remote_ip)) {
> u32 hash_index = fdb_head_index(vxlan, all_zeros_mac, conf.vni);
> @@ -3962,7 +3993,7 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> NTF_SELF, true, extack);
> if (err) {
> spin_unlock_bh(&vxlan->hash_lock[hash_index]);
> - return err;
> + goto err;
> }
> }
> if (!vxlan_addr_any(&dst->remote_ip))
> @@ -3979,8 +4010,24 @@ static int vxlan_changelink(struct net_device *dev, struct nlattr *tb[],
> if (conf.age_interval != vxlan->cfg.age_interval)
> mod_timer(&vxlan->age_timer, jiffies);
>
> + if (disabled) {
> + netdev_adjacent_dev_enable(dst->remote_dev, dev);
> + netdev_upper_dev_unlink(dst->remote_dev, dev);
> + dev_put(dst->remote_dev);
> + }
> + if (linked) {
> + dst->remote_dev = lowerdev;
> + dev_hold(dst->remote_dev);
> + }
> +
> vxlan_config_apply(dev, &conf, lowerdev, vxlan->net, true);
> return 0;
> +err:
> + if (linked)
> + netdev_upper_dev_unlink(lowerdev, dev);
> + if (disabled)
> + netdev_adjacent_dev_enable(dst->remote_dev, dev);
> + return err;
> }
>
> static void vxlan_dellink(struct net_device *dev, struct list_head *head)
> @@ -3991,6 +4038,10 @@ static void vxlan_dellink(struct net_device *dev, struct list_head *head)
>
> list_del(&vxlan->next);
> unregister_netdevice_queue(dev, head);
> + if (vxlan->default_dst.remote_dev) {
> + netdev_upper_dev_unlink(vxlan->default_dst.remote_dev, dev);
> + dev_put(vxlan->default_dst.remote_dev);
> + }
> }
>
> static size_t vxlan_get_size(const struct net_device *dev)
> diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> index dc1583a1fb8a..08e237d7aa73 100644
> --- a/include/net/vxlan.h
> +++ b/include/net/vxlan.h
> @@ -197,6 +197,7 @@ struct vxlan_rdst {
> u8 offloaded:1;
> __be32 remote_vni;
> u32 remote_ifindex;
> + struct net_device *remote_dev;
> struct list_head list;
> struct rcu_head rcu;
> struct dst_cache dst_cache;
^ permalink raw reply
* Verify ACK packets in handshake in kernel module (Access TCP state table)
From: Swarm @ 2019-09-20 23:43 UTC (permalink / raw)
To: netdev
First time emailing to this mailing list so please let me know if I made a mistake in how I sent it. I'm trying to receive a notification from the kernel once it verifies an ACK packet in a handshake. Problem is, there is no API or kernel resource I've seen that supports this feature for both syncookies and normal handshakes. Where exactly in the kernel does the ACK get verified? If there isn't a way to be notified of it, where should I start adding that feature into the kernel?
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Daniel Borkmann @ 2019-09-20 23:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Vlad Buslov, David Miller, Paul Blakey, netdev@vger.kernel.org,
Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar,
Simon Horman, Alexei Starovoitov, Or Gerlitz
In-Reply-To: <20190920155605.7c81c2af@cakuba.netronome.com>
On 9/21/19 12:56 AM, Jakub Kicinski wrote:
> On Sat, 21 Sep 2019 00:15:16 +0200, Daniel Borkmann wrote:
>> On 9/20/19 6:16 PM, Jakub Kicinski wrote:
>>> On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
>>>> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
>>>>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
>>>>> is really not acceptable.
>>>>>
>>>>> It should not be enabled by default at all.
>>>>>
>>>>> Instead the actual users should turn it on or depend upon it, which in
>>>>> this case seems to be OVS.
>>>>>
>>>>> Please fix this, thank you.
>>>>
>>>> Hi David,
>>>>
>>>> We are working on it, but Paul is OoO today. Is it okay if we send the
>>>> fix early next week?
>>>
>>> Doesn't really seem like we have too many ways forward here, right?
>>>
>>> How about this?
>>>
>>> ------>8-----------------------------------
>>>
>>> net: hide NET_TC_SKB_EXT as a config option
>>>
>>> Linus points out the NET_TC_SKB_EXT config option looks suspicious.
>>> Indeed, it should really be selected to ensure correct OvS operation
>>> if TC offload is used. Hopefully those who care about TC-only and
>>> OvS-only performance disable the other one at compilation time.
>>>
>>> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> ---
>>> net/openvswitch/Kconfig | 1 +
>>> net/sched/Kconfig | 13 +++----------
>>> 2 files changed, 4 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
>>> index 22d7d5604b4c..bd407ea7c263 100644
>>> --- a/net/openvswitch/Kconfig
>>> +++ b/net/openvswitch/Kconfig
>>> @@ -15,6 +15,7 @@ config OPENVSWITCH
>>> select NET_MPLS_GSO
>>> select DST_CACHE
>>> select NET_NSH
>>> + select NET_TC_SKB_EXT if NET_CLS_ACT
>>> ---help---
>>> Open vSwitch is a multilayer Ethernet switch targeted at virtualized
>>> environments. In addition to supporting a variety of features
>>> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
>>> index b3faafeafab9..f1062ef55098 100644
>>> --- a/net/sched/Kconfig
>>> +++ b/net/sched/Kconfig
>>> @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
>>> config NET_CLS_ACT
>>> bool "Actions"
>>> select NET_CLS
>>> + select NET_TC_SKB_EXT if OPENVSWITCH
>>
>> But how would that make much of a difference :( Distros are still going to
>> enable all of this blindlessly. Given discussion in [0], could we just get
>> rid of this tasteless hack altogether which is for such a narrow use case
>> anyway?
>
> Agreed. I take it you're opposed to the use of skb extensions here
> in general? Distros would have enabled NET_TC_SKB_EXT even when it
> was a config option.
Yeah, as it stands, motivation for this extension is to tie tc [HW] offload
and OVS to work together in case of recirculation ... from commit:
Received packets will first travel though tc, and if they aren't stolen
by it, like in the above rule, they will continue to OvS datapath.
Since we already did some actions (action ct in this case) which might
modify the packets, and updated action stats, we would like to continue
the proccessing with the correct recirc_id in OvS (here recirc_id(2))
where we left off.
I would perhaps see more of a point in an skb extension if there is rather
generic use and also out of the SW datapath (and I really doubt anyone is
seriously using tc + OVS combo for non-HW workloads). Right now this feels
like duck taping.
Adding new skb extensions should really have a strong justification behind
it (close but slightly less strict to how we treat adding new members to
skb itself).
^ permalink raw reply
* [PATCH bpf] selftests/bpf: test_progs: fix client/server race in tcp_rtt
From: Stanislav Fomichev @ 2019-09-20 23:30 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
This is the same problem I found earlier in test_sockopt_inherit:
there is a race between server thread doing accept() and client
thread doing connect(). Let's explicitly synchronize them via
pthread conditional variable.
Fixes: b55873984dab ("selftests/bpf: test BPF_SOCK_OPS_RTT_CB")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/prog_tests/tcp_rtt.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
index fdc0b3614a9e..e64058906bcd 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
@@ -203,6 +203,9 @@ static int start_server(void)
return fd;
}
+static pthread_mutex_t server_started_mtx = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t server_started = PTHREAD_COND_INITIALIZER;
+
static void *server_thread(void *arg)
{
struct sockaddr_storage addr;
@@ -215,6 +218,10 @@ static void *server_thread(void *arg)
return NULL;
}
+ pthread_mutex_lock(&server_started_mtx);
+ pthread_cond_signal(&server_started);
+ pthread_mutex_unlock(&server_started_mtx);
+
client_fd = accept(fd, (struct sockaddr *)&addr, &len);
if (CHECK_FAIL(client_fd < 0)) {
perror("Failed to accept client");
@@ -248,7 +255,14 @@ void test_tcp_rtt(void)
if (CHECK_FAIL(server_fd < 0))
goto close_cgroup_fd;
- pthread_create(&tid, NULL, server_thread, (void *)&server_fd);
+ if (CHECK_FAIL(pthread_create(&tid, NULL, server_thread,
+ (void *)&server_fd)))
+ goto close_cgroup_fd;
+
+ pthread_mutex_lock(&server_started_mtx);
+ pthread_cond_wait(&server_started, &server_started_mtx);
+ pthread_mutex_unlock(&server_started_mtx);
+
CHECK_FAIL(run_test(cgroup_fd, server_fd));
close(server_fd);
close_cgroup_fd:
--
2.23.0.351.gc4317032e6-goog
^ permalink raw reply related
* Re: [PATCH net v3 09/11] net: core: add ignore flag to netdev_adjacent structure
From: Jakub Kicinski @ 2019-09-20 23:24 UTC (permalink / raw)
To: Taehee Yoo
Cc: davem, netdev, j.vosburgh, vfalico, andy, jiri, sd, roopa, saeedm,
manishc, rahulv, kys, haiyangz, stephen, sashal, hare, varun,
ubraun, kgraul, jay.vosburgh
In-Reply-To: <20190916134802.8252-10-ap420073@gmail.com>
On Mon, 16 Sep 2019 22:48:00 +0900, Taehee Yoo wrote:
> In order to link an adjacent node, netdev_upper_dev_link() is used
> and in order to unlink an adjacent node, netdev_upper_dev_unlink() is used.
> unlink operation does not fail, but link operation can fail.
>
> In order to exchange adjacent nodes, we should unlink an old adjacent
> node first. then, link a new adjacent node.
> If link operation is failed, we should link an old adjacent node again.
> But this link operation can fail too.
> It eventually breaks the adjacent link relationship.
>
> This patch adds an ignore flag into the netdev_adjacent structure.
> If this flag is set, netdev_upper_dev_link() ignores an old adjacent
> node for a moment.
> So we can skip unlink operation before link operation.
>
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
I think there's a number of local functions here which are missing the
static keyword, we'll get a flurry of fixes as soon as we apply this.
Could you please take a look at fixing that, I think W=1 should help,
-Wmissing-prototypes in particular, if I'm remembering right.
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Jakub Kicinski @ 2019-09-20 22:56 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Vlad Buslov, David Miller, Paul Blakey, netdev@vger.kernel.org,
Jiri Pirko, Cong Wang, Jamal Hadi Salim, Pravin Shelar,
Simon Horman, Alexei Starovoitov, Or Gerlitz
In-Reply-To: <0e9a1701-356f-5f94-b88e-a39175dee77a@iogearbox.net>
On Sat, 21 Sep 2019 00:15:16 +0200, Daniel Borkmann wrote:
> On 9/20/19 6:16 PM, Jakub Kicinski wrote:
> > On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
> >> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
> >>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
> >>> is really not acceptable.
> >>>
> >>> It should not be enabled by default at all.
> >>>
> >>> Instead the actual users should turn it on or depend upon it, which in
> >>> this case seems to be OVS.
> >>>
> >>> Please fix this, thank you.
> >>
> >> Hi David,
> >>
> >> We are working on it, but Paul is OoO today. Is it okay if we send the
> >> fix early next week?
> >
> > Doesn't really seem like we have too many ways forward here, right?
> >
> > How about this?
> >
> > ------>8-----------------------------------
> >
> > net: hide NET_TC_SKB_EXT as a config option
> >
> > Linus points out the NET_TC_SKB_EXT config option looks suspicious.
> > Indeed, it should really be selected to ensure correct OvS operation
> > if TC offload is used. Hopefully those who care about TC-only and
> > OvS-only performance disable the other one at compilation time.
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> > net/openvswitch/Kconfig | 1 +
> > net/sched/Kconfig | 13 +++----------
> > 2 files changed, 4 insertions(+), 10 deletions(-)
> >
> > diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> > index 22d7d5604b4c..bd407ea7c263 100644
> > --- a/net/openvswitch/Kconfig
> > +++ b/net/openvswitch/Kconfig
> > @@ -15,6 +15,7 @@ config OPENVSWITCH
> > select NET_MPLS_GSO
> > select DST_CACHE
> > select NET_NSH
> > + select NET_TC_SKB_EXT if NET_CLS_ACT
> > ---help---
> > Open vSwitch is a multilayer Ethernet switch targeted at virtualized
> > environments. In addition to supporting a variety of features
> > diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> > index b3faafeafab9..f1062ef55098 100644
> > --- a/net/sched/Kconfig
> > +++ b/net/sched/Kconfig
> > @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
> > config NET_CLS_ACT
> > bool "Actions"
> > select NET_CLS
> > + select NET_TC_SKB_EXT if OPENVSWITCH
>
> But how would that make much of a difference :( Distros are still going to
> enable all of this blindlessly. Given discussion in [0], could we just get
> rid of this tasteless hack altogether which is for such a narrow use case
> anyway?
Agreed. I take it you're opposed to the use of skb extensions here
in general? Distros would have enabled NET_TC_SKB_EXT even when it
was a config option.
> I thought idea of stuffing things into skb extensions are only justified if
> it's not enabled by default for everyone. :(
>
> [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
The skb ext allocation is only done with GOTO_CHAIN, which AFAIK only
has practical use for offload. We could perhaps add another static
branch there or move the OvS static branch out of the OvS module so
there are no linking issues?
I personally have little sympathy for this piece of code, it is perhaps
the purest form of a wobbly narrow-use construct pushed into TC for HW
offload.
Any suggestions on the way forward? :(
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Daniel Borkmann @ 2019-09-20 22:15 UTC (permalink / raw)
To: Jakub Kicinski, Vlad Buslov
Cc: David Miller, Paul Blakey, netdev@vger.kernel.org, Jiri Pirko,
Cong Wang, Jamal Hadi Salim, Pravin Shelar, Simon Horman,
Alexei Starovoitov, Or Gerlitz
In-Reply-To: <20190920091647.0129e65f@cakuba.netronome.com>
On 9/20/19 6:16 PM, Jakub Kicinski wrote:
> On Thu, 19 Sep 2019 15:13:55 +0000, Vlad Buslov wrote:
>> On Thu 19 Sep 2019 at 14:21, David Miller <davem@davemloft.net> wrote:
>>> As Linus pointed out, the Kconfig logic for CONFIG_NET_TC_SKB_EXT
>>> is really not acceptable.
>>>
>>> It should not be enabled by default at all.
>>>
>>> Instead the actual users should turn it on or depend upon it, which in
>>> this case seems to be OVS.
>>>
>>> Please fix this, thank you.
>>
>> Hi David,
>>
>> We are working on it, but Paul is OoO today. Is it okay if we send the
>> fix early next week?
>
> Doesn't really seem like we have too many ways forward here, right?
>
> How about this?
>
> ------>8-----------------------------------
>
> net: hide NET_TC_SKB_EXT as a config option
>
> Linus points out the NET_TC_SKB_EXT config option looks suspicious.
> Indeed, it should really be selected to ensure correct OvS operation
> if TC offload is used. Hopefully those who care about TC-only and
> OvS-only performance disable the other one at compilation time.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
> net/openvswitch/Kconfig | 1 +
> net/sched/Kconfig | 13 +++----------
> 2 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig
> index 22d7d5604b4c..bd407ea7c263 100644
> --- a/net/openvswitch/Kconfig
> +++ b/net/openvswitch/Kconfig
> @@ -15,6 +15,7 @@ config OPENVSWITCH
> select NET_MPLS_GSO
> select DST_CACHE
> select NET_NSH
> + select NET_TC_SKB_EXT if NET_CLS_ACT
> ---help---
> Open vSwitch is a multilayer Ethernet switch targeted at virtualized
> environments. In addition to supporting a variety of features
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index b3faafeafab9..f1062ef55098 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -719,6 +719,7 @@ config NET_EMATCH_IPT
> config NET_CLS_ACT
> bool "Actions"
> select NET_CLS
> + select NET_TC_SKB_EXT if OPENVSWITCH
But how would that make much of a difference :( Distros are still going to
enable all of this blindlessly. Given discussion in [0], could we just get
rid of this tasteless hack altogether which is for such a narrow use case
anyway?
I thought idea of stuffing things into skb extensions are only justified if
it's not enabled by default for everyone. :(
[0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
> ---help---
> Say Y here if you want to use traffic control actions. Actions
> get attached to classifiers and are invoked after a successful
> @@ -964,18 +965,10 @@ config NET_IFE_SKBTCINDEX
> depends on NET_ACT_IFE
>
> config NET_TC_SKB_EXT
> - bool "TC recirculation support"
> - depends on NET_CLS_ACT
> - default y if NET_CLS_ACT
> + bool
> + depends on NET_CLS_ACT && OPENVSWITCH
> select SKB_EXTENSIONS
>
> - help
> - Say Y here to allow tc chain misses to continue in OvS datapath in
> - the correct recirc_id, and hardware chain misses to continue in
> - the correct chain in tc software datapath.
> -
> - Say N here if you won't be using tc<->ovs offload or tc chains offload.
> -
> endif # NET_SCHED
>
> config NET_SCH_FIFO
>
^ permalink raw reply
* Re: [PATCH bpf] libbpf: fix version identification on busybox
From: Ivan Khoronzhuk @ 2019-09-20 22:14 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko,
Yonghong Song, Networking, bpf, open list
In-Reply-To: <CAEf4BzZGeY-WD17mq6FTd7Rae_f26j4kBAWCmuppeu4VjZxvUg@mail.gmail.com>
On Fri, Sep 20, 2019 at 02:51:14PM -0700, Andrii Nakryiko wrote:
>On Fri, Sep 20, 2019 at 12:19 PM Ivan Khoronzhuk
><ivan.khoronzhuk@linaro.org> wrote:
>>
>> On Fri, Sep 20, 2019 at 09:34:51PM +0300, Ivan Khoronzhuk wrote:
>> >On Fri, Sep 20, 2019 at 09:41:54AM -0700, Andrii Nakryiko wrote:
>> >>On Fri, Sep 20, 2019 at 1:22 AM Ivan Khoronzhuk
>> >><ivan.khoronzhuk@linaro.org> wrote:
>> >>>
>> >>>On Thu, Sep 19, 2019 at 01:02:40PM -0700, Andrii Nakryiko wrote:
>> >>>>On Thu, Sep 19, 2019 at 11:22 AM Ivan Khoronzhuk
>> >>>><ivan.khoronzhuk@linaro.org> wrote:
>> >>>>>
>> >>>>> It's very often for embedded to have stripped version of sort in
>> >>>>> busybox, when no -V option present. It breaks build natively on target
>> >>>>> board causing recursive loop.
>> >>>>>
>> >>>>> BusyBox v1.24.1 (2019-04-06 04:09:16 UTC) multi-call binary. \
>> >>>>> Usage: sort [-nrugMcszbdfimSTokt] [-o FILE] [-k \
>> >>>>> start[.offset][opts][,end[.offset][opts]] [-t CHAR] [FILE]...
>> >>>>>
>> >>>>> Lets modify command a little to avoid -V option.
>> >>>>>
>> >>>>> Fixes: dadb81d0afe732 ("libbpf: make libbpf.map source of truth for libbpf version")
>> >>>>>
>> >>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >>>>> ---
>> >>>>>
>> >>>>> Based on bpf/master
>> >>>>>
>> >>>>> tools/lib/bpf/Makefile | 2 +-
>> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>>>
>> >>>>> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
>> >>>>> index c6f94cffe06e..a12490ad6215 100644
>> >>>>> --- a/tools/lib/bpf/Makefile
>> >>>>> +++ b/tools/lib/bpf/Makefile
>> >>>>> @@ -3,7 +3,7 @@
>> >>>>>
>> >>>>> LIBBPF_VERSION := $(shell \
>> >>>>> grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
>> >>>>> - sort -rV | head -n1 | cut -d'_' -f2)
>> >>>>> + cut -d'_' -f2 | sort -r | head -n1)
>> >>>>
>> >>>>You can't just sort alphabetically, because:
>> >>>>
>> >>>>1.2
>> >>>>1.11
>> >>>>
>> >>>>should be in that order. See discussion on mailing thread for original commit.
>> >>>
>> >>>if X1.X2.X3, where X = {0,1,....99999}
>> >>>Then it can be:
>> >>>
>> >>>-LIBBPF_VERSION := $(shell \
>> >>>- grep -oE '^LIBBPF_([0-9.]+)' libbpf.map | \
>> >>>- sort -rV | head -n1 | cut -d'_' -f2)
>> >>>+_LBPFLIST := $(patsubst %;,%,$(patsubst LIBBPF_%,%,$(filter LIBBPF_%, \
>> >>>+ $(shell cat libbpf.map))))
>> >>>+_LBPFLIST2 := $(foreach v,$(_LBPFLIST), \
>> >>>+ $(subst $() $(),,$(foreach n,$(subst .,$() $(),$(v)), \
>> >>>+ $(shell printf "%05d" $(n)))))
>> >>>+_LBPF_VER := $(word $(words $(sort $(_LBPFLIST2))), $(sort $(_LBPFLIST2)))
>> >>>+LIBBPF_VERSION := $(patsubst %_$(_LBPF_VER),%,$(filter %_$(_LBPF_VER), \
>> >>>+ $(join $(addsuffix _, $(_LBPFLIST)),$(_LBPFLIST2))))
>> >>>
>> >>>It's bigger but avoids invocations of grep/sort/cut/head, only cat/printf
>> >>>, thus -V option also.
>> >>>
>> >>
>> >>No way, this is way too ugly (and still unreliable, if we ever have
>> >>X.Y.Z.W or something). I'd rather go with my original approach of
>> >Yes, forgot to add
>> >X1,X2,X3,...XN, where X = {0,1,....99999} and N = const for all versions.
>> >But frankly, 1.0.0 looks too far.
>>
>> It actually works for any numbs of X1.X2...X100
>> but not when you have couple kindof:
>> X1.X2.X3
>> and
>> X1.X2.X3.X4
>>
>> But, no absolutely any problem to extend this solution to handle all cases,
>> by just adding leading 0 to every "transformed version", say limit it to 10
>> possible 'dots' (%5*10d) and it will work as clocks. Advantage - mostly make
>> functions.
>>
>> Here can be couple more solutions with sed, not sure it can look less maniac.
>>
>> >
>> >>fetching the last version in libbpf.map file. See
>> >>https://www.spinics.net/lists/netdev/msg592703.html.
>>
>> Yes it's nice but, no sort, no X1.X2.X3....XN
>>
>> Main is to solve it for a long time.
>
>Thinking a bit more about this, I'm even more convinced that we should
>just go with my original approach: find last section in libbpf.map and
>extract LIBBPF version from that. That will handle whatever crazy
>version format we might decide to use (e.g., 1.2.3-experimental).
>We'll just need to make sure that latest version is the last in
>libbpf.map, which will just happen naturally. So instead of this
>Makefile complexity, please can you port back my original approach?
>Thanks!
I don't insist, placed it for history and to show it can be sorted
alphabetically, I can live with cross-compilation that I hope goes soon,
on host no need to worry about this at all. So I better leave this change
up to you.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox