* Re: [RFC bpf-next 3/4] tools/bpf: bpftool, split the function do_dump()
From: Jakub Kicinski @ 2018-09-19 15:26 UTC (permalink / raw)
To: Prashant Bhole
Cc: Alexei Starovoitov, Daniel Borkmann, Quentin Monnet,
David S . Miller, netdev
In-Reply-To: <20180919075143.9308-4-bhole_prashant_q7@lab.ntt.co.jp>
On Wed, 19 Sep 2018 16:51:42 +0900, Prashant Bhole wrote:
> +static int dump_map_elem(int fd, void *key, void *value,
> + struct bpf_map_info *map_info, struct btf *btf,
> + json_writer_t *btf_wtr)
> +{
> + int num_elems = 0;
> +
> + if (!bpf_map_lookup_elem(fd, key, value)) {
> + if (json_output) {
> + print_entry_json(map_info, key, value, btf);
> + } else {
> + if (btf) {
> + struct btf_dumper d = {
> + .btf = btf,
> + .jw = btf_wtr,
> + .is_plain_text = true,
> + };
> +
> + do_dump_btf(&d, map_info, key, value);
> + } else {
> + print_entry_plain(map_info, key, value);
> + }
> + num_elems++;
> + }
> + goto out;
> + }
> +
> + /* lookup error handling */
> + if (map_is_map_of_maps(map_info->type) ||
> + map_is_map_of_progs(map_info->type))
> + goto out;
> +
nit: why not just return? the goto seems to only do a return anyway,
is this suggested by some coding style? Is it to help the
compiler? I see people do this from time to time..
[...]
> +out:
> + return num_elems;
^ permalink raw reply
* Re: [PATCH] ixgbevf: off by one in ixgbevf_ipsec_tx()
From: Shannon Nelson @ 2018-09-19 15:28 UTC (permalink / raw)
To: Dan Carpenter, Jeff Kirsher
Cc: David S. Miller, intel-wired-lan, netdev, kernel-janitors
In-Reply-To: <20180919103529.GC9238@mwanda>
On 9/19/2018 3:35 AM, Dan Carpenter wrote:
> The ipsec->tx_tbl[] array has IXGBE_IPSEC_MAX_SA_COUNT elements so the >
> should be a >=.
>
> Fixes: 0062e7cc955e ("ixgbevf: add VF IPsec offload code")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
>
> diff --git a/drivers/net/ethernet/intel/ixgbevf/ipsec.c b/drivers/net/ethernet/intel/ixgbevf/ipsec.c
> index 997cea675a37..4fcbeffce52b 100644
> --- a/drivers/net/ethernet/intel/ixgbevf/ipsec.c
> +++ b/drivers/net/ethernet/intel/ixgbevf/ipsec.c
> @@ -470,7 +470,7 @@ int ixgbevf_ipsec_tx(struct ixgbevf_ring *tx_ring,
> }
>
> sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_TX_INDEX;
> - if (unlikely(sa_idx > IXGBE_IPSEC_MAX_SA_COUNT)) {
> + if (unlikely(sa_idx >= IXGBE_IPSEC_MAX_SA_COUNT)) {
> netdev_err(tx_ring->netdev, "%s: bad sa_idx=%d handle=%lu\n",
> __func__, sa_idx, xs->xso.offload_handle);
> return 0;
>
^ permalink raw reply
* Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed
From: Jakub Kicinski @ 2018-09-19 15:29 UTC (permalink / raw)
To: Prashant Bhole
Cc: Alexei Starovoitov, Daniel Borkmann, Quentin Monnet,
David S . Miller, netdev
In-Reply-To: <20180919075143.9308-5-bhole_prashant_q7@lab.ntt.co.jp>
On Wed, 19 Sep 2018 16:51:43 +0900, Prashant Bhole wrote:
> Let's add a check for EOPNOTSUPP error when map lookup is failed.
> Also in case map doesn't support lookup, the output of map dump is
> changed from "can't lookup element" to "lookup not supported for
> this map".
>
> Patch adds function print_entry_error() function to print the error
> value.
>
> Following example dumps a map which does not support lookup.
>
> Output before:
> root# bpftool map -jp dump id 40
> [
> "key": ["0x0a","0x00","0x00","0x00"
> ],
> "value": {
> "error": "can\'t lookup element"
> },
> "key": ["0x0b","0x00","0x00","0x00"
> ],
> "value": {
> "error": "can\'t lookup element"
> }
> ]
>
> root# bpftool map dump id 40
> can't lookup element with key:
> 0a 00 00 00
> can't lookup element with key:
> 0b 00 00 00
> Found 0 elements
>
> Output after changes:
> root# bpftool map dump -jp id 45
> [
> "key": ["0x0a","0x00","0x00","0x00"
> ],
> "value": {
> "error": "lookup not supported for this map"
> },
> "key": ["0x0b","0x00","0x00","0x00"
> ],
> "value": {
> "error": "lookup not supported for this map"
> }
> ]
>
> root# bpftool map dump id 45
> key:
> 0a 00 00 00
> value:
> lookup not supported for this map
> key:
> 0b 00 00 00
> value:
> lookup not supported for this map
> Found 0 elements
Nice improvement, thanks for the changes! I wonder what your thoughts
would be on just printing some form of "lookup not supported for this
map" only once? It seems slightly like repeated information - if
lookup is not supported for one key it likely won't be for other keys
too, so we could shorten the output. Would that make sense?
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> ---
> tools/bpf/bpftool/main.h | 5 +++++
> tools/bpf/bpftool/map.c | 35 ++++++++++++++++++++++++++++++-----
> 2 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 40492cdc4e53..1a8c683f949b 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -46,6 +46,11 @@
>
> #include "json_writer.h"
>
> +#define ERR_CANNOT_LOOKUP \
> + "can't lookup element"
> +#define ERR_LOOKUP_NOT_SUPPORTED \
> + "lookup not supported for this map"
Do we need these? Are we going to reused them in more parts of the
code?
> #define ptr_to_u64(ptr) ((__u64)(unsigned long)(ptr))
>
> #define NEXT_ARG() ({ argc--; argv++; if (argc < 0) usage(); })
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 284e12a289c0..2faccd2098c9 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -333,6 +333,25 @@ static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
> jsonw_end_object(json_wtr);
> }
>
> +static void print_entry_error(struct bpf_map_info *info, unsigned char *key,
> + char *value)
> +{
> + bool single_line, break_names;
> + int value_size = strlen(value);
nit: order variables declaration lines to shortest, please.
> +
> + break_names = info->key_size > 16 || value_size > 16;
> + single_line = info->key_size + value_size <= 24 && !break_names;
> +
> + printf("key:%c", break_names ? '\n' : ' ');
> + fprint_hex(stdout, key, info->key_size, " ");
> +
> + printf(single_line ? " " : "\n");
> +
> + printf("value:%c%s", break_names ? '\n' : ' ', value);
> +
> + printf("\n");
> +}
> +
> static void print_entry_plain(struct bpf_map_info *info, unsigned char *key,
> unsigned char *value)
> {
> @@ -660,6 +679,8 @@ static int dump_map_elem(int fd, void *key, void *value,
> json_writer_t *btf_wtr)
> {
> int num_elems = 0;
> + int lookup_errno;
> + char *errstr;
nit: const char?
>
> if (!bpf_map_lookup_elem(fd, key, value)) {
> if (json_output) {
^ permalink raw reply
* Re: [PATCH ethtool] ethtool: support combinations of FEC modes
From: Andrew Lunn @ 2018-09-19 15:33 UTC (permalink / raw)
To: Michal Kubecek
Cc: Edward Cree, John W. Linville, netdev, Ganesh Goudar,
Jakub Kicinski, Dustin Byford, Dirk van der Merwe
In-Reply-To: <20180919144117.GF3876@unicorn.suse.cz>
> One loosely related question: how sure can we be that we are never going
> to need more than 32 bits for FEC encodings? Is it something completely
> hypothetical or is it something that could happen in the future?
>
Hi Michal
Hopefully we have moved to a netlink socket by that time :-)
I recently found that EEE still uses a u32 for advertise link modes.
We should fix that in the netlink API.
Andrew
^ permalink raw reply
* Re: [PATCH ethtool] ethtool: support combinations of FEC modes
From: Edward Cree @ 2018-09-19 15:38 UTC (permalink / raw)
To: Michal Kubecek
Cc: John W. Linville, netdev, Ganesh Goudar, Jakub Kicinski,
Dustin Byford, Dirk van der Merwe
In-Reply-To: <20180919144117.GF3876@unicorn.suse.cz>
On 19/09/18 15:41, Michal Kubecek wrote:
> I'm sorry I didn't notice this before the patch was accepted but as it's
> not in a release yet, maybe it's still not too late.
>
> Could I suggest to make the syntax consistent with other options?
I didn't realise ethtool had any patterns to be consistent with ;)
> I mean rather than a comma separated list to use either
>
> ethtool --set-fec <dev> encoding enc1 enc2 ...
but yes this looks fine to me, as long as we're reasonably confident that
we won't want to add new parameters (that might require determining
whether enc2 is an encoding or a parameter name) in the future, because
while the parsing wouldn't be impossible it might get ugly.
I'll rustle up an RFC patch.
-Ed
^ permalink raw reply
* Re: [PATCH ethtool] ethtool: support combinations of FEC modes
From: Michal Kubecek @ 2018-09-19 15:49 UTC (permalink / raw)
To: Andrew Lunn
Cc: Edward Cree, John W. Linville, netdev, Ganesh Goudar,
Jakub Kicinski, Dustin Byford, Dirk van der Merwe
In-Reply-To: <20180919153338.GE17466@lunn.ch>
On Wed, Sep 19, 2018 at 05:33:38PM +0200, Andrew Lunn wrote:
> > One loosely related question: how sure can we be that we are never going
> > to need more than 32 bits for FEC encodings? Is it something completely
> > hypothetical or is it something that could happen in the future?
> >
> Hi Michal
>
> Hopefully we have moved to a netlink socket by that time :-)
Actually, that's why I'm asking. :-)
> I recently found that EEE still uses a u32 for advertise link modes.
> We should fix that in the netlink API.
For EEE it felt obvious that arbitrary length bitmap is the way to go so
I used it (it's still u32 in ethtool_ops but that's easier to change
when needed).
For FEC I wasn't sure if it wouldn't be an overkill.
Michal Kubecek
^ permalink raw reply
* Re: [RFC] net;sched: Try to find idle cpu for RPS to handle packets
From: Eric Dumazet @ 2018-09-19 15:49 UTC (permalink / raw)
To: Kirill Tkhai
Cc: Peter Zijlstra, David Miller, Daniel Borkmann, tom, netdev, LKML
In-Reply-To: <2fdf2bd7-1cc4-a1e1-15c2-e2badfcd4d59@virtuozzo.com>
[-- Attachment #1: Type: text/plain, Size: 4801 bytes --]
On Wed, Sep 19, 2018 at 8:41 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> On 19.09.2018 17:55, Eric Dumazet wrote:
> > On Wed, Sep 19, 2018 at 5:29 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> >>
> >> Many workloads have polling mode of work. The application
> >> checks for incomming packets from time to time, but it also
> >> has a work to do, when there is no packets. This RFC
> >> tries to develop an idea to queue RPS packets on idle
> >> CPU in the the L3 domain of the consumer, so backlog
> >> processing of the packets and the application can execute
> >> in parallel.
> >>
> >> We require this in case of network cards does not
> >> have enough RX queues to cover all online CPUs (this seems
> >> to be the most cards), and get_rps_cpu() actually chooses
> >> remote cpu, and SMP interrupt is sent. Here we may try
> >> our best, and to find idle CPU nearly the consumer's CPU.
> >> Note, that in case of consumer works in poll mode and it
> >> does not waits for incomming packets, its CPU will be not
> >> idle, while CPU of a sleeping consumer may be idle. So,
> >> not polling consumers will still be able to have skb
> >> handled on its CPU.
> >>
> >> In case of network card has many queues, the device
> >> interrupts will come on consumer's CPU, and this patch
> >> won't try to find idle cpu for them.
> >>
> >> I've tried simple netperf test for this:
> >> netserver -p 1234
> >> netperf -L 127.0.0.1 -p 1234 -l 100
> >>
> >> Before:
> >> 87380 16384 16384 100.00 60323.56
> >> 87380 16384 16384 100.00 60388.46
> >> 87380 16384 16384 100.00 60217.68
> >> 87380 16384 16384 100.00 57995.41
> >> 87380 16384 16384 100.00 60659.00
> >>
> >> After:
> >> 87380 16384 16384 100.00 64569.09
> >> 87380 16384 16384 100.00 64569.25
> >> 87380 16384 16384 100.00 64691.63
> >> 87380 16384 16384 100.00 64930.14
> >> 87380 16384 16384 100.00 62670.15
> >>
> >> The difference between best runs is +7%,
> >> the worst runs differ +8%.
> >>
> >> What do you think about following somehow in this way?
> >
> > Hi Kirill
> >
> > In my experience, scheduler has a poor view of softirq processing
> > happening on various cpus.
> > A cpu spending 90% of its cycles processing IRQ might be considered 'idle'
>
> Yes, in case of there is softirq on top of irq_exit(), the cpu is not
> considered as busy. But after MAX_SOFTIRQ_TIME (=2ms), ksoftirqd are
> waken up to execute the work in process context, and the processor is
> considered as !idle. 2ms is 2 timer ticks in case of HZ=1000. So, we
> don't restart softirq in case of it was executed for more then 2ms.
>
That's the theory, but reality is very different unfortunately.
If RFS/RPS is setup properly, we really do not hit MAX_SOFTIRQ_TIME condition
unless in some synthetic benchmarks maybe.
> The similar way, single net_rx_action() can't be executed longer
> than 2ms.
>
> Having 90% load in softirq (called on top of irq_exit()) should be
> very unlikely situation, when there are too many interrupts with small
> amount of work, which related softirq calls are doing for each of them.
> I think it had be a problem even in plain napi case, since it would
> worked not like expected.
>
> But anyway. You worry, that during handling of next portion of skbs,
> we find that previous portion of skbs already woken ksoftirqd, and
> we don't see this cpu as idle? Yeah, then we'll try to change cpu,
> and this is not what we want. We want to continue use the cpu, where
> previous portion was handler. Hm, not so fast I'll answer, but certainly,
> this may be handled somehow in more creative way.
>
> > So please run a real workload (it is _very_ uncommon anyone set up RPS
> > on lo interface !)
> >
> > Like 400 or more concurrent netperf -t TCP_RR on a 10Gbit NIC.
>
> Yeah, it's just a simulation of a single irq nic. I'll try on something
> more real hardware.
Also my concern is that you might have results that are tied to a particular
version of process scheduling, platform, workload...
One month later, a small change in process scheduler,
and very different results.
This is why I believe this new feature must be controllable, via a new
tunable (like RPS/RFS are controllable per rx queue)
>
> How do you execute such the tests? I don't see the appropriate parameter
> of netperf. Does this mean just to start 400 copies of netperf? How is
> to aggregate their results in this case?
Yeah, there are various 'super_netperf' scripts available on the net
(almost trivial to write anyway)
( I am attaching one of them)
Thanks.
>
> > Thanks.
> >
> > PS: Idea of playing with L3 domains is interesting, I have personally
> > tried various strategies in the past but none of them
> > demonstrated a clear win.
>
> Thanks,
> Kirill
[-- Attachment #2: super_netperf --]
[-- Type: application/octet-stream, Size: 435 bytes --]
#!/bin/bash
run_netperf() {
loops=$1
shift
for ((i=0; i<loops; i++)); do
./netperf -s 2 $@ | awk '/Min/{
if (!once) {
print;
once=1;
}
}
{
if (NR == 6)
save = $NF
else if (NR==7) {
if (NF > 0)
print $NF
else
print save
} else if (NR==11) {
print $0
}
}' &
done
wait
return 0
}
run_netperf $@ | awk '{if (NF==7) {print $0; next}} {sum += $1} END {printf "%7u\n",sum}'
^ permalink raw reply
* Re: [PATCH ethtool] ethtool: support combinations of FEC modes
From: Michal Kubecek @ 2018-09-19 15:56 UTC (permalink / raw)
To: Edward Cree
Cc: John W. Linville, netdev, Ganesh Goudar, Jakub Kicinski,
Dustin Byford, Dirk van der Merwe
In-Reply-To: <613d954c-d88c-fa20-f436-75625f11a2ae@solarflare.com>
On Wed, Sep 19, 2018 at 04:38:27PM +0100, Edward Cree wrote:
> On 19/09/18 15:41, Michal Kubecek wrote:
> > I'm sorry I didn't notice this before the patch was accepted but as it's
> > not in a release yet, maybe it's still not too late.
> >
> > Could I suggest to make the syntax consistent with other options?
> I didn't realise ethtool had any patterns to be consistent with ;)
Way too many, I must say. :-) That is why I wasn't happy about adding
another.
> > I mean rather than a comma separated list to use either
> >
> > ethtool --set-fec <dev> encoding enc1 enc2 ...
> but yes this looks fine to me, as long as we're reasonably confident that
> we won't want to add new parameters (that might require determining
> whether enc2 is an encoding or a parameter name) in the future, because
> while the parsing wouldn't be impossible it might get ugly.
This problem already exists for "-s ... msglvl". In the parser for the
netlink series I introduced an "end of list" marker (tentatively "--")
for this purpose, perhaps that could be a way.
> I'll rustle up an RFC patch.
Thank you.
Michal Kubecek
^ permalink raw reply
* [RFC PATCH ethtool] ethtool: better syntax for combinations of FEC modes
From: Edward Cree @ 2018-09-19 16:06 UTC (permalink / raw)
To: John W. Linville; +Cc: Michal Kubecek, netdev
In-Reply-To: <20180919144117.GF3876@unicorn.suse.cz>
Instead of commas, just have them as separate argvs.
The parsing state machine might look heavyweight, but it makes it easy to add
more parameters later and distinguish parameter names from encoding names.
Suggested-by: Michal Kubecek <mkubecek@suse.cz>
Signed-off-by: Edward Cree <ecree@solarflare.com>
---
ethtool.8.in | 6 +++---
ethtool.c | 63 ++++++++++++++++------------------------------------------
test-cmdline.c | 10 +++++-----
3 files changed, 25 insertions(+), 54 deletions(-)
diff --git a/ethtool.8.in b/ethtool.8.in
index 414eaa1..7ea2cc0 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -390,7 +390,7 @@ ethtool \- query or control network driver and hardware settings
.B ethtool \-\-set\-fec
.I devname
.B encoding
-.BR auto | off | rs | baser [ , ...]
+.BR auto | off | rs | baser \ [...]
.
.\" Adjust lines (i.e. full justification) and hyphenate.
.ad
@@ -1120,11 +1120,11 @@ current FEC mode, the driver or firmware must take the link down
administratively and report the problem in the system logs for users to correct.
.RS 4
.TP
-.BR encoding\ auto | off | rs | baser [ , ...]
+.BR encoding\ auto | off | rs | baser \ [...]
Sets the FEC encoding for the device. Combinations of options are specified as
e.g.
-.B auto,rs
+.B encoding auto rs
; the semantics of such combinations vary between drivers.
.TS
nokeep;
diff --git a/ethtool.c b/ethtool.c
index 9997930..2f7e96b 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4979,39 +4979,6 @@ static int fecmode_str_to_type(const char *str)
return 0;
}
-/* Takes a comma-separated list of FEC modes, returns the bitwise OR of their
- * corresponding ETHTOOL_FEC_* constants.
- * Accepts repetitions (e.g. 'auto,auto') and trailing comma (e.g. 'off,').
- */
-static int parse_fecmode(const char *str)
-{
- int fecmode = 0;
- char buf[6];
-
- if (!str)
- return 0;
- while (*str) {
- size_t next;
- int mode;
-
- next = strcspn(str, ",");
- if (next >= 6) /* Bad mode, longest name is 5 chars */
- return 0;
- /* Copy into temp buffer and nul-terminate */
- memcpy(buf, str, next);
- buf[next] = 0;
- mode = fecmode_str_to_type(buf);
- if (!mode) /* Bad mode encountered */
- return 0;
- fecmode |= mode;
- str += next;
- /* Skip over ',' (but not nul) */
- if (*str)
- str++;
- }
- return fecmode;
-}
-
static int do_gfec(struct cmd_context *ctx)
{
struct ethtool_fecparam feccmd = { 0 };
@@ -5041,22 +5008,26 @@ static int do_gfec(struct cmd_context *ctx)
static int do_sfec(struct cmd_context *ctx)
{
- char *fecmode_str = NULL;
+ enum { ARG_NONE, ARG_ENCODING } state = ARG_NONE;
struct ethtool_fecparam feccmd;
- struct cmdline_info cmdline_fec[] = {
- { "encoding", CMDL_STR, &fecmode_str, &feccmd.fec},
- };
- int changed;
- int fecmode;
- int rv;
+ int fecmode = 0, newmode;
+ int rv, i;
- parse_generic_cmdline(ctx, &changed, cmdline_fec,
- ARRAY_SIZE(cmdline_fec));
-
- if (!fecmode_str)
+ for (i = 0; i < ctx->argc; i++) {
+ if (!strcmp(ctx->argp[i], "encoding")) {
+ state = ARG_ENCODING;
+ continue;
+ }
+ if (state == ARG_ENCODING) {
+ newmode = fecmode_str_to_type(ctx->argp[i]);
+ if (!newmode)
+ exit_bad_args();
+ fecmode |= newmode;
+ continue;
+ }
exit_bad_args();
+ }
- fecmode = parse_fecmode(fecmode_str);
if (!fecmode)
exit_bad_args();
@@ -5265,7 +5236,7 @@ static const struct option {
" [ all ]\n"},
{ "--show-fec", 1, do_gfec, "Show FEC settings"},
{ "--set-fec", 1, do_sfec, "Set FEC settings",
- " [ encoding auto|off|rs|baser ]\n"},
+ " [ encoding auto|off|rs|baser [...]]\n"},
{ "-h|--help", 0, show_usage, "Show this help" },
{ "--version", 0, do_version, "Show version number" },
{}
diff --git a/test-cmdline.c b/test-cmdline.c
index 9c51cca..84630a5 100644
--- a/test-cmdline.c
+++ b/test-cmdline.c
@@ -268,12 +268,12 @@ static struct test_case {
{ 1, "--set-eee devname advertise foo" },
{ 1, "--set-fec devname" },
{ 0, "--set-fec devname encoding auto" },
- { 0, "--set-fec devname encoding off," },
- { 0, "--set-fec devname encoding baser,rs" },
- { 0, "--set-fec devname encoding auto,auto," },
+ { 0, "--set-fec devname encoding off" },
+ { 0, "--set-fec devname encoding baser rs" },
+ { 0, "--set-fec devname encoding auto auto" },
{ 1, "--set-fec devname encoding foo" },
- { 1, "--set-fec devname encoding auto,foo" },
- { 1, "--set-fec devname encoding auto,," },
+ { 1, "--set-fec devname encoding auto foo" },
+ { 1, "--set-fec devname encoding none" },
{ 1, "--set-fec devname auto" },
/* can't test --set-priv-flags yet */
{ 0, "-h" },
^ permalink raw reply related
* [PATCH] net: macb: Clean 64b dma addresses if they are not detected
From: Michal Simek @ 2018-09-19 16:08 UTC (permalink / raw)
To: linux-kernel, monstr, Edgar E. Iglesias
Cc: David S. Miller, netdev, u-boot, Joe Hershberger, Nicolas Ferre
Clear ADDR64 dma bit in DMACFG register in case that HW_DMA_CAP_64B
is not detected on 64bit system.
The issue was observed when bootloader(u-boot) does not check macb
feature at DCFG6 register (DAW64_OFFSET) and enabling 64bit dma support
by default. Then macb driver is reading DMACFG register back and only
adding 64bit dma configuration but not cleaning it out.
This is also align with other features which are also cleared if they are not
present.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
drivers/net/ethernet/cadence/macb_main.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 16e4ef7d7185..79707dff3f13 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -2163,6 +2163,8 @@ static void macb_configure_dma(struct macb *bp)
#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
if (bp->hw_dma_cap & HW_DMA_CAP_64B)
dmacfg |= GEM_BIT(ADDR64);
+ else
+ dmacfg &= ~GEM_BIT(ADDR64);
#endif
#ifdef CONFIG_MACB_USE_HWSTAMP
if (bp->hw_dma_cap & HW_DMA_CAP_PTP)
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 3/7] netlink: set extack error message in nla_validate()
From: David Ahern @ 2018-09-19 16:20 UTC (permalink / raw)
To: Johannes Berg, linux-wireless, netdev; +Cc: Johannes Berg
In-Reply-To: <20180919120900.28708-4-johannes@sipsolutions.net>
On 9/19/18 5:08 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> In nla_parse() we already set this, but it makes sense to
> also do it in nla_validate() which already also sets the
> bad attribute pointer.
>
> CC: David Ahern <dsahern@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> lib/nlattr.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index e2e5b38394d5..33404745bec4 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -180,9 +180,13 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
> int rem;
>
> nla_for_each_attr(nla, head, len, rem) {
> - int err = validate_nla(nla, maxtype, policy, NULL);
> + static const char _msg[] = "Attribute failed policy validation";
> + const char *msg = _msg;
> + int err = validate_nla(nla, maxtype, policy, &msg);
>
> if (err < 0) {
> + if (extack)
> + extack->_msg = msg;
> NL_SET_BAD_ATTR(extack, nla);
msg, NL_SET_BAD_ATTR and extack handling all can be done in validate_nla
removing the need for the same message ("Attribute failed policy
validation") to be declared twice and simplifying the extack setting.
> return err;
> }
>
^ permalink raw reply
* Re: [PATCH 2/7] netlink: make validation_data const
From: David Ahern @ 2018-09-19 16:21 UTC (permalink / raw)
To: Johannes Berg, linux-wireless, netdev; +Cc: Johannes Berg
In-Reply-To: <20180919120900.28708-3-johannes@sipsolutions.net>
On 9/19/18 5:08 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> The validation data is only used within the policy that
> should usually already be const, and isn't changed in any
> code that uses it. Therefore, make the validation_data
> pointer const.
>
> While at it, remove the duplicate variable in the bitfield
> validation that I'd otherwise have to change to const.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> include/net/netlink.h | 2 +-
> lib/nlattr.c | 5 ++---
> 2 files changed, 3 insertions(+), 4 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* [PATCH 03/22] linux/net.h: use DYNAMIC_DEBUG_BRANCH in net_dbg_ratelimited
From: Rasmus Villemoes @ 2018-09-19 22:04 UTC (permalink / raw)
To: Jason Baron, Andrew Morton; +Cc: linux-kernel, Rasmus Villemoes, netdev
In-Reply-To: <20180919220444.23190-1-linux@rasmusvillemoes.dk>
net_dbg_ratelimited tests the dynamic debug descriptor the old-fashioned
way, and doesn't utilize the static key/jump label implementation on
architectures that HAVE_JUMP_LABEL. Use the DYNAMIC_DEBUG_BRANCH which
is defined appropriately.
Cc: netdev@vger.kernel.org
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
include/linux/net.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/net.h b/include/linux/net.h
index e0930678c8bf..651fca72286c 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -263,7 +263,7 @@ do { \
#define net_dbg_ratelimited(fmt, ...) \
do { \
DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
- if (unlikely(descriptor.flags & _DPRINTK_FLAGS_PRINT) && \
+ if (DYNAMIC_DEBUG_BRANCH(descriptor) && \
net_ratelimit()) \
__dynamic_pr_debug(&descriptor, pr_fmt(fmt), \
##__VA_ARGS__); \
--
2.16.4
^ permalink raw reply related
* Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion
From: David Ahern @ 2018-09-19 16:28 UTC (permalink / raw)
To: Johannes Berg, linux-wireless, netdev; +Cc: Johannes Berg
In-Reply-To: <20180919120900.28708-6-johannes@sipsolutions.net>
On 9/19/18 5:08 AM, Johannes Berg wrote:
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 966cd3dcf31b..2b015e43b725 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -69,7 +69,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
>
> static int validate_nla(const struct nlattr *nla, int maxtype,
> const struct nla_policy *policy,
> - const char **error_msg)
> + struct netlink_ext_ack *extack, bool *extack_set)
extack_set arg is not needed if you handle the "Attribute failed policy
validation" message and NL_SET_BAD_ATTR here as well.
^ permalink raw reply
* Re: [PATCH 3/7] netlink: set extack error message in nla_validate()
From: Johannes Berg @ 2018-09-19 16:31 UTC (permalink / raw)
To: David Ahern, linux-wireless, netdev
In-Reply-To: <3e7aadff-5228-660a-537a-9b54c11fa5cc@gmail.com>
On Wed, 2018-09-19 at 09:20 -0700, David Ahern wrote:
> > nla_for_each_attr(nla, head, len, rem) {
> > - int err = validate_nla(nla, maxtype, policy, NULL);
> > + static const char _msg[] = "Attribute failed policy validation";
> > + const char *msg = _msg;
> > + int err = validate_nla(nla, maxtype, policy, &msg);
> >
> > if (err < 0) {
> > + if (extack)
> > + extack->_msg = msg;
> > NL_SET_BAD_ATTR(extack, nla);
>
> msg, NL_SET_BAD_ATTR and extack handling all can be done in validate_nla
> removing the need for the same message ("Attribute failed policy
> validation") to be declared twice and simplifying the extack setting.
Yeah, perhaps I should take another look at that. I didn't want to do
that originally as validate_nla() has so many exit points, but perhaps
it's better to put a goto label there.
FWIW, in the next patch I'm getting rid of the duplication again - I
wanted to have the patch that has an effect (this one, setting the
message) more clearly separated.
johannes
^ permalink raw reply
* Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion
From: Johannes Berg @ 2018-09-19 16:36 UTC (permalink / raw)
To: David Ahern, linux-wireless, netdev
In-Reply-To: <18ea5293-5a12-7aca-7373-12d1ab3a0821@gmail.com>
On Wed, 2018-09-19 at 09:28 -0700, David Ahern wrote:
> On 9/19/18 5:08 AM, Johannes Berg wrote:
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index 966cd3dcf31b..2b015e43b725 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -69,7 +69,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
> >
> > static int validate_nla(const struct nlattr *nla, int maxtype,
> > const struct nla_policy *policy,
> > - const char **error_msg)
> > + struct netlink_ext_ack *extack, bool *extack_set)
>
> extack_set arg is not needed if you handle the "Attribute failed policy
> validation" message and NL_SET_BAD_ATTR here as well.
I'm not sure that's true, but perhaps you have a better idea than me?
My thought would be to introduce an "error" label in validate_nla(),
that sets up the extack data.
Then we could skip over that if we have a separate message to report,
making the NLA_REJECT case easier.
However, if we do nested validation, I'm not sure it really is that much
easier? We still need to figure out if the nested validation was setting
the message (and bad attr), rather than it having been set before we
even get into this function.
So let's say we have
case NLA_NESTED:
/* a nested attributes is allowed to be empty; if its not,
* it must have a size of at least NLA_HDRLEN.
*/
if (attrlen == 0)
break;
if (attrlen < NLA_HDRLEN)
return -ERANGE;
if (pt->validation_data) {
int err;
err = nla_validate_parse(nla_data(nla), nla_len(nla),
pt->len, pt->validation_data,
extack, extack_set, NULL);
if (err < 0)
return err;
}
break;
right now after all the patches.
The "return -ERANGE;" would become "{ err = -ERANGE; goto error; }", but
I'm not really sure we can cleanly handle the other case?
Hmm. Maybe it works if we ensure that nla_validate_parse() has no other
return points that can fail outside of validate_nla(), or we set up the
extack data there as well, so that once we have a nested
nla_validate_parse() we know that it's been set.
Actually, we need to do that anyway so that we can move the setting into
validate_nla(), and then it should work.
Mechanics aside - I'll take a look later tonight or tomorrow - do you
think the goal/external interface of this makes sense?
johannes
^ permalink raw reply
* Re: [PATCH 1/1] macsec: reflect the master interface state
From: Radu Rendec @ 2018-09-19 16:44 UTC (permalink / raw)
To: sd; +Cc: netdev, davem, ptalbert
In-Reply-To: <20180919152425.GA3046@bistromath.localdomain>
Hello,
On Wed, Sep 19, 2018 at 11:24 AM Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2018-09-18, 20:16:12 -0400, Radu Rendec wrote:
> > This patch makes macsec interfaces reflect the state of the underlying
> > interface: if the master interface changes state to up/down, the macsec
> > interface changes its state accordingly.
>
> Yes, that's a bit unfortunate. I was wondering if we should just allow
> setting the device up, and let link state handle the rest.
Yes, that could work. It would also be consistent with the idea of
propagating only the link state.
> It's missing small parts of link state handling that I have been
> testing, mainly when creating a new device.
Can you please be more specific? I've just looked at macsec_newlink()
and I didn't notice anything related to link state handling.
> > - list_for_each_entry_safe(m, n, &rxd->secys, secys) {
> > + list_for_each_entry_safe(m, n, &rxd->secys, secys)
> > macsec_common_dellink(m->secy.netdev, &head);
> > - }
>
> Please don't mix coding style changes with bug fixes.
Noted.
> > + case NETDEV_DOWN: {
> > + struct net_device *dev, *tmp;
> > + LIST_HEAD(close_list);
> > +
> > + list_for_each_entry(m, &rxd->secys, secys) {
> > + dev = m->secy.netdev;
> > +
> > + if (dev->flags & IFF_UP)
> > + list_add(&dev->close_list, &close_list);
> > + }
> > +
> > + dev_close_many(&close_list, false);
> > +
> > + list_for_each_entry_safe(dev, tmp, &close_list, close_list) {
> > + netif_stacked_transfer_operstate(real_dev, dev);
> > + list_del_init(&dev->close_list);
> > + }
> > + list_del(&close_list);
> > + break;
>
> My version of the patch only does netif_stacked_transfer_operstate(),
> and skips setting the device administratively down (ie, same as
> NETDEV_CHANGE).
That makes sense. But, since you mentioned you had your own patch, does
it make sense for me to continue working on mine?
> > }
> > + case NETDEV_UP:
> > + list_for_each_entry(m, &rxd->secys, secys) {
> > + struct net_device *dev = m->secy.netdev;
> > + int flags = dev_get_flags(dev);
> > +
> > + if (!(flags & IFF_UP)) {
> > + dev_change_flags(dev, flags | IFF_UP);
> > + netif_stacked_transfer_operstate(real_dev, dev);
> > + }
> > + }
> > + break;
>
> I don't like that this completely ignores whether the device was done
> independently of the lower link. Maybe the administrator actually
> wants the macsec device down, regardless of state changes on the lower
> device.
I thought about that too and I don't like it either. Perhaps the vlan
approach of having a "loose binding" flag is worth considering. Then the
admin can decice for themselves.
However, I guess the problem disappears if only the link state is
propagated. The only caveat to that is to not propagate an "up" link
state while the macsec interface is administratively down, but
"remember" to propagate it later if the macsec interface is
administratively set to "up".
Thanks for the feedback!
Radu Rendec
^ permalink raw reply
* Re: Bridge connectivity interruptions while devices join or leave the bridge
From: Ido Schimmel @ 2018-09-19 16:45 UTC (permalink / raw)
To: Johannes Wienke; +Cc: netdev
In-Reply-To: <cc7b7c8d-16bb-18a5-4b3a-91690acf47dc@techfak.uni-bielefeld.de>
On Wed, Sep 19, 2018 at 01:00:15PM +0200, Johannes Wienke wrote:
> This behavior of inheriting the mac address is really unexpected to us.
> Is it documented somewhere?
Not that I'm aware, but it's a well established behavior.
^ permalink raw reply
* Re: array bounds warning in xfrm_output_resume
From: David Ahern @ 2018-09-19 16:57 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev@vger.kernel.org
In-Reply-To: <f116bb15-7fed-3b2e-4b20-a7a2c4317964@gmail.com>
On 6/18/18 11:10 AM, David Ahern wrote:
> Florian:
>
> I am seeing this warning:
>
> $ make O=kbuild/perf -j 24 -s
> In file included from /home/dsa/kernel-3.git/include/linux/kernel.h:10:0,
> from /home/dsa/kernel-3.git/include/linux/list.h:9,
> from /home/dsa/kernel-3.git/include/linux/module.h:9,
> from /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c:13:
> /home/dsa/kernel-3.git/net/xfrm/xfrm_output.c: In function
> ‘xfrm_output_resume’:
> /home/dsa/kernel-3.git/include/linux/compiler.h:252:20: warning: array
> subscript is above array bounds [-Warray-bounds]
> __read_once_size(&(x), __u.__c, sizeof(x)); \
> ^~~~
> /home/dsa/kernel-3.git/include/linux/compiler.h:258:22: note: in
> expansion of macro ‘__READ_ONCE’
> #define READ_ONCE(x) __READ_ONCE(x, 1)
> ^~~~~~~~~~~
> /home/dsa/kernel-3.git/include/linux/rcupdate.h:350:48: note: in
> expansion of macro ‘READ_ONCE’
> typeof(*p) *________p1 = (typeof(*p) *__force)READ_ONCE(p); \
> ^~~~~~~~~
> /home/dsa/kernel-3.git/include/linux/rcupdate.h:487:2: note: in
> expansion of macro ‘__rcu_dereference_check’
> __rcu_dereference_check((p), (c) || rcu_read_lock_held(), __rcu)
> ^~~~~~~~~~~~~~~~~~~~~~~
> /home/dsa/kernel-3.git/include/linux/rcupdate.h:545:28: note: in
> expansion of macro ‘rcu_dereference_check’
> #define rcu_dereference(p) rcu_dereference_check(p, 0)
> ^~~~~~~~~~~~~~~~~~~~~
> /home/dsa/kernel-3.git/include/linux/netfilter.h:218:15: note: in
> expansion of macro ‘rcu_dereference’
> hook_head = rcu_dereference(net->nf.hooks_arp[hook]);
> ^~~~~~~~~~~~~~~
>
> Line in question is the nf_hook in xfrm_output_resume.
> NF_INET_POST_ROUTING = 4 which is greater than NF_ARP_NUMHOOKS = 3
>
> I believe ef57170bbfdd6 is the commit that introduced the warning
>
Hi Florian:
I am still seeing this. I realize the compiler is not understanding the
conditions properly, but it is a distraction every time I do a kernel
build on Debian Stretch having to filter the above noise from whether I
introduce another warning.
^ permalink raw reply
* [PATCH net] bnxt_en: don't try to offload VLAN 'modify' action
From: Davide Caratti @ 2018-09-19 17:01 UTC (permalink / raw)
To: Michael Chan, David S. Miller, netdev; +Cc: Sathya Perla
bnxt offload code currently supports only 'push' and 'pop' operation: let
.ndo_setup_tc() return -EOPNOTSUPP if VLAN 'modify' action is configured.
Fixes: 2ae7408fedfe ("bnxt_en: bnxt: add TC flower filter offload support")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
index 092c817f8f11..e1594c9df4c6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c
@@ -75,17 +75,23 @@ static int bnxt_tc_parse_redir(struct bnxt *bp,
return 0;
}
-static void bnxt_tc_parse_vlan(struct bnxt *bp,
- struct bnxt_tc_actions *actions,
- const struct tc_action *tc_act)
+static int bnxt_tc_parse_vlan(struct bnxt *bp,
+ struct bnxt_tc_actions *actions,
+ const struct tc_action *tc_act)
{
- if (tcf_vlan_action(tc_act) == TCA_VLAN_ACT_POP) {
+ switch (tcf_vlan_action(tc_act)) {
+ case TCA_VLAN_ACT_POP:
actions->flags |= BNXT_TC_ACTION_FLAG_POP_VLAN;
- } else if (tcf_vlan_action(tc_act) == TCA_VLAN_ACT_PUSH) {
+ break;
+ case TCA_VLAN_ACT_PUSH:
actions->flags |= BNXT_TC_ACTION_FLAG_PUSH_VLAN;
actions->push_vlan_tci = htons(tcf_vlan_push_vid(tc_act));
actions->push_vlan_tpid = tcf_vlan_push_proto(tc_act);
+ break;
+ default:
+ return -EOPNOTSUPP;
}
+ return 0;
}
static int bnxt_tc_parse_tunnel_set(struct bnxt *bp,
@@ -134,7 +140,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,
/* Push/pop VLAN */
if (is_tcf_vlan(tc_act)) {
- bnxt_tc_parse_vlan(bp, actions, tc_act);
+ rc = bnxt_tc_parse_vlan(bp, actions, tc_act);
+ if (rc)
+ return rc;
continue;
}
--
2.17.1
^ permalink raw reply related
* Re: Bridge connectivity interruptions while devices join or leave the bridge
From: Stephen Hemminger @ 2018-09-19 17:03 UTC (permalink / raw)
To: Ido Schimmel; +Cc: Johannes Wienke, netdev
In-Reply-To: <20180919164508.GA4975@splinter>
On Wed, 19 Sep 2018 19:45:08 +0300
Ido Schimmel <idosch@idosch.org> wrote:
> On Wed, Sep 19, 2018 at 01:00:15PM +0200, Johannes Wienke wrote:
> > This behavior of inheriting the mac address is really unexpected to us.
> > Is it documented somewhere?
>
> Not that I'm aware, but it's a well established behavior.
Not documented, has always been that way. It seems to be part of 802 standard maybe?
Anyway, if you set a MAC address of the bridge device it makes it sticky;
i.e it won't change if ports of bridge change.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: Allow the l3mdev to be a loopback
From: David Ahern @ 2018-09-19 17:19 UTC (permalink / raw)
To: Mike Manning, netdev; +Cc: Robert Shearman
In-Reply-To: <20180919125653.15913-1-mmanning@vyatta.att-mail.com>
On 9/19/18 5:56 AM, Mike Manning wrote:
> From: Robert Shearman <rshearma@vyatta.att-mail.com>
>
> There is no way currently for an IPv6 client connect using a loopback
> address in a VRF, whereas for IPv4 the loopback address can be added:
>
> $ sudo ip addr add dev vrfred 127.0.0.1/8
> $ sudo ip -6 addr add ::1/128 dev vrfred
> RTNETLINK answers: Cannot assign requested address
>
> So allow ::1 to be configured on an L3 master device. In order for
> this to be usable ip_route_output_flags needs to not consider ::1 to
> be a link scope address (since oif == l3mdev and so it would be
> dropped), and ipv6_rcv needs to consider the l3mdev to be a loopback
> device so that it doesn't drop the packets.
>
> Signed-off-by: Robert Shearman <rshearma@vyatta.att-mail.com>
> Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
> ---
> net/ipv6/addrconf.c | 1 +
> net/ipv6/ip6_input.c | 3 ++-
> net/ipv6/route.c | 3 ++-
> 3 files changed, 5 insertions(+), 2 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
Been on my to-do list for a while. Thanks for the patch. This resolves,
for example, a harmless error message from the 'host' command from
bind9-host-9.10.3 which probes for dscp support via the loopback
address. e.g.,
$ host www.google.com
../../../../lib/isc/unix/net.c:581: sendmsg() failed: Network is unreachable
^ permalink raw reply
* Re: [PATCH net-next v5 00/20] WireGuard: Secure Network Tunnel
From: Ard Biesheuvel @ 2018-09-19 17:21 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Linux Kernel Mailing List, <netdev@vger.kernel.org>,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, David S. Miller,
Greg Kroah-Hartman
In-Reply-To: <20180918210120.GA29812@zx2c4.com>
On 18 September 2018 at 14:01, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hi Ard,
>
> On Tue, Sep 18, 2018 at 11:28:50AM -0700, Ard Biesheuvel wrote:
>> On 18 September 2018 at 09:16, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> > - While I initially wasn't going to do this for the initial
>> > patchset, it was just so simple to do: now there's a nosimd
>> > module parameter that can be used to disable simd instructions
>> > for debugging and testing, or on weird systems.
>> >
>>
>> I was going to respond in the other thread but it is probably better
>> to move the discussion here.
>>
>> My concern about the monolithic nature of each algo module is not only
>> about SIMD, and it has nothing to do with weird systems. It has to do
>> with micro-architectural differences which are more common on ARM than
>> on other architectures *, I suppose. But generalizing from that, it
>> has to do with policy which is currently owned by userland and not by
>> the kernel. This will also be important for choosing between the time
>> variant but less safe table based scalar AES and the much slower time
>> invariant version (which is substantially slower, especially on
>> decryption) once we move AES into this library.
>>
>> So a command line option for the kernel is not the solution here. If
>> we can't have separate modules, could we at least have per-module
>> options that put the policy decisions back into userland?
>>
>> * as an example, the SHA256 NEON code I collaborated on with Andy
>> Polyakov 2 years ago is significantly faster on some cores and not on
>> others
>
> Interesting concern. There are micro-architectural quirks on x86 too
> that the current code actually already considers. Notably, we use an
> AVX-512VL path for Skylake-X but an AVX-512F path for Knights Landing
> and Coffee Lake and others, due to thermal throttling when touching the
> zmm registers on Skylake-X. So, in the code, we have it automatically
> select the right thing based on the micro-architecture.
>
> Is the same thing not possible with ARM? Do you not have access to this
> information already, such that the module can just always do the right
> thing and not require any user intervention?
>
That depends on what the right thing is. 'Fastest' does not
necessarily mean 'optimal', and I guess the thermal throttling on
Skylake-X may still result in the most power efficient implementation,
which may be the preferred one in some contexts.
The point is that this is a policy decision, and those belong in
userland not in the kernel.
> If so, that would be ideal. If not (and I'm curious to learn why not
> exactly), then indeed we could add some runtime nobs in /sys/module/
> {algo}/parameters/{nob}, or the like. This would be super easy to do,
> should we ever encounter a situation where we're unable to auto-detect
> the correct thing.
>
> Regards,
> Jason
^ permalink raw reply
* Re: [PATCH mlx5-next 02/25] net/mlx5: Set uid as part of QP commands
From: Jason Gunthorpe @ 2018-09-19 17:27 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180917110418.18937-3-leon@kernel.org>
On Mon, Sep 17, 2018 at 02:03:55PM +0300, Leon Romanovsky wrote:
> From: Yishai Hadas <yishaih@mellanox.com>
>
> Set uid as part of QP commands so that the firmware can manage the
> QP object in a secured way.
>
> That will enable using a QP that was created by verbs application to
> be used by the DEVX flow in case the uid is equal.
>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/net/ethernet/mellanox/mlx5/core/qp.c | 45 +++++++++++++++++-----------
> include/linux/mlx5/mlx5_ifc.h | 22 +++++++-------
> include/linux/mlx5/qp.h | 1 +
> 3 files changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/qp.c b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> index 4ca07bfb6b14..04f72a1cdbcc 100644
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> @@ -240,6 +240,7 @@ int mlx5_core_create_qp(struct mlx5_core_dev *dev,
> if (err)
> return err;
>
> + qp->uid = MLX5_GET(create_qp_in, in, uid);
> qp->qpn = MLX5_GET(create_qp_out, out, qpn);
> mlx5_core_dbg(dev, "qpn = 0x%x\n", qp->qpn);
>
> @@ -261,6 +262,7 @@ int mlx5_core_create_qp(struct mlx5_core_dev *dev,
> memset(dout, 0, sizeof(dout));
> MLX5_SET(destroy_qp_in, din, opcode, MLX5_CMD_OP_DESTROY_QP);
> MLX5_SET(destroy_qp_in, din, qpn, qp->qpn);
> + MLX5_SET(destroy_qp_in, din, uid, qp->uid);
> mlx5_cmd_exec(dev, din, sizeof(din), dout, sizeof(dout));
> return err;
> }
> @@ -320,6 +322,7 @@ int mlx5_core_destroy_qp(struct mlx5_core_dev *dev,
>
> MLX5_SET(destroy_qp_in, in, opcode, MLX5_CMD_OP_DESTROY_QP);
> MLX5_SET(destroy_qp_in, in, qpn, qp->qpn);
> + MLX5_SET(destroy_qp_in, in, uid, qp->uid);
> err = mlx5_cmd_exec(dev, in, sizeof(in), out, sizeof(out));
> if (err)
> return err;
> @@ -373,7 +376,7 @@ static void mbox_free(struct mbox_info *mbox)
>
> static int modify_qp_mbox_alloc(struct mlx5_core_dev *dev, u16 opcode, int qpn,
> u32 opt_param_mask, void *qpc,
> - struct mbox_info *mbox)
> + struct mbox_info *mbox, u16 uid)
> {
> mbox->out = NULL;
> mbox->in = NULL;
> @@ -381,26 +384,32 @@ static int modify_qp_mbox_alloc(struct mlx5_core_dev *dev, u16 opcode, int qpn,
> #define MBOX_ALLOC(mbox, typ) \
> mbox_alloc(mbox, MLX5_ST_SZ_BYTES(typ##_in), MLX5_ST_SZ_BYTES(typ##_out))
>
> -#define MOD_QP_IN_SET(typ, in, _opcode, _qpn) \
> - MLX5_SET(typ##_in, in, opcode, _opcode); \
> - MLX5_SET(typ##_in, in, qpn, _qpn)
> -
> -#define MOD_QP_IN_SET_QPC(typ, in, _opcode, _qpn, _opt_p, _qpc) \
> - MOD_QP_IN_SET(typ, in, _opcode, _qpn); \
> - MLX5_SET(typ##_in, in, opt_param_mask, _opt_p); \
> - memcpy(MLX5_ADDR_OF(typ##_in, in, qpc), _qpc, MLX5_ST_SZ_BYTES(qpc))
> +#define MOD_QP_IN_SET(typ, in, _opcode, _qpn, _uid) \
> + do { \
> + MLX5_SET(typ##_in, in, opcode, _opcode); \
> + MLX5_SET(typ##_in, in, qpn, _qpn); \
> + MLX5_SET(typ##_in, in, uid, _uid); \
> + } while (0)
> +
> +#define MOD_QP_IN_SET_QPC(typ, in, _opcode, _qpn, _opt_p, _qpc, _uid) \
> + do { \
> + MOD_QP_IN_SET(typ, in, _opcode, _qpn, _uid); \
> + MLX5_SET(typ##_in, in, opt_param_mask, _opt_p); \
> + memcpy(MLX5_ADDR_OF(typ##_in, in, qpc), \
> + _qpc, MLX5_ST_SZ_BYTES(qpc)); \
> + } while (0)
These should get touched by clang-format to follow the usual
formatting for macros..
Jason
^ permalink raw reply
* Re: [PATCH mlx5-next 03/25] net/mlx5: Set uid as part of RQ commands
From: Jason Gunthorpe @ 2018-09-19 17:28 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180917110418.18937-4-leon@kernel.org>
On Mon, Sep 17, 2018 at 02:03:56PM +0300, Leon Romanovsky wrote:
> From: Yishai Hadas <yishaih@mellanox.com>
>
> Set uid as part of RQ commands so that the firmware can manage the
> RQ object in a secured way.
>
> That will enable using an RQ that was created by verbs application
> to be used by the DEVX flow in case the uid is equal.
>
> Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> drivers/net/ethernet/mellanox/mlx5/core/qp.c | 16 ++++++++++++++--
> include/linux/mlx5/mlx5_ifc.h | 6 +++---
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/qp.c b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> index 04f72a1cdbcc..0ca68ef54d93 100644
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> @@ -540,6 +540,17 @@ int mlx5_core_xrcd_dealloc(struct mlx5_core_dev *dev, u32 xrcdn)
> }
> EXPORT_SYMBOL_GPL(mlx5_core_xrcd_dealloc);
>
> +static void destroy_rq_tracked(struct mlx5_core_dev *dev, u32 rqn, u16 uid)
> +{
> + u32 in[MLX5_ST_SZ_DW(destroy_rq_in)] = {0};
> + u32 out[MLX5_ST_SZ_DW(destroy_rq_out)] = {0};
= {} is the preferred version of this, right?
{0} explicitly initializes the first element to zero and only works if
the first element happens to be something integral..
Jason
^ 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