* [PATCH] bpf: Replace a seq_printf() call by seq_puts() in btf_enum_seq_show()
From: Markus Elfring @ 2019-07-02 17:13 UTC (permalink / raw)
To: bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 2 Jul 2019 19:04:08 +0200
A string which did not contain a data format specification should be put
into a sequence. Thus use the corresponding function “seq_puts”.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
kernel/bpf/btf.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 546ebee39e2a..679a19968f29 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -2426,9 +2426,8 @@ static void btf_enum_seq_show(const struct btf *btf, const struct btf_type *t,
for (i = 0; i < nr_enums; i++) {
if (v == enums[i].val) {
- seq_printf(m, "%s",
- __btf_name_by_offset(btf,
- enums[i].name_off));
+ seq_puts(m,
+ __btf_name_by_offset(btf, enums[i].name_off));
return;
}
}
--
2.22.0
^ permalink raw reply related
* Re: [RFC net-next] net: dsa: add support for MC_DISABLED attribute
From: Ido Schimmel @ 2019-07-02 17:11 UTC (permalink / raw)
To: Linus Lüssing
Cc: Russell King - ARM Linux admin, nikolay, Ido Schimmel,
Vivien Didelot, Florian Fainelli, netdev@vger.kernel.org,
Jiri Pirko, andrew@lunn.ch, davem@davemloft.net, bridge,
b.a.t.m.a.n
In-Reply-To: <20190630165601.GC2500@otheros>
On Sun, Jun 30, 2019 at 06:56:01PM +0200, Linus Lüssing wrote:
> > On Sun, Jun 23, 2019 at 10:44:27AM +0300, Ido Schimmel wrote:
> > > See commit b00589af3b04 ("bridge: disable snooping if there is no
> > > querier"). I think that's unfortunate behavior that we need because
> > > multicast snooping is enabled by default. If it weren't enabled by
> > > default, then anyone enabling it would also make sure there's a querier
> > > in the network.
>
> I do not quite understand that point. In a way, that's what we
> have right now, isn't it? By default it's disabled, because by
> default there is no querier on the link. So anyone wanting to use
> multicast snooping will need to make sure there's a querier in the
> network.
Hi Linus,
Querier state is not reflected to drivers ATM, so drivers believe the
bridge is multicast aware and unregistered multicast packets are only
flooded to mrouter ports. Hosts that are silent (because there is no
querier) never get the traffic addressed to them (f.e., IPv6 neighbour
solicitation).
> Overall I think the querier (election) mechanism in the standards could
> need an update. While the lowest-address first might have
> worked well back then, in uniform, fully wired networks where the
> position of the querier did not matter, this is not a good
> solution anymore in networks involving wireless, dynamic connections.
> Especially in wireless mesh networks this is a bit of an issue for
> us. Ideally, the querier mechanism were dismissed in favour of simply
> unsolicited, periodic IGMP/MLD reports...
>
> But of course, updating IETF standards is no solution for now.
>
> While more complicated, it would not be impossible to consider the
> querier state, would it? I mean you probably already need to
> consider the case of a user disabling multicast snooping during
> runtime, right?
Sure, this is implemented.
> So similarly, you could react to appearing or disappearing queriers?
Yes, but it's a bit more complicated since we need to differentiate
between IPv4 and IPv6. If the bridge is multicast aware, but there is
only IPv4 querier on the link, then:
1. All the IPv6 MDB entries need to be removed from the device. At least
in mlxsw, we do not have a way to ignore only IPv6 entries. From the
device's perspective, an MDB entry is just a multicast DMAC with a
bitmap of ports packets should be replicated to.
2. We need to split the flood tables used for IPv4 and IPv6 unregistered
multicast packets. For IPv4, packets should only be flooded to mrouter
ports whereas for IPv6 packets should be flooded to all the member
ports.
Do you differentiate between IPv4 and IPv6 in batman-adv?
> Cheers, Linus
Thanks for the feedback!
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Ilya Leoshkevich @ 2019-07-02 16:58 UTC (permalink / raw)
To: Y Song; +Cc: bpf, netdev
In-Reply-To: <CAH3MdRUk5x2D9yRuKpGpVuDMFF0JbYeB+Y0Qz6chtPgfm-1vxA@mail.gmail.com>
> Am 02.07.2019 um 18:42 schrieb Y Song <ys114321@gmail.com>:
>
> On Tue, Jul 2, 2019 at 8:40 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>>
>> -#elif defined(__s390x__)
>> - #define bpf_target_s930x
>
> I see in some other places (e.g., bcc) where
> macro __s390x__ is also used to indicate a s390 architecture.
> Could you explain the difference between __s390__ and
> __s390x__?
__s390__ is defined for 32-bit and 64-bit variants, __s390x__ is defined
for 64-bit variant only.
>> #if defined(bpf_target_x86)
>>
>> +#ifdef __KERNEL__
>
> In samples/bpf/, __KERNEL__ is defined at clang options and
> in selftests/bpf/, the __KERNEL__ is not defined.
>
> I checked x86 pt_regs definition with and without __KERNEL__.
> They are identical except some register name difference.
> I am wondering whether we can unify into all without
> __KERNEL__. Is __KERNEL__ really needed?
Right now removing it causes the build to fail, but the errors look
fixable. However, I wonder whether there is a plan regarding this:
should eBPF programs be built with user headers, kernel headers,
or both? Status quo appears to be "both", so I’ve decided to stick with
that in this patch.
>> +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
>> +struct pt_regs;
>> +#define PT_REGS_PARM1(x) (((const volatile user_pt_regs *)(x))->gprs[2])
>
> Is user_pt_regs a recent change or has been there for quite some time?
> I am asking since bcc did not use user_pt_regs yet.
It was added in late 2017 in commit 466698e654e8 ("s390/bpf: correct
broken uapi for BPF_PROG_TYPE_PERF_EVENT program type“).
^ permalink raw reply
* Re: [PATCH net-next] r8169: add random MAC address fallback
From: Heiner Kallweit @ 2019-07-02 16:49 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <61a7754f-bdf9-f69a-296d-47353a78c8b4@gmail.com>
On 02.07.2019 08:18, Heiner Kallweit wrote:
>>From 1c8bacf724f1450e5256c68fbff407305faf9cbd Mon Sep 17 00:00:00 2001
>
>
>
Sorry, something went wrong when preparing the commit message. I'll resubmit.
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/ethernet/realtek/r8169_main.c | 40 +++++++++++++++--------
> 1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 450c74dc1..d6c137b7f 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -6651,13 +6651,36 @@ static int rtl_get_ether_clk(struct rtl8169_private *tp)
> return rc;
> }
>
> +static void rtl_init_mac_address(struct rtl8169_private *tp)
> +{
> + struct net_device *dev = tp->dev;
> + u8 *mac_addr = dev->dev_addr;
> + int rc, i;
> +
> + rc = eth_platform_get_mac_address(tp_to_dev(tp), mac_addr);
> + if (!rc)
> + goto done;
> +
> + rtl_read_mac_address(tp, mac_addr);
> + if (is_valid_ether_addr(mac_addr))
> + goto done;
> +
> + for (i = 0; i < ETH_ALEN; i++)
> + mac_addr[i] = RTL_R8(tp, MAC0 + i);
> + if (is_valid_ether_addr(mac_addr))
> + goto done;
> +
> + eth_hw_addr_random(dev);
> + dev_warn(tp_to_dev(tp), "can't read MAC address, setting random one\n");
> +done:
> + rtl_rar_set(tp, mac_addr);
> +}
> +
> static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> - /* align to u16 for is_valid_ether_addr() */
> - u8 mac_addr[ETH_ALEN] __aligned(2) = {};
> struct rtl8169_private *tp;
> struct net_device *dev;
> - int chipset, region, i;
> + int chipset, region;
> int jumbo_max, rc;
>
> dev = devm_alloc_etherdev(&pdev->dev, sizeof (*tp));
> @@ -6749,16 +6772,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> u64_stats_init(&tp->rx_stats.syncp);
> u64_stats_init(&tp->tx_stats.syncp);
>
> - /* get MAC address */
> - rc = eth_platform_get_mac_address(&pdev->dev, mac_addr);
> - if (rc)
> - rtl_read_mac_address(tp, mac_addr);
> -
> - if (is_valid_ether_addr(mac_addr))
> - rtl_rar_set(tp, mac_addr);
> -
> - for (i = 0; i < ETH_ALEN; i++)
> - dev->dev_addr[i] = RTL_R8(tp, MAC0 + i);
> + rtl_init_mac_address(tp);
>
> dev->ethtool_ops = &rtl8169_ethtool_ops;
>
>
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: santosh.shilimkar @ 2019-07-02 16:49 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <6ff00a46-07f6-7be2-8e75-c87448568aa4@oracle.com>
On 7/1/19 10:11 PM, Gerd Rausch wrote:
> Hi Santosh,
>
> On 01/07/2019 19.28, santosh.shilimkar@oracle.com wrote:
>>>
>> Below. All command timeouts are 60 seconds.
>>
>> enum {
>> MLX4_CMD_TIME_CLASS_A = 60000,
>> MLX4_CMD_TIME_CLASS_B = 60000,
>> MLX4_CMD_TIME_CLASS_C = 60000,
>> };
>>
>
> Thank you for the pointer.
>
>> But having said that, I re-looked the code you are patching
>> and thats actually only FRWR code which is purely work-request
>> based so this command timeout shouldn't matter.
>>
>
> Which brings us back full circle to the question of
> what the timeout ought to be?
>
> Please keep in mind that prior to this fix,
> the RDS code didn't wait at all:
>
> It simply posted those registration (IB_WR_REG_MR)
> and invalidation (IB_WR_LOCAL_INV)
> work-requests, with no regards to when the firmware
> would execute them.
>
> Arguably, waiting any amount time greater than zero
> for the operation to complete is better than not waiting at all.
>
> We can change the timeout to a high value, or even make it infinite
> by using "wait_event" instead of "wait_event_timeout".
>
> For the registration work-requests there is a benefit to wait a short
> amount of time only (the trade-off described in patch #1 of this series).
>
Actually we should just switch this code to what Avinash has
finally made in downstream code. That keeps the RDS_GET_MR
semantics and makes sure MR is really valid before handing over
the key to userland. There is no need for any timeout
for registration case.
> For de-registration work-requests, it is beneficial to wait
> until they are truly done.
> But: Function "rds_ib_unreg_frmr" prior and post this change
> simply moves on after a failed de-registration attempt,
> and releases the pages owned by the memory region.
>
> This patch does _not_ change that behavior.
>
>> If the work request fails, then it will lead to flush errors and
>> MRs will be marked as STALE. So this wait may not be necessary
>>
>
> This wait is necessary to avoid the 2 scenarios described
> in the commit-log message:
>
> #1) Memory regions bouncing between "drop_list" and "clean_list"
> as items on the "clean_list" aren't really clean until
> their state transitions to "FRMR_IS_FREE".
>
> #2) Prevent an access error as "rds_ib_post_inv" is called
> just prior to de-referencing pages via "__rds_ib_teardown_mr".
> And you certainly don't want those pages populated in the
> HCA's memory-translation-table with full access, while
> the Linux kernel 'thinks' you gave them back already
> and starts re-purposing them.
>
>> RDS_GET_MR case is what actually showing the issue you saw
>> and the fix for that Avinash has it in production kernel.
>
> Actually, no:
> Socket option RDS_GET_MR wasn't even in the code-path of the
> tests I performed:
>
> It were there RDS_CMSG_RDMA_MAP / RDS_CMSG_RDMA_DEST control
> messages that ended up calling '__rds_rdma_map".
>
What option did you use ? Default option with rds-stress is
RDS_GET_MR and hence the question.
>>
>> I believe with that change, registration issue becomes non-issue
>> already.
>>
>
> Please explain how that is related to this fix-suggestion?
>
> I submitted this patch #3 and the others in this series in order
> to fix bugs in the RDS that is currently shipped with Linux.
>
> It may very well be the case that there are other changes
> that Avinash put into production kernels that would be better
> suited to fix this and other problems.
>
> But that should not eliminate the need to fix what is currently broken.
>
> Fixing what's broken does not preclude replacing the fixed code
> with newer or better versions of the same.
>
>> And as far as invalidation concerned with proxy qp, it not longer
>> races with data path qp.
>>
>
> I don't understand, please elaborate.
>
>> May be you can try those changes if not already to see if it
>> addresses the couple of cases where you ended up adding
>> timeouts.
>>
>
> I don't understand, please elaborate:
> a) Are you saying this issue should not be fixed?
> b) Or are you suggesting to replace this fix with a different fix?
> If it's the later, please point out what you have in mind.
> c) ???
>
All am saying is the code got changed for good reason and that changed
code makes some of these race conditions possibly not applicable.
So instead of these timeout fixes, am suggesting to use that
code as fix. At least test it with those changes and see whats
the behavior.
Regards,
Santosh
^ permalink raw reply
* [PATCH bpf-next v2] bpftool: Add BPF_F_QUERY_EFFECTIVE support in bpftool cgroup [show|tree]
From: Takshak Chahande @ 2019-07-02 16:48 UTC (permalink / raw)
To: netdev; +Cc: ast, jakub.kicinski, daniel, ctakshak, kernel-team
With different bpf attach_flags available to attach bpf programs specially
with BPF_F_ALLOW_OVERRIDE and BPF_F_ALLOW_MULTI, the list of effective
bpf-programs available to any sub-cgroups really needs to be available for
easy debugging.
Using BPF_F_QUERY_EFFECTIVE flag, one can get the list of not only attached
bpf-programs to a cgroup but also the inherited ones from parent cgroup.
So additional "effective" option is introduced to use BPF_F_QUERY_EFFECTIVE
query flag here to list all the effective bpf-programs available for execution
at a specified cgroup.
Reused modified test program test_cgroup_attach from tools/testing/selftests/bpf:
# ./test_cgroup_attach
With old bpftool (without 'effective' option)
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/
ID AttachType AttachFlags Name
129 egress multi pkt_cntr_1
130 egress multi pkt_cntr_2
Attached new program pkt_cntr_4 in cg2 gives following:
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2/
ID AttachType AttachFlags Name
131 egress override pkt_cntr_4
And with 'effective' option it shows all effective programs for cg2:
# bpftool cgroup show /sys/fs/cgroup/cgroup-test-work-dir/cg1/cg2/ effective
ID AttachType AttachFlags Name
131 egress override pkt_cntr_4
129 egress override pkt_cntr_1
130 egress override pkt_cntr_2
Signed-off-by: Takshak Chahande <ctakshak@fb.com>
---
Changelog:
v2:
- Removed global '-e|--effective' query flag
- Added 'effective' option after cgroup path argument as proposed by Jakub
.../bpftool/Documentation/bpftool-cgroup.rst | 17 +++--
tools/bpf/bpftool/bash-completion/bpftool | 15 +++--
tools/bpf/bpftool/cgroup.c | 66 ++++++++++++-------
3 files changed, 66 insertions(+), 32 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
index 585f270c2d25..8c12e8bb5dd4 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-cgroup.rst
@@ -20,8 +20,8 @@ SYNOPSIS
CGROUP COMMANDS
===============
-| **bpftool** **cgroup { show | list }** *CGROUP*
-| **bpftool** **cgroup tree** [*CGROUP_ROOT*]
+| **bpftool** **cgroup { show | list }** *CGROUP* [**effective**]
+| **bpftool** **cgroup tree** [*CGROUP_ROOT*] [**effective**]
| **bpftool** **cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
| **bpftool** **cgroup detach** *CGROUP* *ATTACH_TYPE* *PROG*
| **bpftool** **cgroup help**
@@ -35,13 +35,18 @@ CGROUP COMMANDS
DESCRIPTION
===========
- **bpftool cgroup { show | list }** *CGROUP*
+ **bpftool cgroup { show | list }** *CGROUP* [**effective**]
List all programs attached to the cgroup *CGROUP*.
Output will start with program ID followed by attach type,
attach flags and program name.
- **bpftool cgroup tree** [*CGROUP_ROOT*]
+ If **effective** is specified, then it retrieves all the
+ effective programs that will be executed for events within
+ a cgroup. This includes inherited along with attached ones
+ to the cgroup.
+
+ **bpftool cgroup tree** [*CGROUP_ROOT*] [**effective**]
Iterate over all cgroups in *CGROUP_ROOT* and list all
attached programs. If *CGROUP_ROOT* is not specified,
bpftool uses cgroup v2 mountpoint.
@@ -50,6 +55,10 @@ DESCRIPTION
commands: it starts with absolute cgroup path, followed by
program ID, attach type, attach flags and program name.
+ With **effective** it retrieves all the effective programs
+ that will be executed for events at each cgroup level as
+ similar to show/list command output for a cgroup.
+
**bpftool cgroup attach** *CGROUP* *ATTACH_TYPE* *PROG* [*ATTACH_FLAGS*]
Attach program *PROG* to the cgroup *CGROUP* with attach type
*ATTACH_TYPE* and optional *ATTACH_FLAGS*.
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index ba37095e1f62..b9b76b812dda 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -679,12 +679,15 @@ _bpftool()
;;
cgroup)
case $command in
- show|list)
- _filedir
- return 0
- ;;
- tree)
- _filedir
+ show|list|tree)
+ case $cword in
+ 3)
+ _filedir
+ ;;
+ 4)
+ COMPREPLY=( $( compgen -W 'effective' -- "$cur" ) )
+ ;;
+ esac
return 0
;;
attach|detach)
diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
index 390b89a224f1..09040dbdc20a 100644
--- a/tools/bpf/bpftool/cgroup.c
+++ b/tools/bpf/bpftool/cgroup.c
@@ -29,6 +29,7 @@
" recvmsg4 | recvmsg6 | sysctl |\n" \
" getsockopt | setsockopt }"
+static unsigned int query_flags;
static const char * const attach_type_strings[] = {
[BPF_CGROUP_INET_INGRESS] = "ingress",
[BPF_CGROUP_INET_EGRESS] = "egress",
@@ -107,7 +108,8 @@ static int count_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
__u32 prog_cnt = 0;
int ret;
- ret = bpf_prog_query(cgroup_fd, type, 0, NULL, NULL, &prog_cnt);
+ ret = bpf_prog_query(cgroup_fd, type, query_flags, NULL, NULL,
+ &prog_cnt);
if (ret)
return -1;
@@ -125,8 +127,8 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
int ret;
prog_cnt = ARRAY_SIZE(prog_ids);
- ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids,
- &prog_cnt);
+ ret = bpf_prog_query(cgroup_fd, type, query_flags, &attach_flags,
+ prog_ids, &prog_cnt);
if (ret)
return ret;
@@ -158,20 +160,32 @@ static int show_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type,
static int do_show(int argc, char **argv)
{
enum bpf_attach_type type;
+ char *cgroup_path;
int cgroup_fd;
int ret = -1;
- if (argc < 1) {
+ if (!REQ_ARGS(1)) {
p_err("too few parameters for cgroup show");
goto exit;
- } else if (argc > 1) {
- p_err("too many parameters for cgroup show");
- goto exit;
}
- cgroup_fd = open(argv[0], O_RDONLY);
+ query_flags = 0;
+ cgroup_path = GET_ARG();
+
+ if (argc) {
+ if (argc == 1 && is_prefix(*argv, "effective")) {
+ query_flags |= BPF_F_QUERY_EFFECTIVE;
+ NEXT_ARG();
+ } else {
+ p_err("invalid argument after cgroup path, "
+ "got: '%s'?, expect only effective", *argv);
+ goto exit;
+ }
+ }
+
+ cgroup_fd = open(cgroup_path, O_RDONLY);
if (cgroup_fd < 0) {
- p_err("can't open cgroup %s", argv[1]);
+ p_err("can't open cgroup %s", cgroup_path);
goto exit;
}
@@ -294,26 +308,34 @@ static char *find_cgroup_root(void)
static int do_show_tree(int argc, char **argv)
{
+ bool free_cg_root = false;
char *cgroup_root;
int ret;
- switch (argc) {
- case 0:
+ query_flags = 0;
+
+ if (!argc) {
cgroup_root = find_cgroup_root();
if (!cgroup_root) {
p_err("cgroup v2 isn't mounted");
return -1;
}
- break;
- case 1:
- cgroup_root = argv[0];
- break;
- default:
- p_err("too many parameters for cgroup tree");
- return -1;
+ free_cg_root = true;
+ } else {
+ cgroup_root = GET_ARG();
+
+ if (argc) {
+ if (argc == 1 && is_prefix(*argv, "effective")) {
+ query_flags |= BPF_F_QUERY_EFFECTIVE;
+ NEXT_ARG();
+ } else {
+ p_err("invalid argument after cgroup path, "
+ "got: '%s'? expect only effective", *argv);
+ return -1;
+ }
+ }
}
-
if (json_output)
jsonw_start_array(json_wtr);
else
@@ -338,7 +360,7 @@ static int do_show_tree(int argc, char **argv)
if (json_output)
jsonw_end_array(json_wtr);
- if (argc == 0)
+ if (free_cg_root)
free(cgroup_root);
return ret;
@@ -459,8 +481,8 @@ static int do_help(int argc, char **argv)
}
fprintf(stderr,
- "Usage: %s %s { show | list } CGROUP\n"
- " %s %s tree [CGROUP_ROOT]\n"
+ "Usage: %s %s { show | list } CGROUP [effective]\n"
+ " %s %s tree [CGROUP_ROOT] [effective]\n"
" %s %s attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]\n"
" %s %s detach CGROUP ATTACH_TYPE PROG\n"
" %s %s help\n"
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the port parameters.
From: Andrew Lunn @ 2019-07-02 16:48 UTC (permalink / raw)
To: Parav Pandit
Cc: Sudarsana Reddy Kalluru, davem@davemloft.net,
netdev@vger.kernel.org, mkalderon@marvell.com, aelior@marvell.com,
jiri@resnulli.us
In-Reply-To: <AM0PR05MB4866D7B26F48AF0BED9055EED1F80@AM0PR05MB4866.eurprd05.prod.outlook.com>
> A vendor driver calling these APIs is needed at minimum.
Not a vendor driver, but a mainline driver.
But yes, a new API should not be added without at least one user.
Andrew
^ permalink raw reply
* Re: [PATCH 7/7] kbuild: compile-test kernel headers to ensure they are self-contained
From: Joel Fernandes @ 2019-07-02 16:47 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, Sam Ravnborg, Tony Luck, John Fastabend,
Jakub Kicinski, Daniel Borkmann, xdp-newbies, Anton Vorontsov,
Matthias Brugger, Song Liu, Yonghong Song, Michal Marek,
Jesper Dangaard Brouer, Martin KaFai Lau, linux-mediatek,
linux-arm-kernel, Colin Cross, David S. Miller, Kees Cook,
Alexei Starovoitov, netdev, linux-kernel, bpf
In-Reply-To: <20190701005845.12475-8-yamada.masahiro@socionext.com>
On Mon, Jul 01, 2019 at 09:58:45AM +0900, Masahiro Yamada wrote:
> The headers in include/ are globally used in the kernel source tree
> to provide common APIs. They are included from external modules, too.
>
> It will be useful to make as many headers self-contained as possible
> so that we do not have to rely on a specific include order.
>
> There are more than 4000 headers in include/. In my rough analysis,
> 70% of them are already self-contained. With efforts, most of them
> can be self-contained.
>
> For now, we must exclude more than 1000 headers just because they
> cannot be compiled as standalone units. I added them to header-test-.
> The blacklist was mostly generated by a script, so the reason of the
> breakage should be checked later.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Tested-by: Jani Nikula <jani.nikula@intel.com>
> ---
>
> Changes in v4:
> - Fix vmlinux build error
> - Exclude more headers for sparc
>
> Changes in v3:
> - Exclude more headers
> (Tested for allnoconfig + CONFIG_HEADER_TEST=y)
>
> Changes in v2:
> - Add everything to test coverage, and exclude broken ones
> - Rename 'Makefile' to 'Kbuild'
> - Add CONFIG_KERNEL_HEADER_TEST option
>
> Makefile | 1 +
> include/Kbuild | 1253 ++++++++++++++++++++++++++++++++++++++++++++++++
> init/Kconfig | 11 +
> 3 files changed, 1265 insertions(+)
> create mode 100644 include/Kbuild
[snip
> diff --git a/init/Kconfig b/init/Kconfig
> index 74192de8ada6..e2e99544da8d 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -108,6 +108,17 @@ config HEADER_TEST
> If you are a developer or tester and want to ensure the requested
> headers are self-contained, say Y here. Otherwise, choose N.
>
> +config KERNEL_HEADER_TEST
> + bool "Compile test kernel headers"
> + depends on HEADER_TEST
> + help
> + Headers in include/ are used to build external moduls.
Nit:
modules.
Otherwise lgtm, thanks for the cc.
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
^ permalink raw reply
* Re: [PATCH 2/3 bpf-next] i40e: Support zero-copy XDP_TX on the RX path for AF_XDP sockets.
From: Jonathan Lemon @ 2019-07-02 16:44 UTC (permalink / raw)
To: Magnus Karlsson
Cc: Network Development, Björn Töpel, Karlsson, Magnus,
Jakub Kicinski, jeffrey.t.kirsher, kernel-team
In-Reply-To: <CAJ8uoz0EL9gx87JmhjBmYscx-J2UCYK73OV73T3eohOEp0BEUw@mail.gmail.com>
On 1 Jul 2019, at 4:04, Magnus Karlsson wrote:
> On Sat, Jun 29, 2019 at 12:18 AM Jonathan Lemon
> <jonathan.lemon@gmail.com> wrote:
>>
>> When the XDP program attached to a zero-copy AF_XDP socket returns
>> XDP_TX,
>> queue the umem frame on the XDP TX ring. Space on the recycle stack
>> is
>> pre-allocated when the xsk is created. (taken from tx_ring, since
>> the
>> xdp ring is not initialized yet)
>>
>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>> ---
>> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 1 +
>> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 54
>> +++++++++++++++++++--
>> 2 files changed, 51 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> index 100e92d2982f..3e7954277737 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> @@ -274,6 +274,7 @@ static inline unsigned int
>> i40e_txd_use_count(unsigned int size)
>> #define I40E_TX_FLAGS_TSYN BIT(8)
>> #define I40E_TX_FLAGS_FD_SB BIT(9)
>> #define I40E_TX_FLAGS_UDP_TUNNEL BIT(10)
>> +#define I40E_TX_FLAGS_ZC_FRAME BIT(11)
>> #define I40E_TX_FLAGS_VLAN_MASK 0xffff0000
>> #define I40E_TX_FLAGS_VLAN_PRIO_MASK 0xe0000000
>> #define I40E_TX_FLAGS_VLAN_PRIO_SHIFT 29
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> index ce8650d06962..020f9859215d 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> @@ -91,7 +91,8 @@ static int i40e_xsk_umem_enable(struct i40e_vsi
>> *vsi, struct xdp_umem *umem,
>> qid >= netdev->real_num_tx_queues)
>> return -EINVAL;
>>
>> - if (!xsk_umem_recycle_alloc(umem, vsi->rx_rings[0]->count))
>> + if (!xsk_umem_recycle_alloc(umem, vsi->rx_rings[0]->count +
>> + vsi->tx_rings[0]->count))
>> return -ENOMEM;
>>
>> err = i40e_xsk_umem_dma_map(vsi, umem);
>> @@ -175,6 +176,48 @@ int i40e_xsk_umem_setup(struct i40e_vsi *vsi,
>> struct xdp_umem *umem,
>> i40e_xsk_umem_disable(vsi, qid);
>> }
>>
>> +static int i40e_xmit_rcvd_zc(struct i40e_ring *rx_ring, struct
>> xdp_buff *xdp)
>
> This function looks very much like i40e_xmit_xdp_ring(). How can we
> refactor them to make them share more code and not lose performance at
> the same time? This comment is also valid for the ixgbe driver patch
> that follows.
The next patch will split these into a small preamble setup and then
call a common send function.
--
Jonathan
>
> Thanks: Magnus
>
>> +{
>> + struct i40e_ring *xdp_ring;
>> + struct i40e_tx_desc *tx_desc;
>> + struct i40e_tx_buffer *tx_bi;
>> + struct xdp_frame *xdpf;
>> + dma_addr_t dma;
>> +
>> + xdp_ring = rx_ring->vsi->xdp_rings[rx_ring->queue_index];
>> +
>> + if (!unlikely(I40E_DESC_UNUSED(xdp_ring))) {
>> + xdp_ring->tx_stats.tx_busy++;
>> + return I40E_XDP_CONSUMED;
>> + }
>> + xdpf = convert_to_xdp_frame_keep_zc(xdp);
>> + if (unlikely(!xdpf))
>> + return I40E_XDP_CONSUMED;
>> + xdpf->handle = xdp->handle;
>> +
>> + dma = xdp_umem_get_dma(rx_ring->xsk_umem, xdp->handle);
>> + tx_bi = &xdp_ring->tx_bi[xdp_ring->next_to_use];
>> + tx_bi->bytecount = xdpf->len;
>> + tx_bi->gso_segs = 1;
>> + tx_bi->xdpf = xdpf;
>> + tx_bi->tx_flags = I40E_TX_FLAGS_ZC_FRAME;
>> +
>> + tx_desc = I40E_TX_DESC(xdp_ring, xdp_ring->next_to_use);
>> + tx_desc->buffer_addr = cpu_to_le64(dma);
>> + tx_desc->cmd_type_offset_bsz =
>> build_ctob(I40E_TX_DESC_CMD_ICRC |
>> +
>> I40E_TX_DESC_CMD_EOP,
>> + 0, xdpf->len, 0);
>> + smp_wmb();
>> +
>> + xdp_ring->next_to_use++;
>> + if (xdp_ring->next_to_use == xdp_ring->count)
>> + xdp_ring->next_to_use = 0;
>> +
>> + tx_bi->next_to_watch = tx_desc;
>> +
>> + return I40E_XDP_TX;
>> +}
>> +
>> /**
>> * i40e_run_xdp_zc - Executes an XDP program on an xdp_buff
>> * @rx_ring: Rx ring
>> @@ -187,7 +230,6 @@ int i40e_xsk_umem_setup(struct i40e_vsi *vsi,
>> struct xdp_umem *umem,
>> static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct
>> xdp_buff *xdp)
>> {
>> int err, result = I40E_XDP_PASS;
>> - struct i40e_ring *xdp_ring;
>> struct bpf_prog *xdp_prog;
>> u32 act;
>>
>> @@ -202,8 +244,7 @@ static int i40e_run_xdp_zc(struct i40e_ring
>> *rx_ring, struct xdp_buff *xdp)
>> case XDP_PASS:
>> break;
>> case XDP_TX:
>> - xdp_ring =
>> rx_ring->vsi->xdp_rings[rx_ring->queue_index];
>> - result = i40e_xmit_xdp_tx_ring(xdp, xdp_ring);
>> + result = i40e_xmit_rcvd_zc(rx_ring, xdp);
>> break;
>> case XDP_REDIRECT:
>> err = xdp_do_redirect(rx_ring->netdev, xdp,
>> xdp_prog);
>> @@ -628,6 +669,11 @@ static bool i40e_xmit_zc(struct i40e_ring
>> *xdp_ring, unsigned int budget)
>> static void i40e_clean_xdp_tx_buffer(struct i40e_ring *tx_ring,
>> struct i40e_tx_buffer *tx_bi)
>> {
>> + if (tx_bi->tx_flags & I40E_TX_FLAGS_ZC_FRAME) {
>> + xsk_umem_recycle_addr(tx_ring->xsk_umem,
>> tx_bi->xdpf->handle);
>> + tx_bi->tx_flags = 0;
>> + return;
>> + }
>> xdp_return_frame(tx_bi->xdpf);
>> dma_unmap_single(tx_ring->dev,
>> dma_unmap_addr(tx_bi, dma),
>> --
>> 2.17.1
>>
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Y Song @ 2019-07-02 16:42 UTC (permalink / raw)
To: Ilya Leoshkevich; +Cc: bpf, netdev
In-Reply-To: <20190702153908.41562-1-iii@linux.ibm.com>
On Tue, Jul 2, 2019 at 8:40 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Use PT_REGS_RC(ctx) instead of ctx->rax, which is not present on s390.
>
> Pass -D__TARGET_ARCH_$(ARCH) to selftests in order to choose a proper
> PT_REGS_RC variant.
>
> Fix s930 -> s390 typo.
>
> On s390, provide the forward declaration of struct pt_regs and cast it
> to user_pt_regs in PT_REGS_* macros. This is necessary, because instead
> of the full struct pt_regs, s390 exposes only its first field
> user_pt_regs to userspace, and bpf_helpers.h is used with both userspace
> (in selftests) and kernel (in samples) headers.
>
> On x86, provide userspace versions of PT_REGS_* macros. Unlike s390, x86
> provides struct pt_regs to both userspace and kernel, however, with
> different field names.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> tools/testing/selftests/bpf/Makefile | 4 +-
> tools/testing/selftests/bpf/bpf_helpers.h | 46 +++++++++++++++--------
> tools/testing/selftests/bpf/progs/loop1.c | 2 +-
> tools/testing/selftests/bpf/progs/loop2.c | 2 +-
> tools/testing/selftests/bpf/progs/loop3.c | 2 +-
> 5 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index d60fee59fbd1..599b320bef65 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> include ../../../../scripts/Kbuild.include
> +include ../../../scripts/Makefile.arch
>
> LIBDIR := ../../../lib
> BPFDIR := $(LIBDIR)/bpf
> @@ -138,7 +139,8 @@ CLANG_SYS_INCLUDES := $(shell $(CLANG) -v -E - </dev/null 2>&1 \
>
> CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
> $(CLANG_SYS_INCLUDES) \
> - -Wno-compare-distinct-pointer-types
> + -Wno-compare-distinct-pointer-types \
> + -D__TARGET_ARCH_$(ARCH)
>
> $(OUTPUT)/test_l4lb_noinline.o: CLANG_FLAGS += -fno-inline
> $(OUTPUT)/test_xdp_noinline.o: CLANG_FLAGS += -fno-inline
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index 1a5b1accf091..faf86d83301a 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -312,8 +312,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #if defined(__TARGET_ARCH_x86)
> #define bpf_target_x86
> #define bpf_target_defined
> -#elif defined(__TARGET_ARCH_s930x)
> - #define bpf_target_s930x
> +#elif defined(__TARGET_ARCH_s390)
> + #define bpf_target_s390
> #define bpf_target_defined
> #elif defined(__TARGET_ARCH_arm)
> #define bpf_target_arm
> @@ -338,8 +338,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #ifndef bpf_target_defined
> #if defined(__x86_64__)
> #define bpf_target_x86
> -#elif defined(__s390x__)
> - #define bpf_target_s930x
I see in some other places (e.g., bcc) where
macro __s390x__ is also used to indicate a s390 architecture.
Could you explain the difference between __s390__ and
__s390x__?
> +#elif defined(__s390__)
> + #define bpf_target_s390
> #elif defined(__arm__)
> #define bpf_target_arm
> #elif defined(__aarch64__)
> @@ -355,6 +355,7 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
>
> #if defined(bpf_target_x86)
>
> +#ifdef __KERNEL__
In samples/bpf/, __KERNEL__ is defined at clang options and
in selftests/bpf/, the __KERNEL__ is not defined.
I checked x86 pt_regs definition with and without __KERNEL__.
They are identical except some register name difference.
I am wondering whether we can unify into all without
__KERNEL__. Is __KERNEL__ really needed?
> #define PT_REGS_PARM1(x) ((x)->di)
> #define PT_REGS_PARM2(x) ((x)->si)
> #define PT_REGS_PARM3(x) ((x)->dx)
> @@ -365,19 +366,34 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #define PT_REGS_RC(x) ((x)->ax)
> #define PT_REGS_SP(x) ((x)->sp)
> #define PT_REGS_IP(x) ((x)->ip)
> +#else
> +#define PT_REGS_PARM1(x) ((x)->rdi)
> +#define PT_REGS_PARM2(x) ((x)->rsi)
> +#define PT_REGS_PARM3(x) ((x)->rdx)
> +#define PT_REGS_PARM4(x) ((x)->rcx)
> +#define PT_REGS_PARM5(x) ((x)->r8)
> +#define PT_REGS_RET(x) ((x)->rsp)
> +#define PT_REGS_FP(x) ((x)->rbp)
> +#define PT_REGS_RC(x) ((x)->rax)
> +#define PT_REGS_SP(x) ((x)->rsp)
> +#define PT_REGS_IP(x) ((x)->rip)
> +#endif
>
> -#elif defined(bpf_target_s390x)
> +#elif defined(bpf_target_s390)
>
> -#define PT_REGS_PARM1(x) ((x)->gprs[2])
> -#define PT_REGS_PARM2(x) ((x)->gprs[3])
> -#define PT_REGS_PARM3(x) ((x)->gprs[4])
> -#define PT_REGS_PARM4(x) ((x)->gprs[5])
> -#define PT_REGS_PARM5(x) ((x)->gprs[6])
> -#define PT_REGS_RET(x) ((x)->gprs[14])
> -#define PT_REGS_FP(x) ((x)->gprs[11]) /* Works only with CONFIG_FRAME_POINTER */
> -#define PT_REGS_RC(x) ((x)->gprs[2])
> -#define PT_REGS_SP(x) ((x)->gprs[15])
> -#define PT_REGS_IP(x) ((x)->psw.addr)
> +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
> +struct pt_regs;
> +#define PT_REGS_PARM1(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> +#define PT_REGS_PARM2(x) (((const volatile user_pt_regs *)(x))->gprs[3])
> +#define PT_REGS_PARM3(x) (((const volatile user_pt_regs *)(x))->gprs[4])
> +#define PT_REGS_PARM4(x) (((const volatile user_pt_regs *)(x))->gprs[5])
> +#define PT_REGS_PARM5(x) (((const volatile user_pt_regs *)(x))->gprs[6])
> +#define PT_REGS_RET(x) (((const volatile user_pt_regs *)(x))->gprs[14])
> +/* Works only with CONFIG_FRAME_POINTER */
> +#define PT_REGS_FP(x) (((const volatile user_pt_regs *)(x))->gprs[11])
> +#define PT_REGS_RC(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> +#define PT_REGS_SP(x) (((const volatile user_pt_regs *)(x))->gprs[15])
> +#define PT_REGS_IP(x) (((const volatile user_pt_regs *)(x))->psw.addr)
Is user_pt_regs a recent change or has been there for quite some time?
I am asking since bcc did not use user_pt_regs yet.
>
> #elif defined(bpf_target_arm)
>
> diff --git a/tools/testing/selftests/bpf/progs/loop1.c b/tools/testing/selftests/bpf/progs/loop1.c
> index dea395af9ea9..7cdb7f878310 100644
> --- a/tools/testing/selftests/bpf/progs/loop1.c
> +++ b/tools/testing/selftests/bpf/progs/loop1.c
> @@ -18,7 +18,7 @@ int nested_loops(volatile struct pt_regs* ctx)
> for (j = 0; j < 300; j++)
> for (i = 0; i < j; i++) {
> if (j & 1)
> - m = ctx->rax;
> + m = PT_REGS_RC(ctx);
> else
> m = j;
> sum += i * m;
> diff --git a/tools/testing/selftests/bpf/progs/loop2.c b/tools/testing/selftests/bpf/progs/loop2.c
> index 0637bd8e8bcf..9b2f808a2863 100644
> --- a/tools/testing/selftests/bpf/progs/loop2.c
> +++ b/tools/testing/selftests/bpf/progs/loop2.c
> @@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
> int i = 0;
>
> while (true) {
> - if (ctx->rax & 1)
> + if (PT_REGS_RC(ctx) & 1)
> i += 3;
> else
> i += 7;
> diff --git a/tools/testing/selftests/bpf/progs/loop3.c b/tools/testing/selftests/bpf/progs/loop3.c
> index 30a0f6cba080..d727657d51e2 100644
> --- a/tools/testing/selftests/bpf/progs/loop3.c
> +++ b/tools/testing/selftests/bpf/progs/loop3.c
> @@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
> __u64 i = 0, sum = 0;
> do {
> i++;
> - sum += ctx->rax;
> + sum += PT_REGS_RC(ctx);
> } while (i < 0x100000000ULL);
> return sum;
> }
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH v2 bpf-next] libbpf: fix GCC8 warning for strncpy
From: Magnus Karlsson @ 2019-07-02 16:38 UTC (permalink / raw)
To: Y Song
Cc: Andrii Nakryiko, Andrii Nakryiko, bpf, netdev, Alexei Starovoitov,
Daniel Borkmann, Magnus Karlsson
In-Reply-To: <CAH3MdRWeH=Ko_mAvWk2mUaMK50iNHLbZkHKK=dVTzuwihZeRuA@mail.gmail.com>
On Tue, Jul 2, 2019 at 6:11 PM Y Song <ys114321@gmail.com> wrote:
>
> On Tue, Jul 2, 2019 at 8:17 AM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > GCC8 started emitting warning about using strncpy with number of bytes
> > exactly equal destination size, which is generally unsafe, as can lead
> > to non-zero terminated string being copied. Use IFNAMSIZ - 1 as number
> > of bytes to ensure name is always zero-terminated.
> >
> > Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Acked-by: Yonghong Song <yhs@fb.com>
Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
>
> > ---
> > tools/lib/bpf/xsk.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > index bf15a80a37c2..b33740221b7e 100644
> > --- a/tools/lib/bpf/xsk.c
> > +++ b/tools/lib/bpf/xsk.c
> > @@ -327,7 +327,8 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
> >
> > channels.cmd = ETHTOOL_GCHANNELS;
> > ifr.ifr_data = (void *)&channels;
> > - strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ);
> > + strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ - 1);
> > + ifr.ifr_name[IFNAMSIZ - 1] = '\0';
> > err = ioctl(fd, SIOCETHTOOL, &ifr);
> > if (err && errno != EOPNOTSUPP) {
> > ret = -errno;
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [RFC PATCH v2 2/2] Documentation: net: dsa: b53: Describe b53 configuration
From: Benedikt Spranger @ 2019-07-02 16:28 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, netdev, Sebastian Andrzej Siewior,
Kurt Kanzenbach, Vivien Didelot
In-Reply-To: <20190701173550.GH30468@lunn.ch>
Am Mon, 1 Jul 2019 19:35:50 +0200
schrieb Andrew Lunn <andrew@lunn.ch>:
> > +Configuration without tagging support
> > +-------------------------------------
>
> How does this differ to the text you just added in the previous patch?
The b53 has some implementation specific detail:
The b53 DSA driver tags the CPU port in all VLANs, since otherwise any
PVID untagged VLAN programming would basically change the CPU port's
default PVID and make it untagged, undesirable.
This need some attention while configuring. Therefore the configuration
is slightly different to the generic one:
The following extra commands are needed on b53 single port and gateway
configuration:
bridge vlan del dev lan1 vid 1
bridge vlan del dev lan2 vid 1
On bridge config the following commands are not needed:
bridge vlan add dev lan1 vid 1 pvid untagged
bridge vlan add dev lan2 vid 1 pvid untagged
bridge vlan add dev lan3 vid 1 pvid untagged
> Do we need both?
I like full configuration examples. But it can also be done by
describing the changes. I would prefer both - description and full
script:
...
Configuration without tagging support
-------------------------------------
Older models (5325, 5365) support a different tag format that is not
supported yet. 539x and 531x5 require managed mode and some special
handling, which is also not yet supported. The tagging support is
disabled in these cases and the switch need a different configuration.
The configuration slightly differ from
the :ref:`dsa-vlan-configuration`.
+ The b53 tags the CPU port in all VLANs, since otherwise any PVID
+ untagged VLAN programming would basically change the CPU port's
+ default PVID and make it untagged, undesirable.
+ In difference to the configuration described
+ in :ref:`dsa-vlan-configuration` the default VLAN 1 has to be removed
+ from the slave interface configuration in single port and gateway
+ configuration, while there is no need to add an extra VLAN
+ configuration in the bridge showcase.
single port
~~~~~~~~~~~
...
Regards
Bene Spranger
^ permalink raw reply
* Re: [PATCH net-next v6 01/15] rtnetlink: provide permanent hardware address in RTM_NEWLINK
From: Michal Kubecek @ 2019-07-02 16:35 UTC (permalink / raw)
To: netdev
Cc: Stephen Hemminger, David Miller, Jakub Kicinski, Jiri Pirko,
Andrew Lunn, Florian Fainelli, John Linville, Johannes Berg,
linux-kernel
In-Reply-To: <20190702075500.1b9845e1@hermes.lan>
On Tue, Jul 02, 2019 at 07:55:00AM -0700, Stephen Hemminger wrote:
> On Tue, 2 Jul 2019 13:49:44 +0200 (CEST)
> Michal Kubecek <mkubecek@suse.cz> wrote:
>
> > Permanent hardware address of a network device was traditionally provided
> > via ethtool ioctl interface but as Jiri Pirko pointed out in a review of
> > ethtool netlink interface, rtnetlink is much more suitable for it so let's
> > add it to the RTM_NEWLINK message.
> >
> > Add IFLA_PERM_ADDRESS attribute to RTM_NEWLINK messages unless the
> > permanent address is all zeros (i.e. device driver did not fill it). As
> > permanent address is not modifiable, reject userspace requests containing
> > IFLA_PERM_ADDRESS attribute.
> >
> > Note: we already provide permanent hardware address for bond slaves;
> > unfortunately we cannot drop that attribute for backward compatibility
> > reasons.
> >
> > v5 -> v6: only add the attribute if permanent address is not zero
> >
> > Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>
> Do you want to make an iproute patch to display this?
Yes, I'm going to submit it once this patch gets into net-next.
Michal
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply
* Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink interface
From: Michal Kubecek @ 2019-07-02 16:34 UTC (permalink / raw)
To: netdev
Cc: Jiri Pirko, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190702130515.GO2250@nanopsycho>
On Tue, Jul 02, 2019 at 03:05:15PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:50:04PM CEST, mkubecek@suse.cz wrote:
> >
> >+/* request header */
> >+
> >+/* use compact bitsets in reply */
> >+#define ETHTOOL_RF_COMPACT (1 << 0)
>
> "COMPACT_BITSETS"?
>
> >+/* provide optional reply for SET or ACT requests */
> >+#define ETHTOOL_RF_REPLY (1 << 1)
>
> "OPTIONAL_REPLY"?
OK
> >+ ret = nla_parse_nested(tb, ETHTOOL_A_HEADER_MAX, nest,
> >+ policy ?: dflt_header_policy, extack);
> >+ if (ret < 0)
>
> if (ret)
>
> Same remark goes to the rest of the code (also the rest of the patches),
> in case called function cannot return positive values.
The "if (ret < 0)" idiom for "on error do ..." is so ubiquitous through
the whole kernel that I don't think it's worth it to carefully check
which function can return a positive value and which cannot and risk
that one day I overlook that some function. And yet another question is
what exactly "cannot return" means: is it whenever the function does not
return a positive value or only if it's explicitly documented not to?
Looking at existing networking code, e.g. net/netfilter (except ipvs),
net/sched or net/core/rtnetlink.c are using "if (ret < 0)" rather
uniformly. And (as you objected to the check of genl_register_family()
previous patch) even genetlink itself has
err = genl_register_family(&genl_ctrl);
if (err < 0)
goto problem;
in genl_init().
>
>
> >+ return ret;
> >+ devname_attr = tb[ETHTOOL_A_HEADER_DEV_NAME];
> >+
> >+ if (tb[ETHTOOL_A_HEADER_DEV_INDEX]) {
> >+ u32 ifindex = nla_get_u32(tb[ETHTOOL_A_HEADER_DEV_INDEX]);
> >+
> >+ dev = dev_get_by_index(net, ifindex);
> >+ if (!dev) {
> >+ NL_SET_ERR_MSG_ATTR(extack,
> >+ tb[ETHTOOL_A_HEADER_DEV_INDEX],
> >+ "no device matches ifindex");
> >+ return -ENODEV;
> >+ }
> >+ /* if both ifindex and ifname are passed, they must match */
> >+ if (devname_attr &&
> >+ strncmp(dev->name, nla_data(devname_attr), IFNAMSIZ)) {
> >+ dev_put(dev);
> >+ NL_SET_ERR_MSG_ATTR(extack, nest,
> >+ "ifindex and name do not match");
> >+ return -ENODEV;
> >+ }
> >+ } else if (devname_attr) {
> >+ dev = dev_get_by_name(net, nla_data(devname_attr));
> >+ if (!dev) {
> >+ NL_SET_ERR_MSG_ATTR(extack, devname_attr,
> >+ "no device matches name");
> >+ return -ENODEV;
> >+ }
> >+ } else if (require_dev) {
> >+ NL_SET_ERR_MSG_ATTR(extack, nest,
> >+ "neither ifindex nor name specified");
> >+ return -EINVAL;
> >+ }
> >+
> >+ if (dev && !netif_device_present(dev)) {
> >+ dev_put(dev);
> >+ NL_SET_ERR_MSG(extack, "device not present");
> >+ return -ENODEV;
> >+ }
> >+
> >+ req_info->dev = dev;
> >+ ethnl_update_u32(&req_info->req_mask, tb[ETHTOOL_A_HEADER_INFOMASK]);
> >+ ethnl_update_u32(&req_info->global_flags, tb[ETHTOOL_A_HEADER_GFLAGS]);
> >+ ethnl_update_u32(&req_info->req_flags, tb[ETHTOOL_A_HEADER_RFLAGS]);
>
> Just:
> req_info->req_mask = nla_get_u32(tb[ETHTOOL_A_HEADER_INFOMASK];
> ...
>
> Not sure what ethnl_update_u32() is good for, but it is not needed here.
That would result in null pointer dereference if the attribute is
missing. So you would need at least
if (tb[ETHTOOL_A_HEADER_INFOMASK])
req_info->req_mask = nla_get_u32(tb[ETHTOOL_A_HEADER_INFOMASK]);
if (tb[ETHTOOL_A_HEADER_GFLAGS])
req_info->global_flags =
nla_get_u32(tb[ETHTOOL_A_HEADER_GFLAGS]);
if (tb[ETHTOOL_A_HEADER_RFLAGS])
req_info->req_flags = nla_get_u32(tb[ETHTOOL_A_HEADER_RFLAGS]);
I don't think it looks better.
> >+
> >+ return 0;
> >+}
> >+
> >+/**
> >+ * ethnl_fill_reply_header() - Put standard header into a reply message
> >+ * @skb: skb with the message
> >+ * @dev: network device to describe in header
> >+ * @attrtype: attribute type to use for the nest
> >+ *
> >+ * Create a nested attribute with attributes describing given network device.
> >+ * Clean up on error.
>
> Cleanup is obvious, no need to mention it in API docs.
OK
> >+ *
> >+ * Return: 0 on success, error value (-EMSGSIZE only) on error
> >+ */
> >+int ethnl_fill_reply_header(struct sk_buff *skb, struct net_device *dev,
> >+ u16 attrtype)
> >+{
> >+ struct nlattr *nest;
> >+
> >+ if (!dev)
> >+ return 0;
> >+ nest = nla_nest_start(skb, attrtype);
> >+ if (!nest)
> >+ return -EMSGSIZE;
> >+
> >+ if (nla_put_u32(skb, ETHTOOL_A_HEADER_DEV_INDEX, (u32)dev->ifindex) ||
> >+ nla_put_string(skb, ETHTOOL_A_HEADER_DEV_NAME, dev->name))
> >+ goto nla_put_failure;
> >+ /* If more attributes are put into reply header, ethnl_header_size()
> >+ * must be updated to account for them.
> >+ */
> >+
> >+ nla_nest_end(skb, nest);
> >+ return 0;
> >+
> >+nla_put_failure:
> >+ nla_nest_cancel(skb, nest);
> >+ return -EMSGSIZE;
> >+}
> >+
> >+/**
> >+ * ethnl_reply_init() - Create skb for a reply and fill device identification
> >+ * @payload: payload length (without netlink and genetlink header)
> >+ * @dev: device the reply is about (may be null)
> >+ * @cmd: ETHTOOL_MSG_* message type for reply
> >+ * @info: genetlink info of the received packet we respond to
> >+ * @ehdrp: place to store payload pointer returned by genlmsg_new()
> >+ *
> >+ * Return: pointer to allocated skb on success, NULL on error
> >+ */
> >+struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
> >+ u16 hdr_attrtype, struct genl_info *info,
> >+ void **ehdrp)
> >+{
> >+ struct sk_buff *skb;
> >+
> >+ skb = genlmsg_new(payload, GFP_KERNEL);
> >+ if (!skb)
> >+ goto err;
> >+ *ehdrp = genlmsg_put_reply(skb, info, ðtool_genl_family, 0, cmd);
> >+ if (!*ehdrp)
> >+ goto err_free;
> >+
> >+ if (dev) {
> >+ int ret;
> >+
> >+ ret = ethnl_fill_reply_header(skb, dev, hdr_attrtype);
> >+ if (ret < 0)
> >+ goto err;
> >+ }
> >+ return skb;
> >+
> >+err_free:
> >+ nlmsg_free(skb);
> >+ if (info)
> >+ GENL_SET_ERR_MSG(info, "failed to setup reply message");
> >+err:
>
> Why also not fillup extack msg here?
Right, err label should be right below the nlmsg_free(skb), thanks. And
now I noticed another mistake: on ethnl_fill_reply_header() failure, we
should go to err_free, not err.
> >+static inline int ethnl_str_size(const char *s)
>
> If you really need this helper, put it into netlink code. There's nothing
> ethtool-specific about this.
OK, I'll look into it. I've been already thinking about some kind of
NLA_SIZEOF() macro as about 1/3 of all uses of nla_total_size() follow
the nla_total_size(sizeof(...)) pattern (and lot more should follow it
but are written like e.g. nla_total_size(4) instead). This is another
common pattern.
> >+/* The ethnl_update_* helpers set value pointed to by @dst to the value of
> >+ * netlink attribute @attr (if attr is not null). They return true if *dst
> >+ * value was changed, false if not.
> >+ */
> >+static inline bool ethnl_update_u32(u32 *dst, struct nlattr *attr)
>
> I'm still not sure I'm convinced about these "update helpers" :)
Just imagine what would e.g.
if (ethnl_update_u32(&data.rx_pending, tb[ETHTOOL_A_RING_RX_PENDING]))
mod = true;
if (ethnl_update_u32(&data.rx_mini_pending,
tb[ETHTOOL_A_RING_RX_MINI_PENDING]))
mod = true;
if (ethnl_update_u32(&data.rx_jumbo_pending,
tb[ETHTOOL_A_RING_RX_JUMBO_PENDING]))
mod = true;
if (ethnl_update_u32(&data.tx_pending, tb[ETHTOOL_A_RING_TX_PENDING]))
mod = true;
if (!mod)
return 0;
look like without them. And coalescing parameters would be much worse
(22 attributes / struct members).
> >+{
> >+ u32 val;
> >+
> >+ if (!attr)
> >+ return false;
> >+ val = nla_get_u32(attr);
> >+ if (*dst == val)
> >+ return false;
> >+
> >+ *dst = val;
> >+ return true;
> >+}
...
> >+static inline bool ethnl_update_binary(u8 *dst, unsigned int len,
>
> void *dst
OK.
> >+/**
> >+ * ethnl_is_privileged() - check if request has sufficient privileges
> >+ * @skb: skb with client request
> >+ *
> >+ * Checks if client request has CAP_NET_ADMIN in its netns. Unlike the flags
> >+ * in genl_ops, this allows finer access control, e.g. allowing or denying
> >+ * the request based on its contents or witholding only part of the data
> >+ * from unprivileged users.
> >+ *
> >+ * Return: true if request is privileged, false if not
> >+ */
> >+static inline bool ethnl_is_privileged(struct sk_buff *skb)
>
> I wonder why you need this helper. Genetlink uses
> ops->flags & GENL_ADMIN_PERM for this.
It's explained in the function description. Sometimes we need finer
control than by request message type. An example is the WoL password:
ETHTOOL_GWOL is privileged because of it but I believe there si no
reason why unprivileged user couldn't see enabled WoL modes, we can
simply omit the password for him. (Also, it allows to combine query for
WoL settings with other unprivileged settings.)
> >+/**
> >+ * ethnl_reply_header_size() - total size of reply header
> >+ *
> >+ * This is an upper estimate so that we do not need to hold RTNL lock longer
> >+ * than necessary (to prevent rename between size estimate and composing the
>
> I guess this description is not relevant anymore. I don't see why to
> hold rtnl mutex for this function...
You don't need it for this function, it's the other way around: unless
you hold RTNL lock for the whole time covering both checking needed
message size and filling the message - and we don't - the device could
be renamed in between. Thus if we returned size based on current device
name, it might not be sufficient at the time the header is filled.
That's why this function returns maximum possible size (which is
actually a constant).
Michal
> >+ * message). Accounts only for device ifindex and name as those are the only
> >+ * attributes ethnl_fill_reply_header() puts into the reply header.
> >+ */
> >+static inline unsigned int ethnl_reply_header_size(void)
> >+{
> >+ return nla_total_size(nla_total_size(sizeof(u32)) +
> >+ nla_total_size(IFNAMSIZ));
> >+}
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Jonathan Lemon @ 2019-07-02 16:33 UTC (permalink / raw)
To: Richardson, Bruce
Cc: Jakub Kicinski, Laatz, Kevin, netdev, ast, daniel, Topel, Bjorn,
Karlsson, Magnus, bpf, intel-wired-lan, Loftus, Ciara
In-Reply-To: <59AF69C657FD0841A61C55336867B5B07ED8B210@IRSMSX103.ger.corp.intel.com>
On 2 Jul 2019, at 2:27, Richardson, Bruce wrote:
>> -----Original Message-----
>> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
>> Sent: Monday, July 1, 2019 10:20 PM
>> To: Laatz, Kevin <kevin.laatz@intel.com>
>> Cc: Jonathan Lemon <jonathan.lemon@gmail.com>;
>> netdev@vger.kernel.org;
>> ast@kernel.org; daniel@iogearbox.net; Topel, Bjorn
>> <bjorn.topel@intel.com>; Karlsson, Magnus
>> <magnus.karlsson@intel.com>;
>> bpf@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Richardson,
>> Bruce
>> <bruce.richardson@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>
>> Subject: Re: [PATCH 00/11] XDP unaligned chunk placement support
>>
>> On Mon, 1 Jul 2019 15:44:29 +0100, Laatz, Kevin wrote:
>>> On 28/06/2019 21:29, Jonathan Lemon wrote:
>>>> On 28 Jun 2019, at 9:19, Laatz, Kevin wrote:
>>>>> On 27/06/2019 22:25, Jakub Kicinski wrote:
>>>>>> I think that's very limiting. What is the challenge in
>>>>>> providing
>>>>>> aligned addresses, exactly?
>>>>> The challenges are two-fold:
>>>>> 1) it prevents using arbitrary buffer sizes, which will be an
>>>>> issue
>>>>> supporting e.g. jumbo frames in future.
>>>>> 2) higher level user-space frameworks which may want to use
>>>>> AF_XDP,
>>>>> such as DPDK, do not currently support having buffers with 'fixed'
>>>>> alignment.
>>>>> The reason that DPDK uses arbitrary placement is that:
>>>>> - it would stop things working on certain NICs which
>>>>> need
>>>>> the actual writable space specified in units of 1k - therefore we
>>>>> need 2k
>>>>> + metadata space.
>>>>> - we place padding between buffers to avoid
>>>>> constantly
>>>>> hitting the same memory channels when accessing memory.
>>>>> - it allows the application to choose the actual
>>>>> buffer
>>>>> size it wants to use.
>>>>> We make use of the above to allow us to speed up processing
>>>>> significantly and also reduce the packet buffer memory size.
>>>>>
>>>>> Not having arbitrary buffer alignment also means an AF_XDP
>>>>> driver for DPDK cannot be a drop-in replacement for existing
>>>>> drivers in those frameworks. Even with a new capability to allow
>>>>> an
>>>>> arbitrary buffer alignment, existing apps will need to be modified
>>>>> to use that new capability.
>>>>
>>>> Since all buffers in the umem are the same chunk size, the original
>>>> buffer address can be recalculated with some multiply/shift math.
>>>> However, this is more expensive than just a mask operation.
>>>
>>> Yes, we can do this.
>>
>> That'd be best, can DPDK reasonably guarantee the slicing is uniform?
>> E.g. it's not desperate buffer pools with different bases?
>
> It's generally uniform, but handling the crossing of (huge)page
> boundaries
> complicates things a bit. Therefore I think the final option below
> is best as it avoids any such problems.
>
>>
>>> Another option we have is to add a socket option for querying the
>>> metadata length from the driver (assuming it doesn't vary per
>>> packet).
>>> We can use that information to get back to the original address
>>> using
>>> subtraction.
>>
>> Unfortunately the metadata depends on the packet and how much info
>> the
>> device was able to extract. So it's variable length.
>>
>>> Alternatively, we can change the Rx descriptor format to include the
>>> metadata length. We could do this in a couple of ways, for example,
>>> rather than returning the address as the start of the packet,
>>> instead
>>> return the buffer address that was passed in, and adding another
>>> 16-bit field to specify the start of packet offset with that buffer.
>>> If using another 16-bits of the descriptor space is not desirable,
>>> an
>>> alternative could be to limit umem sizes to e.g. 2^48 bits (256
>>> terabytes should be enough, right :-) ) and use the remaining 16
>>> bits
>>> of the address as a packet offset. Other variations on these
>>> approach
>>> are obviously possible too.
>>
>> Seems reasonable to me..
>
> I think this is probably the best solution, and also has the advantage
> that
> a buffer retains its base address the full way through the cycle of Rx
> and Tx.
I like this as well - it also has the advantage that drivers can keep
performing adjustments on the handle, which ends up just modifying the
offset.
--
Jonathan
^ permalink raw reply
* Re: [PATCH 2/3 bpf-next] i40e: Support zero-copy XDP_TX on the RX path for AF_XDP sockets.
From: Jonathan Lemon @ 2019-07-02 16:31 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: netdev, bjorn.topel, magnus.karlsson, jakub.kicinski,
jeffrey.t.kirsher, kernel-team
In-Reply-To: <f9c7b86c-91a6-9585-d1f1-f6f325794038@mellanox.com>
On 2 Jul 2019, at 0:07, Maxim Mikityanskiy wrote:
> On 2019-06-29 01:15, Jonathan Lemon wrote:
>> + xdpf = convert_to_xdp_frame_keep_zc(xdp);
>> + if (unlikely(!xdpf))
>> + return I40E_XDP_CONSUMED;
>> + xdpf->handle = xdp->handle;
>
> Shouldn't this line belong to convert_to_xdp_frame_keep_zc (and the
> previous patch)? It looks like it's code common for all drivers, and
> also patch 1 adds the handle field, but doesn't use it, which looks weird.
Good point. I'll move it into the function.
--
Jonathan
^ permalink raw reply
* Re: WARNING: locking bug in do_ipv6_getsockopt
From: Eric Dumazet @ 2019-07-02 16:30 UTC (permalink / raw)
To: syzbot, davem, kuznet, linux-kernel, netdev, syzkaller-bugs,
yoshfuji
In-Reply-To: <000000000000607bf4058cb5135c@google.com>
On 7/2/19 9:17 AM, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 6fbc7275 Linux 5.2-rc7
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=13fb8fe5a00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=f6451f0da3d42d53
> dashboard link: https://syzkaller.appspot.com/bug?extid=babfdd7368c72aac3875
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1460390ba00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+babfdd7368c72aac3875@syzkaller.appspotmail.com
>
> WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735 arch_local_save_flags arch/x86/include/asm/paravirt.h:762 [inline]
> WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735 arch_local_save_flags arch/x86/include/asm/paravirt.h:760 [inline]
> WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735 look_up_lock_class kernel/locking/lockdep.c:726 [inline]
> WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735 register_lock_class+0xe10/0x1860 kernel/locking/lockdep.c:1079
WARN_ON_ONCE(class->name != lock->name);
Might be useful to trace the content of class->name and lock->name.
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 1 PID: 18374 Comm: syz-executor.0 Not tainted 5.2.0-rc7 #65
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> panic+0x2cb/0x744 kernel/panic.c:219
> __warn.cold+0x20/0x4d kernel/panic.c:576
> report_bug+0x263/0x2b0 lib/bug.c:186
> fixup_bug arch/x86/kernel/traps.c:179 [inline]
> fixup_bug arch/x86/kernel/traps.c:174 [inline]
> do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:272
> do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:291
> invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:986
> RIP: 0010:look_up_lock_class kernel/locking/lockdep.c:735 [inline]
> RIP: 0010:register_lock_class+0xe10/0x1860 kernel/locking/lockdep.c:1079
> Code: 00 48 89 da 4d 8b 76 c0 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 80 3c 02 00 0f 85 23 07 00 00 4c 89 33 e9 e3 f4 ff ff 0f 0b <0f> 0b e9 ea f3 ff ff 44 89 e0 4c 8b 95 50 ff ff ff 83 c0 01 4c 8b
> RSP: 0018:ffff88809ea3f678 EFLAGS: 00010087
> RAX: dffffc0000000000 RBX: ffff888090b948e0 RCX: 0000000000000000
> RDX: 1ffff1101217291f RSI: 0000000000000000 RDI: ffff888090b948f8
> RBP: ffff88809ea3f740 R08: 1ffff11013d47ed7 R09: ffffffff8a65ad40
> R10: ffffffff8a2e9180 R11: 0000000000000000 R12: ffffffff8a3243a0
> R13: 0000000000000000 R14: 0000000000000000 R15: ffffffff88023e60
> __lock_acquire+0x116/0x5490 kernel/locking/lockdep.c:3674
> lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4303
> __raw_spin_lock_bh include/linux/spinlock_api_smp.h:135 [inline]
> _raw_spin_lock_bh+0x33/0x50 kernel/locking/spinlock.c:175
> spin_lock_bh include/linux/spinlock.h:343 [inline]
> lock_sock_nested+0x41/0x120 net/core/sock.c:2913
> lock_sock include/net/sock.h:1522 [inline]
> do_ipv6_getsockopt.isra.0+0x289/0x2590 net/ipv6/ipv6_sockglue.c:1167
> ipv6_getsockopt+0x186/0x280 net/ipv6/ipv6_sockglue.c:1391
> udpv6_getsockopt+0x68/0xa0 net/ipv6/udp.c:1590
> sock_common_getsockopt+0x94/0xd0 net/core/sock.c:3085
> __sys_getsockopt+0x15f/0x240 net/socket.c:2109
> __do_sys_getsockopt net/socket.c:2120 [inline]
> __se_sys_getsockopt net/socket.c:2117 [inline]
> __x64_sys_getsockopt+0xbe/0x150 net/socket.c:2117
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x4597c9
> Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007fb5fd78ac78 EFLAGS: 00000246 ORIG_RAX: 0000000000000037
> RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00000000004597c9
> RDX: 0000000000000036 RSI: 0000000000000029 RDI: 0000000000000004
> RBP: 000000000075bfc8 R08: 0000000020000080 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007fb5fd78b6d4
> R13: 00000000004c0803 R14: 00000000004d2b88 R15: 00000000ffffffff
> Kernel Offset: disabled
> Rebooting in 86400 seconds..
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> syzbot can test patches for this bug, for details see:
> https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* RE: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the port parameters.
From: Parav Pandit @ 2019-07-02 16:18 UTC (permalink / raw)
To: Andrew Lunn, Sudarsana Reddy Kalluru
Cc: davem@davemloft.net, netdev@vger.kernel.org,
mkalderon@marvell.com, aelior@marvell.com, jiri@resnulli.us
In-Reply-To: <20190702161133.GP30468@lunn.ch>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Andrew Lunn
> Sent: Tuesday, July 2, 2019 9:42 PM
> To: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; mkalderon@marvell.com;
> aelior@marvell.com; jiri@resnulli.us
> Subject: Re: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the
> port parameters.
>
> On Tue, Jul 02, 2019 at 08:20:56AM -0700, Sudarsana Reddy Kalluru wrote:
> > The patch adds devlink interfaces for drivers to publish/unpublish the
> > devlink port parameters.
>
> Hi Sudarsana
>
> A good commit message says more about 'why' than 'what'. I can see the
> 'what' by reading the code. But the 'why' is often not so clear.
>
> Why would i want to unpublish port parameters?
>
> Thanks
> Andrew
A vendor driver calling these APIs is needed at minimum.
^ permalink raw reply
* WARNING: locking bug in do_ipv6_getsockopt
From: syzbot @ 2019-07-02 16:17 UTC (permalink / raw)
To: davem, kuznet, linux-kernel, netdev, syzkaller-bugs, yoshfuji
Hello,
syzbot found the following crash on:
HEAD commit: 6fbc7275 Linux 5.2-rc7
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13fb8fe5a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=f6451f0da3d42d53
dashboard link: https://syzkaller.appspot.com/bug?extid=babfdd7368c72aac3875
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1460390ba00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+babfdd7368c72aac3875@syzkaller.appspotmail.com
WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735
arch_local_save_flags arch/x86/include/asm/paravirt.h:762 [inline]
WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735
arch_local_save_flags arch/x86/include/asm/paravirt.h:760 [inline]
WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735
look_up_lock_class kernel/locking/lockdep.c:726 [inline]
WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735
register_lock_class+0xe10/0x1860 kernel/locking/lockdep.c:1079
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 18374 Comm: syz-executor.0 Not tainted 5.2.0-rc7 #65
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
panic+0x2cb/0x744 kernel/panic.c:219
__warn.cold+0x20/0x4d kernel/panic.c:576
report_bug+0x263/0x2b0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:179 [inline]
fixup_bug arch/x86/kernel/traps.c:174 [inline]
do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:272
do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:291
invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:986
RIP: 0010:look_up_lock_class kernel/locking/lockdep.c:735 [inline]
RIP: 0010:register_lock_class+0xe10/0x1860 kernel/locking/lockdep.c:1079
Code: 00 48 89 da 4d 8b 76 c0 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 80
3c 02 00 0f 85 23 07 00 00 4c 89 33 e9 e3 f4 ff ff 0f 0b <0f> 0b e9 ea f3
ff ff 44 89 e0 4c 8b 95 50 ff ff ff 83 c0 01 4c 8b
RSP: 0018:ffff88809ea3f678 EFLAGS: 00010087
RAX: dffffc0000000000 RBX: ffff888090b948e0 RCX: 0000000000000000
RDX: 1ffff1101217291f RSI: 0000000000000000 RDI: ffff888090b948f8
RBP: ffff88809ea3f740 R08: 1ffff11013d47ed7 R09: ffffffff8a65ad40
R10: ffffffff8a2e9180 R11: 0000000000000000 R12: ffffffff8a3243a0
R13: 0000000000000000 R14: 0000000000000000 R15: ffffffff88023e60
__lock_acquire+0x116/0x5490 kernel/locking/lockdep.c:3674
lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4303
__raw_spin_lock_bh include/linux/spinlock_api_smp.h:135 [inline]
_raw_spin_lock_bh+0x33/0x50 kernel/locking/spinlock.c:175
spin_lock_bh include/linux/spinlock.h:343 [inline]
lock_sock_nested+0x41/0x120 net/core/sock.c:2913
lock_sock include/net/sock.h:1522 [inline]
do_ipv6_getsockopt.isra.0+0x289/0x2590 net/ipv6/ipv6_sockglue.c:1167
ipv6_getsockopt+0x186/0x280 net/ipv6/ipv6_sockglue.c:1391
udpv6_getsockopt+0x68/0xa0 net/ipv6/udp.c:1590
sock_common_getsockopt+0x94/0xd0 net/core/sock.c:3085
__sys_getsockopt+0x15f/0x240 net/socket.c:2109
__do_sys_getsockopt net/socket.c:2120 [inline]
__se_sys_getsockopt net/socket.c:2117 [inline]
__x64_sys_getsockopt+0xbe/0x150 net/socket.c:2117
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x4597c9
Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fb5fd78ac78 EFLAGS: 00000246 ORIG_RAX: 0000000000000037
RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00000000004597c9
RDX: 0000000000000036 RSI: 0000000000000029 RDI: 0000000000000004
RBP: 000000000075bfc8 R08: 0000000020000080 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fb5fd78b6d4
R13: 00000000004c0803 R14: 00000000004d2b88 R15: 00000000ffffffff
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [for-next V2 06/10] linux/dim: Move implementation to .c files
From: Geert Uytterhoeven @ 2019-07-02 16:15 UTC (permalink / raw)
To: Saeed Mahameed, Tal Gilboa
Cc: David S. Miller, Doug Ledford, Jason Gunthorpe, Leon Romanovsky,
Or Gerlitz, Sagi Grimberg, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-kernel
In-Reply-To: <20190625205701.17849-7-saeedm@mellanox.com>
Hi Saeed, Tal,
On Tue, 25 Jun 2019, Saeed Mahameed wrote:
> From: Tal Gilboa <talgi@mellanox.com>
>
> Moved all logic from dim.h and net_dim.h to dim.c and net_dim.c.
> This is both more structurally appealing and would allow to only
> expose externally used functions.
>
> Signed-off-by: Tal Gilboa <talgi@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
This is now commit 4f75da3666c0c572 ("linux/dim: Move implementation to
.c files") in net-next.
> --- a/drivers/net/ethernet/broadcom/Kconfig
> +++ b/drivers/net/ethernet/broadcom/Kconfig
> @@ -8,6 +8,7 @@ config NET_VENDOR_BROADCOM
> default y
> depends on (SSB_POSSIBLE && HAS_DMA) || PCI || BCM63XX || \
> SIBYTE_SB1xxx_SOC
> + select DIMLIB
Merely enabling a NET_VENDOR_* symbol should not enable inclusion of
any additional code, cfr. the help text for the NET_VENDOR_BROADCOM
option.
Hence please move the select to the config symbol(s) for the driver(s)
that need it.
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -562,6 +562,14 @@ config SIGNATURE
> Digital signature verification. Currently only RSA is supported.
> Implementation is done using GnuPG MPI library
>
> +config DIMLIB
> + bool "DIM library"
> + default y
Please drop this line, as optional library code should never be included
by default.
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
* [PATCH bpf-next v2 8/8] samples/bpf: fix tcp_bpf.readme detach command
From: Stanislav Fomichev @ 2019-07-02 16:14 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Soheil Hassas Yeganeh,
Yuchung Cheng
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Copy-paste, should be detach, not attach.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
---
samples/bpf/tcp_bpf.readme | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/samples/bpf/tcp_bpf.readme b/samples/bpf/tcp_bpf.readme
index fee746621aec..78e247f62108 100644
--- a/samples/bpf/tcp_bpf.readme
+++ b/samples/bpf/tcp_bpf.readme
@@ -25,4 +25,4 @@ attached to the cgroupv2).
To remove (unattach) a socket_ops BPF program from a cgroupv2:
- bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
+ bpftool cgroup detach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 7/8] samples/bpf: add sample program that periodically dumps TCP stats
From: Stanislav Fomichev @ 2019-07-02 16:14 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Uses new RTT callback to dump stats every second.
$ mkdir -p /tmp/cgroupv2
$ mount -t cgroup2 none /tmp/cgroupv2
$ mkdir -p /tmp/cgroupv2/foo
$ echo $$ >> /tmp/cgroupv2/foo/cgroup.procs
$ bpftool prog load ./tcp_dumpstats_kern.o /sys/fs/bpf/tcp_prog
$ bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
$ bpftool prog tracelog
$ # run neper/netperf/etc
Used neper to compare performance with and without this program attached
and didn't see any noticeable performance impact.
Sample output:
<idle>-0 [015] ..s. 2074.128800: 0: dsack_dups=0 delivered=242526
<idle>-0 [015] ..s. 2074.128808: 0: delivered_ce=0 icsk_retransmits=0
<idle>-0 [015] ..s. 2075.130133: 0: dsack_dups=0 delivered=323599
<idle>-0 [015] ..s. 2075.130138: 0: delivered_ce=0 icsk_retransmits=0
<idle>-0 [005] .Ns. 2076.131440: 0: dsack_dups=0 delivered=404648
<idle>-0 [005] .Ns. 2076.131447: 0: delivered_ce=0 icsk_retransmits=0
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
samples/bpf/Makefile | 1 +
samples/bpf/tcp_dumpstats_kern.c | 68 ++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+)
create mode 100644 samples/bpf/tcp_dumpstats_kern.c
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 0917f8cf4fab..eaebbeead42f 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -154,6 +154,7 @@ always += tcp_iw_kern.o
always += tcp_clamp_kern.o
always += tcp_basertt_kern.o
always += tcp_tos_reflect_kern.o
+always += tcp_dumpstats_kern.o
always += xdp_redirect_kern.o
always += xdp_redirect_map_kern.o
always += xdp_redirect_cpu_kern.o
diff --git a/samples/bpf/tcp_dumpstats_kern.c b/samples/bpf/tcp_dumpstats_kern.c
new file mode 100644
index 000000000000..8557913106a0
--- /dev/null
+++ b/samples/bpf/tcp_dumpstats_kern.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Refer to samples/bpf/tcp_bpf.readme for the instructions on
+ * how to run this sample program.
+ */
+#include <linux/bpf.h>
+
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+#define INTERVAL 1000000000ULL
+
+int _version SEC("version") = 1;
+char _license[] SEC("license") = "GPL";
+
+struct {
+ __u32 type;
+ __u32 map_flags;
+ int *key;
+ __u64 *value;
+} bpf_next_dump SEC(".maps") = {
+ .type = BPF_MAP_TYPE_SK_STORAGE,
+ .map_flags = BPF_F_NO_PREALLOC,
+};
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+ struct bpf_tcp_sock *tcp_sk;
+ struct bpf_sock *sk;
+ __u64 *next_dump;
+ __u64 now;
+
+ switch (ctx->op) {
+ case BPF_SOCK_OPS_TCP_CONNECT_CB:
+ bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
+ return 1;
+ case BPF_SOCK_OPS_RTT_CB:
+ break;
+ default:
+ return 1;
+ }
+
+ sk = ctx->sk;
+ if (!sk)
+ return 1;
+
+ next_dump = bpf_sk_storage_get(&bpf_next_dump, sk, 0,
+ BPF_SK_STORAGE_GET_F_CREATE);
+ if (!next_dump)
+ return 1;
+
+ now = bpf_ktime_get_ns();
+ if (now < *next_dump)
+ return 1;
+
+ tcp_sk = bpf_tcp_sock(sk);
+ if (!tcp_sk)
+ return 1;
+
+ *next_dump = now + INTERVAL;
+
+ bpf_printk("dsack_dups=%u delivered=%u\n",
+ tcp_sk->dsack_dups, tcp_sk->delivered);
+ bpf_printk("delivered_ce=%u icsk_retransmits=%u\n",
+ tcp_sk->delivered_ce, tcp_sk->icsk_retransmits);
+
+ return 1;
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 6/8] selftests/bpf: test BPF_SOCK_OPS_RTT_CB
From: Stanislav Fomichev @ 2019-07-02 16:14 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Make sure the callback is invoked for syn-ack and data packet.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/Makefile | 3 +-
tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
tools/testing/selftests/bpf/test_tcp_rtt.c | 254 ++++++++++++++++++++
3 files changed, 317 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index de1754a8f5fe..2620406a53ec 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -27,7 +27,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
test_cgroup_storage test_select_reuseport test_section_names \
test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
test_btf_dump test_cgroup_attach xdping test_sockopt test_sockopt_sk \
- test_sockopt_multi
+ test_sockopt_multi test_tcp_rtt
BPF_OBJ_FILES = $(patsubst %.c,%.o, $(notdir $(wildcard progs/*.c)))
TEST_GEN_FILES = $(BPF_OBJ_FILES)
@@ -107,6 +107,7 @@ $(OUTPUT)/test_cgroup_attach: cgroup_helpers.c
$(OUTPUT)/test_sockopt: cgroup_helpers.c
$(OUTPUT)/test_sockopt_sk: cgroup_helpers.c
$(OUTPUT)/test_sockopt_multi: cgroup_helpers.c
+$(OUTPUT)/test_tcp_rtt: cgroup_helpers.c
.PHONY: force
diff --git a/tools/testing/selftests/bpf/progs/tcp_rtt.c b/tools/testing/selftests/bpf/progs/tcp_rtt.c
new file mode 100644
index 000000000000..233bdcb1659e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tcp_rtt.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+__u32 _version SEC("version") = 1;
+
+struct tcp_rtt_storage {
+ __u32 invoked;
+ __u32 dsack_dups;
+ __u32 delivered;
+ __u32 delivered_ce;
+ __u32 icsk_retransmits;
+};
+
+struct bpf_map_def SEC("maps") socket_storage_map = {
+ .type = BPF_MAP_TYPE_SK_STORAGE,
+ .key_size = sizeof(int),
+ .value_size = sizeof(struct tcp_rtt_storage),
+ .map_flags = BPF_F_NO_PREALLOC,
+};
+BPF_ANNOTATE_KV_PAIR(socket_storage_map, int, struct tcp_rtt_storage);
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+ struct tcp_rtt_storage *storage;
+ struct bpf_tcp_sock *tcp_sk;
+ int op = (int) ctx->op;
+ struct bpf_sock *sk;
+
+ sk = ctx->sk;
+ if (!sk)
+ return 1;
+
+ storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
+ BPF_SK_STORAGE_GET_F_CREATE);
+ if (!storage)
+ return 1;
+
+ if (op == BPF_SOCK_OPS_TCP_CONNECT_CB) {
+ bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
+ return 1;
+ }
+
+ if (op != BPF_SOCK_OPS_RTT_CB)
+ return 1;
+
+ tcp_sk = bpf_tcp_sock(sk);
+ if (!tcp_sk)
+ return 1;
+
+ storage->invoked++;
+
+ storage->dsack_dups = tcp_sk->dsack_dups;
+ storage->delivered = tcp_sk->delivered;
+ storage->delivered_ce = tcp_sk->delivered_ce;
+ storage->icsk_retransmits = tcp_sk->icsk_retransmits;
+
+ return 1;
+}
diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
new file mode 100644
index 000000000000..90c3862f74a8
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <error.h>
+#include <errno.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <pthread.h>
+
+#include <linux/filter.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "bpf_util.h"
+#include "cgroup_helpers.h"
+
+#define CG_PATH "/tcp_rtt"
+
+struct tcp_rtt_storage {
+ __u32 invoked;
+ __u32 dsack_dups;
+ __u32 delivered;
+ __u32 delivered_ce;
+ __u32 icsk_retransmits;
+};
+
+static void send_byte(int fd)
+{
+ char b = 0x55;
+
+ if (write(fd, &b, sizeof(b)) != 1)
+ error(1, errno, "Failed to send single byte");
+}
+
+static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
+ __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
+ __u32 icsk_retransmits)
+{
+ int err = 0;
+ struct tcp_rtt_storage val;
+
+ if (bpf_map_lookup_elem(map_fd, &client_fd, &val) < 0)
+ error(1, errno, "Failed to read socket storage");
+
+ if (val.invoked != invoked) {
+ log_err("%s: unexpected bpf_tcp_sock.invoked %d != %d",
+ msg, val.invoked, invoked);
+ err++;
+ }
+
+ if (val.dsack_dups != dsack_dups) {
+ log_err("%s: unexpected bpf_tcp_sock.dsack_dups %d != %d",
+ msg, val.dsack_dups, dsack_dups);
+ err++;
+ }
+
+ if (val.delivered != delivered) {
+ log_err("%s: unexpected bpf_tcp_sock.delivered %d != %d",
+ msg, val.delivered, delivered);
+ err++;
+ }
+
+ if (val.delivered_ce != delivered_ce) {
+ log_err("%s: unexpected bpf_tcp_sock.delivered_ce %d != %d",
+ msg, val.delivered_ce, delivered_ce);
+ err++;
+ }
+
+ if (val.icsk_retransmits != icsk_retransmits) {
+ log_err("%s: unexpected bpf_tcp_sock.icsk_retransmits %d != %d",
+ msg, val.icsk_retransmits, icsk_retransmits);
+ err++;
+ }
+
+ return err;
+}
+
+static int connect_to_server(int server_fd)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd;
+
+ fd = socket(AF_INET, SOCK_STREAM, 0);
+ if (fd < 0) {
+ log_err("Failed to create client socket");
+ return -1;
+ }
+
+ if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+ log_err("Failed to get server addr");
+ goto out;
+ }
+
+ if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
+ log_err("Fail to connect to server");
+ goto out;
+ }
+
+ return fd;
+
+out:
+ close(fd);
+ return -1;
+}
+
+static int run_test(int cgroup_fd, int server_fd)
+{
+ struct bpf_prog_load_attr attr = {
+ .prog_type = BPF_PROG_TYPE_SOCK_OPS,
+ .file = "./tcp_rtt.o",
+ .expected_attach_type = BPF_CGROUP_SOCK_OPS,
+ };
+ struct bpf_object *obj;
+ struct bpf_map *map;
+ int client_fd;
+ int prog_fd;
+ int map_fd;
+ int err;
+
+ err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
+ if (err) {
+ log_err("Failed to load BPF object");
+ return -1;
+ }
+
+ map = bpf_map__next(NULL, obj);
+ map_fd = bpf_map__fd(map);
+
+ err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS, 0);
+ if (err) {
+ log_err("Failed to attach BPF program");
+ goto close_bpf_object;
+ }
+
+ client_fd = connect_to_server(server_fd);
+ if (client_fd < 0) {
+ err = -1;
+ goto close_bpf_object;
+ }
+
+ err += verify_sk(map_fd, client_fd, "syn-ack",
+ /*invoked=*/1,
+ /*dsack_dups=*/0,
+ /*delivered=*/1,
+ /*delivered_ce=*/0,
+ /*icsk_retransmits=*/0);
+
+ send_byte(client_fd);
+
+ err += verify_sk(map_fd, client_fd, "first payload byte",
+ /*invoked=*/2,
+ /*dsack_dups=*/0,
+ /*delivered=*/2,
+ /*delivered_ce=*/0,
+ /*icsk_retransmits=*/0);
+
+ close(client_fd);
+
+close_bpf_object:
+ bpf_object__close(obj);
+ return err;
+}
+
+static int start_server(void)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
+ };
+ int fd;
+
+ fd = socket(AF_INET, SOCK_STREAM, 0);
+ if (fd < 0) {
+ log_err("Failed to create server socket");
+ return -1;
+ }
+
+ if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
+ log_err("Failed to bind socket");
+ close(fd);
+ return -1;
+ }
+
+ return fd;
+}
+
+static void *server_thread(void *arg)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd = *(int *)arg;
+ int client_fd;
+
+ if (listen(fd, 1) < 0)
+ error(1, errno, "Failed to listed on socket");
+
+ client_fd = accept(fd, (struct sockaddr *)&addr, &len);
+ if (client_fd < 0)
+ error(1, errno, "Failed to accept client");
+
+ /* Wait for the next connection (that never arrives)
+ * to keep this thread alive to prevent calling
+ * close() on client_fd.
+ */
+ if (accept(fd, (struct sockaddr *)&addr, &len) >= 0)
+ error(1, errno, "Unexpected success in second accept");
+
+ close(client_fd);
+
+ return NULL;
+}
+
+int main(int args, char **argv)
+{
+ int server_fd, cgroup_fd;
+ int err = EXIT_SUCCESS;
+ pthread_t tid;
+
+ if (setup_cgroup_environment())
+ goto cleanup_obj;
+
+ cgroup_fd = create_and_get_cgroup(CG_PATH);
+ if (cgroup_fd < 0)
+ goto cleanup_cgroup_env;
+
+ if (join_cgroup(CG_PATH))
+ goto cleanup_cgroup;
+
+ server_fd = start_server();
+ if (server_fd < 0) {
+ err = EXIT_FAILURE;
+ goto cleanup_cgroup;
+ }
+
+ pthread_create(&tid, NULL, server_thread, (void *)&server_fd);
+
+ if (run_test(cgroup_fd, server_fd))
+ err = EXIT_FAILURE;
+
+ close(server_fd);
+
+ printf("test_sockopt_sk: %s\n",
+ err == EXIT_SUCCESS ? "PASSED" : "FAILED");
+
+cleanup_cgroup:
+ close(cgroup_fd);
+cleanup_cgroup_env:
+ cleanup_cgroup_environment();
+cleanup_obj:
+ return err;
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 5/8] bpf/tools: sync bpf.h
From: Stanislav Fomichev @ 2019-07-02 16:14 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Sync new bpf_tcp_sock fields and new BPF_PROG_TYPE_SOCK_OPS RTT callback.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/include/uapi/linux/bpf.h | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a396b516a2b2..cecf42c871d4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1767,6 +1767,7 @@ union bpf_attr {
* * **BPF_SOCK_OPS_RTO_CB_FLAG** (retransmission time out)
* * **BPF_SOCK_OPS_RETRANS_CB_FLAG** (retransmission)
* * **BPF_SOCK_OPS_STATE_CB_FLAG** (TCP state change)
+ * * **BPF_SOCK_OPS_RTT_CB_FLAG** (every RTT)
*
* Therefore, this function can be used to clear a callback flag by
* setting the appropriate bit to zero. e.g. to disable the RTO
@@ -3069,6 +3070,12 @@ struct bpf_tcp_sock {
* sum(delta(snd_una)), or how many bytes
* were acked.
*/
+ __u32 dsack_dups; /* RFC4898 tcpEStatsStackDSACKDups
+ * total number of DSACK blocks received
+ */
+ __u32 delivered; /* Total data packets delivered incl. rexmits */
+ __u32 delivered_ce; /* Like the above but only ECE marked packets */
+ __u32 icsk_retransmits; /* Number of unrecovered [RTO] timeouts */
};
struct bpf_sock_tuple {
@@ -3311,7 +3318,8 @@ struct bpf_sock_ops {
#define BPF_SOCK_OPS_RTO_CB_FLAG (1<<0)
#define BPF_SOCK_OPS_RETRANS_CB_FLAG (1<<1)
#define BPF_SOCK_OPS_STATE_CB_FLAG (1<<2)
-#define BPF_SOCK_OPS_ALL_CB_FLAGS 0x7 /* Mask of all currently
+#define BPF_SOCK_OPS_RTT_CB_FLAG (1<<3)
+#define BPF_SOCK_OPS_ALL_CB_FLAGS 0xF /* Mask of all currently
* supported cb flags
*/
@@ -3366,6 +3374,8 @@ enum {
BPF_SOCK_OPS_TCP_LISTEN_CB, /* Called on listen(2), right after
* socket transition to LISTEN state.
*/
+ BPF_SOCK_OPS_RTT_CB, /* Called on every RTT.
+ */
};
/* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 4/8] bpf: add icsk_retransmits to bpf_tcp_sock
From: Stanislav Fomichev @ 2019-07-02 16:13 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Add some inet_connection_sock fields to bpf_tcp_sock that might be useful
for debugging congestion control issues.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/uapi/linux/bpf.h | 1 +
net/core/filter.c | 20 +++++++++++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bfb0b1a76684..ead27aebf491 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3078,6 +3078,7 @@ struct bpf_tcp_sock {
*/
__u32 delivered; /* Total data packets delivered incl. rexmits */
__u32 delivered_ce; /* Like the above but only ECE marked packets */
+ __u32 icsk_retransmits; /* Number of unrecovered [RTO] timeouts */
};
struct bpf_sock_tuple {
diff --git a/net/core/filter.c b/net/core/filter.c
index 3da4b6c38b46..089aaea0ccc6 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5544,7 +5544,8 @@ static const struct bpf_func_proto bpf_sock_addr_sk_lookup_udp_proto = {
bool bpf_tcp_sock_is_valid_access(int off, int size, enum bpf_access_type type,
struct bpf_insn_access_aux *info)
{
- if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, delivered_ce))
+ if (off < 0 || off >= offsetofend(struct bpf_tcp_sock,
+ icsk_retransmits))
return false;
if (off % size != 0)
@@ -5575,6 +5576,20 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, FIELD)); \
} while (0)
+#define BPF_INET_SOCK_GET_COMMON(FIELD) \
+ do { \
+ BUILD_BUG_ON(FIELD_SIZEOF(struct inet_connection_sock, \
+ FIELD) > \
+ FIELD_SIZEOF(struct bpf_tcp_sock, FIELD)); \
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( \
+ struct inet_connection_sock, \
+ FIELD), \
+ si->dst_reg, si->src_reg, \
+ offsetof( \
+ struct inet_connection_sock, \
+ FIELD)); \
+ } while (0)
+
if (insn > insn_buf)
return insn - insn_buf;
@@ -5661,6 +5676,9 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
case offsetof(struct bpf_tcp_sock, delivered_ce):
BPF_TCP_SOCK_GET_COMMON(delivered_ce);
break;
+ case offsetof(struct bpf_tcp_sock, icsk_retransmits):
+ BPF_INET_SOCK_GET_COMMON(icsk_retransmits);
+ break;
}
return insn - insn_buf;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox