* [BUG] [FIX] net: dsa: oops in br_vlan_enabled
From: Frank Wunderlich @ 2019-02-16 16:22 UTC (permalink / raw)
To: netdev
Hi,
i've found an oops in 4.19.23/10, seems to be fixed anyhow in 5.0 (also works in 4.14.101)
root@bpi-r2:~# ip link add link lan0 name lan0.5 type vlan id 5
root@bpi-r2:~# ip addr add 192.168.5.200/24 brd 192.168.5.255 dev lan0.5
root@bpi-r2:~# ip link set dev lan0 up
root@bpi-r2:~# ip link set dev lan0.5 up
12: lan0.5@lan0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc noqueue state LOWERLAYERDOWN group default qlen 1000
link/ether 02:02:02:02:02:02 brd ff:ff:ff:ff:ff:ff
inet 192.168.5.200/24 brd 192.168.5.255 scope global lan0.5
valid_lft forever preferred_lft forever
root@bpi-r2:~# brctl addbr bridge_name
root@bpi-r2:~# brctl addif bridge_name lan0.5
[ 352.057128] bridge_name: port 1(lan0.5) entered blocking state
[ 352.063065] bridge_name: port 1(lan0.5) entered disabled state
[ 352.069181] device lan0.5 entered promiscuous mode
[ 352.074018] device lan0 entered promiscuous mode
[ 352.078906] Unable to handle kernel NULL pointer dereference at virtual address 00000558
...
[ 352.493085] [<bf0fde88>] (br_vlan_enabled [bridge]) from [<bf12c234>] (dsa_port_vlan_add+0x60/0xbc [dsa_core])
[ 352.503050] [<bf12c234>] (dsa_port_vlan_add [dsa_core]) from [<bf12cb64>] (dsa_slave_port_obj_add+0x4c/0x50 [dsa_core])
[ 352.513776] [<bf12cb64>] (dsa_slave_port_obj_add [dsa_core]) from [<c0b4e2d4>] (__switchdev_port_obj_add+0x50/0xc4)
[ 352.524138] [<c0b4e2d4>] (__switchdev_port_obj_add) from [<c0b4e324>] (__switchdev_port_obj_add+0xa0/0xc4)
[ 352.533721] [<c0b4e324>] (__switchdev_port_obj_add) from [<c0b4e3a8>] (switchdev_port_obj_add_now+0x60/0x130)
[ 352.543562] [<c0b4e3a8>] (switchdev_port_obj_add_now) from [<c0b4e7e4>] (switchdev_port_obj_add+0x44/0x190)
[ 352.553284] [<c0b4e7e4>] (switchdev_port_obj_add) from [<bf1013d0>] (br_switchdev_port_vlan_add+0x60/0x7c [bridge])
[ 352.563733] [<bf1013d0>] (br_switchdev_port_vlan_add [bridge]) from [<bf0ff250>] (__vlan_add+0xb0/0x620 [bridge])
[ 352.574007] [<bf0ff250>] (__vlan_add [bridge]) from [<bf0ffd04>] (nbp_vlan_add+0xc4/0x150 [bridge])
[ 352.583073] [<bf0ffd04>] (nbp_vlan_add [bridge]) from [<bf0ffec4>] (nbp_vlan_init+0x134/0x164 [bridge])
[ 352.592482] [<bf0ffec4>] (nbp_vlan_init [bridge]) from [<bf0edd4c>] (br_add_if+0x40c/0x5fc [bridge])
[ 352.601632] [<bf0edd4c>] (br_add_if [bridge]) from [<bf0eeb14>] (add_del_if+0x6c/0x80 [bridge])
[ 352.610351] [<bf0eeb14>] (add_del_if [bridge]) from [<bf0ef5b0>] (br_dev_ioctl+0x7c/0x9c [bridge])
[ 352.619290] [<bf0ef5b0>] (br_dev_ioctl [bridge]) from [<c09583d4>] (dev_ifsioc+0x184/0x324)
[ 352.627582] [<c09583d4>] (dev_ifsioc) from [<c09589e8>] (dev_ioctl+0x32c/0x5cc)
[ 352.634837] [<c09589e8>] (dev_ioctl) from [<c090913c>] (sock_ioctl+0x3bc/0x580)
since my 4.19.23 kernel is modified a bit i tried with 4.19.10 without my net modifications and it is still reproducable with steps above (create a vlan on dsa-user-port and then use it in a bridge)
i fixed it with these changes:
diff --git a/net/dsa/port.c b/net/dsa/port.c
index ed0595459df1..962887752ae8 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -255,8 +255,9 @@ int dsa_port_vlan_add(struct dsa_port *dp,
if (netif_is_bridge_master(vlan->obj.orig_dev))
return -EOPNOTSUPP;
- if (br_vlan_enabled(dp->bridge_dev))
- return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_ADD, &info);
+ printk(KERN_ALERT "DEBUG: Passed %s %d 0x%x \n",__FUNCTION__,__LINE__,(unsigned int)dp->bridge_dev);
+ if (!dp->bridge_dev || br_vlan_enabled(dp->bridge_dev))
+ return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
return 0;
}
@@ -273,7 +274,7 @@ int dsa_port_vlan_del(struct dsa_port *dp,
if (netif_is_bridge_master(vlan->obj.orig_dev))
return -EOPNOTSUPP;
- if (br_vlan_enabled(dp->bridge_dev))
+ if (!dp->bridge_dev || br_vlan_enabled(dp->bridge_dev))
return dsa_port_notify(dp, DSA_NOTIFIER_VLAN_DEL, &info);
return 0;
i've found in a Patch from florian/vivien: https://www.mail-archive.com/netdev@vger.kernel.org/msg281415.html
Strange that 5.0-rc1 does not crash,because these 2 code-sections are unchanged: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/dsa/port.c#n255 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/bridge/br_vlan.c#n788
maybe you know why only 4.19 is affected...
regards Frank
^ permalink raw reply related
* [PATCH net-next v2 0/2] net: phy: add helpers for handling C45 10GBT AN register values
From: Heiner Kallweit @ 2019-02-16 16:24 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
Similar to the existing helpers for the Clause 22 registers add helpers
to deal with converting Clause 45 advertisement registers to / from
link mode bitmaps.
Note that these helpers are defined in linux/mdio.h, not like the
Clause 22 helpers in linux/mii.h. Reason is that the Clause 45 register
constants are defined in uapi/linux/mdio.h. And uapi/linux/mdio.h
includes linux/mii.h before defining the C45 register constants.
v2:
- Remove few helpers which aren't used by this series. They will
follow together with the users.
Heiner Kallweit (2):
net: phy: add helper mii_10gbt_stat_mod_linkmode_lpa_t
net: phy: use mii_10gbt_stat_mod_linkmode_lpa_t in genphy_c45_read_lpa
drivers/net/phy/phy-c45.c | 10 +---------
include/linux/mdio.h | 19 +++++++++++++++++++
2 files changed, 20 insertions(+), 9 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH net-next v2 1/2] net: phy: add helper mii_10gbt_stat_mod_linkmode_lpa_t
From: Heiner Kallweit @ 2019-02-16 16:26 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <c6be00e5-d4e5-69a1-cb9c-31cd446556be@gmail.com>
Similar to the existing helpers for the Clause 22 registers add helper
mii_10gbt_stat_mod_linkmode_lpa_t.
Note that this helper is defined in linux/mdio.h, not like the
Clause 22 helpers in linux/mii.h. Reason is that the Clause 45 register
constants are defined in uapi/linux/mdio.h. And uapi/linux/mdio.h
includes linux/mii.h before defining the C45 register constants.
v2:
- remove helpers that don't have users in this series
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
include/linux/mdio.h | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index bfa711416..5b872c45f 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -261,6 +261,25 @@ static inline u16 ethtool_adv_to_mmd_eee_adv_t(u32 adv)
return reg;
}
+/**
+ * mii_10gbt_stat_mod_linkmode_lpa_t
+ * @advertising: target the linkmode advertisement settings
+ * @adv: value of the C45 10GBASE-T AN STATUS register
+ *
+ * A small helper function that translates C45 10GBASE-T AN STATUS register bits
+ * to linkmode advertisement settings. Other bits in advertising aren't changed.
+ */
+static inline void mii_10gbt_stat_mod_linkmode_lpa_t(unsigned long *advertising,
+ u32 lpa)
+{
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+ advertising, lpa & MDIO_AN_10GBT_STAT_LP2_5G);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ advertising, lpa & MDIO_AN_10GBT_STAT_LP5G);
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
+ advertising, lpa & MDIO_AN_10GBT_STAT_LP10G);
+}
+
int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum);
int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val);
--
2.20.1
^ permalink raw reply related
* [PATCH net-next v2 2/2] net: phy: use mii_10gbt_stat_mod_linkmode_lpa_t in genphy_c45_read_lpa
From: Heiner Kallweit @ 2019-02-16 16:26 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <c6be00e5-d4e5-69a1-cb9c-31cd446556be@gmail.com>
Use mii_10gbt_stat_mod_linkmode_lpa_t() in genphy_c45_read_lpa() to
simplify the code.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy-c45.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 7af5fa81d..bef126344 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -204,15 +204,7 @@ int genphy_c45_read_lpa(struct phy_device *phydev)
if (val < 0)
return val;
- if (val & MDIO_AN_10GBT_STAT_LP2_5G)
- linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
- phydev->lp_advertising);
- if (val & MDIO_AN_10GBT_STAT_LP5G)
- linkmode_set_bit(ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
- phydev->lp_advertising);
- if (val & MDIO_AN_10GBT_STAT_LP10G)
- linkmode_set_bit(ETHTOOL_LINK_MODE_10000baseT_Full_BIT,
- phydev->lp_advertising);
+ mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, val);
return 0;
}
--
2.20.1
^ permalink raw reply related
* [PATCH v2 net-next 0/2] clean up SOCK_DEBUG()
From: Yafang Shao @ 2019-02-16 16:28 UTC (permalink / raw)
To: davem
Cc: daniel, edumazet, joe, xiyou.wangcong, netdev, shaoyafang,
Yafang Shao
Per discussion with Daniel[1] and Eric[2], these SOCK_DEBUG() calles in
TCP are not needed now.
We'd better clean up it.
Plus undefine SOCK_DEBUGGING by default.
[1] https://patchwork.ozlabs.org/patch/1035573/
[2] https://patchwork.ozlabs.org/patch/1040533/
Yafang Shao (2):
tcp: clean up SOCK_DEBUG()
net: sock: undefine SOCK_DEBUGGING
include/net/sock.h | 13 ++++++++-----
net/core/sock.c | 3 +++
net/ipv4/tcp_input.c | 19 +------------------
net/ipv6/tcp_ipv6.c | 2 --
4 files changed, 12 insertions(+), 25 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH v2 net-next 1/2] tcp: clean up SOCK_DEBUG()
From: Yafang Shao @ 2019-02-16 16:28 UTC (permalink / raw)
To: davem
Cc: daniel, edumazet, joe, xiyou.wangcong, netdev, shaoyafang,
Yafang Shao
In-Reply-To: <1550334537-380-1-git-send-email-laoar.shao@gmail.com>
Per discussion with Daniel[1] and Eric[2], these SOCK_DEBUG() calles in
TCP are not needed now.
We'd better clean up it.
[1] https://patchwork.ozlabs.org/patch/1035573/
[2] https://patchwork.ozlabs.org/patch/1040533/
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
net/ipv4/tcp_input.c | 19 +------------------
net/ipv6/tcp_ipv6.c | 2 --
2 files changed, 1 insertion(+), 20 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7a027dec..6d2750e 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3595,7 +3595,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
* this segment (RFC793 Section 3.9).
*/
if (after(ack, tp->snd_nxt))
- goto invalid_ack;
+ return -1;
if (after(ack, prior_snd_una)) {
flag |= FLAG_SND_UNA_ADVANCED;
@@ -3714,10 +3714,6 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
tcp_process_tlp_ack(sk, ack, flag);
return 1;
-invalid_ack:
- SOCK_DEBUG(sk, "Ack %u after %u:%u\n", ack, tp->snd_una, tp->snd_nxt);
- return -1;
-
old_ack:
/* If data was SACKed, tag it and see if we should send more data.
* If data was DSACKed, see if we can undo a cwnd reduction.
@@ -3731,7 +3727,6 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
tcp_xmit_recovery(sk, rexmit);
}
- SOCK_DEBUG(sk, "Ack %u before %u:%u\n", ack, tp->snd_una, tp->snd_nxt);
return 0;
}
@@ -4432,13 +4427,9 @@ static void tcp_ofo_queue(struct sock *sk)
rb_erase(&skb->rbnode, &tp->out_of_order_queue);
if (unlikely(!after(TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt))) {
- SOCK_DEBUG(sk, "ofo packet was already received\n");
tcp_drop(sk, skb);
continue;
}
- SOCK_DEBUG(sk, "ofo requeuing : rcv_next %X seq %X - %X\n",
- tp->rcv_nxt, TCP_SKB_CB(skb)->seq,
- TCP_SKB_CB(skb)->end_seq);
tail = skb_peek_tail(&sk->sk_receive_queue);
eaten = tail && tcp_try_coalesce(sk, tail, skb, &fragstolen);
@@ -4502,8 +4493,6 @@ static void tcp_data_queue_ofo(struct sock *sk, struct sk_buff *skb)
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPOFOQUEUE);
seq = TCP_SKB_CB(skb)->seq;
end_seq = TCP_SKB_CB(skb)->end_seq;
- SOCK_DEBUG(sk, "out of order segment: rcv_next %X seq %X - %X\n",
- tp->rcv_nxt, seq, end_seq);
p = &tp->out_of_order_queue.rb_node;
if (RB_EMPTY_ROOT(&tp->out_of_order_queue)) {
@@ -4779,10 +4768,6 @@ static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
if (before(TCP_SKB_CB(skb)->seq, tp->rcv_nxt)) {
/* Partial packet, seq < rcv_next < end_seq */
- SOCK_DEBUG(sk, "partial packet: rcv_next %X seq %X - %X\n",
- tp->rcv_nxt, TCP_SKB_CB(skb)->seq,
- TCP_SKB_CB(skb)->end_seq);
-
tcp_dsack_set(sk, TCP_SKB_CB(skb)->seq, tp->rcv_nxt);
/* If window is closed, drop tail of packet. But after
@@ -5061,8 +5046,6 @@ static int tcp_prune_queue(struct sock *sk)
{
struct tcp_sock *tp = tcp_sk(sk);
- SOCK_DEBUG(sk, "prune_queue: c=%x\n", tp->copied_seq);
-
NET_INC_STATS(sock_net(sk), LINUX_MIB_PRUNECALLED);
if (atomic_read(&sk->sk_rmem_alloc) >= sk->sk_rcvbuf)
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index e51cda7..57ef69a1 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -220,8 +220,6 @@ static int tcp_v6_connect(struct sock *sk, struct sockaddr *uaddr,
u32 exthdrlen = icsk->icsk_ext_hdr_len;
struct sockaddr_in sin;
- SOCK_DEBUG(sk, "connect: ipv4 mapped\n");
-
if (__ipv6_only_sock(sk))
return -ENETUNREACH;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 net-next 2/2] net: sock: undefine SOCK_DEBUGGING
From: Yafang Shao @ 2019-02-16 16:28 UTC (permalink / raw)
To: davem
Cc: daniel, edumazet, joe, xiyou.wangcong, netdev, shaoyafang,
Yafang Shao
In-Reply-To: <1550334537-380-1-git-send-email-laoar.shao@gmail.com>
SOCK_DEBUG() is a old facility for debugging.
If the user want to use it for debugging, the user must modify the
application first, that doesn't seem like a good way.
Now we have more powerful facilities, i.e. bpf or tracepoint, for this kind
of debugging purpose.
So we'd better disable it by default.
The reason why I don't remove it comepletely is that someone may still
would like to use it for debugging.
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Suggested-by: Joe Perches <joe@perches.com>
---
include/net/sock.h | 13 ++++++++-----
net/core/sock.c | 3 +++
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/include/net/sock.h b/include/net/sock.h
index 6679f3c..d41e8f4 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -81,14 +81,17 @@
*/
/* Define this to get the SOCK_DBG debugging facility. */
-#define SOCK_DEBUGGING
+/* #define SOCK_DEBUGGING */
#ifdef SOCK_DEBUGGING
-#define SOCK_DEBUG(sk, msg...) do { if ((sk) && sock_flag((sk), SOCK_DBG)) \
- printk(KERN_DEBUG msg); } while (0)
+#define SOCK_DEBUG(sk, fmt, ...) \
+do { \
+ if ((sk) && sock_flag((sk), SOCK_DBG)) \
+ pr_debug(fmt, ##__VA_ARGS__); \
+} while (0)
#else
/* Validate arguments and do nothing */
-static inline __printf(2, 3)
-void SOCK_DEBUG(const struct sock *sk, const char *msg, ...)
+__printf(2, 3)
+static inline void SOCK_DEBUG(const struct sock *sk, const char *fmt, ...)
{
}
#endif
diff --git a/net/core/sock.c b/net/core/sock.c
index 71ded4d..7c15835 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -753,6 +753,9 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
switch (optname) {
case SO_DEBUG:
+ /* This option takes effect only when SOCK_DEBUGGING
+ * is defined.
+ */
if (val && !capable(CAP_NET_ADMIN))
ret = -EACCES;
else
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next] ip_tunnel: Fix DST_METADATA dst_entry handle in tnl_update_pmtu
From: Alexei Starovoitov @ 2019-02-16 16:34 UTC (permalink / raw)
To: wenxu
Cc: David S. Miller, rong.a.chen, Network Development,
Stephen Rothwell, LKP
In-Reply-To: <1550278683-17239-1-git-send-email-wenxu@ucloud.cn>
On Sat, Feb 16, 2019 at 2:11 AM <wenxu@ucloud.cn> wrote:
>
> From: wenxu <wenxu@ucloud.cn>
>
> BUG report in selftests: bpf: test_tunnel.sh
>
> Testing IPIP tunnel...
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
> PGD 0 P4D 0
> Oops: 0010 [#1] SMP PTI
> CPU: 0 PID: 16822 Comm: ping Not tainted 5.0.0-rc3-00352-gc8b34e6 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> RIP: 0010: (null)
> Code: Bad RIP value.
> RSP: 0018:ffffc9000104f9c8 EFLAGS: 00010286
> RAX: 0000000000000000 RBX: ffffe8ffffc071a8 RCX: 0000000000000000
> RDX: ffff888054e33000 RSI: ffff88807796f500 RDI: ffffe8ffffc07130
> RBP: ffff88807796f500 R08: ffff88806da4f0a0 R09: 0000000000000000
> R10: 0000000000000004 R11: ffff888054e33000 R12: 0000000000000054
> R13: ffff88805e714000 R14: ffff88806da4f0a0 R15: 0000000000000000
> FS: 00007f4c00431500(0000) GS:ffff88813fc00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffffffffffffd6 CR3: 000000008276e000 CR4: 00000000000406f0
> Call Trace:
> ? tnl_update_pmtu+0x21b/0x250 [ip_tunnel]
> ? ip_md_tunnel_xmit+0x1b7/0xdc0 [ip_tunnel]
> ? ipip_tunnel_xmit+0x90/0xc0 [ipip]
> ? dev_hard_start_xmit+0x98/0x210
> ? __dev_queue_xmit+0x6a9/0x8e0
>
> The bpf program set tunnel_key through bpf_skb_set_tunnel_key which will
> drop the old dst_entry and create a DST_METADATA dst_entry. It will lead
> the tunnel_update_pmtu operator the dst_entry incorrect. So It should be
> check the dst_entry is valid.
>
> Fixes: c8b34e680a09 ("ip_tunnel: Add tnl_update_pmtu in ip_md_tunnel_xmit")
> Signed-off-by: wenxu <wenxu@ucloud.cn>
different fix for this issue was sent earlier:
https://patchwork.ozlabs.org/patch/1042687/
I think it's more complete than this one.
^ permalink raw reply
* Re: [PATCH net-next 1/3] net: stmmac: Fix NAPI poll in TX path when in multi-queue
From: Lendacky, Thomas @ 2019-02-16 17:21 UTC (permalink / raw)
To: Florian Fainelli, Jose Abreu, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: Joao Pinto, David S . Miller, Giuseppe Cavallaro,
Alexandre Torgue
In-Reply-To: <c6a38836-a0d7-608d-39cf-ea770db78c95@gmail.com>
On 2/15/19 6:35 PM, Florian Fainelli wrote:
> On 2/15/19 5:42 AM, Jose Abreu wrote:
>> Commit 8fce33317023 introduced the concept of NAPI per-channel and
>> independent cleaning of TX path.
>>
>> This is currently breaking performance in some cases. The scenario
>> happens when all packets are being received in Queue 0 but the TX is
>> performed in Queue != 0.
>>
>> Fix this by using different NAPI instances per each TX and RX queue, as
>> suggested by Florian.
>>
>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Joao Pinto <jpinto@synopsys.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>> Cc: Alexandre Torgue <alexandre.torgue@st.com>
>> ---
>
> [snip]
>
>> - if (work_done < budget && napi_complete_done(napi, work_done)) {
>> - int stat;
>> + priv->xstats.napi_poll++;
>>
>> + work_done = stmmac_tx_clean(priv, budget, chan);
>> + if (work_done < budget && napi_complete_done(napi, work_done))
>
> You should not be bounding your TX queue against the NAPI budge, it
> should run unbound and clean as much as it can, which could be the
> entire ring size if that is how many packets you pushed between
> interrupts. That could be the cause of poor performance as well.
Won't returning the budget value cause this napi_poll routine to be called
again, where the driver can continue to clean TX packets? I thought this
just gives other drivers the opportunity to run their napi_poll routines
in between so as not to be starved.
Thanks,
Tom
>
^ permalink raw reply
* Re: [RFC PATCH] bonding: use mutex lock in bond_get_stats()
From: Eric Dumazet @ 2019-02-16 18:18 UTC (permalink / raw)
To: Kefeng Wang
Cc: netdev, Willem de Bruijn, David S . Miller, Jay Vosburgh,
Veaceslav Falico, weiyongjun1
In-Reply-To: <09569f86-2a12-ccdf-3ae7-00a228d1f957@huawei.com>
On Fri, Feb 15, 2019 at 9:36 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
>
>
>
> On 2019/2/15 21:57, Eric Dumazet wrote:
> > On Fri, Feb 15, 2019 at 5:37 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote:
> >>
> >> With CONFIG_DEBUG_SPINLOCK=y, we find following stack,
> >>
> >> BUG: spinlock wrong CPU on CPU#0, ip/16047
> >> lock: 0xffff803f5febc998, .magic: dead4ead, .owner: ip/16047, .owner_cpu: 0
> >> CPU: 1 PID: 16047 Comm: ip Kdump: loaded Tainted: G E 4.19.12.aarch64 #1
> >> Hardware name: Huawei TaiShan 2280 V2/BC82AMDA, BIOS TA BIOS TaiShan 2280 V2 - B900 01/29/2019
> >> Call trace:
> >> dump_backtrace+0x0/0x1c0
> >> show_stack+0x24/0x30
> >> dump_stack+0x90/0xbc
> >> spin_dump+0x84/0xa8
> >> do_raw_spin_unlock+0xf8/0x100
> >> _raw_spin_unlock+0x20/0x30
> >> bond_get_stats+0x110/0x140 [bonding]
> >> rtnl_fill_stats+0x50/0x150
> >> rtnl_fill_ifinfo+0x4d4/0xd18
> >> rtnl_dump_ifinfo+0x200/0x3a8
> >> netlink_dump+0x100/0x2b0
> >> netlink_recvmsg+0x310/0x3e8
> >> sock_recvmsg+0x58/0x68
> >> ___sys_recvmsg+0xd0/0x278
> >> __sys_recvmsg+0x74/0xd0
> >> __arm64_sys_recvmsg+0x2c/0x38
> >> el0_svc_common+0x7c/0x118
> >> el0_svc_handler+0x30/0x40
> >> el0_svc+0x8/0xc
> >>
> >> and then lead to softlockup issue, fix this by using mutex lock instead
> >> of spin lock.
> >>
> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> >> ---
> >>
> >> Not sure if this is right fix, please correct me if I'm wrong.
> >>
> >
> > Make sure to also try :
> >
> > cat /proc/net/dev
>
> Yes, no regression with this patch in our test or 'cat /proc/net/dev'
>
You really should have a warning showing up.
Make sure you have : DEBUG_ATOMIC_SLEEP=y (I thought this was
automatically selected with LOCKDEP on, maybe not...)
Please take a look at net/core/net-procfs.c , functions
dev_seq_start(), dev_seq_printf_stats(), dev_seq_show()
We can not use a mutex in a section protected by rcu_read_lock()
^ permalink raw reply
* Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled
From: Ido Schimmel @ 2019-02-16 18:43 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Linus Lüssing, netdev, roopa, wkok, anuradhak, bridge, davem,
stephen
In-Reply-To: <479a1acf-c7f3-4e6f-4246-e1583e98d356@cumulusnetworks.com>
On Sat, Feb 16, 2019 at 10:05:40AM +0200, Nikolay Aleksandrov wrote:
> On 15/02/2019 19:13, Linus Lüssing wrote:
> > On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov wrote:
> >> Every user would expect to have traffic forwarded only to the configured
> >> mdb destination when snooping is enabled, instead now to get that one
> >> needs to enable both snooping and querier. Enabling querier on all
> >> switches could be problematic and is not a good solution,
> >
> > There is no need to set the querier on all snooping switches.
> > br_multicast_querier_exists() checks if a querier exists on the
> > link in general, not if this particular host/bridge is a querier.
> >
>
> We need a generic solution for the case of existing mdst and no querier.
> More below.
>
> >
> >> for example as summarized by our multicast experts:
> >> "every switch would send an IGMP query
> >
> > What? RFC3810, section 7.1 says:
> >
> > "If it is the case, a querier election mechanism (described in
> > section 7.6.2) is used to elect a single multicast router to be
> > in Querier state. [...] Nevertheless, it is only the [elected] Querier
> > that sends periodical or triggered query messages on the subnet."
> > >> for any random multicast traffic it
> >> received across the entire domain and it would send it forever as long as a
> >> host exists wanting that stream even if it has no downstream/directly
> >> connected receivers"
> >
>
> This was taken out of context and it's my bad, I think everyone is aware
> of the election process, please nevermind the above statement.
>
> [snip]>
> >
> > Have you done some tests with this change yet, Nikolay?
> >
>
> You've raised good questions, IPv6 indeed needs more work - we'll have to flood
> link-local packets etc. but I wanted to have a discussion about no querier/existing mdst.
> To simplify we can modify the patch and have traffic forwarded to the proper ports when an
> mdst exists and there is no querier for both unsolicited report and user-added entry.
> We can keep the current behaviour for unknown traffic with and without querier.
> This would align it closer to what other vendors currently do as well IIRC.
> What do you think ?
The no querier condition is not currently reflected via switchdev, so
the behavior you're proposing in your patch is what actually happens in
the data plane.
We already hit the problem Linus mentioned in commit b00589af3b04
("bridge: disable snooping if there is no querier"). Namely, IPv6 ND
broke because a port joined before the bridge was created.
I introduced a workaround in commit 9d45deb04c59 ("mlxsw: spectrum:
Treat IPv6 unregistered multicast as broadcast"). I'm interested to know
what other vendors are doing. Can you elaborate?
We can trap IPv6 ND packets at L2 (we'll eventually need to do for ND
suppression) and let the bridge take care of flooding them correctly.
I'm not sure it's good enough.
^ permalink raw reply
* [ROSE] Bug : waiting for rose module to become free
From: f6bvp @ 2019-02-16 18:44 UTC (permalink / raw)
To: netdev, linux-hams; +Cc: dvyukov, richard, thomas, f6bvp
Hi,
A nastly bug is still pending in ROSE module that prevents it to be removed.
When trying to remove rose module we enter into an endless loop with displayed message :
"waiting for rose module to become free"
A patch from Richard Stern had been committed a while ago but it did not get a chance to be applied.
https://marc.info/?l=linux-hams&m=150870350516064&w=2
Since then, there have been some changes in af_rose.c and net code architecture that prevents simply adding missing dev_put(dev) in present kernel.
From time to time we can observe a negative count (-1 for integer value is displayed as 65535 in /proc/net/rose_neigh) that probably means there is a bug in rose module refcount (??).
/proc/net/rose_neigh
00015 SV1HCC-9 ax0 2 1 DCE yes 0 0
00013 PE1OWG-11 ax0 2 65535 DCE yes 0 0
00011 F5KTR-11 ax0 1 1 DCE yes 0 0
00007 F3KT-11 ax0 2 1 DCE yes 0 0
00003 SK7ES-9 ax0 2 1 DTE yes 0 0
00002 K4GBB-9 ax0 4 0 DCE yes 0 0
00001 RSLOOP-0 ??? 1 2 DCE yes 0 0
Help is kindly requested to get a valid patch.
Bernard,
f6bvp
^ permalink raw reply
* [Patch net-next 1/2] net_sched: fix a race condition in tcindex_destroy()
From: Cong Wang @ 2019-02-16 18:58 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Adrian, Ben Hutchings, Jamal Hadi Salim, Jiri Pirko
(cherry picked from commit 8015d93ebd27484418d4952284fd02172fa4b0b2)
tcindex_destroy() invokes tcindex_destroy_element() via
a walker to delete each filter result in its perfect hash
table, and tcindex_destroy_element() calls tcindex_delete()
which schedules tcf RCU works to do the final deletion work.
Unfortunately this races with the RCU callback
__tcindex_destroy(), which could lead to use-after-free as
reported by Adrian.
Fix this by migrating this RCU callback to tcf RCU work too,
as that workqueue is ordered, we will not have use-after-free.
Note, we don't need to hold netns refcnt because we don't call
tcf_exts_destroy() here.
Fixes: 27ce4f05e2ab ("net_sched: use tcf_queue_work() in tcindex filter")
Reported-by: Adrian <bugs@abtelecom.ro>
Cc: Ben Hutchings <ben@decadent.org.uk>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/cls_tcindex.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index e1981628047b..81a433ae31b3 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -48,7 +48,7 @@ struct tcindex_data {
u32 hash; /* hash table size; 0 if undefined */
u32 alloc_hash; /* allocated size */
u32 fall_through; /* 0: only classify if explicit match */
- struct rcu_head rcu;
+ struct rcu_work rwork;
};
static inline int tcindex_filter_is_set(struct tcindex_filter_result *r)
@@ -229,9 +229,11 @@ static int tcindex_destroy_element(struct tcf_proto *tp,
return tcindex_delete(tp, arg, &last, false, NULL);
}
-static void __tcindex_destroy(struct rcu_head *head)
+static void tcindex_destroy_work(struct work_struct *work)
{
- struct tcindex_data *p = container_of(head, struct tcindex_data, rcu);
+ struct tcindex_data *p = container_of(to_rcu_work(work),
+ struct tcindex_data,
+ rwork);
kfree(p->perfect);
kfree(p->h);
@@ -258,9 +260,11 @@ static int tcindex_filter_result_init(struct tcindex_filter_result *r)
return tcf_exts_init(&r->exts, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
}
-static void __tcindex_partial_destroy(struct rcu_head *head)
+static void tcindex_partial_destroy_work(struct work_struct *work)
{
- struct tcindex_data *p = container_of(head, struct tcindex_data, rcu);
+ struct tcindex_data *p = container_of(to_rcu_work(work),
+ struct tcindex_data,
+ rwork);
kfree(p->perfect);
kfree(p);
@@ -480,7 +484,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
}
if (oldp)
- call_rcu(&oldp->rcu, __tcindex_partial_destroy);
+ tcf_queue_work(&oldp->rwork, tcindex_partial_destroy_work);
return 0;
errout_alloc:
@@ -572,7 +576,7 @@ static void tcindex_destroy(struct tcf_proto *tp, bool rtnl_held,
walker.fn = tcindex_destroy_element;
tcindex_walk(tp, &walker, true);
- call_rcu(&p->rcu, __tcindex_destroy);
+ tcf_queue_work(&p->rwork, tcindex_destroy_work);
}
--
2.20.1
^ permalink raw reply related
* [Patch net-next 2/2] net_sched: fix a memory leak in cls_tcindex
From: Cong Wang @ 2019-02-16 18:58 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Jamal Hadi Salim, Jiri Pirko
In-Reply-To: <20190216185827.21535-1-xiyou.wangcong@gmail.com>
(cherry picked from commit 033b228e7f26b29ae37f8bfa1bc6b209a5365e9f)
When tcindex_destroy() destroys all the filter results in
the perfect hash table, it invokes the walker to delete
each of them. However, results with class==0 are skipped
in either tcindex_walk() or tcindex_delete(), which causes
a memory leak reported by kmemleak.
This patch fixes it by skipping the walker and directly
deleting these filter results so we don't miss any filter
result.
As a result of this change, we have to initialize exts->net
properly in tcindex_alloc_perfect_hash(). For net-next, we
need to consider whether we should initialize ->net in
tcf_exts_init() instead, before that just directly test
CONFIG_NET_CLS_ACT=y.
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/sched/cls_tcindex.c | 37 ++++++++++++++++++++++++-------------
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index 81a433ae31b3..fbf3519a12d8 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -221,14 +221,6 @@ static int tcindex_delete(struct tcf_proto *tp, void *arg, bool *last,
return 0;
}
-static int tcindex_destroy_element(struct tcf_proto *tp,
- void *arg, struct tcf_walker *walker)
-{
- bool last;
-
- return tcindex_delete(tp, arg, &last, false, NULL);
-}
-
static void tcindex_destroy_work(struct work_struct *work)
{
struct tcindex_data *p = container_of(to_rcu_work(work),
@@ -568,13 +560,32 @@ static void tcindex_destroy(struct tcf_proto *tp, bool rtnl_held,
struct netlink_ext_ack *extack)
{
struct tcindex_data *p = rtnl_dereference(tp->root);
- struct tcf_walker walker;
+ int i;
pr_debug("tcindex_destroy(tp %p),p %p\n", tp, p);
- walker.count = 0;
- walker.skip = 0;
- walker.fn = tcindex_destroy_element;
- tcindex_walk(tp, &walker, true);
+
+ if (p->perfect) {
+ for (i = 0; i < p->hash; i++) {
+ struct tcindex_filter_result *r = p->perfect + i;
+
+ tcf_unbind_filter(tp, &r->res);
+ if (tcf_exts_get_net(&r->exts))
+ tcf_queue_work(&r->rwork,
+ tcindex_destroy_rexts_work);
+ else
+ __tcindex_destroy_rexts(r);
+ }
+ }
+
+ for (i = 0; p->h && i < p->hash; i++) {
+ struct tcindex_filter *f, *next;
+ bool last;
+
+ for (f = rtnl_dereference(p->h[i]); f; f = next) {
+ next = rtnl_dereference(f->next);
+ tcindex_delete(tp, &f->result, &last, rtnl_held, NULL);
+ }
+ }
tcf_queue_work(&p->rwork, tcindex_destroy_work);
}
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 0/2] net: phy: disable aneg in genphy_c45_pma_setup_forced
From: Heiner Kallweit @ 2019-02-16 19:08 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller,
Russell King - ARM Linux
Cc: netdev@vger.kernel.org
When genphy_c45_pma_setup_forced() is called the "aneg enabled" bit
may still be set, therefore clear it. This is also in line with what
genphy_setup_forced() does for Clause 22.
Heiner Kallweit (2):
net: phy: disable aneg in genphy_c45_pma_setup_forced
net: phy: marvell10g: improve mv3310_config_aneg
drivers/net/phy/marvell10g.c | 9 ++-------
drivers/net/phy/phy-c45.c | 4 ++++
2 files changed, 6 insertions(+), 7 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH net-next 1/2] net: phy: disable aneg in genphy_c45_pma_setup_forced
From: Heiner Kallweit @ 2019-02-16 19:10 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller,
Russell King - ARM Linux
Cc: netdev@vger.kernel.org
In-Reply-To: <8945c224-bfff-294d-f855-979016e077ec@gmail.com>
When genphy_c45_pma_setup_forced() is called the "aneg enabled" bit may
still be set, therefore clear it. This is also in line with what
genphy_setup_forced() does for Clause 22.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy-c45.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index bef126344..0374c50b1 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -71,6 +71,10 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
return ret;
return phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL2, ctrl2);
+ if (ret < 0)
+ return ret;
+
+ return genphy_c45_an_disable_aneg(phydev);
}
EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 2/2] net: phy: marvell10g: improve mv3310_config_aneg
From: Heiner Kallweit @ 2019-02-16 19:11 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller,
Russell King - ARM Linux
Cc: netdev@vger.kernel.org
In-Reply-To: <8945c224-bfff-294d-f855-979016e077ec@gmail.com>
Now that genphy_c45_pma_setup_forced() makes sure the "aneg enabled"
bit is cleared, the call to genphy_c45_an_disable_aneg() isn't needed
any longer. And the code pattern is now the same as in
genphy_config_aneg().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/marvell10g.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 496805c0d..4a6ae63ab 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -271,13 +271,8 @@ static int mv3310_config_aneg(struct phy_device *phydev)
/* We don't support manual MDI control */
phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
- if (phydev->autoneg == AUTONEG_DISABLE) {
- ret = genphy_c45_pma_setup_forced(phydev);
- if (ret < 0)
- return ret;
-
- return genphy_c45_an_disable_aneg(phydev);
- }
+ if (phydev->autoneg == AUTONEG_DISABLE)
+ return genphy_c45_pma_setup_forced(phydev);
linkmode_and(phydev->advertising, phydev->advertising,
phydev->supported);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] Provide in-kernel headers for making it easy to extend the kernel
From: Manoj @ 2019-02-16 19:10 UTC (permalink / raw)
To: Joel Fernandes
Cc: Alexei Starovoitov, netdev, linux-kernel, Andrew Morton, ast,
atishp04, dancol, Dan Williams, gregkh, Jonathan Corbet,
karim.yaghmour, Kees Cook, kernel-team, linux-doc,
linux-kselftest, Manoj Rao, Masahiro Yamada, paulmck,
Peter Zijlstra (Intel), rdunlap, rostedt, Shuah Khan,
Thomas Gleixner, yhs
In-Reply-To: <20190215034720.GA230433@google.com>
Joel Fernandes writes:
> On Thu, Feb 14, 2019 at 07:19:29PM -0800, Alexei Starovoitov wrote:
>> On Mon, Feb 11, 2019 at 09:35:59AM -0500, Joel Fernandes (Google) wrote:
>> > Introduce in-kernel headers and other artifacts which are made available
>> > as an archive through proc (/proc/kheaders.txz file). This archive makes
>> > it possible to build kernel modules, run eBPF programs, and other
>> > tracing programs that need to extend the kernel for tracing purposes
>> > without any dependency on the file system having headers and build
>> > artifacts.
>> >
>> > On Android and embedded systems, it is common to switch kernels but not
>> > have kernel headers available on the file system. Raw kernel headers
>> > also cannot be copied into the filesystem like they can be on other
>> > distros, due to licensing and other issues. There's no linux-headers
>> > package on Android. Further once a different kernel is booted, any
>> > headers stored on the file system will no longer be useful. By storing
>> > the headers as a compressed archive within the kernel, we can avoid these
>> > issues that have been a hindrance for a long time.
>>
>> The set looks good to me and since the main use case is building bpf progs
>> I can route it via bpf-next tree if there are no objections.
>> Masahiro, could you please ack it?
>>
>
> Yes, eBPF is one of the usecases. After this, I am also planning to send
> patches to BCC so that it can use this feature when compiling C to eBPF.
>
Tested-by: Manoj Rao <linux@manojrajarao.com>
I think this can definitely make it easier to use eBPF on
Android. Thanks for initiating this.
> Thanks!
>
> - Joel
--
Manoj
http://www.mycpu.org
^ permalink raw reply
* Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled
From: nikolay @ 2019-02-16 19:15 UTC (permalink / raw)
To: Ido Schimmel
Cc: Linus Lüssing, netdev, roopa, wkok, anuradhak, bridge, davem,
stephen
In-Reply-To: <20190216184353.GA10888@splinter>
On 16 February 2019 20:43:53 EET, Ido Schimmel <idosch@idosch.org> wrote:
>On Sat, Feb 16, 2019 at 10:05:40AM +0200, Nikolay Aleksandrov wrote:
>> On 15/02/2019 19:13, Linus Lüssing wrote:
>> > On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov
>wrote:
>> >> Every user would expect to have traffic forwarded only to the
>configured
>> >> mdb destination when snooping is enabled, instead now to get that
>one
>> >> needs to enable both snooping and querier. Enabling querier on all
>> >> switches could be problematic and is not a good solution,
>> >
>> > There is no need to set the querier on all snooping switches.
>> > br_multicast_querier_exists() checks if a querier exists on the
>> > link in general, not if this particular host/bridge is a querier.
>> >
>>
>> We need a generic solution for the case of existing mdst and no
>querier.
>> More below.
>>
>> >
>> >> for example as summarized by our multicast experts:
>> >> "every switch would send an IGMP query
>> >
>> > What? RFC3810, section 7.1 says:
>> >
>> > "If it is the case, a querier election mechanism (described in
>> > section 7.6.2) is used to elect a single multicast router to be
>> > in Querier state. [...] Nevertheless, it is only the [elected]
>Querier
>> > that sends periodical or triggered query messages on the subnet."
>> > >> for any random multicast traffic it
>> >> received across the entire domain and it would send it forever as
>long as a
>> >> host exists wanting that stream even if it has no
>downstream/directly
>> >> connected receivers"
>> >
>>
>> This was taken out of context and it's my bad, I think everyone is
>aware
>> of the election process, please nevermind the above statement.
>>
>> [snip]>
>> >
>> > Have you done some tests with this change yet, Nikolay?
>> >
>>
>> You've raised good questions, IPv6 indeed needs more work - we'll
>have to flood
>> link-local packets etc. but I wanted to have a discussion about no
>querier/existing mdst.
>> To simplify we can modify the patch and have traffic forwarded to the
>proper ports when an
>> mdst exists and there is no querier for both unsolicited report and
>user-added entry.
>> We can keep the current behaviour for unknown traffic with and
>without querier.
>> This would align it closer to what other vendors currently do as well
>IIRC.
>> What do you think ?
>
>The no querier condition is not currently reflected via switchdev, so
>the behavior you're proposing in your patch is what actually happens in
>the data plane.
>
>We already hit the problem Linus mentioned in commit b00589af3b04
>("bridge: disable snooping if there is no querier"). Namely, IPv6 ND
>broke because a port joined before the bridge was created.
>
>I introduced a workaround in commit 9d45deb04c59 ("mlxsw: spectrum:
>Treat IPv6 unregistered multicast as broadcast"). I'm interested to
>know
>what other vendors are doing. Can you elaborate?
>
Exactly like your fix. :) Flood it, this patch unfortunately breaks it
because of mrouters flag, but we can retain the behaviour
by forwarding only known mdsts to their ports and flooding
unregistered mcast when there is no querier. I think that is
what others do by default too, actually I think they flood with querier as well. Maybe unknown mcast flooding should be controlled by a flag when a querier is present
because we've had this behaviour for a long time, personally I'd have it on
by default.
I am currently away and will be able to prepare a rfc patch after the weekend.
>We can trap IPv6 ND packets at L2 (we'll eventually need to do for ND
>suppression) and let the bridge take care of flooding them correctly.
>I'm not sure it's good enough.
^ permalink raw reply
* Re: [PATCH RFC] net: bridge: don't flood known multicast traffic when snooping is enabled
From: nikolay @ 2019-02-16 19:27 UTC (permalink / raw)
To: Ido Schimmel
Cc: Linus Lüssing, netdev, roopa, wkok, anuradhak, bridge, davem,
stephen
In-Reply-To: <0FE44F84-AD68-4FC0-8FEB-D033CF6159D2@cumulusnetworks.com>
On 16 February 2019 21:15:21 EET, nikolay@cumulusnetworks.com wrote:
>On 16 February 2019 20:43:53 EET, Ido Schimmel <idosch@idosch.org>
>wrote:
>>On Sat, Feb 16, 2019 at 10:05:40AM +0200, Nikolay Aleksandrov wrote:
>>> On 15/02/2019 19:13, Linus Lüssing wrote:
>>> > On Fri, Feb 15, 2019 at 03:04:27PM +0200, Nikolay Aleksandrov
>>wrote:
>>> >> Every user would expect to have traffic forwarded only to the
>>configured
>>> >> mdb destination when snooping is enabled, instead now to get that
>>one
>>> >> needs to enable both snooping and querier. Enabling querier on
>all
>>> >> switches could be problematic and is not a good solution,
>>> >
>>> > There is no need to set the querier on all snooping switches.
>>> > br_multicast_querier_exists() checks if a querier exists on the
>>> > link in general, not if this particular host/bridge is a querier.
>>> >
>>>
>>> We need a generic solution for the case of existing mdst and no
>>querier.
>>> More below.
>>>
>>> >
>>> >> for example as summarized by our multicast experts:
>>> >> "every switch would send an IGMP query
>>> >
>>> > What? RFC3810, section 7.1 says:
>>> >
>>> > "If it is the case, a querier election mechanism (described in
>>> > section 7.6.2) is used to elect a single multicast router to be
>>> > in Querier state. [...] Nevertheless, it is only the [elected]
>>Querier
>>> > that sends periodical or triggered query messages on the subnet."
>>> > >> for any random multicast traffic it
>>> >> received across the entire domain and it would send it forever as
>>long as a
>>> >> host exists wanting that stream even if it has no
>>downstream/directly
>>> >> connected receivers"
>>> >
>>>
>>> This was taken out of context and it's my bad, I think everyone is
>>aware
>>> of the election process, please nevermind the above statement.
>>>
>>> [snip]>
>>> >
>>> > Have you done some tests with this change yet, Nikolay?
>>> >
>>>
>>> You've raised good questions, IPv6 indeed needs more work - we'll
>>have to flood
>>> link-local packets etc. but I wanted to have a discussion about no
>>querier/existing mdst.
>>> To simplify we can modify the patch and have traffic forwarded to
>the
>>proper ports when an
>>> mdst exists and there is no querier for both unsolicited report and
>>user-added entry.
>>> We can keep the current behaviour for unknown traffic with and
>>without querier.
>>> This would align it closer to what other vendors currently do as
>well
>>IIRC.
>>> What do you think ?
>>
>>The no querier condition is not currently reflected via switchdev, so
>>the behavior you're proposing in your patch is what actually happens
>in
>>the data plane.
>>
>>We already hit the problem Linus mentioned in commit b00589af3b04
>>("bridge: disable snooping if there is no querier"). Namely, IPv6 ND
>>broke because a port joined before the bridge was created.
>>
>>I introduced a workaround in commit 9d45deb04c59 ("mlxsw: spectrum:
>>Treat IPv6 unregistered multicast as broadcast"). I'm interested to
>>know
>>what other vendors are doing. Can you elaborate?
>>
>
>Exactly like your fix. :) Flood it, this patch unfortunately breaks it
>because of mrouters flag, but we can retain the behaviour
>by forwarding only known mdsts to their ports and flooding
>unregistered mcast when there is no querier. I think that is
>what others do by default too, actually I think they flood with querier
>as well. Maybe unknown mcast flooding should be controlled by a flag
>when a querier is present
>because we've had this behaviour for a long time, personally I'd have
>it on
>by default.
Ugh, mispoke please read the above statement to be only about no querier.
I meant flooding v6 link-local always.
>I am currently away and will be able to prepare a rfc patch after the
>weekend.
>
>
>>We can trap IPv6 ND packets at L2 (we'll eventually need to do for ND
>>suppression) and let the bridge take care of flooding them correctly.
>>I'm not sure it's good enough.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: phy: disable aneg in genphy_c45_pma_setup_forced
From: Stefano Brivio @ 2019-02-16 19:31 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, Florian Fainelli, David Miller,
Russell King - ARM Linux, netdev@vger.kernel.org
In-Reply-To: <27bb134f-eb4e-e43a-5d9d-2c551a22428c@gmail.com>
Hi,
On Sat, 16 Feb 2019 20:10:24 +0100
Heiner Kallweit <hkallweit1@gmail.com> wrote:
> When genphy_c45_pma_setup_forced() is called the "aneg enabled" bit may
> still be set, therefore clear it. This is also in line with what
> genphy_setup_forced() does for Clause 22.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/phy/phy-c45.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index bef126344..0374c50b1 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -71,6 +71,10 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
> return ret;
>
> return phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL2, ctrl2);
> + if (ret < 0)
Perhaps you meant ret = phy_write_mmd(...)?
> + return ret;
> +
> + return genphy_c45_an_disable_aneg(phydev);
> }
> EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
>
--
Stefano
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: phy: disable aneg in genphy_c45_pma_setup_forced
From: Heiner Kallweit @ 2019-02-16 19:39 UTC (permalink / raw)
To: Stefano Brivio
Cc: Andrew Lunn, Florian Fainelli, David Miller,
Russell King - ARM Linux, netdev@vger.kernel.org
In-Reply-To: <20190216203120.06923d0c@redhat.com>
On 16.02.2019 20:31, Stefano Brivio wrote:
> Hi,
>
> On Sat, 16 Feb 2019 20:10:24 +0100
> Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
>> When genphy_c45_pma_setup_forced() is called the "aneg enabled" bit may
>> still be set, therefore clear it. This is also in line with what
>> genphy_setup_forced() does for Clause 22.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/phy/phy-c45.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>> index bef126344..0374c50b1 100644
>> --- a/drivers/net/phy/phy-c45.c
>> +++ b/drivers/net/phy/phy-c45.c
>> @@ -71,6 +71,10 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
>> return ret;
>>
>> return phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL2, ctrl2);
>> + if (ret < 0)
>
> Perhaps you meant ret = phy_write_mmd(...)?
>
Ups, for sure I meant this. Strange that the compiler doesn't complain.
>> + return ret;
>> +
>> + return genphy_c45_an_disable_aneg(phydev);
>> }
>> EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
>>
>
Heiner
^ permalink raw reply
* [PATCH v2 net-next 0/2] net: phy: disable aneg in genphy_c45_pma_setup_forced
From: Heiner Kallweit @ 2019-02-16 19:39 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller,
Russell King - ARM Linux
Cc: netdev@vger.kernel.org
When genphy_c45_pma_setup_forced() is called the "aneg enabled" bit
may still be set, therefore clear it. This is also in line with what
genphy_setup_forced() does for Clause 22.
v2:
- fix a typo in patch 1
Heiner Kallweit (2):
net: phy: disable aneg in genphy_c45_pma_setup_forced
net: phy: marvell10g: improve mv3310_config_aneg
drivers/net/phy/marvell10g.c | 9 ++-------
drivers/net/phy/phy-c45.c | 4 ++++
2 files changed, 6 insertions(+), 7 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH v2 net-next 1/2] net: phy: disable aneg in genphy_c45_pma_setup_forced
From: Heiner Kallweit @ 2019-02-16 19:44 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller,
Russell King - ARM Linux
Cc: netdev@vger.kernel.org
In-Reply-To: <c029e3b6-5fbe-08fb-a012-90ceef3be100@gmail.com>
When genphy_c45_pma_setup_forced() is called the "aneg enabled" bit may
still be set, therefore clear it. This is also in line with what
genphy_setup_forced() does for Clause 22.
v2:
- fix typo
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/phy-c45.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index bef126344..98a04d4cd 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -70,7 +70,11 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
if (ret < 0)
return ret;
- return phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL2, ctrl2);
+ ret = phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL2, ctrl2);
+ if (ret < 0)
+ return ret;
+
+ return genphy_c45_an_disable_aneg(phydev);
}
EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
--
2.20.1
^ permalink raw reply related
* [PATCH v2 net-next 2/2] net: phy: marvell10g: improve mv3310_config_aneg
From: Heiner Kallweit @ 2019-02-16 19:44 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller,
Russell King - ARM Linux
Cc: netdev@vger.kernel.org
In-Reply-To: <c029e3b6-5fbe-08fb-a012-90ceef3be100@gmail.com>
Now that genphy_c45_pma_setup_forced() makes sure the "aneg enabled"
bit is cleared, the call to genphy_c45_an_disable_aneg() isn't needed
any longer. And the code pattern is now the same as in
genphy_config_aneg().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/marvell10g.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 496805c0d..4a6ae63ab 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -271,13 +271,8 @@ static int mv3310_config_aneg(struct phy_device *phydev)
/* We don't support manual MDI control */
phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
- if (phydev->autoneg == AUTONEG_DISABLE) {
- ret = genphy_c45_pma_setup_forced(phydev);
- if (ret < 0)
- return ret;
-
- return genphy_c45_an_disable_aneg(phydev);
- }
+ if (phydev->autoneg == AUTONEG_DISABLE)
+ return genphy_c45_pma_setup_forced(phydev);
linkmode_and(phydev->advertising, phydev->advertising,
phydev->supported);
--
2.20.1
^ permalink raw reply related
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