* RE: [Intel-gfx] [PATCH 1/3] treewide: Lift switch variables out of switches
From: Jani Nikula @ 2019-01-23 15:46 UTC (permalink / raw)
To: Edwin Zimmerman, 'Greg KH', 'Kees Cook'
Cc: dev, 'Ard Biesheuvel', netdev, intel-gfx, linux-usb,
linux-kernel, dri-devel, linux-mm, linux-security-module,
kernel-hardening, intel-wired-lan, linux-fsdevel, xen-devel,
'Laura Abbott', linux-kbuild, 'Alexander Popov'
In-Reply-To: <000001d4b32a$845e06e0$8d1a14a0$@211mainstreet.net>
On Wed, 23 Jan 2019, Edwin Zimmerman <edwin@211mainstreet.net> wrote:
> On Wed, 23 Jan 2019, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Wed, 23 Jan 2019, Greg KH <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
>> >> Variables declared in a switch statement before any case statements
>> >> cannot be initialized, so move all instances out of the switches.
>> >> After this, future always-initialized stack variables will work
>> >> and not throw warnings like this:
>> >>
>> >> fs/fcntl.c: In function ‘send_sigio_to_task’:
>> >> fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
>> >> siginfo_t si;
>> >> ^~
>> >
>> > That's a pain, so this means we can't have any new variables in { }
>> > scope except for at the top of a function?
>> >
>> > That's going to be a hard thing to keep from happening over time, as
>> > this is valid C :(
>>
>> Not all valid C is meant to be used! ;)
>
> Very true. The other thing to keep in mind is the burden of enforcing
> a prohibition on a valid C construct like this. It seems to me that
> patch reviewers and maintainers have enough to do without forcing them
> to watch for variable declarations in switch statements. Automating
> this prohibition, should it be accepted, seems like a good idea to me.
Considering that the treewide diffstat to fix this is:
18 files changed, 45 insertions(+), 46 deletions(-)
and using the gcc plugin in question will trigger the switch-unreachable
warning, I think we're good. There'll probably be the occasional
declarations that pass through, and will get fixed afterwards.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [PATCH iproute2-next] Introduce ip-brctl shell script
From: Nikolay Aleksandrov @ 2019-01-23 15:09 UTC (permalink / raw)
To: Stefano Brivio, David Ahern
Cc: Phil Sutter, Eric Garver, Tomas Dolezal, Stephen Hemminger,
Lennert Buytenhek, netdev
In-Reply-To: <ed6b04eab48a70d6416a6b021f04f9901f7e9f01.1547830302.git.sbrivio@redhat.com>
On 18/01/2019 19:00, Stefano Brivio wrote:
> This script wraps 'ip' and 'bridge' tools to provide a drop-in replacement
> of the standalone 'brctl' utility.
>
> It's bug-to-bug compatible with brctl as of bridge-utils version 1.6,
> has no dependencies other than a POSIX shell, and it's less than half
> the binary size of brctl on x86_64.
>
> As many users (including myself) seem to find brctl usage vastly more
> intuitive than ip-link, possibly due to habit, this might be a lightweight
> approach to provide brctl syntax without the need to maintain bridge-utils
> any longer.
>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> Acked-by: Phil Sutter <phil@nwl.cc>
> ---
> man/man8/Makefile | 5 +-
> man/man8/ip-brctl.8 | 187 +++++++++++++++
> misc/Makefile | 9 +-
> misc/ip-brctl.in | 572 ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 770 insertions(+), 3 deletions(-)
> create mode 100644 man/man8/ip-brctl.8
> create mode 100755 misc/ip-brctl.in
Hi,
IMO the effort should be towards improving iproute2 to be
easier to use and more intuitive. We should be pushing people to use the new tools
instead of trying to find workarounds to keep the old tools alive.
I do like to idea of deprecating bridge-utils, but I think it should be done
via improving ip/bridge enough to be pleasant to use. We will have to
maintain this compatibility layer forever if it gets accepted and we'll never
get rid of brctl this way.
Thanks,
Nik
^ permalink raw reply
* Re: [PATCH v2 net] ravb: expand rx descriptor data to accommodate hw checksum
From: Sergei Shtylyov @ 2019-01-23 15:01 UTC (permalink / raw)
To: Simon Horman, David Miller; +Cc: Magnus Damm, netdev, linux-renesas-soc
In-Reply-To: <20190123111452.27845-1-horms+renesas@verge.net.au>
On 01/23/2019 02:14 PM, Simon Horman wrote:
> EtherAVB may provide a checksum of packet data appended to packet data. In
> order to allow this checksum to be received by the host descriptor data
> needs to be enlarged by 2 bytes to accommodate the checksum.
>
> In the case of MTU-sized packets without a VLAN tag the
> checksum were already accommodated by virtue of the space reserved for the
> VLAN tag. However, a packet of MTU-size with a VLAN tag consumed all
> packet data space provided by a descriptor leaving no space for the
> trailing checksum.
>
> This was not detected by the driver which incorrectly used the last two
> bytes of packet data as the checksum and truncate the packet by two bytes.
> This resulted all such packets being dropped.
>
> A work around is to disable RX checksum offload
> # ethtool -K eth0 rx off
>
> This patch resolves this problem by increasing the size available for
> packet data in RX descriptors by two bytes.
>
> Tested on R-Car E3 (r8a77990) ES1.0 based Ebisu-4D board
>
> v2
> * Use sizeof(__sum16) directly rather than adding a driver-local
> #define for the size of the checksum provided by the hw (2 bytes).
>
> Fixes: 4d86d3818627 ("ravb: RX checksum offload")
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[...]
MBR, Sergei
^ permalink raw reply
* RE: [Intel-gfx] [PATCH 1/3] treewide: Lift switch variables out of switches
From: Edwin Zimmerman @ 2019-01-23 14:47 UTC (permalink / raw)
To: 'Jani Nikula', 'Greg KH', 'Kees Cook'
Cc: dev, 'Ard Biesheuvel', netdev, intel-gfx, linux-usb,
linux-kernel, dri-devel, linux-mm, linux-security-module,
kernel-hardening, intel-wired-lan, linux-fsdevel, xen-devel,
'Laura Abbott', linux-kbuild, 'Alexander Popov'
In-Reply-To: <874l9z31c5.fsf@intel.com>
On Wed, 23 Jan 2019, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 23 Jan 2019, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> >> Variables declared in a switch statement before any case statements
> >> cannot be initialized, so move all instances out of the switches.
> >> After this, future always-initialized stack variables will work
> >> and not throw warnings like this:
> >>
> >> fs/fcntl.c: In function ‘send_sigio_to_task’:
> >> fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
> >> siginfo_t si;
> >> ^~
> >
> > That's a pain, so this means we can't have any new variables in { }
> > scope except for at the top of a function?
> >
> > That's going to be a hard thing to keep from happening over time, as
> > this is valid C :(
>
> Not all valid C is meant to be used! ;)
Very true. The other thing to keep in mind is the burden of enforcing a prohibition on a valid C construct like this.
It seems to me that patch reviewers and maintainers have enough to do without forcing them to watch for variable
declarations in switch statements. Automating this prohibition, should it be accepted, seems like a good idea to me.
-Edwin Zimmerman
^ permalink raw reply
* Re: [PATCH net-next 3/7] devlink: move devlink health reporter to use devlink msg API
From: Jiri Pirko @ 2019-01-23 14:42 UTC (permalink / raw)
To: Eran Ben Elisha
Cc: netdev, Jiri Pirko, David S. Miller, Saeed Mahameed,
Moshe Shemesh
In-Reply-To: <1548172644-30862-4-git-send-email-eranbe@mellanox.com>
Tue, Jan 22, 2019 at 04:57:20PM CET, eranbe@mellanox.com wrote:
>Move devlink reporter diagnose and dump operations to use the new msg API.
>Redefine the signature of diagnose and dump operations and move the mlx5e
>reporter to use it with the new format.
>
>Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> .../mellanox/mlx5/core/en/reporter_tx.c | 1 +
> include/net/devlink.h | 9 +-
> net/core/devlink.c | 95 +++++--------------
> 3 files changed, 28 insertions(+), 77 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>index fc92850c214a..7238cda670ba 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>@@ -264,6 +264,7 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
> .name = "TX",
> .recover = mlx5e_tx_reporter_recover,
>+ .diagnose = mlx5e_tx_reporter_diagnose,
Unrelated to this patch.
> };
>
> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index fe323e9b14e1..d66de8b80cc2 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -442,17 +442,12 @@ struct devlink_health_reporter;
>
> struct devlink_health_reporter_ops {
> char *name;
>- unsigned int dump_size;
>- unsigned int diagnose_size;
> int (*recover)(struct devlink_health_reporter *reporter,
> void *priv_ctx);
> int (*dump)(struct devlink_health_reporter *reporter,
>- struct devlink_health_buffer **buffers_array,
>- unsigned int buffer_size, unsigned int num_buffers,
>- void *priv_ctx);
>+ struct devlink_msg_ctx *msg_ctx, void *priv_ctx);
> int (*diagnose)(struct devlink_health_reporter *reporter,
>- struct devlink_health_buffer **buffers_array,
>- unsigned int buffer_size, unsigned int num_buffers);
>+ struct devlink_msg_ctx *msg_ctx);
> };
>
> struct devlink_ops {
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 57ca096849b3..347b638e6f32 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4555,10 +4555,8 @@ static int devlink_msg_snd(struct genl_info *info,
>
> struct devlink_health_reporter {
> struct list_head list;
>- struct devlink_health_buffer **dump_buffers_array;
>+ struct devlink_msg_ctx *dump_msg_ctx;
> struct mutex dump_lock; /* lock parallel read/write from dump buffers */
>- struct devlink_health_buffer **diagnose_buffers_array;
>- struct mutex diagnose_lock; /* lock parallel read/write from diagnose buffers */
Howcome you don't need the mutex anymore?
> void *priv;
> const struct devlink_health_reporter_ops *ops;
> struct devlink *devlink;
>@@ -4619,9 +4617,7 @@ devlink_health_reporter_create(struct devlink *devlink,
> goto unlock;
> }
>
>- if (WARN_ON(ops->dump && !ops->dump_size) ||
>- WARN_ON(ops->diagnose && !ops->diagnose_size) ||
>- WARN_ON(auto_recover && !ops->recover) ||
>+ if (WARN_ON(auto_recover && !ops->recover) ||
> WARN_ON(graceful_period && !ops->recover)) {
> reporter = ERR_PTR(-EINVAL);
> goto unlock;
>@@ -4633,31 +4629,8 @@ devlink_health_reporter_create(struct devlink *devlink,
> goto unlock;
> }
>
>- if (ops->dump) {
>- reporter->dump_buffers_array =
>- devlink_health_buffers_create(ops->dump_size);
>- if (!reporter->dump_buffers_array) {
>- kfree(reporter);
>- reporter = ERR_PTR(-ENOMEM);
>- goto unlock;
>- }
>- }
>-
>- if (ops->diagnose) {
>- reporter->diagnose_buffers_array =
>- devlink_health_buffers_create(ops->diagnose_size);
>- if (!reporter->diagnose_buffers_array) {
>- devlink_health_buffers_destroy(reporter->dump_buffers_array,
>- DEVLINK_HEALTH_SIZE_TO_BUFFERS(ops->dump_size));
>- kfree(reporter);
>- reporter = ERR_PTR(-ENOMEM);
>- goto unlock;
>- }
>- }
>-
> list_add_tail(&reporter->list, &devlink->reporter_list);
> mutex_init(&reporter->dump_lock);
>- mutex_init(&reporter->diagnose_lock);
>
> reporter->priv = priv;
> reporter->ops = ops;
>@@ -4680,10 +4653,8 @@ devlink_health_reporter_destroy(struct devlink_health_reporter *reporter)
> {
> mutex_lock(&reporter->devlink->lock);
> list_del(&reporter->list);
>- devlink_health_buffers_destroy(reporter->dump_buffers_array,
>- DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>- devlink_health_buffers_destroy(reporter->diagnose_buffers_array,
>- DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size));
>+ if (reporter->dump_msg_ctx)
>+ devlink_msg_ctx_free(reporter->dump_msg_ctx);
> kfree(reporter);
> mutex_unlock(&reporter->devlink->lock);
> }
>@@ -4720,12 +4691,15 @@ static int devlink_health_do_dump(struct devlink_health_reporter *reporter,
> if (reporter->dump_avail)
> return 0;
>
>- devlink_health_buffers_reset(reporter->dump_buffers_array,
>- DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>- err = reporter->ops->dump(reporter, reporter->dump_buffers_array,
>- DEVLINK_HEALTH_BUFFER_SIZE,
>- DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size),
>- priv_ctx);
>+ reporter->dump_msg_ctx = devlink_msg_ctx_alloc();
>+ if (IS_ERR_OR_NULL(reporter->dump_msg_ctx)) {
>+ err = PTR_ERR(reporter->dump_msg_ctx);
>+ reporter->dump_msg_ctx = NULL;
>+ return err;
>+ }
>+
>+ err = reporter->ops->dump(reporter, reporter->dump_msg_ctx,
>+ priv_ctx);
> if (!err) {
> reporter->dump_avail = true;
> reporter->dump_ts = jiffies;
>@@ -4960,7 +4934,7 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
> {
> struct devlink *devlink = info->user_ptr[0];
> struct devlink_health_reporter *reporter;
>- u64 num_of_buffers;
>+ struct devlink_msg_ctx *msg_ctx;
> int err;
>
> reporter = devlink_health_reporter_get_from_info(devlink, info);
>@@ -4970,32 +4944,19 @@ static int devlink_nl_cmd_health_reporter_diagnose_doit(struct sk_buff *skb,
> if (!reporter->ops->diagnose)
> return -EOPNOTSUPP;
>
>- num_of_buffers =
>- DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->diagnose_size);
>+ msg_ctx = devlink_msg_ctx_alloc();
>+ if (IS_ERR_OR_NULL(msg_ctx))
>+ return PTR_ERR(msg_ctx);
>
>- mutex_lock(&reporter->diagnose_lock);
>- devlink_health_buffers_reset(reporter->diagnose_buffers_array,
>- num_of_buffers);
>-
>- err = reporter->ops->diagnose(reporter,
>- reporter->diagnose_buffers_array,
>- DEVLINK_HEALTH_BUFFER_SIZE,
>- num_of_buffers);
>+ err = reporter->ops->diagnose(reporter, msg_ctx);
So this is not needed to be in reporter now? Why it was needed before?
> if (err)
> goto out;
>
>- err = devlink_health_buffer_snd(info,
>- DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>- 0, reporter->diagnose_buffers_array,
>- num_of_buffers);
>- if (err)
>- goto out;
>-
>- mutex_unlock(&reporter->diagnose_lock);
>- return 0;
>+ err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DIAGNOSE,
>+ 0, msg_ctx);
>
> out:
>- mutex_unlock(&reporter->diagnose_lock);
>+ devlink_msg_ctx_free(msg_ctx);
> return err;
> }
>
>@@ -5004,8 +4965,8 @@ devlink_health_dump_clear(struct devlink_health_reporter *reporter)
> {
> reporter->dump_avail = false;
> reporter->dump_ts = 0;
>- devlink_health_buffers_reset(reporter->dump_buffers_array,
>- DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size));
>+ devlink_msg_ctx_free(reporter->dump_msg_ctx);
>+ reporter->dump_msg_ctx = NULL;
> }
>
> static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
>@@ -5013,7 +4974,6 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
> {
> struct devlink *devlink = info->user_ptr[0];
> struct devlink_health_reporter *reporter;
>- u64 num_of_buffers;
> int err;
>
> reporter = devlink_health_reporter_get_from_info(devlink, info);
>@@ -5023,18 +4983,13 @@ static int devlink_nl_cmd_health_reporter_dump_get_doit(struct sk_buff *skb,
> if (!reporter->ops->dump)
> return -EOPNOTSUPP;
>
>- num_of_buffers =
>- DEVLINK_HEALTH_SIZE_TO_BUFFERS(reporter->ops->dump_size);
>-
> mutex_lock(&reporter->dump_lock);
> err = devlink_health_do_dump(reporter, NULL);
> if (err)
> goto out;
>
>- err = devlink_health_buffer_snd(info,
>- DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>- 0, reporter->dump_buffers_array,
>- num_of_buffers);
>+ err = devlink_msg_snd(info, DEVLINK_CMD_HEALTH_REPORTER_DUMP_GET,
>+ 0, reporter->dump_msg_ctx);
>
> out:
> mutex_unlock(&reporter->dump_lock);
>--
>2.17.1
>
^ permalink raw reply
* Re: [PATCH net-next 2/7] net/mlx5e: Move driver to use devlink msg API
From: Jiri Pirko @ 2019-01-23 14:39 UTC (permalink / raw)
To: Eran Ben Elisha
Cc: netdev, Jiri Pirko, David S. Miller, Saeed Mahameed,
Moshe Shemesh
In-Reply-To: <1548172644-30862-3-git-send-email-eranbe@mellanox.com>
Tue, Jan 22, 2019 at 04:57:19PM CET, eranbe@mellanox.com wrote:
>As part of the devlink health reporter diagnose ops callback, the mlx5e TX
>reporter used devlink health buffers API. Which will soon be depracated.
>Modify the reporter to use the new devlink msg API.
>
>The actual set of the new diagnose function will be done later in the
>downstream patch, only when devlink will actually expose a new diagnose
>operation that uses the new API.
>
>Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> .../mellanox/mlx5/core/en/reporter_tx.c | 123 ++++--------------
> 1 file changed, 26 insertions(+), 97 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>index d9675afbb924..fc92850c214a 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/reporter_tx.c
>@@ -192,110 +192,47 @@ static int mlx5e_tx_reporter_recover(struct devlink_health_reporter *reporter,
> }
>
> static int
>-mlx5e_tx_reporter_build_diagnose_output(struct devlink_health_buffer *buffer,
>+mlx5e_tx_reporter_build_diagnose_output(struct devlink_msg_ctx *msg_ctx,
> u32 sqn, u8 state, u8 stopped)
What is "state" and "stopped"? Is "stopped" a bool? Is "state" an enum.
> {
>- int err, i;
>- int nest = 0;
>- char name[20];
>-
>- err = devlink_health_buffer_nest_start(buffer,
>- DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>- if (err)
>- goto buffer_error;
>- nest++;
>-
>- err = devlink_health_buffer_nest_start(buffer,
>- DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>- if (err)
>- goto buffer_error;
>- nest++;
>-
>- sprintf(name, "SQ 0x%x", sqn);
>- err = devlink_health_buffer_put_object_name(buffer, name);
>- if (err)
>- goto buffer_error;
>-
>- err = devlink_health_buffer_nest_start(buffer,
>- DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>- if (err)
>- goto buffer_error;
>- nest++;
>-
>- err = devlink_health_buffer_nest_start(buffer,
>- DEVLINK_ATTR_HEALTH_BUFFER_OBJECT);
>- if (err)
>- goto buffer_error;
>- nest++;
>-
>- err = devlink_health_buffer_nest_start(buffer,
>- DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>- if (err)
>- goto buffer_error;
>- nest++;
>-
>- err = devlink_health_buffer_put_object_name(buffer, "HW state");
>- if (err)
>- goto buffer_error;
>+ int err;
>
>- err = devlink_health_buffer_nest_start(buffer,
>- DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>+ err = devlink_msg_object_start(msg_ctx, "SQ");
> if (err)
>- goto buffer_error;
>- nest++;
>+ return err;
>
>- err = devlink_health_buffer_put_value_u8(buffer, state);
>+ err = devlink_msg_object_start(msg_ctx, NULL);
> if (err)
>- goto buffer_error;
>-
>- devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE */
>- nest--;
>-
>- devlink_health_buffer_nest_end(buffer); /* DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR */
>- nest--;
>+ return err;
>
>- err = devlink_health_buffer_nest_start(buffer,
>- DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_PAIR);
>+ err = devlink_msg_put_name_value_pair(msg_ctx, "sqn", &sqn,
>+ sizeof(sqn), NLA_U32);
> if (err)
>- goto buffer_error;
>- nest++;
>+ return err;
>
>- err = devlink_health_buffer_put_object_name(buffer, "stopped");
>+ err = devlink_msg_put_name_value_pair(msg_ctx, "HW state", &state,
>+ sizeof(state), NLA_U8);
> if (err)
>- goto buffer_error;
>+ return err;
>
>- err = devlink_health_buffer_nest_start(buffer,
>- DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE);
>+ err = devlink_msg_put_name_value_pair(msg_ctx, "stopped", &stopped,
>+ sizeof(stopped), NLA_U8);
> if (err)
>- goto buffer_error;
>- nest++;
>+ return err;
>
>- err = devlink_health_buffer_put_value_u8(buffer, stopped);
>+ err = devlink_msg_object_end(msg_ctx, NULL);
> if (err)
>- goto buffer_error;
>-
>- for (i = 0; i < nest; i++)
>- devlink_health_buffer_nest_end(buffer);
>-
>- return 0;
>+ return err;
>
>-buffer_error:
>- for (i = 0; i < nest; i++)
>- devlink_health_buffer_nest_cancel(buffer);
>+ err = devlink_msg_object_end(msg_ctx, "SQ");
> return err;
> }
>
> static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
>- struct devlink_health_buffer **buffers_array,
>- unsigned int buffer_size,
>- unsigned int num_buffers)
>+ struct devlink_msg_ctx *msg_ctx)
> {
> struct mlx5e_priv *priv = devlink_health_reporter_priv(reporter);
>- unsigned int buff = 0;
>- int i = 0, err = 0;
>-
>- if (buffer_size < MLX5E_TX_REPORTER_PER_SQ_MAX_LEN)
>- return -ENOMEM;
>+ int i, err = 0;
>
> mutex_lock(&priv->state_lock);
>
>@@ -304,7 +241,8 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> return 0;
> }
>
>- while (i < priv->channels.num * priv->channels.params.num_tc) {
>+ for (i = 0; i < priv->channels.num * priv->channels.params.num_tc;
>+ i++) {
> struct mlx5e_txqsq *sq = priv->txq2sq[i];
> u8 state;
>
>@@ -312,15 +250,11 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> if (err)
> break;
>
>- err = mlx5e_tx_reporter_build_diagnose_output(buffers_array[buff],
>- sq->sqn, state,
>+ err = mlx5e_tx_reporter_build_diagnose_output(msg_ctx, sq->sqn,
>+ state,
> netif_xmit_stopped(sq->txq));
This should be an array. On SQ entry : one member of array.
So if you want to change it, you need to do this in 2 patches. One API,
one the actual layout. :/
>- if (err) {
>- if (++buff == num_buffers)
>- break;
>- } else {
>- i++;
>- }
>+ if (err)
>+ break;
> }
>
> mutex_unlock(&priv->state_lock);
>@@ -330,11 +264,6 @@ static int mlx5e_tx_reporter_diagnose(struct devlink_health_reporter *reporter,
> static const struct devlink_health_reporter_ops mlx5_tx_reporter_ops = {
> .name = "TX",
> .recover = mlx5e_tx_reporter_recover,
>- .diagnose_size = MLX5E_MAX_NUM_CHANNELS * MLX5E_MAX_NUM_TC *
>- MLX5E_TX_REPORTER_PER_SQ_MAX_LEN,
>- .diagnose = mlx5e_tx_reporter_diagnose,
So you change the callback, remove it so the dissection is broken.
>- .dump_size = 0,
>- .dump = NULL,
This has 0 relation to the patch. Should be separate.
> };
>
> #define MLX5_REPORTER_TX_GRACEFUL_PERIOD 500
>--
>2.17.1
>
^ permalink raw reply
* Re: [PATCH net-next 1/7] devlink: Add devlink msg API
From: Jiri Pirko @ 2019-01-23 14:31 UTC (permalink / raw)
To: Eran Ben Elisha
Cc: netdev, Jiri Pirko, David S. Miller, Saeed Mahameed,
Moshe Shemesh, Wei Yongjun
In-Reply-To: <1548172644-30862-2-git-send-email-eranbe@mellanox.com>
Tue, Jan 22, 2019 at 04:57:18PM CET, eranbe@mellanox.com wrote:
>Devlink msg is a mechanism to pass descriptors between drivers and
>devlink, in json-like format. The API allows the driver to add objects,
>object pair, value array (nested attributes), value and name.
>
>Driver can use this API to fill the msg context in a format which can be
>translated by the devlink to the netlink message later.
>
>With this API, driver does not need to declare the total size per message
>context, and it dynamically add more messages to the context (bounded by
>the system memory limitation only).
>There is an implicit request to have the preliminary main objects size
>created via this API to be upper bounded by netlink SKB size, as those
>objects do not get fragmented between different SKBs when passed to the
>netlink layer.
>
>Devlink msg will replace devlink buffers for health reporters. Devlink
>buffers which will be deprecated and removed in the downstream patch.
>
>Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>CC: Wei Yongjun <weiyongjun1@huawei.com>
>Reviewed-by: Moshe Shemesh <moshe@mellanox.com>
>---
> include/net/devlink.h | 70 ++++++
> include/uapi/linux/devlink.h | 8 +
> net/core/devlink.c | 455 +++++++++++++++++++++++++++++++++++
> 3 files changed, 533 insertions(+)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index a81a1b7a67d7..fe323e9b14e1 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -424,6 +424,7 @@ struct devlink_region;
>
> typedef void devlink_snapshot_data_dest_t(const void *data);
>
>+struct devlink_msg_ctx;
> struct devlink_health_buffer;
> struct devlink_health_reporter;
>
>@@ -615,6 +616,21 @@ int devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
> u8 *data, u32 snapshot_id,
> devlink_snapshot_data_dest_t *data_destructor);
>
>+int devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype);
devlink_msg_*() functions should work with struct devlink_msg.
devlink_msg_ctx*() functions should work with struct devlink_msg_ctx.
Please be consistent with the naming.
I think you can call this just "struct devlink_msg" of maybe "fmsg" as
for "formatted message" - so it is not confused with any other devlink
netlink message.
>+int devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx);
>+int devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
>+ void *value, u16 value_len, u8 value_nla_type);
>+int devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx,
>+ char *name);
>+int devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
>+ char *name, void *value, u16 value_len,
>+ u8 value_nla_type);
>+int devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
>+ char *name, void **value, u16 *value_len,
>+ u8 *value_nla_type, int array_size);
>+int devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name);
>+int devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name);
>+
> int devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
> int attrtype);
> void devlink_health_buffer_nest_end(struct devlink_health_buffer *buffer);
>@@ -903,6 +919,60 @@ devlink_region_snapshot_create(struct devlink_region *region, u64 data_len,
> return 0;
> }
>
>+static inline int
>+devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype)
>+{
>+ return 0;
>+}
>+
>+static inline int
>+devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx)
>+{
>+ return 0;
>+}
>+
>+static inline int
>+devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
>+ void *value, u16 value_len, u8 value_nla_type)
>+{
>+ return 0;
>+}
>+
>+static inline int
>+devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx,
>+ char *name)
>+{
>+ return 0;
>+}
>+
>+static inline int
>+devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
>+ char *name, void *value, u16 value_len,
>+ u8 value_nla_type)
>+{
>+ return 0;
>+}
>+
>+static inline int
>+devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
>+ char *name, void **value, u16 *value_len,
>+ u8 *value_nla_type, int array_size)
>+{
>+ return 0;
>+}
>+
>+static inline int
>+devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+ return 0;
>+}
>+
>+static inline int
>+devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+ return 0;
>+}
>+
> static inline int
> devlink_health_buffer_nest_start(struct devlink_health_buffer *buffer,
> int attrtype)
>diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
>index 6b26bb2ce4dc..68eeda1d0455 100644
>--- a/include/uapi/linux/devlink.h
>+++ b/include/uapi/linux/devlink.h
>@@ -300,6 +300,14 @@ enum devlink_attr {
> DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_TYPE, /* u8 */
> DEVLINK_ATTR_HEALTH_BUFFER_OBJECT_VALUE_DATA, /* dynamic */
>
>+ DEVLINK_ATTR_MSG_OBJECT, /* nested */
>+ DEVLINK_ATTR_MSG_OBJECT_PAIR, /* nested */
>+ DEVLINK_ATTR_MSG_OBJECT_NAME, /* string */
>+ DEVLINK_ATTR_MSG_OBJECT_VALUE, /* nested */
>+ DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY, /* nested */
>+ DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE, /* u8 */
>+ DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA, /* dynamic */
>+
> DEVLINK_ATTR_HEALTH_REPORTER, /* nested */
> DEVLINK_ATTR_HEALTH_REPORTER_NAME, /* string */
> DEVLINK_ATTR_HEALTH_REPORTER_STATE, /* u8 */
>diff --git a/net/core/devlink.c b/net/core/devlink.c
>index 60248a53c0ad..57ca096849b3 100644
>--- a/net/core/devlink.c
>+++ b/net/core/devlink.c
>@@ -4098,6 +4098,461 @@ devlink_health_buffer_snd(struct genl_info *info,
> return err;
> }
>
>+struct devlink_msg {
Let's call this "struct devlink_msg_item"
>+ struct list_head list;
>+ int attrtype;
>+ u8 nla_type;
>+ u16 len;
>+ int value[0];
>+};
>+
>+struct devlink_msg_ctx {
>+ struct list_head msg_list;
>+ int max_nested_depth;
>+ int curr_nest;
>+};
>+
>+#define DEVLINK_MSG_MAX_SIZE (4096 - GENL_HDRLEN)
>+
>+static struct devlink_msg_ctx *devlink_msg_ctx_alloc(void)
>+{
>+ struct devlink_msg_ctx *msg_ctx;
>+
>+ msg_ctx = kzalloc(sizeof(*msg_ctx), GFP_KERNEL);
>+ if (!msg_ctx)
>+ return ERR_PTR(-ENOMEM);
>+
>+ INIT_LIST_HEAD(&msg_ctx->msg_list);
>+
>+ return msg_ctx;
>+}
>+
>+static void devlink_msg_ctx_free(struct devlink_msg_ctx *msg_ctx)
>+{
>+ struct devlink_msg *msg, *tm;
>+
>+ list_for_each_entry_safe(msg, tm, &msg_ctx->msg_list, list) {
>+ list_del(&msg->list);
>+ kfree(msg);
>+ }
>+ kfree(msg_ctx);
>+}
>+
>+int devlink_msg_nest_start(struct devlink_msg_ctx *msg_ctx, int attrtype)
>+{
>+ struct devlink_msg *nest_msg;
>+
>+ if (attrtype != DEVLINK_ATTR_MSG_OBJECT &&
>+ attrtype != DEVLINK_ATTR_MSG_OBJECT_PAIR &&
>+ attrtype != DEVLINK_ATTR_MSG_OBJECT_VALUE &&
>+ attrtype != DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY)
>+ return -EINVAL;
>+
>+ nest_msg = kzalloc(sizeof(*nest_msg), GFP_KERNEL);
>+ if (!nest_msg)
>+ return -ENOMEM;
>+
>+ nest_msg->attrtype = attrtype;
>+ msg_ctx->curr_nest++;
>+ msg_ctx->max_nested_depth = max(msg_ctx->max_nested_depth,
>+ msg_ctx->curr_nest);
>+ list_add_tail(&nest_msg->list, &msg_ctx->msg_list);
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_nest_start);
>+
>+int devlink_msg_nest_end(struct devlink_msg_ctx *msg_ctx)
>+{
>+ struct devlink_msg *nest_msg;
>+
>+ WARN_ON(!msg_ctx->curr_nest);
>+ nest_msg = kzalloc(sizeof(*nest_msg), GFP_KERNEL);
>+ if (!nest_msg)
>+ return -ENOMEM;
>+
>+ msg_ctx->curr_nest--;
>+ list_add_tail(&nest_msg->list, &msg_ctx->msg_list);
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_nest_end);
>+
>+int devlink_msg_put_value(struct devlink_msg_ctx *msg_ctx,
>+ void *value, u16 value_len, u8 value_nla_type)
>+{
>+ struct devlink_msg *value_msg;
>+
>+ if (value_len > DEVLINK_MSG_MAX_SIZE)
>+ return -EMSGSIZE;
>+
>+ if (value_nla_type != NLA_U8 &&
>+ value_nla_type != NLA_U32 &&
>+ value_nla_type != NLA_U64 &&
>+ value_nla_type != NLA_NUL_STRING &&
>+ value_nla_type != NLA_BINARY)
>+ return -EINVAL;
>+
>+ value_msg = kzalloc(sizeof(*value_msg) + value_len, GFP_KERNEL);
Looks fine.
>+ if (!value_msg)
>+ return -ENOMEM;
>+
>+ value_msg->nla_type = value_nla_type;
>+ value_msg->len = value_len;
>+ value_msg->attrtype = DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA;
>+ memcpy(&value_msg->value, value, value_len);
>+ list_add_tail(&value_msg->list, &msg_ctx->msg_list);
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_put_value);
>+
>+int devlink_msg_put_name(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+ struct devlink_msg *name_msg;
>+
>+ if (strlen(name) + 1 > DEVLINK_MSG_MAX_SIZE)
>+ return -EMSGSIZE;
>+
>+ name_msg = kzalloc(sizeof(*name_msg) + strlen(name) + 1, GFP_KERNEL);
>+ if (!name_msg)
>+ return -ENOMEM;
>+
>+ name_msg->nla_type = NLA_NUL_STRING;
>+ name_msg->len = strlen(name) + 1;
>+ name_msg->attrtype = DEVLINK_ATTR_MSG_OBJECT_NAME;
>+ memcpy(&name_msg->value, name, name_msg->len);
>+ list_add_tail(&name_msg->list, &msg_ctx->msg_list);
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_put_name);
>+
>+int devlink_msg_put_name_value_pair(struct devlink_msg_ctx *msg_ctx,
>+ char *name, void *value, u16 value_len,
>+ u8 value_nla_type)
>+{
>+ int err;
>+
>+ err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_put_name(msg_ctx, name);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_put_value(msg_ctx, value, value_len, value_nla_type);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_nest_end(msg_ctx);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_nest_end(msg_ctx);
>+ if (err)
>+ return err;
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_put_name_value_pair);
>+
>+int devlink_msg_put_name_value_array(struct devlink_msg_ctx *msg_ctx,
>+ char *name, void **value, u16 *value_len,
>+ u8 *value_nla_type, int array_size)
This is limitting the arrays to plain values. One should be able to nest
objects inside. If fact, that is what you can to do with sqs:
object start
name sqs
array start
object start
index 0
xxx yyy
object end
object start
index 1
xxx yyy
object end
array end
object end
So you need to have:
devlink_msg_put_array_start()
devlink_msg_put_array_end()
>+{
>+ int err, i;
>+
>+ err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_put_name(msg_ctx, name);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_nest_start(msg_ctx,
>+ DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY);
>+ if (err)
>+ return err;
>+
>+ for (i = 0; i < array_size; i++) {
>+ err = devlink_msg_nest_start(msg_ctx,
>+ DEVLINK_ATTR_MSG_OBJECT_VALUE);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_put_value(msg_ctx, value[i],
>+ value_len[i], value_nla_type[i]);
>+ if (err)
>+ return err;
>+ err = devlink_msg_nest_end(msg_ctx);
>+ if (err)
>+ return err;
>+ }
>+
>+ err = devlink_msg_nest_end(msg_ctx);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_nest_end(msg_ctx);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_nest_end(msg_ctx);
>+ if (err)
>+ return err;
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_put_name_value_array);
>+
>+int devlink_msg_object_start(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+ int err;
>+
>+ err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT);
>+ if (err)
>+ return err;
>+
>+ if (!name)
>+ return 0;
>+
>+ err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_PAIR);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_put_name(msg_ctx, name);
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_nest_start(msg_ctx, DEVLINK_ATTR_MSG_OBJECT_VALUE);
>+ if (err)
>+ return err;
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_object_start);
>+
>+int devlink_msg_object_end(struct devlink_msg_ctx *msg_ctx, char *name)
>+{
>+ int err;
>+
>+ if (!name)
>+ goto end_object;
>+
>+ err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT_VALUE */
>+ if (err)
>+ return err;
>+
>+ err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT_PAIR */
>+ if (err)
>+ return err;
>+
>+end_object:
>+ err = devlink_msg_nest_end(msg_ctx); /* DEVLINK_ATTR_MSG_OBJECT */
>+ if (err)
>+ return err;
>+
>+ return 0;
>+}
>+EXPORT_SYMBOL_GPL(devlink_msg_object_end);
>+
>+static int
>+devlink_msg_fill_data(struct sk_buff *skb, struct devlink_msg *msg)
>+{
>+ int err;
>+
>+ switch (msg->nla_type) {
>+ case NLA_U8:
>+ err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+ *(u8 *)msg->value);
>+ break;
>+ case NLA_U32:
>+ err = nla_put_u32(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+ *(u32 *)msg->value);
>+ break;
>+ case NLA_U64:
>+ err = nla_put_u64_64bit(skb,
>+ DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+ *(u64 *)msg->value, DEVLINK_ATTR_PAD);
>+ break;
>+ case NLA_NUL_STRING:
>+ err = nla_put_string(skb,
>+ DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+ (char *)&msg->value);
>+ break;
>+ case NLA_BINARY:
>+ err = nla_put(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA,
>+ msg->len, (void *)&msg->value);
>+ break;
>+ default:
>+ err = -EINVAL;
>+ break;
>+ }
>+
>+ return err;
>+}
>+
>+static int
>+devlink_msg_fill_type(struct sk_buff *skb, struct devlink_msg *msg)
>+{
>+ int err;
>+
>+ switch (msg->nla_type) {
>+ case NLA_U8:
>+ err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+ NLA_U8);
>+ break;
>+ case NLA_U32:
>+ err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+ NLA_U32);
>+ break;
>+ case NLA_U64:
>+ err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+ NLA_U64);
>+ break;
>+ case NLA_NUL_STRING:
>+ err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+ NLA_NUL_STRING);
>+ break;
>+ case NLA_BINARY:
>+ err = nla_put_u8(skb, DEVLINK_ATTR_MSG_OBJECT_VALUE_TYPE,
>+ NLA_BINARY);
>+ break;
>+ default:
>+ err = -EINVAL;
>+ break;
>+ }
>+
>+ return err;
>+}
>+
>+static int
>+devlink_msg_prepare_skb(struct sk_buff *skb, struct devlink_msg_ctx *msg_ctx,
>+ int *start)
>+{
>+ struct nlattr **nlattr_arr;
>+ struct devlink_msg *msg;
>+ int j = 0, i = 0;
>+ int err;
>+
>+ nlattr_arr = kcalloc(msg_ctx->max_nested_depth,
>+ sizeof(*nlattr_arr), GFP_KERNEL);
>+ if (!nlattr_arr)
>+ return -EINVAL;
>+
>+ list_for_each_entry(msg, &msg_ctx->msg_list, list) {
>+ if (j < *start) {
>+ j++;
>+ continue;
>+ }
>+
>+ switch (msg->attrtype) {
>+ case DEVLINK_ATTR_MSG_OBJECT:
>+ case DEVLINK_ATTR_MSG_OBJECT_PAIR:
>+ case DEVLINK_ATTR_MSG_OBJECT_VALUE:
>+ case DEVLINK_ATTR_MSG_OBJECT_VALUE_ARRAY:
>+ nlattr_arr[i] = nla_nest_start(skb, msg->attrtype);
>+ if (!nlattr_arr[i]) {
>+ err = -EMSGSIZE;
>+ goto nla_put_failure;
>+ }
>+ i++;
>+ break;
>+ case DEVLINK_ATTR_MSG_OBJECT_VALUE_DATA:
>+ err = devlink_msg_fill_data(skb, msg);
>+ if (err)
>+ goto nla_put_failure;
>+ err = devlink_msg_fill_type(skb, msg);
>+ if (err)
>+ goto nla_put_failure;
>+ break;
>+ case DEVLINK_ATTR_MSG_OBJECT_NAME:
>+ err = nla_put_string(skb, msg->attrtype,
>+ (char *)&msg->value);
>+ if (err)
>+ goto nla_put_failure;
>+ break;
>+ default: /* No attrtype */
>+ nla_nest_end(skb, nlattr_arr[--i]);
>+ break;
>+ }
>+ j++;
>+ if (!i)
>+ *start = j;
>+ }
>+
>+ kfree(nlattr_arr);
>+ return 0;
>+
>+nla_put_failure:
>+ for (j = 0; j < i; j++)
>+ nla_nest_cancel(skb, nlattr_arr[j]);
>+ kfree(nlattr_arr);
>+ return err;
>+}
>+
>+static int devlink_msg_snd(struct genl_info *info,
>+ enum devlink_command cmd, int flags,
>+ struct devlink_msg_ctx *msg_ctx)
>+{
>+ struct sk_buff *skb;
>+ struct nlmsghdr *nlh;
>+ int err, index = 0;
>+ bool last = false;
>+ void *hdr;
>+
>+ while (!last) {
>+ int tmp_index = index;
>+
>+ skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+ if (!skb)
>+ return -ENOMEM;
>+
>+ hdr = genlmsg_put(skb, info->snd_portid, info->snd_seq,
>+ &devlink_nl_family, NLM_F_MULTI, cmd);
>+ if (!hdr)
>+ goto nla_put_failure;
>+
>+ err = devlink_msg_prepare_skb(skb, msg_ctx, &index);
>+ if (!err)
>+ last = true;
>+ else if (err != -EMSGSIZE || tmp_index == index)
>+ goto nla_put_failure;
>+
>+ genlmsg_end(skb, hdr);
>+ err = genlmsg_reply(skb, info);
>+ if (err)
>+ return err;
Looks fine.
>+ }
>+
>+ skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
>+ if (!skb)
>+ return -ENOMEM;
>+ nlh = nlmsg_put(skb, info->snd_portid, info->snd_seq,
>+ NLMSG_DONE, 0, flags | NLM_F_MULTI);
>+ err = genlmsg_reply(skb, info);
>+ if (err)
>+ return err;
>+
>+ return 0;
>+
>+nla_put_failure:
>+ err = -EIO;
>+ nlmsg_free(skb);
>+ return err;
>+}
>+
> struct devlink_health_reporter {
> struct list_head list;
> struct devlink_health_buffer **dump_buffers_array;
>--
>2.17.1
>
^ permalink raw reply
* [PATCH net-next 4/4] mlxsw: spectrum_nve: Enable VXLAN on Spectrum-2
From: Ido Schimmel @ 2019-01-23 14:32 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: davem@davemloft.net, Jiri Pirko, Petr Machata, mlxsw,
Ido Schimmel
In-Reply-To: <20190123143217.17735-1-idosch@mellanox.com>
Enable VXLAN on Spectrum-2 as previous patches added the required
functionality.
Note that for now Spectrum-1 and Spectrum-2 use the same function to
determine whether the VXLAN configuration is valid or not. In the
future, when the driver will be extended to support features not present
in Spectrum-1, two different functions will be needed.
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
---
.../mellanox/mlxsw/spectrum_nve_vxlan.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c
index 91b3c71bf031..93ccd9fc2266 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c
@@ -21,9 +21,9 @@
#define MLXSW_SP_NVE_VXLAN_SUPPORTED_FLAGS (VXLAN_F_UDP_ZERO_CSUM_TX | \
VXLAN_F_LEARN)
-static bool mlxsw_sp1_nve_vxlan_can_offload(const struct mlxsw_sp_nve *nve,
- const struct net_device *dev,
- struct netlink_ext_ack *extack)
+static bool mlxsw_sp_nve_vxlan_can_offload(const struct mlxsw_sp_nve *nve,
+ const struct net_device *dev,
+ struct netlink_ext_ack *extack)
{
struct vxlan_dev *vxlan = netdev_priv(dev);
struct vxlan_config *cfg = &vxlan->cfg;
@@ -240,7 +240,7 @@ mlxsw_sp_nve_vxlan_clear_offload(const struct net_device *nve_dev, __be32 vni)
const struct mlxsw_sp_nve_ops mlxsw_sp1_nve_vxlan_ops = {
.type = MLXSW_SP_NVE_TYPE_VXLAN,
- .can_offload = mlxsw_sp1_nve_vxlan_can_offload,
+ .can_offload = mlxsw_sp_nve_vxlan_can_offload,
.nve_config = mlxsw_sp_nve_vxlan_config,
.init = mlxsw_sp1_nve_vxlan_init,
.fini = mlxsw_sp1_nve_vxlan_fini,
@@ -248,13 +248,6 @@ const struct mlxsw_sp_nve_ops mlxsw_sp1_nve_vxlan_ops = {
.fdb_clear_offload = mlxsw_sp_nve_vxlan_clear_offload,
};
-static bool mlxsw_sp2_nve_vxlan_can_offload(const struct mlxsw_sp_nve *nve,
- const struct net_device *dev,
- struct netlink_ext_ack *extack)
-{
- return false;
-}
-
static bool mlxsw_sp2_nve_vxlan_learning_set(struct mlxsw_sp *mlxsw_sp,
bool learning_en)
{
@@ -374,7 +367,7 @@ static void mlxsw_sp2_nve_vxlan_fini(struct mlxsw_sp_nve *nve)
const struct mlxsw_sp_nve_ops mlxsw_sp2_nve_vxlan_ops = {
.type = MLXSW_SP_NVE_TYPE_VXLAN,
- .can_offload = mlxsw_sp2_nve_vxlan_can_offload,
+ .can_offload = mlxsw_sp_nve_vxlan_can_offload,
.nve_config = mlxsw_sp_nve_vxlan_config,
.init = mlxsw_sp2_nve_vxlan_init,
.fini = mlxsw_sp2_nve_vxlan_fini,
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 3/4] mlxsw: spectrum_nve: Add support for VXLAN on Spectrum-2
From: Ido Schimmel @ 2019-01-23 14:32 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: davem@davemloft.net, Jiri Pirko, Petr Machata, mlxsw,
Ido Schimmel
In-Reply-To: <20190123143217.17735-1-idosch@mellanox.com>
Spectrum-1 and Spectrum-2 are largely backward compatible with regards
to VXLAN. One difference - as explained in previous patch - is that an
underlay RIF needs to be specified instead of an underlay VR during NVE
initialization. This is accomplished by calling the relevant function
that returns the index of such a RIF based on the table ID
(RT_TABLE_MAIN) where underlay look up occurs.
The second difference is that VXLAN learning (snooping) is controlled
via a different register (TNPC).
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
---
.../ethernet/mellanox/mlxsw/spectrum_nve.h | 1 +
.../mellanox/mlxsw/spectrum_nve_vxlan.c | 110 +++++++++++++++++-
2 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.h
index 20d99b41611d..0035640156a1 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve.h
@@ -28,6 +28,7 @@ struct mlxsw_sp_nve {
unsigned int num_nve_tunnels; /* Protected by RTNL */
unsigned int num_max_mc_entries[MLXSW_SP_L3_PROTO_MAX];
u32 tunnel_index;
+ u16 ul_rif_index; /* Reserved for Spectrum */
};
struct mlxsw_sp_nve_ops {
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c
index 60f88e5642e1..91b3c71bf031 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c
@@ -7,6 +7,7 @@
#include <net/vxlan.h>
#include "reg.h"
+#include "spectrum.h"
#include "spectrum_nve.h"
/* Eth (18B) | IPv6 (40B) | UDP (8B) | VxLAN (8B) | Eth (14B) | IPv6 (40B)
@@ -254,14 +255,121 @@ static bool mlxsw_sp2_nve_vxlan_can_offload(const struct mlxsw_sp_nve *nve,
return false;
}
+static bool mlxsw_sp2_nve_vxlan_learning_set(struct mlxsw_sp *mlxsw_sp,
+ bool learning_en)
+{
+ char tnpc_pl[MLXSW_REG_TNPC_LEN];
+
+ mlxsw_reg_tnpc_pack(tnpc_pl, MLXSW_REG_TNPC_TUNNEL_PORT_NVE,
+ learning_en);
+ return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(tnpc), tnpc_pl);
+}
+
+static int
+mlxsw_sp2_nve_vxlan_config_set(struct mlxsw_sp *mlxsw_sp,
+ const struct mlxsw_sp_nve_config *config)
+{
+ char tngcr_pl[MLXSW_REG_TNGCR_LEN];
+ u16 ul_rif_index;
+ int err;
+
+ err = mlxsw_sp_router_ul_rif_get(mlxsw_sp, config->ul_tb_id,
+ &ul_rif_index);
+ if (err)
+ return err;
+ mlxsw_sp->nve->ul_rif_index = ul_rif_index;
+
+ err = mlxsw_sp2_nve_vxlan_learning_set(mlxsw_sp, config->learning_en);
+ if (err)
+ goto err_vxlan_learning_set;
+
+ mlxsw_sp_nve_vxlan_config_prepare(tngcr_pl, config);
+ mlxsw_reg_tngcr_underlay_rif_set(tngcr_pl, ul_rif_index);
+
+ err = mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(tngcr), tngcr_pl);
+ if (err)
+ goto err_tngcr_write;
+
+ return 0;
+
+err_tngcr_write:
+ mlxsw_sp2_nve_vxlan_learning_set(mlxsw_sp, false);
+err_vxlan_learning_set:
+ mlxsw_sp_router_ul_rif_put(mlxsw_sp, ul_rif_index);
+ return err;
+}
+
+static void mlxsw_sp2_nve_vxlan_config_clear(struct mlxsw_sp *mlxsw_sp)
+{
+ char tngcr_pl[MLXSW_REG_TNGCR_LEN];
+
+ mlxsw_reg_tngcr_pack(tngcr_pl, MLXSW_REG_TNGCR_TYPE_VXLAN, false, 0);
+ mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(tngcr), tngcr_pl);
+ mlxsw_sp2_nve_vxlan_learning_set(mlxsw_sp, false);
+ mlxsw_sp_router_ul_rif_put(mlxsw_sp, mlxsw_sp->nve->ul_rif_index);
+}
+
+static int mlxsw_sp2_nve_vxlan_rtdp_set(struct mlxsw_sp *mlxsw_sp,
+ unsigned int tunnel_index,
+ u16 ul_rif_index)
+{
+ char rtdp_pl[MLXSW_REG_RTDP_LEN];
+
+ mlxsw_reg_rtdp_pack(rtdp_pl, MLXSW_REG_RTDP_TYPE_NVE, tunnel_index);
+ mlxsw_reg_rtdp_egress_router_interface_set(rtdp_pl, ul_rif_index);
+
+ return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(rtdp), rtdp_pl);
+}
+
static int mlxsw_sp2_nve_vxlan_init(struct mlxsw_sp_nve *nve,
const struct mlxsw_sp_nve_config *config)
{
- return -EOPNOTSUPP;
+ struct mlxsw_sp *mlxsw_sp = nve->mlxsw_sp;
+ int err;
+
+ err = mlxsw_sp_nve_parsing_set(mlxsw_sp,
+ MLXSW_SP_NVE_VXLAN_PARSING_DEPTH,
+ config->udp_dport);
+ if (err)
+ return err;
+
+ err = mlxsw_sp2_nve_vxlan_config_set(mlxsw_sp, config);
+ if (err)
+ goto err_config_set;
+
+ err = mlxsw_sp2_nve_vxlan_rtdp_set(mlxsw_sp, nve->tunnel_index,
+ nve->ul_rif_index);
+ if (err)
+ goto err_rtdp_set;
+
+ err = mlxsw_sp_router_nve_promote_decap(mlxsw_sp, config->ul_tb_id,
+ config->ul_proto,
+ &config->ul_sip,
+ nve->tunnel_index);
+ if (err)
+ goto err_promote_decap;
+
+ return 0;
+
+err_promote_decap:
+err_rtdp_set:
+ mlxsw_sp2_nve_vxlan_config_clear(mlxsw_sp);
+err_config_set:
+ mlxsw_sp_nve_parsing_set(mlxsw_sp, MLXSW_SP_NVE_DEFAULT_PARSING_DEPTH,
+ config->udp_dport);
+ return err;
}
static void mlxsw_sp2_nve_vxlan_fini(struct mlxsw_sp_nve *nve)
{
+ struct mlxsw_sp_nve_config *config = &nve->config;
+ struct mlxsw_sp *mlxsw_sp = nve->mlxsw_sp;
+
+ mlxsw_sp_router_nve_demote_decap(mlxsw_sp, config->ul_tb_id,
+ config->ul_proto, &config->ul_sip);
+ mlxsw_sp2_nve_vxlan_config_clear(mlxsw_sp);
+ mlxsw_sp_nve_parsing_set(mlxsw_sp, MLXSW_SP_NVE_DEFAULT_PARSING_DEPTH,
+ config->udp_dport);
}
const struct mlxsw_sp_nve_ops mlxsw_sp2_nve_vxlan_ops = {
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 1/4] mlxsw: spectrum: Expose functions to create and destroy underlay RIF
From: Ido Schimmel @ 2019-01-23 14:32 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: davem@davemloft.net, Jiri Pirko, Petr Machata, mlxsw,
Ido Schimmel
In-Reply-To: <20190123143217.17735-1-idosch@mellanox.com>
In Spectrum-2, instead of providing the ID of the virtual router (VR)
where NVE underlay lookups will occur as in Spectrum-1, the ID of a
router interface (RIF) in this VR is required.
Expose functions to create and destroy such a RIF.
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
---
.../net/ethernet/mellanox/mlxsw/spectrum.h | 3 ++
.../ethernet/mellanox/mlxsw/spectrum_router.c | 28 +++++++++++++++++++
2 files changed, 31 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 1fa5c81b209f..9384b108c8c2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -507,6 +507,9 @@ void mlxsw_sp_router_nve_demote_decap(struct mlxsw_sp *mlxsw_sp, u32 ul_tb_id,
const union mlxsw_sp_l3addr *ul_sip);
int mlxsw_sp_router_tb_id_vr_id(struct mlxsw_sp *mlxsw_sp, u32 tb_id,
u16 *vr_id);
+int mlxsw_sp_router_ul_rif_get(struct mlxsw_sp *mlxsw_sp, u32 ul_tb_id,
+ u16 *ul_rif_index);
+void mlxsw_sp_router_ul_rif_put(struct mlxsw_sp *mlxsw_sp, u16 ul_rif_index);
/* spectrum_kvdl.c */
enum mlxsw_sp_kvdl_entry_type {
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 0949404a28e5..230e1f6e192b 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -7595,6 +7595,34 @@ static void mlxsw_sp_ul_rif_put(struct mlxsw_sp_rif *ul_rif)
mlxsw_sp_vr_put(mlxsw_sp, vr);
}
+int mlxsw_sp_router_ul_rif_get(struct mlxsw_sp *mlxsw_sp, u32 ul_tb_id,
+ u16 *ul_rif_index)
+{
+ struct mlxsw_sp_rif *ul_rif;
+
+ ASSERT_RTNL();
+
+ ul_rif = mlxsw_sp_ul_rif_get(mlxsw_sp, ul_tb_id, NULL);
+ if (IS_ERR(ul_rif))
+ return PTR_ERR(ul_rif);
+ *ul_rif_index = ul_rif->rif_index;
+
+ return 0;
+}
+
+void mlxsw_sp_router_ul_rif_put(struct mlxsw_sp *mlxsw_sp, u16 ul_rif_index)
+{
+ struct mlxsw_sp_rif *ul_rif;
+
+ ASSERT_RTNL();
+
+ ul_rif = mlxsw_sp->router->rifs[ul_rif_index];
+ if (WARN_ON(!ul_rif))
+ return;
+
+ mlxsw_sp_ul_rif_put(ul_rif);
+}
+
static int
mlxsw_sp2_rif_ipip_lb_configure(struct mlxsw_sp_rif *rif)
{
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 2/4] mlxsw: spectrum_nve: Breakout common code to a common function
From: Ido Schimmel @ 2019-01-23 14:32 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: davem@davemloft.net, Jiri Pirko, Petr Machata, mlxsw,
Ido Schimmel
In-Reply-To: <20190123143217.17735-1-idosch@mellanox.com>
The configuration of a VXLAN tunnel in Spectrum-1 and Spectrum-2 is
largely the same. To avoid code duplication, breakout the common parts
to a common function that can be invoked from the ASIC-specific code.
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reviewed-by: Petr Machata <petrm@mellanox.com>
---
.../mellanox/mlxsw/spectrum_nve_vxlan.c | 30 ++++++++++++-------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c
index 9ba0b83bd949..60f88e5642e1 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_nve_vxlan.c
@@ -112,13 +112,30 @@ static int mlxsw_sp_nve_parsing_set(struct mlxsw_sp *mlxsw_sp,
return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(mprs), mprs_pl);
}
+static void
+mlxsw_sp_nve_vxlan_config_prepare(char *tngcr_pl,
+ const struct mlxsw_sp_nve_config *config)
+{
+ u8 udp_sport;
+
+ mlxsw_reg_tngcr_pack(tngcr_pl, MLXSW_REG_TNGCR_TYPE_VXLAN, true,
+ config->ttl);
+ /* VxLAN driver's default UDP source port range is 32768 (0x8000)
+ * to 60999 (0xee47). Set the upper 8 bits of the UDP source port
+ * to a random number between 0x80 and 0xee
+ */
+ get_random_bytes(&udp_sport, sizeof(udp_sport));
+ udp_sport = (udp_sport % (0xee - 0x80 + 1)) + 0x80;
+ mlxsw_reg_tngcr_nve_udp_sport_prefix_set(tngcr_pl, udp_sport);
+ mlxsw_reg_tngcr_usipv4_set(tngcr_pl, be32_to_cpu(config->ul_sip.addr4));
+}
+
static int
mlxsw_sp1_nve_vxlan_config_set(struct mlxsw_sp *mlxsw_sp,
const struct mlxsw_sp_nve_config *config)
{
char tngcr_pl[MLXSW_REG_TNGCR_LEN];
u16 ul_vr_id;
- u8 udp_sport;
int err;
err = mlxsw_sp_router_tb_id_vr_id(mlxsw_sp, config->ul_tb_id,
@@ -126,18 +143,9 @@ mlxsw_sp1_nve_vxlan_config_set(struct mlxsw_sp *mlxsw_sp,
if (err)
return err;
- mlxsw_reg_tngcr_pack(tngcr_pl, MLXSW_REG_TNGCR_TYPE_VXLAN, true,
- config->ttl);
- /* VxLAN driver's default UDP source port range is 32768 (0x8000)
- * to 60999 (0xee47). Set the upper 8 bits of the UDP source port
- * to a random number between 0x80 and 0xee
- */
- get_random_bytes(&udp_sport, sizeof(udp_sport));
- udp_sport = (udp_sport % (0xee - 0x80 + 1)) + 0x80;
- mlxsw_reg_tngcr_nve_udp_sport_prefix_set(tngcr_pl, udp_sport);
+ mlxsw_sp_nve_vxlan_config_prepare(tngcr_pl, config);
mlxsw_reg_tngcr_learn_enable_set(tngcr_pl, config->learning_en);
mlxsw_reg_tngcr_underlay_virtual_router_set(tngcr_pl, ul_vr_id);
- mlxsw_reg_tngcr_usipv4_set(tngcr_pl, be32_to_cpu(config->ul_sip.addr4));
return mlxsw_reg_write(mlxsw_sp->core, MLXSW_REG(tngcr), tngcr_pl);
}
--
2.20.1
^ permalink raw reply related
* [PATCH net-next 0/4] mlxsw: Add VXLAN support for Spectrum-2
From: Ido Schimmel @ 2019-01-23 14:32 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: davem@davemloft.net, Jiri Pirko, Petr Machata, mlxsw,
Ido Schimmel
This patchset adds support for VXLAN tunneling on the Spectrum-2 ASIC.
Spectrum-1 and Spectrum-2 are largely backward compatible in this area,
so not too many changes are required.
Patches #1-#2 expose a function and perform small refactoring towards
the actual Spectrum-2 implementation in patches #3-#4.
Patch #3 adds the required initialization steps on Spectrum-2.
Patch #4 finally enables VXLAN on Spectrum-2.
Ido Schimmel (4):
mlxsw: spectrum: Expose functions to create and destroy underlay RIF
mlxsw: spectrum_nve: Breakout common code to a common function
mlxsw: spectrum_nve: Add support for VXLAN on Spectrum-2
mlxsw: spectrum_nve: Enable VXLAN on Spectrum-2
.../net/ethernet/mellanox/mlxsw/spectrum.h | 3 +
.../ethernet/mellanox/mlxsw/spectrum_nve.h | 1 +
.../mellanox/mlxsw/spectrum_nve_vxlan.c | 151 +++++++++++++++---
.../ethernet/mellanox/mlxsw/spectrum_router.c | 28 ++++
4 files changed, 162 insertions(+), 21 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH bpf-next v2 08/16] bpf: functional and min/max reasoning unit tests for JMP32
From: Jiong Wang @ 2019-01-23 14:29 UTC (permalink / raw)
To: Jiong Wang; +Cc: ast, daniel, netdev, oss-drivers
In-Reply-To: <1548076553-31268-9-git-send-email-jiong.wang@netronome.com>
Jiong Wang writes:
<snip>
> +/* BPF_RAND_SEXT_R7 contains 5 instructions, it initializes R7 into a random
> + * negative u32, and sign-extend it into 64-bit.
> + */
> +#define BPF_RAND_SEXT_R7 \
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, \
> + BPF_FUNC_get_prandom_u32), \
> + BPF_MOV64_REG(BPF_REG_7, BPF_REG_0), \
> + BPF_ALU64_IMM(BPF_OR, BPF_REG_7, 0x80000000) \
^~~~ ',' missing
> + BPF_ALU64_IMM(BPF_LSH, BPF_REG_7, 32), \
> + BPF_ALU64_IMM(BPF_ARSH, BPF_REG_7, 32),
^~~~ ',' should be removed
Apology, just found there is a minor fix on testcase that I forget to
commit before generating patch set. One ',' is missing and one ',' should
be removed as described above.
I will correct this in v3 together with other reviews if any.
Regards,
Jiong
^ permalink raw reply
* Re: [PATCH bpf-next 3/3] xsk: add sock_diag interface for AF_XDP
From: Björn Töpel @ 2019-01-23 14:24 UTC (permalink / raw)
To: Daniel Borkmann
Cc: ast, Netdev, Björn Töpel, Karlsson, Magnus,
Magnus Karlsson
In-Reply-To: <176d1001-161c-7ce4-7ccb-8b49375c28a8@iogearbox.net>
Den ons 23 jan. 2019 kl 14:19 skrev Daniel Borkmann <daniel@iogearbox.net>:
>
> On 01/18/2019 02:03 PM, bjorn.topel@gmail.com wrote:
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > This patch adds the sock_diag interface for querying sockets from user
> > space. Tools like iproute2 ss(8) can use this interface to list open
> > AF_XDP sockets.
> >
> > The user-space ABI is defined in linux/xdp_diag.h and includes netlink
> > request and response structs. The request can query sockets and the
> > response contains socket information about the rings, umems, inode and
> > more.
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
>
> Series looks good, few minor nits inline:
>
> > ---
> > include/uapi/linux/xdp_diag.h | 72 +++++++++++++
> > net/xdp/Kconfig | 8 ++
> > net/xdp/Makefile | 1 +
> > net/xdp/xsk.c | 6 +-
> > net/xdp/xsk.h | 12 +++
> > net/xdp/xsk_diag.c | 192 ++++++++++++++++++++++++++++++++++
> > 6 files changed, 286 insertions(+), 5 deletions(-)
> > create mode 100644 include/uapi/linux/xdp_diag.h
> > create mode 100644 net/xdp/xsk.h
> > create mode 100644 net/xdp/xsk_diag.c
> >
> > diff --git a/include/uapi/linux/xdp_diag.h b/include/uapi/linux/xdp_diag.h
> > new file mode 100644
> > index 000000000000..efe8ce281dce
> > --- /dev/null
> > +++ b/include/uapi/linux/xdp_diag.h
> > @@ -0,0 +1,72 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * xdp_diag: interface for query/monitor XDP sockets
> > + * Copyright(c) 2019 Intel Corporation.
> > + */
> > +
> > +#ifndef _LINUX_XDP_DIAG_H
> > +#define _LINUX_XDP_DIAG_H
> > +
> > +#include <linux/types.h>
> > +
> > +struct xdp_diag_req {
> > + __u8 sdiag_family;
> > + __u8 sdiag_protocol;
> > + __u16 pad;
>
> Presumably this one is for future use? Maybe better as '__u16 :16;' to
> avoid compile errors if someone tries to zero 'pad' member manually?
>
The "pad" was there simply to have an explicitly named structure hole.
I'm not following the bitfield argument. Why does that avoid compiler
errors?
> > + __u32 xdiag_ino;
> > + __u32 xdiag_show;
> > + __u32 xdiag_cookie[2];
> > +};
> > +
> > +struct xdp_diag_msg {
> > + __u8 xdiag_family;
> > + __u8 xdiag_type;
> > + __u32 xdiag_ino;
> > + __u32 xdiag_cookie[2];
> > +};
> > +
> > +#define XDP_SHOW_INFO (1 << 0) /* Basic information */
> > +#define XDP_SHOW_RING_CFG (1 << 1)
> > +#define XDP_SHOW_UMEM (1 << 2)
> > +#define XDP_SHOW_MEMINFO (1 << 3)
> > +
> > +enum {
> > + XDP_DIAG_NONE,
> > + XDP_DIAG_INFO,
> > + XDP_DIAG_UID,
> > + XDP_DIAG_RX_RING,
> > + XDP_DIAG_TX_RING,
> > + XDP_DIAG_UMEM,
> > + XDP_DIAG_UMEM_FILL_RING,
> > + XDP_DIAG_UMEM_COMPLETION_RING,
> > + XDP_DIAG_MEMINFO,
> > + __XDP_DIAG_MAX,
> > +};
> > +
> > +#define XDP_DIAG_MAX (__XDP_DIAG_MAX - 1)
> > +
> > +struct xdp_diag_info {
> > + __u32 ifindex;
> > + __u32 queue_id;
> > +};
> > +
> > +struct xdp_diag_ring {
> > + __u32 entries; /*num descs */
> > +};
> > +
> > +#define XDP_DU_F_ZEROCOPY (1 << 0)
> > +
> > +struct xdp_diag_umem {
> > + __u64 size;
> > + __u32 id;
> > + __u32 num_pages;
> > + __u32 chunk_size;
> > + __u32 headroom;
> > + __u32 ifindex;
> > + __u32 queue_id;
> > + __u32 flags;
> > + __u32 refs;
> > +};
> > +
> > +
>
> Nit: one newline too much
>
Good catch!
> > +#endif /* _LINUX_XDP_DIAG_H */
> > diff --git a/net/xdp/Kconfig b/net/xdp/Kconfig
> > index 90e4a7152854..0255b33cff4b 100644
> > --- a/net/xdp/Kconfig
> > +++ b/net/xdp/Kconfig
> > @@ -5,3 +5,11 @@ config XDP_SOCKETS
> > help
> > XDP sockets allows a channel between XDP programs and
> > userspace applications.
> > +
> > +config XDP_SOCKETS_DIAG
> > + tristate "XDP sockets: monitoring interface"
> > + depends on XDP_SOCKETS
> > + default n
> > + help
> > + Support for PF_XDP sockets monitoring interface used by the ss tool.
> > + If unsure, say Y.
> > diff --git a/net/xdp/Makefile b/net/xdp/Makefile
> > index 04f073146256..59dbfdf93dca 100644
> > --- a/net/xdp/Makefile
> > +++ b/net/xdp/Makefile
> > @@ -1 +1,2 @@
> > obj-$(CONFIG_XDP_SOCKETS) += xsk.o xdp_umem.o xsk_queue.o
> > +obj-$(CONFIG_XDP_SOCKETS_DIAG) += xsk_diag.o
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 80ca48cefc42..949d3bbccb2f 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -27,14 +27,10 @@
> >
> > #include "xsk_queue.h"
> > #include "xdp_umem.h"
> > +#include "xsk.h"
> >
> > #define TX_BATCH_SIZE 16
> >
> > -static struct xdp_sock *xdp_sk(struct sock *sk)
> > -{
> > - return (struct xdp_sock *)sk;
> > -}
> > -
> > bool xsk_is_setup_for_bpf_map(struct xdp_sock *xs)
> > {
> > return READ_ONCE(xs->rx) && READ_ONCE(xs->umem) &&
> > diff --git a/net/xdp/xsk.h b/net/xdp/xsk.h
> > new file mode 100644
> > index 000000000000..ba8120610426
> > --- /dev/null
> > +++ b/net/xdp/xsk.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright(c) 2019 Intel Corporation. */
> > +
> > +#ifndef XSK_H_
> > +#define XSK_H_
> > +
> > +static inline struct xdp_sock *xdp_sk(struct sock *sk)
> > +{
> > + return (struct xdp_sock *)sk;
> > +}
> > +
> > +#endif /* XSK_H_ */
> > diff --git a/net/xdp/xsk_diag.c b/net/xdp/xsk_diag.c
> > new file mode 100644
> > index 000000000000..2585d7a4ac2f
> > --- /dev/null
> > +++ b/net/xdp/xsk_diag.c
> > @@ -0,0 +1,192 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* XDP sockets monitoring support
> > + *
> > + * Copyright(c) 2019 Intel Corporation.
> > + *
> > + * Author: Björn Töpel <bjorn.topel@intel.com>
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <net/xdp_sock.h>
> > +#include <linux/xdp_diag.h>
> > +#include <linux/sock_diag.h>
> > +
> > +#include "xsk_queue.h"
> > +#include "xsk.h"
> > +
> > +static int xsk_diag_put_info(const struct xdp_sock *xs, struct sk_buff *nlskb)
> > +{
> > + struct xdp_diag_info di;
>
> Not a bug, but can we generally zero all the structs on stack such that once
> we extend them nothing gets potentially leaked.
>
Makes sense! Will fix here and...
> > + di.ifindex = xs->dev ? xs->dev->ifindex : 0;
> > + di.queue_id = xs->queue_id;
> > + return nla_put(nlskb, XDP_DIAG_INFO, sizeof(di), &di);
> > +}
> > +
> > +static int xsk_diag_put_ring(const struct xsk_queue *queue, int nl_type,
> > + struct sk_buff *nlskb)
> > +{
> > + struct xdp_diag_ring dr;
>
> Ditto.
>
...here...
> > + dr.entries = queue->nentries;
> > + return nla_put(nlskb, nl_type, sizeof(dr), &dr);
> > +}
> > +
> > +static int xsk_diag_put_rings_cfg(const struct xdp_sock *xs,
> > + struct sk_buff *nlskb)
> > +{
> > + int err = 0;
> > +
> > + if (xs->rx)
> > + err = xsk_diag_put_ring(xs->rx, XDP_DIAG_RX_RING, nlskb);
> > + if (!err && xs->tx)
> > + err = xsk_diag_put_ring(xs->tx, XDP_DIAG_TX_RING, nlskb);
> > + return err;
> > +}
> > +
> > +static int xsk_diag_put_umem(const struct xdp_sock *xs, struct sk_buff *nlskb)
> > +{
> > + struct xdp_umem *umem = xs->umem;
> > + struct xdp_diag_umem du;
>
> Ditto
>
...and here.
> > + int err;
> > +
> > + if (!umem)
> > + return 0;
> > +
> > + du.id = umem->id;
> > + du.size = umem->size;
> > + du.num_pages = umem->npgs;
> > + du.chunk_size = (__u32)(~umem->chunk_mask + 1);
> > + du.headroom = umem->headroom;
> > + du.ifindex = umem->dev ? umem->dev->ifindex : 0;
> > + du.queue_id = umem->queue_id;
> > + du.flags = 0;
> > + if (umem->zc)
> > + du.flags |= XDP_DU_F_ZEROCOPY;
> > + du.refs = refcount_read(&umem->users);
> > +
> > + err = nla_put(nlskb, XDP_DIAG_UMEM, sizeof(du), &du);
> > +
> > + if (!err && umem->fq)
> > + err = xsk_diag_put_ring(xs->tx, XDP_DIAG_UMEM_FILL_RING, nlskb);
> > + if (!err && umem->cq) {
> > + err = xsk_diag_put_ring(xs->tx, XDP_DIAG_UMEM_COMPLETION_RING,
> > + nlskb);
> > + }
> > + return err;
> > +}
> > +
> > +static int xsk_diag_fill(struct sock *sk, struct sk_buff *nlskb,
> > + struct xdp_diag_req *req,
> > + struct user_namespace *user_ns,
> > + u32 portid, u32 seq, u32 flags, int sk_ino)
> > +{
> > + struct xdp_sock *xs = xdp_sk(sk);
> > + struct xdp_diag_msg *msg;
> > + struct nlmsghdr *nlh;
> > +
> > + nlh = nlmsg_put(nlskb, portid, seq, SOCK_DIAG_BY_FAMILY, sizeof(*msg),
> > + flags);
> > + if (!nlh)
> > + return -EMSGSIZE;
> > +
> > + msg = nlmsg_data(nlh);
> > + msg->xdiag_family = AF_XDP;
> > + msg->xdiag_type = sk->sk_type;
> > + msg->xdiag_ino = sk_ino;
> > + sock_diag_save_cookie(sk, msg->xdiag_cookie);
>
> Don't we have a hole in struct xdp_diag_msg after xdiag_type? Probably better
> to memset everything in the beginning as well.
>
You're right! Good catch. In general for uapi structures; Explicitly
named "holes" or not? The pad in req is exactly that. So, either add a
pad to _msg or remove in _req. What is preferred?
I'll spin up a v2.
Thanks for looking at the code!
Björn
> > + if ((req->xdiag_show & XDP_SHOW_INFO) && xsk_diag_put_info(xs, nlskb))
> > + goto out_nlmsg_trim;
> > +
> > + if ((req->xdiag_show & XDP_SHOW_INFO) &&
> > + nla_put_u32(nlskb, XDP_DIAG_UID,
> > + from_kuid_munged(user_ns, sock_i_uid(sk))))
> > + goto out_nlmsg_trim;
> > +
> > + if ((req->xdiag_show & XDP_SHOW_RING_CFG) &&
> > + xsk_diag_put_rings_cfg(xs, nlskb))
> > + goto out_nlmsg_trim;
> > +
> > + if ((req->xdiag_show & XDP_SHOW_UMEM) &&
> > + xsk_diag_put_umem(xs, nlskb))
> > + goto out_nlmsg_trim;
> > +
> > + if ((req->xdiag_show & XDP_SHOW_MEMINFO) &&
> > + sock_diag_put_meminfo(sk, nlskb, XDP_DIAG_MEMINFO))
> > + goto out_nlmsg_trim;
> > +
> > + nlmsg_end(nlskb, nlh);
> > + return 0;
> > +
> > +out_nlmsg_trim:
> > + nlmsg_cancel(nlskb, nlh);
> > + return -EMSGSIZE;
> > +}
> > +
> > +static int xsk_diag_dump(struct sk_buff *nlskb, struct netlink_callback *cb)
> > +{
> > + struct xdp_diag_req *req = nlmsg_data(cb->nlh);
> > + struct net *net = sock_net(nlskb->sk);
> > + int num = 0, s_num = cb->args[0];
> > + struct sock *sk;
> > +
> > + mutex_lock(&net->xdp.lock);
> > +
> > + sk_for_each(sk, &net->xdp.list) {
> > + if (!net_eq(sock_net(sk), net))
> > + continue;
> > + if (num++ < s_num)
> > + continue;
> > +
> > + if (xsk_diag_fill(sk, nlskb, req,
> > + sk_user_ns(NETLINK_CB(cb->skb).sk),
> > + NETLINK_CB(cb->skb).portid,
> > + cb->nlh->nlmsg_seq, NLM_F_MULTI,
> > + sock_i_ino(sk)) < 0) {
> > + num--;
> > + break;
> > + }
> > + }
> > +
> > + mutex_unlock(&net->xdp.lock);
> > + cb->args[0] = num;
> > + return nlskb->len;
> > +}
> > +
> > +static int xsk_diag_handler_dump(struct sk_buff *nlskb, struct nlmsghdr *hdr)
> > +{
> > + struct netlink_dump_control c = { .dump = xsk_diag_dump };
> > + int hdrlen = sizeof(struct xdp_diag_req);
> > + struct net *net = sock_net(nlskb->sk);
> > + struct xdp_diag_req *req;
> > +
> > + if (nlmsg_len(hdr) < hdrlen)
> > + return -EINVAL;
> > +
> > + if (!(hdr->nlmsg_flags & NLM_F_DUMP))
> > + return -EOPNOTSUPP;
> > +
> > + req = nlmsg_data(hdr);
> > + return netlink_dump_start(net->diag_nlsk, nlskb, hdr, &c);
> > +}
> > +
> > +static const struct sock_diag_handler xsk_diag_handler = {
> > + .family = AF_XDP,
> > + .dump = xsk_diag_handler_dump,
> > +};
> > +
> > +static int __init xsk_diag_init(void)
> > +{
> > + return sock_diag_register(&xsk_diag_handler);
> > +}
> > +
> > +static void __exit xsk_diag_exit(void)
> > +{
> > + sock_diag_unregister(&xsk_diag_handler);
> > +}
> > +
> > +module_init(xsk_diag_init);
> > +module_exit(xsk_diag_exit);
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS_NET_PF_PROTO_TYPE(PF_NETLINK, NETLINK_SOCK_DIAG, AF_XDP);
> >
>
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/8] libbpf: Add a helper for retrieving a prog via index
From: Jesper Dangaard Brouer @ 2019-01-23 14:24 UTC (permalink / raw)
To: Maciej Fijalkowski; +Cc: Daniel Borkmann, ast, netdev, jakub.kicinski, brouer
In-Reply-To: <20190123144159.000051bd@gmail.com>
On Wed, 23 Jan 2019 14:41:59 +0100
Maciej Fijalkowski <maciejromanfijalkowski@gmail.com> wrote:
> On Wed, 23 Jan 2019 11:41:11 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> > On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
> > > xdp_redirect_cpu has a 6 different XDP programs that can be attached to
> > > network interface. This sample has a option --prognum that allows user
> > > for specifying which particular program from a given set will be
> > > attached to network interface.
> > > In order to make it easier when converting the mentioned sample to
> > > libbpf usage, add a function to libbpf that will return program's fd for
> > > a given index.
> > >
> > > Note that there is already a bpf_object__find_prog_by_idx, which could
> > > be exported and might be used for that purpose, but it operates on the
> > > number of ELF section and here we need an index from a programs array
> > > within the bpf_object.
> >
> > Series in general looks good to me. Few minor comments, mainly in relation
> > to the need for libbpf extensions.
> >
> > Would it not be a better interface to the user to instead choose the prog
> > based on section name and then retrieve it via bpf_object__find_program_by_title()
> > instead of prognum (which feels less user friendly) at least?
> >
>
> I couldn't decide which one from:
> * adding a libbpf helper
> * changing the xdp_redirect_cpu behaviour
>
> would be more invasive when I was converting this sample to libbpf
> support.
>
> Your suggestion sounds good, but I'm wondering about the actual
> implementation. I suppose that we would choose the program via
> command line. Some program section names in this sample are a bit
> long and it might be irritating for user to type in for example
> "xdp_cpu_map5_lb_hash_ip_pairs", no? Or maybe we can live with this.
> In case of typo and program being not found it would be good to print
> out the section names to choose from in usage().
Please feel free to deprecate or remove the xdp_redirect_cpu --prognum
option. I would prefer if this was indeed converted to selecting the
program based on the name in the _kern.c file, instead of a number.
Listing the avail programs will be helpful, and you can also
shorten/rename the program names.
For easy of use, consider doing like 'ip' tool from[1] iproute2, and
find the first best matching string. Copy pasted code below signature
for inspiration.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
[1] git://git.kernel.org/pub/scm/network/iproute2/iproute2.git
lib/utils.c
int matches(const char *cmd, const char *pattern)
{
int len = strlen(cmd);
if (len > strlen(pattern))
return -1;
return memcmp(pattern, cmd, len);
}
ip/ip.c
static const struct cmd {
const char *cmd;
int (*func)(int argc, char **argv);
} cmds[] = {
{ "address", do_ipaddr },
{ "addrlabel", do_ipaddrlabel },
{ "maddress", do_multiaddr },
{ "route", do_iproute },
{ "rule", do_iprule },
{ "neighbor", do_ipneigh },
{ "neighbour", do_ipneigh },
{ "ntable", do_ipntable },
{ "ntbl", do_ipntable },
{ "link", do_iplink },
{ "l2tp", do_ipl2tp },
{ "fou", do_ipfou },
{ "ila", do_ipila },
{ "macsec", do_ipmacsec },
{ "tunnel", do_iptunnel },
{ "tunl", do_iptunnel },
{ "tuntap", do_iptuntap },
{ "tap", do_iptuntap },
{ "token", do_iptoken },
{ "tcpmetrics", do_tcp_metrics },
{ "tcp_metrics", do_tcp_metrics },
{ "monitor", do_ipmonitor },
{ "xfrm", do_xfrm },
{ "mroute", do_multiroute },
{ "mrule", do_multirule },
{ "netns", do_netns },
{ "netconf", do_ipnetconf },
{ "vrf", do_ipvrf},
{ "sr", do_seg6 },
{ "help", do_help },
{ 0 }
};
static int do_cmd(const char *argv0, int argc, char **argv)
{
const struct cmd *c;
for (c = cmds; c->cmd; ++c) {
if (matches(argv0, c->cmd) == 0)
return -(c->func(argc-1, argv+1));
}
fprintf(stderr, "Object \"%s\" is unknown, try \"ip help\".\n", argv0);
return EXIT_FAILURE;
}
^ permalink raw reply
* Re: [Intel-gfx] [PATCH 1/3] treewide: Lift switch variables out of switches
From: Jani Nikula @ 2019-01-23 14:23 UTC (permalink / raw)
To: Greg KH, Kees Cook
Cc: dev, Ard Biesheuvel, netdev, intel-gfx, linux-usb, linux-kernel,
dri-devel, linux-mm, linux-security-module, kernel-hardening,
intel-wired-lan, linux-fsdevel, xen-devel, Laura Abbott,
linux-kbuild, Alexander Popov
In-Reply-To: <874l9z31c5.fsf@intel.com>
On Wed, 23 Jan 2019, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 23 Jan 2019, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
>>> Variables declared in a switch statement before any case statements
>>> cannot be initialized, so move all instances out of the switches.
>>> After this, future always-initialized stack variables will work
>>> and not throw warnings like this:
>>>
>>> fs/fcntl.c: In function ‘send_sigio_to_task’:
>>> fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
>>> siginfo_t si;
>>> ^~
>>
>> That's a pain, so this means we can't have any new variables in { }
>> scope except for at the top of a function?
>>
>> That's going to be a hard thing to keep from happening over time, as
>> this is valid C :(
>
> Not all valid C is meant to be used! ;)
>
> Anyway, I think you're mistaking the limitation to arbitrary blocks
> while it's only about the switch block IIUC.
>
> Can't have:
>
> switch (i) {
> int j;
> case 0:
> /* ... */
> }
>
> because it can't be turned into:
>
> switch (i) {
> int j = 0; /* not valid C */
> case 0:
> /* ... */
> }
>
> but can have e.g.:
>
> switch (i) {
> case 0:
> {
> int j = 0;
> /* ... */
> }
> }
>
> I think Kees' approach of moving such variable declarations to the
> enclosing block scope is better than adding another nesting block.
PS. The patch is
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
and the drivers/gpu/drm/i915/* parts are
Acked-by: Jani Nikula <jani.nikula@intel.com>
for merging via whichever tree is appropriate. (There'll be minor
conflicts with in-flight work in our -next tree, but no biggie.)
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [Intel-gfx] [PATCH 1/3] treewide: Lift switch variables out of switches
From: Jani Nikula @ 2019-01-23 14:17 UTC (permalink / raw)
To: Greg KH, Kees Cook
Cc: dev, Ard Biesheuvel, netdev, intel-gfx, linux-usb, linux-kernel,
dri-devel, linux-mm, linux-security-module, kernel-hardening,
intel-wired-lan, linux-fsdevel, xen-devel, Laura Abbott,
linux-kbuild, Alexander Popov
In-Reply-To: <20190123115829.GA31385@kroah.com>
On Wed, 23 Jan 2019, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
>> Variables declared in a switch statement before any case statements
>> cannot be initialized, so move all instances out of the switches.
>> After this, future always-initialized stack variables will work
>> and not throw warnings like this:
>>
>> fs/fcntl.c: In function ‘send_sigio_to_task’:
>> fs/fcntl.c:738:13: warning: statement will never be executed [-Wswitch-unreachable]
>> siginfo_t si;
>> ^~
>
> That's a pain, so this means we can't have any new variables in { }
> scope except for at the top of a function?
>
> That's going to be a hard thing to keep from happening over time, as
> this is valid C :(
Not all valid C is meant to be used! ;)
Anyway, I think you're mistaking the limitation to arbitrary blocks
while it's only about the switch block IIUC.
Can't have:
switch (i) {
int j;
case 0:
/* ... */
}
because it can't be turned into:
switch (i) {
int j = 0; /* not valid C */
case 0:
/* ... */
}
but can have e.g.:
switch (i) {
case 0:
{
int j = 0;
/* ... */
}
}
I think Kees' approach of moving such variable declarations to the
enclosing block scope is better than adding another nesting block.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply
* Re: [PATCH 3/7] net/ethernet: Add parse_protocol header_ops support
From: Willem de Bruijn @ 2019-01-23 14:14 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: David S. Miller, Saeed Mahameed, Willem de Bruijn, Jason Wang,
Eric Dumazet, netdev@vger.kernel.org, Eran Ben Elisha,
Tariq Toukan
In-Reply-To: <20190114131841.1932-4-maximmi@mellanox.com>
On Mon, Jan 14, 2019 at 8:21 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> The previous commit introduced parse_protocol callback which should
> extract the protocol number from the L2 header. Make all Ethernet
> devices support it.
>
> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com>
> ---
> include/linux/etherdevice.h | 1 +
> net/ethernet/eth.c | 13 +++++++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
> index 2c0af7b00715..e2f3b21cd72a 100644
> --- a/include/linux/etherdevice.h
> +++ b/include/linux/etherdevice.h
> @@ -44,6 +44,7 @@ int eth_header_cache(const struct neighbour *neigh, struct hh_cache *hh,
> __be16 type);
> void eth_header_cache_update(struct hh_cache *hh, const struct net_device *dev,
> const unsigned char *haddr);
> +__be16 eth_header_parse_protocol(const struct sk_buff *skb);
Does not need to be exposed in the header file or exported.
^ permalink raw reply
* Re: [PATCH 0/7] AF_PACKET transport_offset fix
From: Willem de Bruijn @ 2019-01-23 14:12 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: Willem de Bruijn, David S. Miller, netdev@vger.kernel.org,
Saeed Mahameed, Eran Ben Elisha, Tariq Toukan, Jason Wang,
Eric Dumazet
In-Reply-To: <AM6PR05MB58795FBCEC44777DEDD5EB72D1990@AM6PR05MB5879.eurprd05.prod.outlook.com>
On Wed, Jan 23, 2019 at 5:19 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> From my perspective, after the discussion we had with Willem, the
> current version of this series can be merged to net-next.
net-next has moved forward. The patchset as is no longer applies cleanly.
> Willem, do you approve it?
>
> Thanks for reviewing!
>
> > -----Original Message-----
> > From: Maxim Mikityanskiy
> > Sent: 14 January, 2019 15:19
> > To: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> > <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> > <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>
> > Cc: netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq
> > Toukan <tariqt@mellanox.com>; Maxim Mikityanskiy <maximmi@mellanox.com>
> > Subject: [PATCH 0/7] AF_PACKET transport_offset fix
> >
> > This patch series contains the implementation of the RFC that was posted
> > on this mailing list previously:
> > https://www.spinics.net/lists/netdev/msg541709.html
> >
> > It fixes having incorrect skb->transport_header values in cases when
> > dissect fails. Having correct values set by the kernel fixes mlx5
> > operation and allows to remove some unnecessary code flows in mlx5.
> >
> > Maxim Mikityanskiy (7):
> > net: Don't set transport offset to invalid value
> > net: Introduce parse_protocol header_ops callback
> > net/ethernet: Add parse_protocol header_ops support
> > net/packet: Ask driver for protocol if not provided by user
> > net/packet: Remove redundant skb->protocol set
> > net/mlx5e: Remove the wrong assumption about transport offset
> > net/mlx5e: Trust kernel regarding transport offset
> >
> > .../net/ethernet/mellanox/mlx5/core/en_tx.c | 15 ++---------
> > drivers/net/tap.c | 4 +--
> > drivers/net/tun.c | 4 +--
> > drivers/net/xen-netback/netback.c | 19 ++++++++------
> > include/linux/etherdevice.h | 1 +
> > include/linux/netdevice.h | 10 +++++++
> > include/linux/skbuff.h | 14 +++++-----
> > net/ethernet/eth.c | 13 ++++++++++
> > net/packet/af_packet.c | 26 +++++++++----------
> > 9 files changed, 60 insertions(+), 46 deletions(-)
> >
> > --
> > 2.19.1
>
^ permalink raw reply
* Re: [PATCH 1/7] net: Don't set transport offset to invalid value
From: Willem de Bruijn @ 2019-01-23 14:11 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: David S. Miller, Saeed Mahameed, Willem de Bruijn, Jason Wang,
Eric Dumazet, netdev@vger.kernel.org, Eran Ben Elisha,
Tariq Toukan
In-Reply-To: <AM6PR05MB58797C01D8F2E6AD1389AE40D1990@AM6PR05MB5879.eurprd05.prod.outlook.com>
On Wed, Jan 23, 2019 at 5:17 AM Maxim Mikityanskiy <maximmi@mellanox.com> wrote:
>
> > -----Original Message-----
> > From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Sent: 17 January, 2019 17:16
> > To: Maxim Mikityanskiy <maximmi@mellanox.com>
> > Cc: David S. Miller <davem@davemloft.net>; Saeed Mahameed
> > <saeedm@mellanox.com>; Willem de Bruijn <willemb@google.com>; Jason Wang
> > <jasowang@redhat.com>; Eric Dumazet <edumazet@google.com>;
> > netdev@vger.kernel.org; Eran Ben Elisha <eranbe@mellanox.com>; Tariq Toukan
> > <tariqt@mellanox.com>
> > Subject: Re: [PATCH 1/7] net: Don't set transport offset to invalid value
> >
> > On Thu, Jan 17, 2019 at 4:10 AM Maxim Mikityanskiy <maximmi@mellanox.com>
> > wrote:
> > >
> > > > This is a lot of code change. This would do.
> > > >
> > > > @@ -2434,8 +2434,6 @@ static inline void
> > > > skb_probe_transport_header(struct sk_buff *skb,
> > > >
> > > > if (skb_flow_dissect_flow_keys_basic(skb, &keys, NULL, 0, 0, 0,
> > 0))
> > > > skb_set_transport_header(skb, keys.control.thoff);
> > > > - else
> > > > - skb_set_transport_header(skb, offset_hint);
> > > > }
> > > >
> > > > Though leaving an unused argument is a bit ugly. For net-next, indeed
> > > > better to clean up (please mark your patchset with net or net-next,
> > > > btw)
> > >
> > > It's for net-next (I'll resend with the correct mark), so I'll stick
> > > with the current implementation.
> >
> > Absolutely, sounds good.
> >
> > > > > diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-
> > > > netback/netback.c
> > > > > index 80aae3a32c2a..b49b6e56ca47 100644
> > > > > --- a/drivers/net/xen-netback/netback.c
> > > > > +++ b/drivers/net/xen-netback/netback.c
> > > > > @@ -1105,6 +1105,7 @@ static int xenvif_tx_submit(struct xenvif_queue
> > > > *queue)
> > > > > struct xen_netif_tx_request *txp;
> > > > > u16 pending_idx;
> > > > > unsigned data_len;
> > > > > + bool th_set;
> > > > >
> > > > > pending_idx = XENVIF_TX_CB(skb)->pending_idx;
> > > > > txp = &queue->pending_tx_info[pending_idx].req;
> > > > > @@ -1169,20 +1170,22 @@ static int xenvif_tx_submit(struct
> > xenvif_queue
> > > > *queue)
> > > > > continue;
> > > > > }
> > > > >
> > > > > - skb_probe_transport_header(skb, 0);
> > > > > + th_set = skb_try_probe_transport_header(skb);
> > > >
> > > > Can use skb_transport_header_was_set(). Then at least there is no need
> > > > to change the function's return value.
> > >
> > > I suppose this comment relates to the previous one, and if we do it for
> > > net-next, it's fine to make change I made, isn't it?
> >
> > If this is the only reason for the boolean return value, using
> > skb_transport_header_was_set() is more standard (I immediately know
> > what's happening when I read it), slightly less code change and avoids
> > introducing a situation where the majority of callers ignore a return
> > value. I think it's preferable. But these merits are certainly
> > debatable, so either is fine.
>
> From my side, I wanted to avoid calling skb_transport_header_was_set
> twice, so I made skb_try_probe_transport_header return whether it
> succeeded or not. I think "try" in the function name indicates this idea
> pretty clearly. This result status is pretty useful, it just happened
> that it's not needed in many places,
Which is an indication that it's perhaps not needed.
> but the general idea is that we
> report this status, so if you say that my version is also good for you,
> I'll leave it as is. It was just a rationale for my decision.
It's fine. But please avoid the code churn in xenvif_tx_submit
with to extra indentation. This is equivalent:
- if (skb_is_gso(skb)) {
+ if (skb_is_gso(skb) && th_set) {
More fundamentally, the code has the assumption that th_set
always holds if skb_is_gso(skb). Why add the check? This is
another example that the return value is not really needed.
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/8] libbpf: Add a helper for retrieving a prog via index
From: Daniel Borkmann @ 2019-01-23 14:11 UTC (permalink / raw)
To: Maciej Fijalkowski; +Cc: ast, netdev, jakub.kicinski, brouer, quentin.monnet
In-Reply-To: <20190123144159.000051bd@gmail.com>
On 01/23/2019 02:41 PM, Maciej Fijalkowski wrote:
> On Wed, 23 Jan 2019 11:41:11 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
>>> xdp_redirect_cpu has a 6 different XDP programs that can be attached to
>>> network interface. This sample has a option --prognum that allows user
>>> for specifying which particular program from a given set will be
>>> attached to network interface.
>>> In order to make it easier when converting the mentioned sample to
>>> libbpf usage, add a function to libbpf that will return program's fd for
>>> a given index.
>>>
>>> Note that there is already a bpf_object__find_prog_by_idx, which could
>>> be exported and might be used for that purpose, but it operates on the
>>> number of ELF section and here we need an index from a programs array
>>> within the bpf_object.
>>
>> Series in general looks good to me. Few minor comments, mainly in relation
>> to the need for libbpf extensions.
>>
>> Would it not be a better interface to the user to instead choose the prog
>> based on section name and then retrieve it via bpf_object__find_program_by_title()
>> instead of prognum (which feels less user friendly) at least?
>
> I couldn't decide which one from:
> * adding a libbpf helper
> * changing the xdp_redirect_cpu behaviour
> would be more invasive when I was converting this sample to libbpf support.
>
> Your suggestion sounds good, but I'm wondering about the actual implementation.
> I suppose that we would choose the program via command line. Some program
Yes, selection for loading prog could be done through that.
> section names in this sample are a bit long and it might be irritating for
> user to type in for example "xdp_cpu_map5_lb_hash_ip_pairs", no? Or maybe we
I think that's an implementation detail of this specific sample. For the
prog number, the user would need to first look up the source and figure out
the prog number correlation to the name that should be loaded which is a
bit ugly (unless I missed something). So we might as well make it easier
and allow to choose by name.
Figuring out which programs are available in an object file is a separate
tooling issue, imho. readelf is one way but not optimal, and it would be
a nice addition to bpftool as brought up at plumbers [0] to dump the contents
of a bpf obj file. Latter might also be nice to enrich with BTF info when
present (e.g. types, line annotation etc) so this would be much more powerful
than plain introspection via readelf.
[0] http://vger.kernel.org/lpc-bpf2018.html#session-6
> can live with this. In case of typo and program being not found it would be
> good to print out the section names to choose from in usage().
Agree, might be nice as well via libbpf.
>>> Signed-off-by: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> ---
>>> tools/lib/bpf/libbpf.c | 8 ++++++++
>>> tools/lib/bpf/libbpf.h | 3 +++
>>> tools/lib/bpf/libbpf.map | 1 +
>>> 3 files changed, 12 insertions(+)
>>>
>>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>>> index dc838bea403f..21c84d0f6128 100644
>>> --- a/tools/lib/bpf/libbpf.c
>>> +++ b/tools/lib/bpf/libbpf.c
>>> @@ -935,6 +935,14 @@ static int bpf_object__elf_collect(struct bpf_object *obj, int flags)
>>> return err;
>>> }
>>>
>>> +int
>>> +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx)
>>> +{
>>> + if (idx >= 0 && idx < obj->nr_programs)
>>> + return bpf_program__fd(&obj->programs[idx]);
>>> + return -ENOENT;
>>> +}
>>> +
>>> static struct bpf_program *
>>> bpf_object__find_prog_by_idx(struct bpf_object *obj, int idx)
>>> {
>>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>>> index 7f10d36abdde..ca1b381cb3ad 100644
>>> --- a/tools/lib/bpf/libbpf.h
>>> +++ b/tools/lib/bpf/libbpf.h
>>> @@ -95,6 +95,9 @@ LIBBPF_API int bpf_object__btf_fd(const struct bpf_object *obj);
>>> LIBBPF_API struct bpf_program *
>>> bpf_object__find_program_by_title(struct bpf_object *obj, const char *title);
>>>
>>> +LIBBPF_API int
>>> +bpf_object__get_prog_fd_by_num(struct bpf_object *obj, int idx);
>>> +
>>> LIBBPF_API struct bpf_object *bpf_object__next(struct bpf_object *prev);
>>> #define bpf_object__for_each_safe(pos, tmp) \
>>> for ((pos) = bpf_object__next(NULL), \
>>> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
>>> index 7c59e4f64082..871d2fc07150 100644
>>> --- a/tools/lib/bpf/libbpf.map
>>> +++ b/tools/lib/bpf/libbpf.map
>>> @@ -127,4 +127,5 @@ LIBBPF_0.0.1 {
>>> LIBBPF_0.0.2 {
>>> global:
>>> bpf_object__find_map_fd_by_name;
>>> + bpf_object__get_prog_fd_by_num;
>>> } LIBBPF_0.0.1;
>>>
>>
>
^ permalink raw reply
* Re: [net-next] net: phy: fix issue with loading PHY driver w/o initramfs
From: Geert Uytterhoeven @ 2019-01-23 10:06 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Krzysztof Kozlowski, Andrew Lunn, Florian Fainelli, David Miller,
netdev, Geert Uytterhoeven
In-Reply-To: <8699eccc-4a12-09d2-5322-4e2533215258@gmail.com>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Hi Heiner,
> On 22.01.2019 23:45, David Miller wrote:
> > From: Heiner Kallweit <hkallweit1@gmail.com>
> > Date: Sat, 19 Jan 2019 10:30:21 +0100
> >
> >> It was reported that on a system with nfsboot and w/o initramfs network
> >> fails because trying to load the PHY driver returns -ENOENT. Reason was
> >> that due to missing initramfs the modprobe binary isn't available.
> >> So we have to ignore error code -ENOENT.
> >>
> >> Fixes: 13d0ab6750b2 ("net: phy: check return code when requesting PHY driver module")
> >> Reported-by: Krzysztof Kozlowski <krzk@kernel.org>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >
> > Applied.
> >
> > However, I agree with Geert that we should adopt the:
> >
> > if (module_not_present)
> > request_module();
> > if (module_not_present)
> > goto failed_to_load;
> >
> > pattern.
>
> I know this is the standard pattern for request_module().
> Unfortunately the situation is a little bit tricky with PHY drivers.
> We don't know whether there's a module matching the PHY ID and
> it's a valid use case that there's no such module.
> In such a case we bind the genphy driver later, and a lot of PHY's
> are totally happy with the genphy driver and therefore no dedicated
> PHY drivers exist.
Currently, if request_module() fails (for whatever reason, except
-ENOENT), you don't bind to the genphy driver, but propagate an
error[*], leaving the user without network interface.
Is that better than ignoring the error, and binding to the genphy
driver?
When do you expect the phy-specific driver to become available, if
ever?
[*] The actual error code returned by request_module(), and not
-EPROBE_DEFER. The latter may sound attractive, as it is meant to
cause a retry later, but has its own set of problems with optional
drivers that may never become available (e.g. missing drivers for
DMA controllers).
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH net-next V4 5/5] vhost: access vq metadata through kernel virtual address
From: Michael S. Tsirkin @ 2019-01-23 14:08 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization, netdev, linux-kernel, kvm
In-Reply-To: <20190123095557.30168-6-jasowang@redhat.com>
On Wed, Jan 23, 2019 at 05:55:57PM +0800, Jason Wang wrote:
> It was noticed that the copy_user() friends that was used to access
> virtqueue metdata tends to be very expensive for dataplane
> implementation like vhost since it involves lots of software checks,
> speculation barrier, hardware feature toggling (e.g SMAP). The
> extra cost will be more obvious when transferring small packets since
> the time spent on metadata accessing become more significant.
>
> This patch tries to eliminate those overheads by accessing them
> through kernel virtual address by vmap(). To make the pages can be
> migrated, instead of pinning them through GUP, we use MMU notifiers to
> invalidate vmaps and re-establish vmaps during each round of metadata
> prefetching if necessary. For devices that doesn't use metadata
> prefetching, the memory accessors fallback to normal copy_user()
> implementation gracefully. The invalidation was synchronized with
> datapath through vq mutex, and in order to avoid hold vq mutex during
> range checking, MMU notifier was teared down when trying to modify vq
> metadata.
>
> Another thing is kernel lacks efficient solution for tracking dirty
> pages by vmap(), this will lead issues if vhost is using file backed
> memory which needs care of writeback. This patch solves this issue by
> just skipping the vma that is file backed and fallback to normal
> copy_user() friends. This might introduce some overheads for file
> backed users but consider this use case is rare we could do
> optimizations on top.
>
> Note that this was only done when device IOTLB is not enabled. We
> could use similar method to optimize it in the future.
>
> Tests shows at most about 22% improvement on TX PPS when using
> virtio-user + vhost_net + xdp1 + TAP on 2.6GHz Broadwell:
>
> SMAP on | SMAP off
> Before: 5.0Mpps | 6.6Mpps
> After: 6.1Mpps | 7.4Mpps
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
So this is the bulk of the change.
Threee things that I need to look into
- Are there any security issues with bypassing the speculation barrier
that is normally present after access_ok?
- How hard does the special handling for
file backed storage make testing?
On the one hand we could add a module parameter to
force copy to/from user. on the other that's
another configuration we need to support.
But iotlb is not using vmap, so maybe that's enough
for testing.
- How hard is it to figure out which mode uses which code.
Meanwhile, could you pls post data comparing this last patch with the
below? This removes the speculation barrier replacing it with a
(useless but at least more lightweight) data dependency.
Thanks!
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index bac939af8dbb..352ee7e14476 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -739,7 +739,7 @@ static int vhost_copy_to_user(struct vhost_virtqueue *vq, void __user *to,
int ret;
if (!vq->iotlb)
- return __copy_to_user(to, from, size);
+ return copy_to_user(to, from, size);
else {
/* This function should be called after iotlb
* prefetch, which means we're sure that all vq
@@ -752,7 +752,7 @@ static int vhost_copy_to_user(struct vhost_virtqueue *vq, void __user *to,
VHOST_ADDR_USED);
if (uaddr)
- return __copy_to_user(uaddr, from, size);
+ return copy_to_user(uaddr, from, size);
ret = translate_desc(vq, (u64)(uintptr_t)to, size, vq->iotlb_iov,
ARRAY_SIZE(vq->iotlb_iov),
@@ -774,7 +774,7 @@ static int vhost_copy_from_user(struct vhost_virtqueue *vq, void *to,
int ret;
if (!vq->iotlb)
- return __copy_from_user(to, from, size);
+ return copy_from_user(to, from, size);
else {
/* This function should be called after iotlb
* prefetch, which means we're sure that vq
@@ -787,7 +787,7 @@ static int vhost_copy_from_user(struct vhost_virtqueue *vq, void *to,
struct iov_iter f;
if (uaddr)
- return __copy_from_user(to, uaddr, size);
+ return copy_from_user(to, uaddr, size);
ret = translate_desc(vq, (u64)(uintptr_t)from, size, vq->iotlb_iov,
ARRAY_SIZE(vq->iotlb_iov),
@@ -855,13 +855,13 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
({ \
int ret = -EFAULT; \
if (!vq->iotlb) { \
- ret = __put_user(x, ptr); \
+ ret = put_user(x, ptr); \
} else { \
__typeof__(ptr) to = \
(__typeof__(ptr)) __vhost_get_user(vq, ptr, \
sizeof(*ptr), VHOST_ADDR_USED); \
if (to != NULL) \
- ret = __put_user(x, to); \
+ ret = put_user(x, to); \
else \
ret = -EFAULT; \
} \
@@ -872,14 +872,14 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
({ \
int ret; \
if (!vq->iotlb) { \
- ret = __get_user(x, ptr); \
+ ret = get_user(x, ptr); \
} else { \
__typeof__(ptr) from = \
(__typeof__(ptr)) __vhost_get_user(vq, ptr, \
sizeof(*ptr), \
type); \
if (from != NULL) \
- ret = __get_user(x, from); \
+ ret = get_user(x, from); \
else \
ret = -EFAULT; \
} \
^ permalink raw reply related
* Re: [PATCH bpf-next v2 1/8] libbpf: Add a helper for retrieving a map fd for a given name
From: Maciej Fijalkowski @ 2019-01-23 14:03 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: ast, netdev, jakub.kicinski, brouer
In-Reply-To: <3597b94e-f4f2-473e-b7be-e7493f287d98@iogearbox.net>
On Wed, 23 Jan 2019 11:54:41 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 01/21/2019 10:10 AM, Maciej Fijalkowski wrote:
> > XDP samples are mostly cooperating with eBPF maps through their file
> > descriptors. In case of a eBPF program that contains multiple maps it
> > might be tiresome to iterate through them and call bpf_map__fd for each
> > one. Add a helper mostly based on bpf_object__find_map_by_name, but
> > instead of returning the struct bpf_map pointer, return map fd.
> >
> > Bump libbpf ABI version to 0.0.2.
> >
> > Suggested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Signed-off-by: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
> > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > ---
> > tools/lib/bpf/libbpf.c | 12 ++++++++++++
> > tools/lib/bpf/libbpf.h | 3 +++
> > tools/lib/bpf/libbpf.map | 4 ++++
> > 3 files changed, 19 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 169e347c76f6..dc838bea403f 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -2840,6 +2840,18 @@ bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
> > return NULL;
> > }
>
> Application could just do: bpf_map__fd(bpf_object__find_map_by_name(...)) or
> bpf_object__find_map_by_name(...)->fd as both are exposed via library, though
> I guess it may be okay to have a helper for it as it feels this might be needed
> in many cases.
>
> > +int
> > +bpf_object__find_map_fd_by_name(struct bpf_object *obj, const char *name)
> > +{
> > + struct bpf_map *pos;
> > +
> > + bpf_map__for_each(pos, obj) {
> > + if (pos->name && !strcmp(pos->name, name))
> > + return bpf_map__fd(pos);
> > + }
> > + return -ENOENT;
>
> Can we instead just do:
>
> int
> bpf_object__find_map_fd_by_name(struct bpf_object *obj, const char *name)
> {
> return bpf_map__fd(bpf_object__find_map_by_name(obj, name));
> }
>
Yes of course, I will send v3 once we figure out what's the best way to supply
the prog section name for xdp_redirect_cpu.
> > +}
> > +
> > struct bpf_map *
> > bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset)
> > {
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 5f68d7b75215..7f10d36abdde 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -264,6 +264,9 @@ struct bpf_map;
> > LIBBPF_API struct bpf_map *
> > bpf_object__find_map_by_name(struct bpf_object *obj, const char *name);
> >
> > +LIBBPF_API int
> > +bpf_object__find_map_fd_by_name(struct bpf_object *obj, const char *name);
> > +
> > /*
> > * Get bpf_map through the offset of corresponding struct bpf_map_def
> > * in the BPF object file.
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index cd02cd4e2cc3..7c59e4f64082 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -124,3 +124,7 @@ LIBBPF_0.0.1 {
> > local:
> > *;
> > };
> > +LIBBPF_0.0.2 {
> > + global:
> > + bpf_object__find_map_fd_by_name;
> > +} LIBBPF_0.0.1;
> >
>
^ permalink raw reply
* Re: [PATCH net-next V4 0/5] vhost: accelerate metadata access through vmap()
From: Michael S. Tsirkin @ 2019-01-23 13:58 UTC (permalink / raw)
To: Jason Wang; +Cc: virtualization, netdev, linux-kernel, kvm
In-Reply-To: <20190123095557.30168-1-jasowang@redhat.com>
On Wed, Jan 23, 2019 at 05:55:52PM +0800, Jason Wang wrote:
> This series tries to access virtqueue metadata through kernel virtual
> address instead of copy_user() friends since they had too much
> overheads like checks, spec barriers or even hardware feature
> toggling.
>
> Test shows about 24% improvement on TX PPS. It should benefit other
> cases as well.
ok I think this addresses most comments but it's a big change and we
just started 1.1 review so to pls give me a week to review this ok?
> Changes from V3:
> - don't try to use vmap for file backed pages
> - rebase to master
> Changes from V2:
> - fix buggy range overlapping check
> - tear down MMU notifier during vhost ioctl to make sure invalidation
> request can read metadata userspace address and vq size without
> holding vq mutex.
> Changes from V1:
> - instead of pinning pages, use MMU notifier to invalidate vmaps and
> remap duing metadata prefetch
> - fix build warning on MIPS
>
> Jason Wang (5):
> vhost: generalize adding used elem
> vhost: fine grain userspace memory accessors
> vhost: rename vq_iotlb_prefetch() to vq_meta_prefetch()
> vhost: introduce helpers to get the size of metadata area
> vhost: access vq metadata through kernel virtual address
>
> drivers/vhost/net.c | 4 +-
> drivers/vhost/vhost.c | 441 +++++++++++++++++++++++++++++++++++++-----
> drivers/vhost/vhost.h | 15 +-
> mm/shmem.c | 1 +
> 4 files changed, 410 insertions(+), 51 deletions(-)
>
> --
> 2.17.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