* 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
* Re: routing UAPI mismatch invalid state behavior?
From: David Ahern @ 2017-10-16 1:56 UTC (permalink / raw)
To: Alexander Aring, netdev; +Cc: Jamal Hadi Salim
In-Reply-To: <CAOHTApje2ABpRPghhqabD=JkBOzjFjVmc1N67ReDBLHeGGVe1Q@mail.gmail.com>
On 10/15/17 11:02 AM, Alexander Aring wrote:
> 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
This fails in fib_check_nh. Basically the lookup of the address fails
making it an invalid gateway.
>
> so it will forbid me to do that, but:
>
> $ ip route replace 169.254.65.37 dev dummy0
This succeeds because iproute2 adds NLM_F_CREATE to flags along with
REPLACE:
if (matches(*argv, "replace") == 0)
return iproute_modify(RTM_NEWROUTE,
NLM_F_CREATE|NLM_F_REPLACE,
argc-1, argv+1);
but your overall intent here of resolving 169.254.65.37 is the key.
> $ ip route replace default via 169.254.65.37 dev dummy0
The existence of the previous route allows the gateway check in
fib_check_nh to succeed.
> $ 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?
There are no recursive checks to gateway lookups once the routes are
installed. With the way routes and nexthops are currently created doing
so would be very expensive in some setups.
^ permalink raw reply
* To learn the netdev code, please give me some advice...
From: admin @ 2017-10-16 2:23 UTC (permalink / raw)
To: netdev
Hi, all
I want to learn network programming, and hope to read the kernel
code of netdev.
Please give me some advice.
Thanks!
^ permalink raw reply
* RE: [PATCH v2 5/5] fsl/fman: add dpaa in module names
From: Madalin-cristian Bucur @ 2017-10-16 3:15 UTC (permalink / raw)
To: Florian Fainelli, netdev@vger.kernel.org, davem@davemloft.net
Cc: andrew@lunn.ch, vivien.didelot@savoirfairelinux.com,
junote@outlook.com, linux-kernel@vger.kernel.org
In-Reply-To: <ba387138-03e5-52df-dc5b-98c6c9d57be8@gmail.com>
> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Friday, October 13, 2017 8:39 PM
> To: Madalin-cristian Bucur <madalin.bucur@nxp.com>;
> netdev@vger.kernel.org; davem@davemloft.net
> Cc: andrew@lunn.ch; vivien.didelot@savoirfairelinux.com;
> junote@outlook.com; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 5/5] fsl/fman: add dpaa in module names
>
> On 10/13/2017 07:50 AM, Madalin Bucur wrote:
> > Signed-off-by: Madalin Bucur <madalin.bucur@nxp.com>
>
> You should provide a line or two to explain why are you making this
> change, it is to resolve modular build configurations?
This change just renames the FMan driver modules, using a common prefix for
the DPAA FMan and DPAA Ethernet drivers. Besides making the names more aligned,
this allows writing udev rules that match on either driver name, if needed,
using the fsl_dpaa_* prefix. The change of netdev dev required for the DSA
probing makes the previous rules written using this prefix fail, this change
makes them work again.
> > ---
> > drivers/net/ethernet/freescale/fman/Makefile | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/fman/Makefile
> b/drivers/net/ethernet/freescale/fman/Makefile
> > index 2c38119..4ae524a 100644
> > --- a/drivers/net/ethernet/freescale/fman/Makefile
> > +++ b/drivers/net/ethernet/freescale/fman/Makefile
> > @@ -1,9 +1,9 @@
> > subdir-ccflags-y += -I$(srctree)/drivers/net/ethernet/freescale/fman
> >
> > -obj-$(CONFIG_FSL_FMAN) += fsl_fman.o
> > -obj-$(CONFIG_FSL_FMAN) += fsl_fman_port.o
> > -obj-$(CONFIG_FSL_FMAN) += fsl_mac.o
> > +obj-$(CONFIG_FSL_FMAN) += fsl_dpaa_fman.o
> > +obj-$(CONFIG_FSL_FMAN) += fsl_dpaa_fman_port.o
> > +obj-$(CONFIG_FSL_FMAN) += fsl_dpaa_mac.o
> >
> > -fsl_fman-objs := fman_muram.o fman.o fman_sp.o fman_keygen.o
> > -fsl_fman_port-objs := fman_port.o
> > -fsl_mac-objs:= mac.o fman_dtsec.o fman_memac.o fman_tgec.o
> > +fsl_dpaa_fman-objs := fman_muram.o fman.o fman_sp.o fman_keygen.o
> > +fsl_dpaa_fman_port-objs := fman_port.o
> > +fsl_dpaa_mac-objs:= mac.o fman_dtsec.o fman_memac.o fman_tgec.o
> >
>
>
> --
> Florian
^ permalink raw reply
* RE: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine
From: Andy Duan @ 2017-10-16 3:41 UTC (permalink / raw)
To: Troy Kisky, shawn.guo@linaro.org, netdev@vger.kernel.org,
davem@davemloft.net
Cc: Fabio Estevam, lznuaa@gmail.com, andrew@lunn.ch
In-Reply-To: <20171014020940.32736-2-troy.kisky@boundarydevices.com>
From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Saturday, October 14, 2017 10:10 AM
>This is better for code locality and should slightly speed up normal interrupts.
>
>This also allows PPS clock output to start working for i.mx7. This is because
>i.mx7 was already using the limit of 3 interrupts, and needed another.
>
>Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>
>---
>
>v2: made this change independent of any devicetree change so that old dtbs
>continue to work.
>
>Continue to register ptp clock if interrupt is not found.
>---
> drivers/net/ethernet/freescale/fec.h | 3 +-
> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>drivers/net/ethernet/freescale/fec_ptp.c | 82 ++++++++++++++++++--------
>-----
> 3 files changed, 65 insertions(+), 45 deletions(-)
>
>diff --git a/drivers/net/ethernet/freescale/fec.h
>b/drivers/net/ethernet/freescale/fec.h
>index ede1876a9a19..be56ac1f1ac4 100644
>--- a/drivers/net/ethernet/freescale/fec.h
>+++ b/drivers/net/ethernet/freescale/fec.h
>@@ -582,12 +582,11 @@ struct fec_enet_private {
> u64 ethtool_stats[0];
> };
>
>-void fec_ptp_init(struct platform_device *pdev);
>+void fec_ptp_init(struct platform_device *pdev, int irq_index);
> void fec_ptp_stop(struct platform_device *pdev); void
>fec_ptp_start_cyclecounter(struct net_device *ndev); int fec_ptp_set(struct
>net_device *ndev, struct ifreq *ifr); int fec_ptp_get(struct net_device *ndev,
>struct ifreq *ifr); -uint fec_ptp_check_pps_event(struct fec_enet_private
>*fep);
>
>
>/**********************************************************
>******************/
> #endif /* FEC_H */
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 3dc2d771a222..21afabbc560f 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
> ret = IRQ_HANDLED;
> complete(&fep->mdio_done);
> }
>-
>- if (fep->ptp_clock)
>- if (fec_ptp_check_pps_event(fep))
>- ret = IRQ_HANDLED;
> return ret;
> }
>
>@@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
> struct device_node *np = pdev->dev.of_node, *phy_node;
> int num_tx_qs;
> int num_rx_qs;
>+ char irq_name[8];
>+ int irq_cnt;
>
> fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>
>@@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
> if (ret)
> goto failed_reset;
>
>+ irq_cnt = platform_irq_count(pdev);
>+ if (irq_cnt > FEC_IRQ_NUM)
>+ irq_cnt = FEC_IRQ_NUM; /* last for ptp */
>+ else if (irq_cnt == 2)
>+ irq_cnt = 1; /* last for ptp */
>+ else if (irq_cnt <= 0)
>+ irq_cnt = 1; /* Let the for loop fail */
Don't do like this. Don't suppose pps interrupt is the last one.
And if irq_cnt is 1 like imx28/imx5x, the patch will break fec interrupt function.
I suggest to use .platform_get_irq_byname() to get pps(ptp) interrupt like your v1 logic check.
>+
> if (fep->bufdesc_ex)
>- fec_ptp_init(pdev);
>+ fec_ptp_init(pdev, irq_cnt);
>
> ret = fec_enet_init(ndev);
> if (ret)
> goto failed_init;
>
>- for (i = 0; i < FEC_IRQ_NUM; i++) {
>- irq = platform_get_irq(pdev, i);
>+ for (i = 0; i < irq_cnt; i++) {
>+ sprintf(irq_name, "int%d", i);
>+ irq = platform_get_irq_byname(pdev, irq_name);
>+ if (irq < 0)
>+ irq = platform_get_irq(pdev, i);
> if (irq < 0) {
>- if (i)
>- break;
> ret = irq;
> goto failed_irq;
> }
>diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
>b/drivers/net/ethernet/freescale/fec_ptp.c
>index 6ebad3fac81d..3abeee0d16dd 100644
>--- a/drivers/net/ethernet/freescale/fec_ptp.c
>+++ b/drivers/net/ethernet/freescale/fec_ptp.c
>@@ -549,6 +549,37 @@ static void fec_time_keep(struct work_struct *work)
> schedule_delayed_work(&fep->time_keep, HZ); }
>
>+/* This function checks the pps event and reloads the timer compare
>+counter. */ static irqreturn_t fec_ptp_interrupt(int irq, void *dev_id)
>+{
>+ struct net_device *ndev = dev_id;
>+ struct fec_enet_private *fep = netdev_priv(ndev);
>+ u32 val;
>+ u8 channel = fep->pps_channel;
>+ struct ptp_clock_event event;
>+
>+ val = readl(fep->hwp + FEC_TCSR(channel));
>+ if (val & FEC_T_TF_MASK) {
>+ /* Write the next next compare(not the next according the
>spec)
>+ * value to the register
>+ */
>+ writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
>+ do {
>+ writel(val, fep->hwp + FEC_TCSR(channel));
>+ } while (readl(fep->hwp + FEC_TCSR(channel)) &
>FEC_T_TF_MASK);
>+
>+ /* Update the counter; */
>+ fep->next_counter = (fep->next_counter + fep-
>>reload_period) &
>+ fep->cc.mask;
>+
>+ event.type = PTP_CLOCK_PPS;
>+ ptp_clock_event(fep->ptp_clock, &event);
>+ return IRQ_HANDLED;
>+ }
>+
>+ return IRQ_NONE;
>+}
>+
> /**
> * fec_ptp_init
> * @ndev: The FEC network adapter
>@@ -558,10 +589,12 @@ static void fec_time_keep(struct work_struct *work)
> * cyclecounter init routine and exits.
> */
>
>-void fec_ptp_init(struct platform_device *pdev)
>+void fec_ptp_init(struct platform_device *pdev, int irq_index)
> {
> struct net_device *ndev = platform_get_drvdata(pdev);
> struct fec_enet_private *fep = netdev_priv(ndev);
>+ int irq;
>+ int ret;
>
> fep->ptp_caps.owner = THIS_MODULE;
> snprintf(fep->ptp_caps.name, 16, "fec ptp"); @@ -587,6 +620,20 @@
>void fec_ptp_init(struct platform_device *pdev)
>
> INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);
>
>+ irq = platform_get_irq_byname(pdev, "ptp");
>+ if (irq < 0)
>+ irq = platform_get_irq(pdev, irq_index);
>+ /* Failure to get an irq is not fatal,
>+ * only the PTP_CLOCK_PPS clock events should stop
>+ */
>+ if (irq >= 0) {
>+ ret = devm_request_irq(&pdev->dev, irq, fec_ptp_interrupt,
>+ 0, pdev->name, ndev);
>+ if (ret < 0)
>+ dev_warn(&pdev->dev, "request for ptp irq
>failed(%d)\n",
>+ ret);
>+ }
>+
> fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
> if (IS_ERR(fep->ptp_clock)) {
> fep->ptp_clock = NULL;
>@@ -605,36 +652,3 @@ void fec_ptp_stop(struct platform_device *pdev)
> if (fep->ptp_clock)
> ptp_clock_unregister(fep->ptp_clock);
> }
>-
>-/**
>- * fec_ptp_check_pps_event
>- * @fep: the fec_enet_private structure handle
>- *
>- * This function check the pps event and reload the timer compare counter.
>- */
>-uint fec_ptp_check_pps_event(struct fec_enet_private *fep) -{
>- u32 val;
>- u8 channel = fep->pps_channel;
>- struct ptp_clock_event event;
>-
>- val = readl(fep->hwp + FEC_TCSR(channel));
>- if (val & FEC_T_TF_MASK) {
>- /* Write the next next compare(not the next according the
>spec)
>- * value to the register
>- */
>- writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
>- do {
>- writel(val, fep->hwp + FEC_TCSR(channel));
>- } while (readl(fep->hwp + FEC_TCSR(channel)) &
>FEC_T_TF_MASK);
>-
>- /* Update the counter; */
>- fep->next_counter = (fep->next_counter + fep-
>>reload_period) & fep->cc.mask;
>-
>- event.type = PTP_CLOCK_PPS;
>- ptp_clock_event(fep->ptp_clock, &event);
>- return 1;
>- }
>-
>- return 0;
>-}
>--
>2.11.0
^ permalink raw reply
* Re: [PATCH 3/3][v2] selftests: silence test output by default
From: Michael Ellerman @ 2017-10-16 3:57 UTC (permalink / raw)
To: Shuah Khan, josef, davem, netdev, linux-kselftest; +Cc: Josef Bacik, Shuah Khan
In-Reply-To: <89b70a11-27d9-9b56-2010-66258618cdb4@kernel.org>
Shuah Khan <shuah@kernel.org> writes:
> On 09/19/2017 07:51 AM, josef@toxicpanda.com wrote:
>> From: Josef Bacik <jbacik@fb.com>
>>
>> Some of the networking tests are very noisy and make it impossible to
>> see if we actually passed the tests as they run. Default to suppressing
>> the output from any tests run in order to make it easier to track what
>> failed.
>>
>> Signed-off-by: Josef Bacik <jbacik@fb.com>
>> ---
>> v1->v2:
>> - dump output into /tmp/testname instead of /dev/null
>>
>
> Thanks for the fix. Applied to linux-kselftest for 4.14-rc2
Sorry this is not a fix.
This is a regression, it breaks all my test infrastructure, because we
use the output of the test case.
Can we please revert this and fix any tests that are overly verbose.
cheers
^ permalink raw reply
* RE: [PATCH v2 2/5] dpaa_eth: move of_phy_connect() to the eth driver
From: Madalin-cristian Bucur @ 2017-10-16 4:26 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev@vger.kernel.org, davem@davemloft.net, f.fainelli@gmail.com,
vivien.didelot@savoirfairelinux.com, junote@outlook.com,
linux-kernel@vger.kernel.org
In-Reply-To: <20171015183349.GD6374@lunn.ch>
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Sunday, October 15, 2017 9:34 PM
> To: Madalin-cristian Bucur <madalin.bucur@nxp.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; f.fainelli@gmail.com;
> vivien.didelot@savoirfairelinux.com; junote@outlook.com; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v2 2/5] dpaa_eth: move of_phy_connect() to the eth
> driver
>
> 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
Hi Andrew,
This is obsolete and it will be removed, I'll send a v3. It remained there from
a time it used to be valid (the original DPAA Ethernet driver was developed
and maintained out of tree since about 9 years ago). I see this thread on the
subject which is relatively recent:
https://www.spinics.net/lists/netdev/msg404288.html
Thanks,
Madalin
^ permalink raw reply
* Re: [pull request][for-next 00/12] Mellanox, mlx5 IPoIB Muli Pkey support 2017-10-11,Re: [pull request][for-next 00/12] Mellanox, mlx5 IPoIB Muli Pkey support 2017-10-11
From: David Miller @ 2017-10-16 4:45 UTC (permalink / raw)
To: dledford; +Cc: saeedm, netdev, linux-rdma, leonro
In-Reply-To: <af28440c-bb1c-c47b-92dd-60bfbeaa839d@redhat.com>
From: Doug Ledford <dledford@redhat.com>
Date: Sat, 14 Oct 2017 17:19:34 -0400
> On 10/14/2017 2:48 PM, Saeed Mahameed wrote:
>> Hi Dave and Doug,
>>
>> This series includes updates for mlx5 IPoIB offloading driver from Alex
>> and Feras to add the support for Muli Pkey in the mlx5i ipoib offloading netdev,
>> to be merged into net-next and rdma-next trees.
>
> As far as the two IPoIB patches are concerned, they're fine.
>
>> Doug, I am sorry I couldn't base this on rc2 since the series needs and conflicts
>> with a fix that was submitted to rc3, so to keep things simple I based it on rc4,
>> I hope this is ok with you..
>
> No worries, it just means I have to submit it under another branch. But
> I'm already holding one patch series in a stand alone branch, so no big
> deal. And, actually, the IPoIB changes are so small they can simply go
> through Dave's tree if you don't have any dependent code in the IPoIB
> driver to submit after this but still in this devel cycle.
>
>> Please pull and let me know if there's any problem.
>
> Once I hear that Dave is OK with the net changes, I'm ready to pull (if
> I need to).
They look fine and I've pulled this into net-next.
Thanks.
^ permalink raw reply
* Re: [PATCH v1] pch_gbe: Switch to new PCI IRQ allocation API
From: kbuild test robot @ 2017-10-16 6:23 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: kbuild-all, David S. Miller, netdev, Andy Shevchenko
In-Reply-To: <20171013170221.70056-1-andriy.shevchenko@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 2304 bytes --]
Hi Andy,
[auto build test ERROR on net-next/master]
[also build test ERROR on v4.14-rc5 next-20171013]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Andy-Shevchenko/pch_gbe-Switch-to-new-PCI-IRQ-allocation-API/20171016-131405
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c: In function 'pch_gbe_request_irq':
>> drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:1904:30: error: 'pdev' undeclared (first use in this function)
err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
^
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c:1904:30: note: each undeclared identifier is reported only once for each function it appears in
vim +/pdev +1904 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c
1891
1892 /**
1893 * pch_gbe_request_irq - Allocate an interrupt line
1894 * @adapter: Board private structure
1895 * Returns:
1896 * 0: Successfully
1897 * Negative value: Failed
1898 */
1899 static int pch_gbe_request_irq(struct pch_gbe_adapter *adapter)
1900 {
1901 struct net_device *netdev = adapter->netdev;
1902 int err;
1903
> 1904 err = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_ALL_TYPES);
1905 if (err < 0)
1906 return err;
1907
1908 adapter->irq = pci_irq_vector(pdev, 0);
1909
1910 err = request_irq(adapter->irq, &pch_gbe_intr, IRQF_SHARED,
1911 netdev->name, netdev);
1912 if (err)
1913 netdev_err(netdev, "Unable to allocate interrupt Error: %d\n",
1914 err);
1915 netdev_dbg(netdev, "have_msi : %d return : 0x%04x\n",
1916 pci_dev_msi_enabled(adapter->pdev), err);
1917 return err;
1918 }
1919
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51658 bytes --]
^ permalink raw reply
* RE: [PATCH net-next 0/2] Add mqprio hardware offload support in hns3 driver
From: Yuval Mintz @ 2017-10-16 6:25 UTC (permalink / raw)
To: Yunsheng Lin, 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: <f651e2c2-5233-bcc7-7488-3ec5905b92d4@huawei.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.
So there is a qdisc attached - mqprio_attach() attches to all transmission
queues [num_tx_queues] and not only the active ones.
But the flow for mqprio might be lacking the additional qdisc_hash_add()
for the additional queue's qdisc.
>
> >
> >>
> >> 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 7:05 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: <AM0PR0502MB3683372D5A1C9DF65431113CBF4F0@AM0PR0502MB3683.eurprd05.prod.outlook.com>
Hi, Yuval
On 2017/10/16 14:25, Yuval Mintz wrote:
>> 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.
>
> So there is a qdisc attached - mqprio_attach() attches to all transmission
> queues [num_tx_queues] and not only the active ones.
> But the flow for mqprio might be lacking the additional qdisc_hash_add()
> for the additional queue's qdisc.
Yes, I think you may be right.
static void mqprio_attach(struct Qdisc *sch)
{
struct net_device *dev = qdisc_dev(sch);
struct mqprio_sched *priv = qdisc_priv(sch);
struct Qdisc *qdisc, *old;
unsigned int ntx;
/* Attach underlying qdisc */
for (ntx = 0; ntx < dev->num_tx_queues; ntx++) {
qdisc = priv->qdiscs[ntx];
old = dev_graft_qdisc(qdisc->dev_queue, qdisc);
if (old)
qdisc_destroy(old);
----------Only call qdisc_hash_add when ntx < dev->real_num_tx_queues---------------
if (ntx < dev->real_num_tx_queues)
qdisc_hash_add(qdisc, false);
}
kfree(priv->qdiscs);
priv->qdiscs = NULL;
}
>
>>
>>>
>>>>
>>>> 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: [net-next RFC 3/4] openvswitch: Add meter infrastructure
From: Andy Zhou @ 2017-10-16 7:05 UTC (permalink / raw)
To: Pravin Shelar; +Cc: Linux Kernel Network Developers, Joe Stringer, Greg Rose
In-Reply-To: <CAOrHB_Cc1cB7mYbYr+uDqsNusP=xALQAWZo=Mr+X9xzgepGFtw@mail.gmail.com>
On Fri, Oct 13, 2017 at 5:12 PM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Thu, Oct 12, 2017 at 3:38 PM, Andy Zhou <azhou@ovn.org> wrote:
>> OVS kernel datapath so far does not support Openflow meter action.
>> This is the first stab at adding kernel datapath meter support.
>> This implementation supports only drop band type.
>>
>> Signed-off-by: Andy Zhou <azhou@ovn.org>
>> ---
>> net/openvswitch/Makefile | 1 +
>> net/openvswitch/datapath.c | 14 +-
>> net/openvswitch/datapath.h | 3 +
>> net/openvswitch/meter.c | 611 +++++++++++++++++++++++++++++++++++++++++++++
>> net/openvswitch/meter.h | 54 ++++
>> 5 files changed, 681 insertions(+), 2 deletions(-)
>> create mode 100644 net/openvswitch/meter.c
>> create mode 100644 net/openvswitch/meter.h
>>
> ...
>
>> diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c
>> new file mode 100644
>> index 000000000000..f24ebb5f7af4
>> --- /dev/null
>> +++ b/net/openvswitch/meter.c
>
> ....
> ....
>> +static int ovs_meter_cmd_features(struct sk_buff *skb, struct genl_info *info)
>> +{
>> + struct datapath *dp;
>> + struct ovs_header *ovs_header = info->userhdr;
>> + struct sk_buff *reply;
>> + struct ovs_header *ovs_reply_header;
>> + struct nlattr *nla, *band_nla;
>> + int err;
>> +
>> + /* Check that the datapath exists */
>> + ovs_lock();
>> + dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> + ovs_unlock();
>> + if (!dp)
>> + return -ENODEV;
>> +
> why dp check is required for this API?
Is it possible for another core delete the dp, before ovs_lock()
returns? Then, in theory, get_dp() can
return NULL, no?
>
>> + reply = ovs_meter_cmd_reply_start(info, OVS_METER_CMD_FEATURES,
>> + &ovs_reply_header);
>> + if (!reply)
>> + return PTR_ERR(reply);
>> +
>> + if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, U32_MAX) ||
>> + nla_put_u32(reply, OVS_METER_ATTR_MAX_BANDS, DP_MAX_BANDS))
>> + goto nla_put_failure;
>> +
>> + nla = nla_nest_start(reply, OVS_METER_ATTR_BANDS);
>> + if (!nla)
>> + goto nla_put_failure;
>> +
>> + band_nla = nla_nest_start(reply, OVS_BAND_ATTR_UNSPEC);
>> + if (!band_nla)
>> + goto nla_put_failure;
>> + /* Currently only DROP band type is supported. */
>> + if (nla_put_u32(reply, OVS_BAND_ATTR_TYPE, OVS_METER_BAND_TYPE_DROP))
>> + goto nla_put_failure;
>> + nla_nest_end(reply, band_nla);
>> + nla_nest_end(reply, nla);
>> +
>> + genlmsg_end(reply, ovs_reply_header);
>> + return genlmsg_reply(reply, info);
>> +
>> +nla_put_failure:
>> + nlmsg_free(reply);
>> + err = -EMSGSIZE;
>> + return err;
>> +}
>> +
> ....
>
>> +static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info)
>> +{
>> + struct nlattr **a = info->attrs;
>> + struct dp_meter *meter, *old_meter;
>> + struct sk_buff *reply;
>> + struct ovs_header *ovs_reply_header;
>> + struct ovs_header *ovs_header = info->userhdr;
>> + struct datapath *dp;
>> + int err;
>> + u32 meter_id;
>> + bool failed;
>> +
>> + meter = dp_meter_create(a);
>> + if (IS_ERR_OR_NULL(meter))
>> + return PTR_ERR(meter);
>> +
>> + reply = ovs_meter_cmd_reply_start(info, OVS_METER_CMD_SET,
>> + &ovs_reply_header);
>> + if (IS_ERR(reply)) {
>> + err = PTR_ERR(reply);
>> + goto exit_free_meter;
>> + }
>> +
>> + ovs_lock();
>> + dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> + if (!dp) {
>> + err = -ENODEV;
>> + goto exit_unlock;
>> + }
>> +
>> + if (!a[OVS_METER_ATTR_ID]) {
>> + err = -ENODEV;
>> + goto exit_unlock;
>> + }
>> +
>> + meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
>> +
>> + /* Cannot fail after this. */
>> + old_meter = lookup_meter(dp, meter_id);
>> + attach_meter(dp, meter);
>> + ovs_unlock();
>> +
> After the unlock, it is not safe to keep the ref to old_meter. better
> to release lock at the end. we could optimize it later if required.
I see a problem here: the old_meter has not been removed from the list before
unlock. The code should have been:
old_meter = lookup_meter(dp, meter_id);
detch_meter(dp, old_meter);
attach_meter(dp, meter);
ovs_unlock();
Do you still see a problem w.r.t. unlock() here? Once detch_meter() is called,
another thread should not have access to 'old_meter' any more. right?
>
>> + /* Build response with the meter_id and stats from
>> + * the old meter, if any.
>> + */
>> + failed = nla_put_u32(reply, OVS_METER_ATTR_ID, meter_id);
>> + WARN_ON(failed);
>> + if (old_meter) {
>> + spin_lock_bh(&old_meter->lock);
>> + if (old_meter->keep_stats) {
>> + err = ovs_meter_cmd_reply_stats(reply, meter_id,
>> + old_meter);
>> + WARN_ON(err);
>> + }
>> + spin_unlock_bh(&old_meter->lock);
>> + ovs_meter_free(old_meter);
>> + }
>> +
>> + genlmsg_end(reply, ovs_reply_header);
>> + return genlmsg_reply(reply, info);
>> +
>> +exit_unlock:
>> + ovs_unlock();
>> + nlmsg_free(reply);
>> +exit_free_meter:
>> + kfree(meter);
>> + return err;
>> +}
>> +
> ....
>
>> +bool ovs_meter_execute(struct datapath *dp, struct sk_buff *skb,
>> + struct sw_flow_key *key, u32 meter_id)
>> +{
>> + struct dp_meter *meter;
>> + struct dp_meter_band *band;
>> + long long int now_ms = ktime_get_ns() / 1000 / 1000;
>> + long long int long_delta_ms;
>> + u32 delta_ms;
>> + u32 cost;
>> + int i, band_exceeded_max = -1;
>> + u32 band_exceeded_rate = 0;
>> +
>> + meter = lookup_meter(dp, meter_id);
>> + /* Do not drop the packet when there is no meter. */
>> + if (!meter)
>> + return false;
>> +
>> + /* Lock the meter while using it. */
>> + spin_lock(&meter->lock);
>> +
>> + long_delta_ms = (now_ms - meter->used); /* ms */
>> +
>> + /* Make sure delta_ms will not be too large, so that bucket will not
>> + * wrap around below.
>> + */
>> + delta_ms = (long_delta_ms > (long long int)meter->max_delta_t)
>> + ? meter->max_delta_t : (u32)long_delta_ms;
>> +
>> + /* Update meter statistics.
>> + */
>> + meter->used = now_ms;
>> + meter->stats.n_packets += 1;
>> + meter->stats.n_bytes += skb->len;
>> +
>> + /* Bucket rate is either in kilobits per second, or in packets per
>> + * second. We maintain the bucket in the units of either bits or
>> + * 1/1000th of a packet, correspondingly.
>> + * Then, when rate is multiplied with milliseconds, we get the
>> + * bucket units:
>> + * msec * kbps = bits, and
>> + * msec * packets/sec = 1/1000 packets.
>> + *
>> + * 'cost' is the number of bucket units in this packet.
>> + */
>> + cost = (meter->kbps) ? skb->len * 8 : 1000;
>> +
>> + /* Update all bands and find the one hit with the highest rate. */
>> + for (i = 0; i < meter->n_bands; ++i) {
>> + long long int max_bucket_size;
>> +
>> + band = &meter->bands[i];
>> + max_bucket_size = (band->burst_size + band->rate) * 1000;
>> +
>> + band->bucket += delta_ms * band->rate;
>> + if (band->bucket > max_bucket_size)
>> + band->bucket = max_bucket_size;
>> +
>> + if (band->bucket >= cost) {
>> + band->bucket -= cost;
>> + } else if (band->rate > band_exceeded_rate) {
>> + band_exceeded_rate = band->rate;
>> + band_exceeded_max = i;
>> + }
>> + }
>> +
>> + spin_unlock(&meter->lock);
>> +
>> + if (band_exceeded_max >= 0) {
>> + /* Update band statistics. */
>> + band = &meter->bands[band_exceeded_max];
>> + band->stats.n_packets += 1;
>> + band->stats.n_bytes += skb->len;
>> +
> Is it safe to do outside of the sipinlock?
Good catch, those should be covered by the same lock. Will fix.
>
>> + /* Drop band triggered, let the caller drop the 'skb'. */
>> + if (band->type == OVS_METER_BAND_TYPE_DROP)
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
^ permalink raw reply
* Re: [net-next RFC 4/4] openvswitch: Add meter action support
From: Andy Zhou @ 2017-10-16 7:06 UTC (permalink / raw)
To: Pravin Shelar; +Cc: Linux Kernel Network Developers, Joe Stringer, Greg Rose
In-Reply-To: <CAOrHB_AaUYb_s4Mp9gOBsH26P1CHcnEcLjPW8ZDeJxPCESP3pw@mail.gmail.com>
On Fri, Oct 13, 2017 at 5:13 PM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Thu, Oct 12, 2017 at 3:38 PM, Andy Zhou <azhou@ovn.org> wrote:
>> Implements OVS kernel meter action support.
>>
>> Signed-off-by: Andy Zhou <azhou@ovn.org>
>> ---
>> include/uapi/linux/openvswitch.h | 1 +
>> net/openvswitch/actions.c | 12 ++++++++++++
>> net/openvswitch/datapath.h | 1 +
>> net/openvswitch/flow_netlink.c | 6 ++++++
>> 4 files changed, 20 insertions(+)
>>
>> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
>> index 325049a129e4..11fe1a06cdd6 100644
>> --- a/include/uapi/linux/openvswitch.h
>> +++ b/include/uapi/linux/openvswitch.h
>> @@ -835,6 +835,7 @@ enum ovs_action_attr {
>> OVS_ACTION_ATTR_TRUNC, /* u32 struct ovs_action_trunc. */
>> OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
>> OVS_ACTION_ATTR_POP_ETH, /* No argument. */
>> + OVS_ACTION_ATTR_METER, /* u32 meter ID. */
>>
>> __OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
>> * from userspace. */
>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>> index a54a556fcdb5..4eb160ac5a27 100644
>> --- a/net/openvswitch/actions.c
>> +++ b/net/openvswitch/actions.c
>> @@ -1210,6 +1210,12 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>> case OVS_ACTION_ATTR_POP_ETH:
>> err = pop_eth(skb, key);
>> break;
>> +
>> + case OVS_ACTION_ATTR_METER:
>> + if (ovs_meter_execute(dp, skb, key, nla_get_u32(a))) {
>> + consume_skb(skb);
>> + return 0;
>> + }
>> }
>>
>> if (unlikely(err)) {
>> @@ -1341,6 +1347,12 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb,
>> err = do_execute_actions(dp, skb, key,
>> acts->actions, acts->actions_len);
>>
>> + /* OVS action has dropped the packet, do not expose it
>> + * to the user.
>> + */
>> + if (err == -ENODATA)
>> + err = 0;
>> +
> I am not sure who is returning this error code?
Ah, this hunk was left over from experimenting with per band actions
list. Will remove. Thanks for catching this!
^ 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