* Re: [PATCH net-next 00/10] korina cleanups/optimizations
From: Florian Fainelli @ 2017-10-15 16:38 UTC (permalink / raw)
To: Roman Yeryomin, netdev
In-Reply-To: <20171015162226.606-1-roman@advem.lv>
On October 15, 2017 9:22:26 AM PDT, Roman Yeryomin <roman@advem.lv> wrote:
>TX optimizations have led to ~15% performance increase (35->40Mbps)
>in local tx usecase (tested with iperf v3.2).
Could you avoid empty commit messages and write a paragraph or two for each commit that explains what and why are you changing? The changes look fine but they lack any explanation.
>
>Roman Yeryomin (10):
> net: korina: optimize korina_send_packet logic
> net: korina: reorder functions
> net: korina: introduce and use interrupt disable/enable helpers
> net: korina: optimize tx/rx interrupt handlers
> net: korina: remove unused korina_private members
> net: korina: optimize tx descriptor flags processing
> net: korina: move tx to napi context
> net: korina: optimize tx_full condition
> net: korina: use dma api instead of dma_cache_* functions
> net: korina: bump version
>
>drivers/net/ethernet/korina.c | 503
>+++++++++++++++++-------------------------
> 1 file changed, 204 insertions(+), 299 deletions(-)
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 00/10] korina cleanups/optimizations
From: Roman Yeryomin @ 2017-10-15 16:46 UTC (permalink / raw)
To: Florian Fainelli, netdev
In-Reply-To: <0FA80CB3-C0DC-4B30-B638-26B5D46AE69E@gmail.com>
On 2017-10-15 19:38, Florian Fainelli wrote:
> On October 15, 2017 9:22:26 AM PDT, Roman Yeryomin <roman@advem.lv>
> wrote:
>> TX optimizations have led to ~15% performance increase (35->40Mbps)
>> in local tx usecase (tested with iperf v3.2).
>
> Could you avoid empty commit messages and write a paragraph or two for
> each commit that explains what and why are you changing? The changes
> look fine but they lack any explanation.
I thought that short descriptions are already self explanatory and just
didn't know what to write more.
To me they are very obvious.
Do you really want me to make up something more?
>>
>> Roman Yeryomin (10):
>> net: korina: optimize korina_send_packet logic
>> net: korina: reorder functions
>> net: korina: introduce and use interrupt disable/enable helpers
>> net: korina: optimize tx/rx interrupt handlers
>> net: korina: remove unused korina_private members
>> net: korina: optimize tx descriptor flags processing
>> net: korina: move tx to napi context
>> net: korina: optimize tx_full condition
>> net: korina: use dma api instead of dma_cache_* functions
>> net: korina: bump version
>>
>> drivers/net/ethernet/korina.c | 503
>> +++++++++++++++++-------------------------
>> 1 file changed, 204 insertions(+), 299 deletions(-)
^ permalink raw reply
* [PATCH] ipv6: addrconf: Use normal debugging style
From: Joe Perches @ 2017-10-15 16:49 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI
Cc: David Ahern, netdev, linux-kernel
In-Reply-To: <1507935733-18950-3-git-send-email-dsahern@gmail.com>
Remove local ADBG macro and use netdev_dbg/pr_debug
Miscellanea:
o Remove unnecessary debug message after allocation failure as there
already is a dump_stack() on the failure paths
o Leave the allocation failure message on snmp6_alloc_dev as there
is one code path that does not do a dump_stack()
Signed-off-by: Joe Perches <joe@perches.com>
---
o This is on top of David Ahern's proposed patch
net: ipv6: Make inet6addr_validator a blocking notifier
net/ipv6/addrconf.c | 28 ++++++++--------------------
1 file changed, 8 insertions(+), 20 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 31ff12277bcf..c505e84dda1f 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -94,15 +94,6 @@
#include <linux/seq_file.h>
#include <linux/export.h>
-/* Set to 3 to get tracing... */
-#define ACONF_DEBUG 2
-
-#if ACONF_DEBUG >= 3
-#define ADBG(fmt, ...) printk(fmt, ##__VA_ARGS__)
-#else
-#define ADBG(fmt, ...) do { if (0) printk(fmt, ##__VA_ARGS__); } while (0)
-#endif
-
#define INFINITY_LIFE_TIME 0xFFFFFFFF
#define IPV6_MAX_STRLEN \
@@ -409,9 +400,8 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
dev_hold(dev);
if (snmp6_alloc_dev(ndev) < 0) {
- ADBG(KERN_WARNING
- "%s: cannot allocate memory for statistics; dev=%s.\n",
- __func__, dev->name);
+ netdev_dbg(dev, "%s: cannot allocate memory for statistics\n",
+ __func__);
neigh_parms_release(&nd_tbl, ndev->nd_parms);
dev_put(dev);
kfree(ndev);
@@ -419,9 +409,8 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
}
if (snmp6_register_dev(ndev) < 0) {
- ADBG(KERN_WARNING
- "%s: cannot create /proc/net/dev_snmp6/%s\n",
- __func__, dev->name);
+ netdev_dbg(dev, "%s: cannot create /proc/net/dev_snmp6/%s\n",
+ __func__, dev->name);
goto err_release;
}
@@ -966,7 +955,7 @@ static int ipv6_add_addr_hash(struct net_device *dev, struct inet6_ifaddr *ifa)
/* Ignore adding duplicate addresses on an interface */
if (ipv6_chk_same_addr(dev_net(dev), &ifa->addr, dev)) {
- ADBG("ipv6_add_addr: already assigned\n");
+ netdev_dbg(dev, "ipv6_add_addr: already assigned\n");
err = -EEXIST;
goto out;
}
@@ -1029,7 +1018,6 @@ ipv6_add_addr(struct inet6_dev *idev, const struct in6_addr *addr,
ifa = kzalloc(sizeof(*ifa), gfp_flags);
if (!ifa) {
- ADBG("ipv6_add_addr: malloc failed\n");
err = -ENOBUFS;
goto out;
}
@@ -2575,7 +2563,7 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
pinfo = (struct prefix_info *) opt;
if (len < sizeof(struct prefix_info)) {
- ADBG("addrconf: prefix option too short\n");
+ netdev_dbg(dev, "addrconf: prefix option too short\n");
return;
}
@@ -4373,8 +4361,8 @@ static void addrconf_verify_rtnl(void)
if (time_before(next_sched, jiffies + ADDRCONF_TIMER_FUZZ_MAX))
next_sched = jiffies + ADDRCONF_TIMER_FUZZ_MAX;
- ADBG(KERN_DEBUG "now = %lu, schedule = %lu, rounded schedule = %lu => %lu\n",
- now, next, next_sec, next_sched);
+ pr_debug("now = %lu, schedule = %lu, rounded schedule = %lu => %lu\n",
+ now, next, next_sec, next_sched);
mod_delayed_work(addrconf_wq, &addr_chk_work, next_sched - now);
rcu_read_unlock_bh();
}
--
2.10.0.rc2.1.g053435c
^ permalink raw reply related
* routing UAPI mismatch invalid state behavior?
From: Alexander Aring @ 2017-10-15 17:02 UTC (permalink / raw)
To: netdev; +Cc: Jamal Hadi Salim
Hi,
I figure out some problem, easy to reproduce:
# setup dummy
$ modprobe dummy
$ ip link set dummy0 up
# issue
$ ip route replace default via 169.254.65.37 dev dummy0
RTNETLINK answers: Network is unreachable
so it will forbid me to do that, but:
$ ip route replace 169.254.65.37 dev dummy0
$ ip route replace default via 169.254.65.37 dev dummy0
$ ip route del 169.254.65.37 dev dummy0
allows me to do that. Is there now a invalid state in my routing table
or is it an expected behavior?
- Alex
^ permalink raw reply
* Re: [RFC PATCH v2 2/5] tcp: implemented pacing_expired
From: Eric Dumazet @ 2017-10-15 17:30 UTC (permalink / raw)
To: Natale Patriciello
Cc: David S . Miller, netdev, Ahmed Said, Francesco Zampognaro,
Cesare Roseti
In-Reply-To: <20171014114714.3694-3-natale.patriciello@gmail.com>
On Sat, 2017-10-14 at 13:47 +0200, Natale Patriciello wrote:
> Inform the congestion control that the pacing timer, previously set,
> has expired. The commit does not consider situations in which another
> kind of timer has expired (e.g., a tail loss probe, a retransmission
> timer...)
>
> Signed-off-by: Natale Patriciello <natale.patriciello@gmail.com>
> ---
> include/net/tcp.h | 2 ++
> net/ipv4/tcp_output.c | 6 ++++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 42c7aa96c4cf..e817f0669d0e 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1017,6 +1017,8 @@ struct tcp_congestion_ops {
> union tcp_cc_info *info);
> /* get the expiration time for the pacing timer (optional) */
> u64 (*get_pacing_time)(struct sock *sk);
> + /* the pacing timer is expired (optional) */
> + void (*pacing_timer_expired)(struct sock *sk);
>
> char name[TCP_CA_NAME_MAX];
> struct module *owner;
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index ec5977156c26..25b4cf0802f2 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2241,6 +2241,7 @@ void tcp_chrono_stop(struct sock *sk, const enum tcp_chrono type)
> static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
> int push_one, gfp_t gfp)
> {
> + const struct tcp_congestion_ops *ca_ops = inet_csk(sk)->icsk_ca_ops;
> struct tcp_sock *tp = tcp_sk(sk);
> struct sk_buff *skb;
> unsigned int tso_segs, sent_pkts;
> @@ -2263,6 +2264,11 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
>
> max_segs = tcp_tso_segs(sk, mss_now);
> tcp_mstamp_refresh(tp);
> +
> + if (!tcp_pacing_timer_check(sk) &&
> + ca_ops && ca_ops->pacing_timer_expired)
> + ca_ops->pacing_timer_expired(sk);
> +
>
1) We do not want hrtimer_active() being called for each
tcp_write_xmit() ... :/
2) ca_ops can not be NULL.
->
if (ca_ops->pacing_timer_expired && !tcp_pacing_timer_check(sk))
ca_ops->pacing_timer_expired(sk);
^ permalink raw reply
* Re: [RFC PATCH v2 3/5] tcp: added get_segs_per_round
From: Eric Dumazet @ 2017-10-15 17:32 UTC (permalink / raw)
To: Natale Patriciello
Cc: David S . Miller, netdev, Ahmed Said, Francesco Zampognaro,
Cesare Roseti
In-Reply-To: <20171014114714.3694-4-natale.patriciello@gmail.com>
On Sat, 2017-10-14 at 13:47 +0200, Natale Patriciello wrote:
> Usually, the pacing time is provided per-segment. In some occasion, this
> time refers to the time between a group of segments. With this commit, add
> the possibility for the congestion control module to tell the TCP socket
> how many segments can be sent out before pausing and setting a pacing
> timer.
>
> Signed-off-by: Natale Patriciello <natale.patriciello@gmail.com>
> ---
> include/net/tcp.h | 2 ++
> net/ipv4/tcp_output.c | 13 +++++++++----
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index e817f0669d0e..3561eca5a61f 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1019,6 +1019,8 @@ struct tcp_congestion_ops {
> u64 (*get_pacing_time)(struct sock *sk);
> /* the pacing timer is expired (optional) */
> void (*pacing_timer_expired)(struct sock *sk);
> + /* get the # segs to send out when the timer expires (optional) */
> + u32 (*get_segs_per_round)(struct sock *sk);
>
> char name[TCP_CA_NAME_MAX];
> struct module *owner;
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 25b4cf0802f2..e37941e4328b 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -2249,6 +2249,7 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
> int result;
> bool is_cwnd_limited = false, is_rwnd_limited = false;
> u32 max_segs;
> + u32 pacing_allowed_segs = 0;
>
> sent_pkts = 0;
>
> @@ -2265,14 +2266,18 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
> max_segs = tcp_tso_segs(sk, mss_now);
> tcp_mstamp_refresh(tp);
>
> - if (!tcp_pacing_timer_check(sk) &&
> - ca_ops && ca_ops->pacing_timer_expired)
> - ca_ops->pacing_timer_expired(sk);
> + if (!tcp_pacing_timer_check(sk)) {
> + pacing_allowed_segs = 1;
> + if (ca_ops && ca_ops->pacing_timer_expired)
> + ca_ops->pacing_timer_expired(sk);
> + if (ca_ops && ca_ops->get_segs_per_round)
> + pacing_allowed_segs = ca_ops->get_segs_per_round(sk);
> + }
>
> while ((skb = tcp_send_head(sk))) {
> unsigned int limit;
>
> - if (tcp_pacing_check(sk))
> + if (sent_pkts >= pacing_allowed_segs)
> break;
>
So, if pacing_allowed_segs == 0, we break immediately of the loop ????
Are you sure you have tested this patch with normal CC like CUBIC and
BBR ?
^ permalink raw reply
* Re: [RFC PATCH v2 5/5] wave: Added TCP Wave
From: Eric Dumazet @ 2017-10-15 17:43 UTC (permalink / raw)
To: Natale Patriciello
Cc: David S . Miller, netdev, Ahmed Said, Francesco Zampognaro,
Cesare Roseti
In-Reply-To: <20171014114714.3694-6-natale.patriciello@gmail.com>
On Sat, 2017-10-14 at 13:47 +0200, Natale Patriciello wrote:
> TCP Wave (TCPW) replaces the window-based transmission paradigm of the
> standard TCP with a burst-based transmission, the ACK-clock scheduling
> with a self-managed timer and the RTT-based congestion control loop
> with an Ack-based Capacity and Congestion Estimation (ACCE) module. In
> non-technical words, it sends data down the stack when its internal
> timer expires, and the timing of the received ACKs contribute to
> updating this timer regularly.
>
> It is the first TCP congestion control that uses the timing constraint
> developed in the Linux kernel.
You seem to have missed BBR.
This is against what we have worked in the last years, preventing bursts
in the first place.
If this series is to address WIFI peculiarities with TSQ (this is a well
known problem, still waiting for a fix), I would rather not add all this
code in TCP and find better alternatives.
Since you do not provide information and testing results, there is no
chance we accept such patches.
Your work seems to be a variation of TSQ, and a very complex one.
^ permalink raw reply
* [PATCH] net: ceph: mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2017-10-15 17:55 UTC (permalink / raw)
To: Ilya Dryomov, Yan, Zheng, Sage Weil, David S. Miller
Cc: ceph-devel, netdev, linux-kernel, Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
net/ceph/ceph_hash.c | 12 +++++++++++-
net/ceph/messenger.c | 1 +
net/ceph/mon_client.c | 1 +
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/net/ceph/ceph_hash.c b/net/ceph/ceph_hash.c
index 67bb1f1..9a5850f2 100644
--- a/net/ceph/ceph_hash.c
+++ b/net/ceph/ceph_hash.c
@@ -47,28 +47,38 @@ unsigned int ceph_str_hash_rjenkins(const char *str, unsigned int length)
/* handle the last 11 bytes */
c = c + length;
- switch (len) { /* all the case statements fall through */
+ switch (len) {
case 11:
c = c + ((__u32)k[10] << 24);
+ /* fall through */
case 10:
c = c + ((__u32)k[9] << 16);
+ /* fall through */
case 9:
c = c + ((__u32)k[8] << 8);
/* the first byte of c is reserved for the length */
+ /* fall through */
case 8:
b = b + ((__u32)k[7] << 24);
+ /* fall through */
case 7:
b = b + ((__u32)k[6] << 16);
+ /* fall through */
case 6:
b = b + ((__u32)k[5] << 8);
+ /* fall through */
case 5:
b = b + k[4];
+ /* fall through */
case 4:
a = a + ((__u32)k[3] << 24);
+ /* fall through */
case 3:
a = a + ((__u32)k[2] << 16);
+ /* fall through */
case 2:
a = a + ((__u32)k[1] << 8);
+ /* fall through */
case 1:
a = a + k[0];
/* case 0: nothing left to add */
diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index a67298c..73dac9d 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -429,6 +429,7 @@ static void ceph_sock_state_change(struct sock *sk)
switch (sk->sk_state) {
case TCP_CLOSE:
dout("%s TCP_CLOSE\n", __func__);
+ /* fall through */
case TCP_CLOSE_WAIT:
dout("%s TCP_CLOSE_WAIT\n", __func__);
con_sock_state_closing(con);
diff --git a/net/ceph/mon_client.c b/net/ceph/mon_client.c
index 63edc6e..b97c047 100644
--- a/net/ceph/mon_client.c
+++ b/net/ceph/mon_client.c
@@ -1281,6 +1281,7 @@ static struct ceph_msg *mon_alloc_msg(struct ceph_connection *con,
* request had a non-zero tid. Workaround this weirdness
* by falling through to the allocate case.
*/
+ /* fall through */
case CEPH_MSG_MON_MAP:
case CEPH_MSG_MDS_MAP:
case CEPH_MSG_OSD_MAP:
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next 00/10] korina cleanups/optimizations
From: Florian Fainelli @ 2017-10-15 17:36 UTC (permalink / raw)
To: Roman Yeryomin, netdev
In-Reply-To: <c3a2f516dcceb7dee9ea4dde634b58ed@advem.lv>
On October 15, 2017 9:46:02 AM PDT, Roman Yeryomin <roman@advem.lv> wrote:
>On 2017-10-15 19:38, Florian Fainelli wrote:
>> On October 15, 2017 9:22:26 AM PDT, Roman Yeryomin <roman@advem.lv>
>> wrote:
>>> TX optimizations have led to ~15% performance increase (35->40Mbps)
>>> in local tx usecase (tested with iperf v3.2).
>>
>> Could you avoid empty commit messages and write a paragraph or two
>for
>> each commit that explains what and why are you changing? The changes
>> look fine but they lack any explanation.
>
>I thought that short descriptions are already self explanatory and just
>
>didn't know what to write more.
>To me they are very obvious.
You can't assume what is obvious to you will be to the reviewers.
>Do you really want me to make up something more?
It's up to David as a maintainer to decide, but if this was up to me I would ask you to respin with a least a paragraph for each commit (except the version bump) why you did these changes.
>
>>>
>>> Roman Yeryomin (10):
>>> net: korina: optimize korina_send_packet logic
>>> net: korina: reorder functions
>>> net: korina: introduce and use interrupt disable/enable helpers
>>> net: korina: optimize tx/rx interrupt handlers
>>> net: korina: remove unused korina_private members
>>> net: korina: optimize tx descriptor flags processing
>>> net: korina: move tx to napi context
>>> net: korina: optimize tx_full condition
>>> net: korina: use dma api instead of dma_cache_* functions
>>> net: korina: bump version
>>>
>>> drivers/net/ethernet/korina.c | 503
>>> +++++++++++++++++-------------------------
>>> 1 file changed, 204 insertions(+), 299 deletions(-)
--
Florian
^ permalink raw reply
* [PATCH] xen/9pfs: mark expected switch fall-through in xen_9pfs_front_changed
From: Gustavo A. R. Silva @ 2017-10-15 18:08 UTC (permalink / raw)
To: Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov,
David S. Miller
Cc: v9fs-developer, netdev, linux-kernel, Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Notice that in this particular case, I placed the "fall through" comment
on its own line, which is what GCC is expecting to find.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
This code was tested by compilation only (GCC 7.2.0 was used).
net/9p/trans_xen.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/9p/trans_xen.c b/net/9p/trans_xen.c
index 6ad3e04..7ec5df9 100644
--- a/net/9p/trans_xen.c
+++ b/net/9p/trans_xen.c
@@ -510,7 +510,8 @@ static void xen_9pfs_front_changed(struct xenbus_device *dev,
case XenbusStateClosed:
if (dev->state == XenbusStateClosed)
break;
- /* Missed the backend's CLOSING state -- fallthrough */
+ /* Missed the backend's CLOSING state */
+ /* fall through */
case XenbusStateClosing:
xenbus_frontend_closed(dev);
break;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next] ipv6: only update __use and lastusetime once per jiffy at most
From: Martin KaFai Lau @ 2017-10-15 18:19 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Wei Wang, David Miller, netdev, Paolo Abeni
In-Reply-To: <CANn89i+09PB1=J8mXO495iOK4oe4hsnRF=YsOQQJAsnw5ydeoA@mail.gmail.com>
On Sat, Oct 14, 2017 at 12:26:28AM +0000, Eric Dumazet wrote:
> On Fri, Oct 13, 2017 at 5:09 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> > On Fri, Oct 13, 2017 at 10:08:07PM +0000, Wei Wang wrote:
> >> From: Wei Wang <weiwan@google.com>
> >>
> >> In order to not dirty the cacheline too often, we try to only update
> >> dst->__use and dst->lastusetime at most once per jiffy.
> >
> >
> >> As dst->lastusetime is only used by ipv6 garbage collector, it should
> >> be good enough time resolution.
> > Make sense.
> >
> >> And __use is only used in ipv6_route_seq_show() to show how many times a
> >> dst has been used. And as __use is not atomic_t right now, it does not
> >> show the precise number of usage times anyway. So we think it should be
> >> OK to only update it at most once per jiffy.
> > If __use is only bumped HZ number of times per second and we can do ~3Mpps now,
> > would __use be way off?
>
> It is not used in the kernel, and is not even reported by user space
> (iproute2) currently.
>
> With the percpu stuff, we never did the sum anyway.
The pcpu_rt currently does not track __use.
>
> I believe we should be fine by being very lazy on this field.
>
> If really someones complain, we will see, but insuring ~one update per
> HZ seems fine.
Fair point. Make sense.
We currently also don't find the ipv6_route proc-file very useful
other than debugging purpose.
^ permalink raw reply
* [PATCH] net: dccp: mark expected switch fall-throughs
From: Gustavo A. R. Silva @ 2017-10-15 18:22 UTC (permalink / raw)
To: Gerrit Renker, David S. Miller
Cc: dccp, netdev, linux-kernel, Gustavo A. R. Silva
In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.
Notice that for options.c file, I placed the "fall through" comment
on its own line, which is what GCC is expecting to find.
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
This code was tested by compilation only (GCC 7.2.0 was used).
Please, verify if the actual intention of the code is to fall through.
net/dccp/input.c | 1 +
net/dccp/options.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/dccp/input.c b/net/dccp/input.c
index fa6be97..d28d46b 100644
--- a/net/dccp/input.c
+++ b/net/dccp/input.c
@@ -534,6 +534,7 @@ static int dccp_rcv_respond_partopen_state_process(struct sock *sk,
case DCCP_PKT_DATA:
if (sk->sk_state == DCCP_RESPOND)
break;
+ /* fall through */
case DCCP_PKT_DATAACK:
case DCCP_PKT_ACK:
/*
diff --git a/net/dccp/options.c b/net/dccp/options.c
index 51cdfc3..4e40db0 100644
--- a/net/dccp/options.c
+++ b/net/dccp/options.c
@@ -227,8 +227,8 @@ int dccp_parse_options(struct sock *sk, struct dccp_request_sock *dreq,
* Ack vectors are processed by the TX CCID if it is
* interested. The RX CCID need not parse Ack Vectors,
* since it is only interested in clearing old state.
- * Fall through.
*/
+ /* fall through */
case DCCPO_MIN_TX_CCID_SPECIFIC ... DCCPO_MAX_TX_CCID_SPECIFIC:
if (ccid_hc_tx_parse_options(dp->dccps_hc_tx_ccid, sk,
pkt_type, opt, value, len))
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2 2/5] dpaa_eth: move of_phy_connect() to the eth driver
From: Andrew Lunn @ 2017-10-15 18:33 UTC (permalink / raw)
To: Madalin Bucur
Cc: netdev, davem, f.fainelli, vivien.didelot, junote, linux-kernel
In-Reply-To: <1507906212-10076-3-git-send-email-madalin.bucur@nxp.com>
On Fri, Oct 13, 2017 at 05:50:09PM +0300, Madalin Bucur wrote:
> Signed-off-by: Madalin Bucur <madalin.bucur@nxp.com>
> ---
> drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 48 +++++++++++--
> drivers/net/ethernet/freescale/fman/mac.c | 97 ++++++--------------------
> drivers/net/ethernet/freescale/fman/mac.h | 5 +-
> 3 files changed, 66 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 4225806..7cf61d6 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2435,6 +2435,48 @@ static void dpaa_eth_napi_disable(struct dpaa_priv *priv)
> }
> }
>
> +static void dpaa_adjust_link(struct net_device *net_dev)
> +{
> + struct mac_device *mac_dev;
> + struct dpaa_priv *priv;
> +
> + priv = netdev_priv(net_dev);
> + mac_dev = priv->mac_dev;
> + mac_dev->adjust_link(mac_dev);
> +}
> +
> +static int dpaa_phy_init(struct net_device *net_dev)
> +{
> + struct mac_device *mac_dev;
> + struct phy_device *phy_dev;
> + struct dpaa_priv *priv;
> +
> + priv = netdev_priv(net_dev);
> + mac_dev = priv->mac_dev;
> +
> + phy_dev = of_phy_connect(net_dev, mac_dev->phy_node,
> + &dpaa_adjust_link, 0,
> + mac_dev->phy_if);
> + if (!phy_dev) {
> + netif_err(priv, ifup, net_dev, "init_phy() failed\n");
> + return -ENODEV;
> + }
> +
> + /* Remove any features not supported by the controller */
> + phy_dev->supported &= mac_dev->if_support;
> +
> + /* Enable the symmetric and asymmetric PAUSE frame advertisements,
> + * as most of the PHY drivers do not enable them by default.
> + */
Hi Madalin
This is just moving code around, so the patch is O.K. However, it
would be nice to have a followup patch. This comment is wrong. The phy
driver should never enable symmetric and asymmetric PAUSE frames. The
MAC needs to, because only the MAC knows if the MAC supports pause
frames.
Andrew
^ permalink raw reply
* [PATCH] hamradio: baycom_par: use new parport device model
From: Sudip Mukherjee @ 2017-10-15 21:00 UTC (permalink / raw)
To: David S. Miller, Thomas Sailer
Cc: linux-kernel, linux-hams, netdev, Sudip Mukherjee
Modify baycom driver to use the new parallel port device model.
Signed-off-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
---
Not tested on real hardware, only tested on kvm with 32 bit guest and
verified that the device is binding to the driver properly in par96_open
but then unbinding as the device was not found.
drivers/net/hamradio/baycom_par.c | 48 +++++++++++++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/drivers/net/hamradio/baycom_par.c b/drivers/net/hamradio/baycom_par.c
index e178383..1f7ceaf 100644
--- a/drivers/net/hamradio/baycom_par.c
+++ b/drivers/net/hamradio/baycom_par.c
@@ -311,7 +311,9 @@ static void par96_wakeup(void *handle)
static int par96_open(struct net_device *dev)
{
struct baycom_state *bc = netdev_priv(dev);
+ struct pardev_cb par_cb;
struct parport *pp;
+ int i;
if (!dev || !bc)
return -ENXIO;
@@ -332,8 +334,21 @@ static int par96_open(struct net_device *dev)
}
memset(&bc->modem, 0, sizeof(bc->modem));
bc->hdrv.par.bitrate = 9600;
- bc->pdev = parport_register_device(pp, dev->name, NULL, par96_wakeup,
- par96_interrupt, PARPORT_DEV_EXCL, dev);
+ memset(&par_cb, 0, sizeof(par_cb));
+ par_cb.wakeup = par96_wakeup;
+ par_cb.irq_func = par96_interrupt;
+ par_cb.private = (void *)dev;
+ par_cb.flags = PARPORT_DEV_EXCL;
+ for (i = 0; i < NR_PORTS; i++)
+ if (baycom_device[i] == dev)
+ break;
+
+ if (i == NR_PORTS) {
+ pr_err("%s: no device found\n", bc_drvname);
+ parport_put_port(pp);
+ return -ENODEV;
+ }
+ bc->pdev = parport_register_dev_model(pp, dev->name, &par_cb, i);
parport_put_port(pp);
if (!bc->pdev) {
printk(KERN_ERR "baycom_par: cannot register parport at 0x%lx\n", dev->base_addr);
@@ -490,12 +505,34 @@ static int baycom_ioctl(struct net_device *dev, struct ifreq *ifr,
/* --------------------------------------------------------------------- */
+static int baycom_par_probe(struct pardevice *par_dev)
+{
+ struct device_driver *drv = par_dev->dev.driver;
+ int len = strlen(drv->name);
+
+ if (strncmp(par_dev->name, drv->name, len))
+ return -ENODEV;
+
+ return 0;
+}
+
+static struct parport_driver baycom_par_driver = {
+ .name = "bcp",
+ .probe = baycom_par_probe,
+ .devmodel = true,
+};
+
static int __init init_baycompar(void)
{
- int i, found = 0;
+ int i, found = 0, ret;
char set_hw = 1;
printk(bc_drvinfo);
+
+ ret = parport_register_driver(&baycom_par_driver);
+ if (ret)
+ return ret;
+
/*
* register net devices
*/
@@ -524,8 +561,10 @@ static int __init init_baycompar(void)
baycom_device[i] = dev;
}
- if (!found)
+ if (!found) {
+ parport_unregister_driver(&baycom_par_driver);
return -ENXIO;
+ }
return 0;
}
@@ -539,6 +578,7 @@ static void __exit cleanup_baycompar(void)
if (dev)
hdlcdrv_unregister(dev);
}
+ parport_unregister_driver(&baycom_par_driver);
}
module_init(init_baycompar);
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next 00/10] korina cleanups/optimizations
From: David Miller @ 2017-10-15 21:05 UTC (permalink / raw)
To: roman; +Cc: f.fainelli, netdev
In-Reply-To: <c3a2f516dcceb7dee9ea4dde634b58ed@advem.lv>
From: Roman Yeryomin <roman@advem.lv>
Date: Sun, 15 Oct 2017 19:46:02 +0300
> On 2017-10-15 19:38, Florian Fainelli wrote:
>> On October 15, 2017 9:22:26 AM PDT, Roman Yeryomin <roman@advem.lv>
>> wrote:
>>> TX optimizations have led to ~15% performance increase (35->40Mbps)
>>> in local tx usecase (tested with iperf v3.2).
>> Could you avoid empty commit messages and write a paragraph or two for
>> each commit that explains what and why are you changing? The changes
>> look fine but they lack any explanation.
>
> I thought that short descriptions are already self explanatory and
> just didn't know what to write more.
"Optimize TX handlers."
In what way? Why? How are things improved? Is it measurable?
etc.
^ permalink raw reply
* [PATCH] bpf: devmap: Check attr->max_entries more carefully
From: Richard Weinberger @ 2017-10-15 22:00 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, daniel, ast, sp3485, Richard Weinberger
max_entries is user controlled and used as input for __alloc_percpu().
This function expects that the allocation size is a power of two and
less than PCPU_MIN_UNIT_SIZE.
Otherwise a WARN() is triggered.
Fixes: 11393cc9b9be ("xdp: Add batching support to redirect map")
Reported-by: Shankara Pailoor <sp3485@columbia.edu>
Reported-by: syzkaller <syzkaller@googlegroups.com>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
kernel/bpf/devmap.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index e093d9a2c4dd..6ce00083103b 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -49,6 +49,7 @@
*/
#include <linux/bpf.h>
#include <linux/filter.h>
+#include <linux/log2.h>
struct bpf_dtab_netdev {
struct net_device *dev;
@@ -77,6 +78,7 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
struct bpf_dtab *dtab;
int err = -EINVAL;
u64 cost;
+ size_t palloc_size;
/* check sanity of attributes */
if (attr->max_entries == 0 || attr->key_size != 4 ||
@@ -95,9 +97,14 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
dtab->map.map_flags = attr->map_flags;
dtab->map.numa_node = bpf_map_attr_numa_node(attr);
+ palloc_size = roundup_pow_of_two(dev_map_bitmap_size(attr));
+ if (palloc_size > PCPU_MIN_UNIT_SIZE ||
+ palloc_size < dev_map_bitmap_size(attr))
+ return ERR_PTR(-EINVAL);
+
/* make sure page count doesn't overflow */
cost = (u64) dtab->map.max_entries * sizeof(struct bpf_dtab_netdev *);
- cost += dev_map_bitmap_size(attr) * num_possible_cpus();
+ cost += palloc_size * num_possible_cpus();
if (cost >= U32_MAX - PAGE_SIZE)
goto free_dtab;
@@ -111,7 +118,7 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
err = -ENOMEM;
/* A per cpu bitfield with a bit per possible net device */
- dtab->flush_needed = __alloc_percpu(dev_map_bitmap_size(attr),
+ dtab->flush_needed = __alloc_percpu(palloc_size,
__alignof__(unsigned long));
if (!dtab->flush_needed)
goto free_dtab;
--
2.13.6
^ permalink raw reply related
* Re: [PATCH] bpf: devmap: Check attr->max_entries more carefully
From: Richard Weinberger @ 2017-10-15 22:13 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel, daniel, ast, sp3485
In-Reply-To: <20171015220020.8157-1-richard@nod.at>
Am Montag, 16. Oktober 2017, 00:00:20 CEST schrieb Richard Weinberger:
> max_entries is user controlled and used as input for __alloc_percpu().
> This function expects that the allocation size is a power of two and
> less than PCPU_MIN_UNIT_SIZE.
> Otherwise a WARN() is triggered.
On a second though, I think we should also have a hard limit for ->max_entries
or check for an overflow here:
static u64 dev_map_bitmap_size(const union bpf_attr *attr)
{
return BITS_TO_LONGS(attr->max_entries) * sizeof(unsigned long);
}
Thanks,
//richard
^ permalink raw reply
* Re: RFC: making cn_proc work in {pid,user} namespaces
From: Eric W. Biederman @ 2017-10-15 22:40 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Linux Containers, netdev, linux-kernel, Christian Brauner,
Evgeniy Polyakov, dev, cyphar@cyphar.com >> Aleksa Sarai
In-Reply-To: <a2fa1602-2280-c5e8-cac9-b718eaea5d22@suse.de>
Aleksa Sarai <asarai@suse.de> writes:
> Hi all,
>
> At the moment, cn_proc is not usable by containers or container runtimes. In
> addition, all connectors have an odd relationship with init_net (for example,
> /proc/net/connectors only exists in init_net). There are two main use-cases that
> would be perfect for cn_proc, which is the reason for me pushing this:
>
> First, when adding a process to an existing container, in certain modes runc
> would like to know that process's exit code. But, when joining a PID namespace,
> it is advisable[1] to always double-fork after doing the setns(2) to reparent
> the joining process to the init of the container (this causes the SIGCHLD to be
> received by the container init). It would also be useful to be able to monitor
> the exit code of the init process in a container without being its parent. At
> the moment, cn_proc doesn't allow unprivileged users to use it (making it a
> problem for user namespaces and "rootless containers"). In addition, it also
> doesn't allow nested containers to use it, because it requires the process to be
> in init_pid. As a result, runc cannot use cn_proc and relies on SIGCHLD (which
> can only be used if we don't double-fork, or keep around a long-running process
> which is something that runc also cannot do).
As far as I know there are no technical issues that require a
daemonizing double fork when injecting a process into a pid namespaces.
A fork is required because the pid is changing and that requires another
process.
Monitoring and acting on the monitored state without keeping around a
single process to do the monitoring does not make sense to me. So I am
just going to ignore that.
So I don't think fixing cn_proc for this issue makes sense.
> Secondly, there are/were some init systems that rely on cn_proc to manage
> service state. From a "it would be neat" perspective, I think it would be quite
> nice if such init systems could be used inside containers. But that requires
> cn_proc to be able to be used as an unprivileged user and in a pid namespace
> other than init_pid.
Any pointers to these init systems? In general I agree. Given how much
work it takes to go through a subsystem and ensure that it is safe for
non-root users I am happy to see the work done, but I am not
volunteering for the work when I have several I have as many tasks as I
have on my plate right now.
> The /proc/net/connectors thing is quite easily resolved (just make it the
> connector driver perdev and make some small changes to make sure the interfaces
> stay sane inside of a container's network namespace). I'm sure that we'll
> probably have to make some changes to the registration API, so that a connector
> can specify whether they want to be visible to non-init_net
> namespaces.
>
> However, the cn_proc problem is a bit harder to resolve nicely and there are
> quite a few interface questions that would need to be agreed upon. The basic
> idea would be that a process can only get cn_proc events if it has
> ptrace_may_access rights over said process (effectively a forced filter -- which
> would ideally be done send-side but it looks like it might have to be done
> receive-side). This should resolve possible concerns about an unprivileged
> process being able to inspect (fairly granular) information about the host. And
> obviously the pids, uids, and gids would all be translated according to the
> receiving process's user namespaces (if it cannot be translated then the message
> is not received). I guess that the translation would be done in the same way as
> SCM_CREDENTIALS (and cgroup.procs files), which is that it's done on the receive
> side not the send side.
Hmm. We have several of these things such as bsd process accounting
which appear to be working fine.
The basic logic winds up being:
for_each_receiver:
compose_msg in receivers namespace
send_msg.
The tricky bit in my mind is dealing with receivers because of the
connection with the network namespace.
SCM_CREDENTIALS is an unfortunate case, that really should not be
followed as a model. The major challenge there is not knowing
the receiving socket, or the receiver. If I had been smarter
when I coded that originally I would have forced everything into
the namespace of the opener of the receiving socket. I may have to
revisit that one again someday and see if there are improvements that
can be made.
> My reason for sending this email rather than just writing the patch is to see
> whether anyone has any solid NACKs against the use-case or whether there is some
> fundamental issue that I'm not seeing. If nobody objects, I'll be happy to work
> on this.
If you want a non-crazy (with respect to namespace involvement) model
please look at kernel/acct.c:acc_process()
If there are use cases that people still care about that use the
proc connector and want to run in a container it seems sensible to dig
in and sort things out. I think I have been hoping it is little enough
used we won't have to mess with making it work in namespaces.
> [1]: https://lwn.net/Articles/532748/
Eric
^ permalink raw reply
* Re: [PATCH net 2/6] rtnetlink: bring NETDEV_CHANGE_TX_QUEUE_LEN event process back in rtnetlink_event
From: David Ahern @ 2017-10-16 1:16 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes
In-Reply-To: <e240a6572c8e495eb2da15bd6178604cdfde08ba.1508062280.git.lucien.xin@gmail.com>
On 10/15/17 4:13 AM, Xin Long wrote:
> The same fix for changing mtu in the patch 'rtnetlink: bring
> NETDEV_CHANGEMTU event process back in rtnetlink_event' is
> needed for changing tx_queue_len.
>
> Note that the redundant notifications issue for tx_queue_len
> will be fixed in the later patch 'rtnetlink: do not send
> notification for tx_queue_len in do_setlink'.
>
> Fixes: 27b3b551d8a7 ("rtnetlink: Do not generate notifications for NETDEV_CHANGE_TX_QUEUE_LEN event")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/core/rtnetlink.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 72053ed..bf47360 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4287,6 +4287,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
> case NETDEV_NOTIFY_PEERS:
> case NETDEV_RESEND_IGMP:
> case NETDEV_CHANGEINFODATA:
> + case NETDEV_CHANGE_TX_QUEUE_LEN:
> rtmsg_ifinfo_event(RTM_NEWLINK, dev, 0, rtnl_get_event(event),
> GFP_KERNEL);
> break;
>
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH net 3/6] rtnetlink: bring NETDEV_POST_TYPE_CHANGE event process back in rtnetlink_event
From: David Ahern @ 2017-10-16 1:16 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes
In-Reply-To: <b4125c6cf42d4d028a9c04f4989627ac1eb83257.1508062280.git.lucien.xin@gmail.com>
On 10/15/17 4:13 AM, Xin Long wrote:
> As I said in patch 'rtnetlink: bring NETDEV_CHANGEMTU event process back
> in rtnetlink_event', removing NETDEV_POST_TYPE_CHANGE event was not the
> right fix for the redundant notifications issue.
>
> So bring this event process back to rtnetlink_event and the old redundant
> notifications issue would be fixed in the later patch 'rtnetlink: check
> DO_SETLINK_NOTIFY correctly in do_setlink'.
>
> Fixes: aef091ae58aa ("rtnetlink: Do not generate notifications for POST_TYPE_CHANGE event")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/core/rtnetlink.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index bf47360..8e44fd5 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4284,6 +4284,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
> case NETDEV_CHANGENAME:
> case NETDEV_FEAT_CHANGE:
> case NETDEV_BONDING_FAILOVER:
> + case NETDEV_POST_TYPE_CHANGE:
> case NETDEV_NOTIFY_PEERS:
> case NETDEV_RESEND_IGMP:
> case NETDEV_CHANGEINFODATA:
>
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH net 4/6] rtnetlink: bring NETDEV_CHANGEUPPER event process back in rtnetlink_event
From: David Ahern @ 2017-10-16 1:16 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes
In-Reply-To: <784f477a1f9e8b9a5ec7bbe9e1dff1dcdfd5553b.1508062280.git.lucien.xin@gmail.com>
On 10/15/17 4:13 AM, Xin Long wrote:
> libteam needs this event notification in userspace when dev's master
> dev has been changed. After this, the redundant notifications issue
> would be fixed in the later patch 'rtnetlink: check DO_SETLINK_NOTIFY
> correctly in do_setlink'.
>
> Fixes: b6b36eb23a46 ("rtnetlink: Do not generate notifications for NETDEV_CHANGEUPPER event")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/core/rtnetlink.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 8e44fd5..ab98c1c 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4286,6 +4286,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
> case NETDEV_BONDING_FAILOVER:
> case NETDEV_POST_TYPE_CHANGE:
> case NETDEV_NOTIFY_PEERS:
> + case NETDEV_CHANGEUPPER:
> case NETDEV_RESEND_IGMP:
> case NETDEV_CHANGEINFODATA:
> case NETDEV_CHANGE_TX_QUEUE_LEN:
>
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH net 5/6] rtnetlink: check DO_SETLINK_NOTIFY correctly in do_setlink
From: David Ahern @ 2017-10-16 1:17 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes, Nicolas Dichtel
In-Reply-To: <22c0aefe18d4451166c5f12714d8520346a63f2b.1508062280.git.lucien.xin@gmail.com>
[ cc'ed Nicolas ]
On 10/15/17 4:13 AM, Xin Long wrote:
> The check 'status & DO_SETLINK_NOTIFY' in do_setlink doesn't really
> work after status & DO_SETLINK_MODIFIED, as:
>
> DO_SETLINK_MODIFIED 0x1
> DO_SETLINK_NOTIFY 0x3
>
> Considering that notifications are suppposed to be sent only when
> status have the flag DO_SETLINK_NOTIFY, the right check would be:
>
> (status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY
>
> This would avoid lots of duplicated notifications when setting some
> properties of a link.
>
> Fixes: ba9989069f4e ("rtnl/do_setlink(): notify when a netdev is modified")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/core/rtnetlink.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index ab98c1c..3e98fb5 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2248,7 +2248,7 @@ static int do_setlink(const struct sk_buff *skb,
>
> errout:
> if (status & DO_SETLINK_MODIFIED) {
> - if (status & DO_SETLINK_NOTIFY)
> + if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
> netdev_state_change(dev);
>
> if (err < 0)
>
Good catch.
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH net 6/6] rtnetlink: do not set notification for tx_queue_len in do_setlink
From: David Ahern @ 2017-10-16 1:17 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes
In-Reply-To: <6337bdec4b0564c0577f90703ea840fd3580912a.1508062280.git.lucien.xin@gmail.com>
On 10/15/17 4:13 AM, Xin Long wrote:
> NETDEV_CHANGE_TX_QUEUE_LEN event process in rtnetlink_event would
> send a notification for userspace and tx_queue_len's setting in
> do_setlink would trigger NETDEV_CHANGE_TX_QUEUE_LEN.
>
> So it shouldn't set DO_SETLINK_NOTIFY status for this change to
> send a notification any more.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/core/rtnetlink.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 3e98fb5..a6bcf86 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2093,7 +2093,7 @@ static int do_setlink(const struct sk_buff *skb,
> dev->tx_queue_len = orig_len;
> goto errout;
> }
> - status |= DO_SETLINK_NOTIFY;
> + status |= DO_SETLINK_MODIFIED;
> }
> }
>
>
Acked-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver
From: Yunsheng Lin @ 2017-10-16 1:41 UTC (permalink / raw)
To: Yuval Mintz, davem@davemloft.net
Cc: huangdaode@hisilicon.com, xuwei5@hisilicon.com,
liguozhu@hisilicon.com, Yisen.Zhuang@huawei.com,
gabriele.paoloni@huawei.com, john.garry@huawei.com,
linuxarm@huawei.com, salil.mehta@huawei.com, lipeng321@huawei.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <AM0PR0502MB368336C6A93618A13ADBB68ABF4E0@AM0PR0502MB3683.eurprd05.prod.outlook.com>
Hi, Yuval
On 2017/10/15 13:14, Yuval Mintz wrote:
>> Hi, Yuval
>>
>> On 2017/10/13 4:21, Yuval Mintz wrote:
>>>> This patchset adds a new hardware offload type in mqprio before adding
>>>> mqprio hardware offload support in hns3 driver.
>>>
>>> I think one of the biggest issues in tying this to DCB configuration is the
>>> non-immediate [and possibly non persistent] configuration.
>>>
>>> Scenario #1:
>>> User is configuring mqprio offloaded with 3 TCs while device is in willing
>> mode.
>>> Would you expect the driver to immediately respond with a success or
>> instead
>>> delay the return until the DCBx negotiation is complete and the operational
>>> num of TCs is actually 3?
>>
>> Well, when user requsts the mqprio offloaded by a hardware shared by DCB,
>> I expect
>> the user is not using the dcb tool.
>> If user is still using dcb tool, then result is undefined.
>>
>> The scenario you mention maybe can be enforced by setting willing to zero
>> when user
>> is requesting the mqprio offload, and restore the willing bit when unloaded
>> the mqprio
>> offload.
>
> Sounds a bit harsh but would probably work.
>
>> But I think the real issue is that dcb and mqprio shares the tc system in the
>> stack,
>> the problem may be better to be fixed in the stack rather than in the driver,
>> as you
>> suggested in the DCB patchset. What do you think?
>
> What did you have in mind?
I was thinking maybe the tc system can provide a notification to mqprio and dcb.
mqprio and dcb register a callback to the tc system, when there is some change of
tc configuration, the tc system call the callback from mqprio and dcb.
>
>>
>>>
>>> Scenario #2:
>>> Assume user explicitly offloaded mqprio with 3 TCs, but now DCB
>> configuration
>>> has changed on the peer side and 4 TCs is the new negotiated operational
>> value.
>>> Your current driver logic would change the number of TCs underneath the
>> user
>>> configuration [and it would actually probably work due to mqprio being a
>> crappy
>>> qdisc]. But was that the user actual intention?
>>> [I think the likely answer in this scenario is 'yes' since the alternative is no
>> better.
>>> But I still thought it was worth mentioning]
>>
>> You are right, the problem also have something to do with mqprio and dcb
>> sharing
>> the tc in the stack.
>>
>> Druing testing, when user explicitly offloaded mqprio with 3 TCs, all
>> queue has a default pfifo mqprio attached, after DCB changes the tc num to
>> 4,
>> using tc qdisc shows some queue does not have a default pfifo mqprio
>> attached.
>
> Really? Then what did it show?
> [I assume it has some pfifo attached, and it's an mqprio dump kind of an issue]
When queue size of the ndev is 16 and tc num is 3, we set the real queue size to
15 ( 5 * 3 = 15), mqprio only attach pfifo to the first 15 queue, when tc num change
to 4 by DCB, we set the real queue size to 16 (4 * 4 = 16).
So tc qdisc shows the last queue has no qdisc attached.
>
>>
>> Maybe we can add a callback to notify mqprio the configuration has changed.
>>
>
> Which would do what?
> You already have the notifications available for monitoring using dcbnl logic if the
> configuration change [for user]; So user can re-configure whatever it wants.
Yes, if user is only using dcb tool.
> But other than dropping all the qdisc configurations and going back to the default
> qdiscs, what default action would mqprio be able to do when configuration changes
> that actually makes sense?
As explained above, after dcb changing the configuration, some queue may have no qdisc
attached, so I was thinking maybe we can add pfifo to it if there is no qdsic attached
to it.
Thanks,
Yunsheng Lin
>
>> Thanks
>> Yunsheng Lin
>>
>>>
>>> Cheers,
>>> Yuval
>>>
>>>>
>>>> Yunsheng Lin (2):
>>>> mqprio: Add a new hardware offload type in mqprio
>>>> net: hns3: Add mqprio hardware offload support in hns3 driver
>>>>
>>>> drivers/net/ethernet/hisilicon/hns3/hnae3.h | 1 +
>>>> .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++++++++++
>>>> .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46
>> ++++++++++++++-
>>>> -------
>>>> include/uapi/linux/pkt_sched.h | 1 +
>>>> 4 files changed, 55 insertions(+), 16 deletions(-)
>>>>
>>>> --
>>>> 1.9.1
>>>
>>>
>>>
>
^ permalink raw reply
* Re: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver
From: Yunsheng Lin @ 2017-10-16 1:55 UTC (permalink / raw)
To: Yuval Mintz, davem@davemloft.net
Cc: huangdaode@hisilicon.com, xuwei5@hisilicon.com,
liguozhu@hisilicon.com, Yisen.Zhuang@huawei.com,
gabriele.paoloni@huawei.com, john.garry@huawei.com,
linuxarm@huawei.com, salil.mehta@huawei.com, lipeng321@huawei.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
amritha.nambiar@intel.com
In-Reply-To: <AM0PR0502MB368382B73695B8DA7B360E71BF4E0@AM0PR0502MB3683.eurprd05.prod.outlook.com>
Hi, Yuval
On 2017/10/15 16:51, Yuval Mintz wrote:
>>>>> This patchset adds a new hardware offload type in mqprio before
>> adding
>>>>> mqprio hardware offload support in hns3 driver.
>
> Apparently Dave has already accepted Amirtha's changes to mqprio:
> https://marc.info/?l=linux-netdev&m=150803219824053&w=2
> so I guess you need to revise your patchs to align to the new conventions.
Ok.
"If offloads are supported by setting the 'hw' option to 1, the default
offload mode is 'dcb' where only the TC values are offloaded to the
device. "
According to the description of the above patchset, the default mode is already
dcb, so i will drop the dcb mode patch.
I think the scenario you mentioned still existed, and I am willing to implement
it if we come to a solution that will suit most in the community.
Thanks,
Yunsheng Lin
>
>>>>
>>>> I think one of the biggest issues in tying this to DCB configuration is the
>>>> non-immediate [and possibly non persistent] configuration.
>>>>
>>>> Scenario #1:
>>>> User is configuring mqprio offloaded with 3 TCs while device is in willing
>>> mode.
>>>> Would you expect the driver to immediately respond with a success or
>>> instead
>>>> delay the return until the DCBx negotiation is complete and the
>> operational
>>>> num of TCs is actually 3?
>>>
>>> Well, when user requsts the mqprio offloaded by a hardware shared by
>> DCB,
>>> I expect
>>> the user is not using the dcb tool.
>>> If user is still using dcb tool, then result is undefined.
>>>
>>> The scenario you mention maybe can be enforced by setting willing to zero
>>> when user
>>> is requesting the mqprio offload, and restore the willing bit when unloaded
>>> the mqprio
>>> offload.
>>
>> Sounds a bit harsh but would probably work.
>>
>>> But I think the real issue is that dcb and mqprio shares the tc system in the
>>> stack,
>>> the problem may be better to be fixed in the stack rather than in the
>> driver,
>>> as you
>>> suggested in the DCB patchset. What do you think?
>>
>> What did you have in mind?
>>
>>>
>>>>
>>>> Scenario #2:
>>>> Assume user explicitly offloaded mqprio with 3 TCs, but now DCB
>>> configuration
>>>> has changed on the peer side and 4 TCs is the new negotiated operational
>>> value.
>>>> Your current driver logic would change the number of TCs underneath
>> the
>>> user
>>>> configuration [and it would actually probably work due to mqprio being a
>>> crappy
>>>> qdisc]. But was that the user actual intention?
>>>> [I think the likely answer in this scenario is 'yes' since the alternative is no
>>> better.
>>>> But I still thought it was worth mentioning]
>>>
>>> You are right, the problem also have something to do with mqprio and dcb
>>> sharing
>>> the tc in the stack.
>>>
>>> Druing testing, when user explicitly offloaded mqprio with 3 TCs, all
>>> queue has a default pfifo mqprio attached, after DCB changes the tc num
>> to
>>> 4,
>>> using tc qdisc shows some queue does not have a default pfifo mqprio
>>> attached.
>>
>> Really? Then what did it show?
>> [I assume it has some pfifo attached, and it's an mqprio dump kind of an
>> issue]
>>
>>>
>>> Maybe we can add a callback to notify mqprio the configuration has
>> changed.
>>>
>>
>> Which would do what?
>> You already have the notifications available for monitoring using dcbnl logic if
>> the
>> configuration change [for user]; So user can re-configure whatever it wants.
>> But other than dropping all the qdisc configurations and going back to the
>> default
>> qdiscs, what default action would mqprio be able to do when configuration
>> changes
>> that actually makes sense?
>>
>>> Thanks
>>> Yunsheng Lin
>>>
>>>>
>>>> Cheers,
>>>> Yuval
>>>>
>>>>>
>>>>> Yunsheng Lin (2):
>>>>> mqprio: Add a new hardware offload type in mqprio
>>>>> net: hns3: Add mqprio hardware offload support in hns3 driver
>>>>>
>>>>> drivers/net/ethernet/hisilicon/hns3/hnae3.h | 1 +
>>>>> .../net/ethernet/hisilicon/hns3/hns3pf/hclge_dcb.c | 23 +++++++++++
>>>>> .../net/ethernet/hisilicon/hns3/hns3pf/hns3_enet.c | 46
>>> ++++++++++++++-
>>>>> -------
>>>>> include/uapi/linux/pkt_sched.h | 1 +
>>>>> 4 files changed, 55 insertions(+), 16 deletions(-)
>>>>>
>>>>> --
>>>>> 1.9.1
>>>>
>>>>
>>>>
>
^ 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