* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-28 17:04 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Josh Poimboeuf
In-Reply-To: <67c5f9b4-b24a-2806-e8d6-8b5241c66d6e@fb.com>
On Wed, 28 Mar 2018 09:43:56 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> >
> > Given that only eBPF needs this parameter count, we can move
> > it to the struct bpf_raw_event_map newly introduced by Steven,
> > right ? This would reduce bloat of struct tracepoint. For instance,
> > we don't need to keep this count around when eBPF is configured
> > out.
>
> Makes sense. That is indeed cleaner. Will respin.
>
> What I don't like though is 'bloat' argument.
> 'u32 num_args' padded to 8-byte takes exactly the same amount
> of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
> The number of these structures is the same as well
> and chances that tracepoints are on while bpf is off are slim.
> More so few emails ago you said:
> "I'm perfectly fine with adding the "num_args" stuff. I think it's
> really useful. It's only the for_each_kernel_tracepoint change for
> which I'm trying to understand the rationale."
I don't really care which one it goes in. The padding bloat is the same
for both :-/ But I wonder if we can shrink it by doing a trick that
Josh did in one of his patches. That is, to use a 32bit offset instead
of a direct pointer. Since you are only accessing core kernel
tracepoints.
Thus, we could have
struct bpf_raw_event_map {
u32 tp_offset;
u32 num_args;
void *bpf_func;
};
and have:
u64 tp_offset = (u64)tp - (u64)_sdata;
if (WARN_ON(tp_offset > UINT_MAX)
return -EINVAL;
btp->tp_offset = (u32)tp_offset;
And to get the tp, all you need to do is:
tp = (struct tracepoint *)(btp->tp_offset + (unsigned long)_sdata);
I've been thinking of doing this for other parts of the tracepoints and
ftrace code.
BTW, thanks for changing your code. I really appreciate it.
-- Steve
^ permalink raw reply
* [iproute2-next 0/2] tipc: changes to addressing structure
From: Jon Maloy @ 2018-03-28 16:52 UTC (permalink / raw)
To: davem, netdev
Cc: mohan.krishna.ghanta.krishnamurthy, tung.q.nguyen, hoang.h.le,
jon.maloy, canh.d.luu, ying.xue, tipc-discussion
1: We introduce ability to set/get 128-bit node identities
2: We rename 'net id' to 'cluster id' in the command API,
of course in a compatible way.
3: We print out all 32-bit node addresses as an integer in hex format,
i.e., we remove the assumption about an internal structure.
Jon Maloy (2):
tipc: introduce command for handling a new 128-bit node identity
tipc: change node address printout formats
include/uapi/linux/tipc_netlink.h | 2 +
tipc/link.c | 3 +-
tipc/misc.c | 78 ++++++++++++++++++++++++++-
tipc/misc.h | 2 +
tipc/nametable.c | 16 ++----
tipc/node.c | 109 +++++++++++++++++++++++++++++++++-----
tipc/socket.c | 3 +-
7 files changed, 183 insertions(+), 30 deletions(-)
--
2.1.4
^ permalink raw reply
* [iproute2-next 2/2] tipc: change node address printout formats
From: Jon Maloy @ 2018-03-28 16:52 UTC (permalink / raw)
To: davem, netdev; +Cc: tipc-discussion, mohan.krishna.ghanta.krishnamurthy
In-Reply-To: <1522255934-497-1-git-send-email-jon.maloy@ericsson.com>
Since a node address now per definition is only an unstructured 32-bit
integer it makes no sense print it out as a structured string.
In this commit, we replace all occurrences of "<Z.C.N>" printouts with
just an "%x".
Acked-by: GhantaKrishnamurthy MohanKrishna <mohan.krishna.ghanta.krishnamurthy@ericsson.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
tipc/link.c | 3 +--
tipc/nametable.c | 16 +++++-----------
tipc/node.c | 11 ++---------
tipc/socket.c | 3 +--
4 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/tipc/link.c b/tipc/link.c
index 4ae1c91..02f14aa 100644
--- a/tipc/link.c
+++ b/tipc/link.c
@@ -616,8 +616,7 @@ static void link_mon_print_non_applied(uint16_t applied, uint16_t member_cnt,
if (i != applied)
printf(",");
- sprintf(addr_str, "%u.%u.%u:", tipc_zone(members[i]),
- tipc_cluster(members[i]), tipc_node(members[i]));
+ sprintf(addr_str, "%x:", members[i]);
state = map_get(up_map, i) ? 'U' : 'D';
printf("%s%c", addr_str, state);
}
diff --git a/tipc/nametable.c b/tipc/nametable.c
index 770a644..2578940 100644
--- a/tipc/nametable.c
+++ b/tipc/nametable.c
@@ -26,7 +26,6 @@
static int nametable_show_cb(const struct nlmsghdr *nlh, void *data)
{
int *iteration = data;
- char port_id[PORTID_STR_LEN];
struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
struct nlattr *info[TIPC_NLA_MAX + 1] = {};
struct nlattr *attrs[TIPC_NLA_NAME_TABLE_MAX + 1] = {};
@@ -46,22 +45,17 @@ static int nametable_show_cb(const struct nlmsghdr *nlh, void *data)
return MNL_CB_ERROR;
if (!*iteration)
- printf("%-10s %-10s %-10s %-26s %-10s\n",
- "Type", "Lower", "Upper", "Port Identity",
+ printf("%-10s %-10s %-10s %-10s %-10s %-10s\n",
+ "Type", "Lower", "Upper", "Node", "Port",
"Publication Scope");
(*iteration)++;
- snprintf(port_id, sizeof(port_id), "<%u.%u.%u:%u>",
- tipc_zone(mnl_attr_get_u32(publ[TIPC_NLA_PUBL_NODE])),
- tipc_cluster(mnl_attr_get_u32(publ[TIPC_NLA_PUBL_NODE])),
- tipc_node(mnl_attr_get_u32(publ[TIPC_NLA_PUBL_NODE])),
- mnl_attr_get_u32(publ[TIPC_NLA_PUBL_REF]));
-
- printf("%-10u %-10u %-10u %-26s %-12u",
+ printf("%-10u %-10u %-10u %-10x %-10u %-12u",
mnl_attr_get_u32(publ[TIPC_NLA_PUBL_TYPE]),
mnl_attr_get_u32(publ[TIPC_NLA_PUBL_LOWER]),
mnl_attr_get_u32(publ[TIPC_NLA_PUBL_UPPER]),
- port_id,
+ mnl_attr_get_u32(publ[TIPC_NLA_PUBL_NODE]),
+ mnl_attr_get_u32(publ[TIPC_NLA_PUBL_REF]),
mnl_attr_get_u32(publ[TIPC_NLA_PUBL_KEY]));
printf("%s\n", scope[mnl_attr_get_u32(publ[TIPC_NLA_PUBL_SCOPE])]);
diff --git a/tipc/node.c b/tipc/node.c
index 3ebbe0b..b73b644 100644
--- a/tipc/node.c
+++ b/tipc/node.c
@@ -40,10 +40,7 @@ static int node_list_cb(const struct nlmsghdr *nlh, void *data)
return MNL_CB_ERROR;
addr = mnl_attr_get_u32(attrs[TIPC_NLA_NODE_ADDR]);
- printf("<%u.%u.%u>: ",
- tipc_zone(addr),
- tipc_cluster(addr),
- tipc_node(addr));
+ printf("%x: ", addr);
if (attrs[TIPC_NLA_NODE_UP])
printf("up\n");
@@ -123,11 +120,7 @@ static int cmd_node_get_addr(struct nlmsghdr *nlh, const struct cmd *cmd,
}
close(sk);
- printf("<%u.%u.%u>\n",
- tipc_zone(addr.addr.id.node),
- tipc_cluster(addr.addr.id.node),
- tipc_node(addr.addr.id.node));
-
+ printf("%x\n", addr.addr.id.node);
return 0;
}
diff --git a/tipc/socket.c b/tipc/socket.c
index 48ba821..852984e 100644
--- a/tipc/socket.c
+++ b/tipc/socket.c
@@ -84,8 +84,7 @@ static int sock_list_cb(const struct nlmsghdr *nlh, void *data)
mnl_attr_parse_nested(attrs[TIPC_NLA_SOCK_CON], parse_attrs, con);
node = mnl_attr_get_u32(con[TIPC_NLA_CON_NODE]);
- printf(" connected to <%u.%u.%u:%u>", tipc_zone(node),
- tipc_cluster(node), tipc_node(node),
+ printf(" connected to %x:%u", node,
mnl_attr_get_u32(con[TIPC_NLA_CON_SOCK]));
if (con[TIPC_NLA_CON_FLAG])
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [iproute2-next 1/2] tipc: introduce command for handling a new 128-bit node identity
From: Jon Maloy @ 2018-03-28 16:52 UTC (permalink / raw)
To: davem, netdev; +Cc: tipc-discussion, mohan.krishna.ghanta.krishnamurthy
In-Reply-To: <1522255934-497-1-git-send-email-jon.maloy@ericsson.com>
We add the possibility to set and get a 128 bit node identifier, as
an alternative to the legacy 32-bit node address we are using now.
We also add an option to set and get 'clusterid' in the node. This
is the same as what we have so far called 'netid' and performs the
same operations. For compatibility the old 'netid' commands are
retained, -we just remove them from the help texts.
Acked-by: GhantaKrishnamurthy MohanKrishna <mohan.krishna.ghanta.krishnamurthy@ericsson.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
include/uapi/linux/tipc_netlink.h | 2 +
tipc/misc.c | 78 ++++++++++++++++++++++++++++++-
tipc/misc.h | 2 +
tipc/node.c | 98 +++++++++++++++++++++++++++++++++++++--
4 files changed, 174 insertions(+), 6 deletions(-)
diff --git a/include/uapi/linux/tipc_netlink.h b/include/uapi/linux/tipc_netlink.h
index 469aa67..6bf8ec6 100644
--- a/include/uapi/linux/tipc_netlink.h
+++ b/include/uapi/linux/tipc_netlink.h
@@ -162,6 +162,8 @@ enum {
TIPC_NLA_NET_UNSPEC,
TIPC_NLA_NET_ID, /* u32 */
TIPC_NLA_NET_ADDR, /* u32 */
+ TIPC_NLA_NET_NODEID, /* u64 */
+ TIPC_NLA_NET_NODEID_W1, /* u64 */
__TIPC_NLA_NET_MAX,
TIPC_NLA_NET_MAX = __TIPC_NLA_NET_MAX - 1
diff --git a/tipc/misc.c b/tipc/misc.c
index 8091222..16849f1 100644
--- a/tipc/misc.c
+++ b/tipc/misc.c
@@ -12,7 +12,7 @@
#include <stdio.h>
#include <stdint.h>
#include <linux/tipc.h>
-
+#include <string.h>
#include "misc.h"
#define IN_RANGE(val, low, high) ((val) <= (high) && (val) >= (low))
@@ -33,3 +33,79 @@ uint32_t str2addr(char *str)
fprintf(stderr, "invalid network address \"%s\"\n", str);
return 0;
}
+
+static int is_hex(char *arr, int last)
+{
+ int i;
+
+ while (!arr[last])
+ last--;
+
+ for (i = 0; i <= last; i++) {
+ if (!IN_RANGE(arr[i], '0', '9') &&
+ !IN_RANGE(arr[i], 'a', 'f') &&
+ !IN_RANGE(arr[i], 'A', 'F'))
+ return 0;
+ }
+ return 1;
+}
+
+static int is_name(char *arr, int last)
+{
+ int i;
+ char c;
+
+ while (!arr[last])
+ last--;
+
+ if (last > 15)
+ return 0;
+
+ for (i = 0; i <= last; i++) {
+ c = arr[i];
+ if (!IN_RANGE(c, '0', '9') && !IN_RANGE(c, 'a', 'z') &&
+ !IN_RANGE(c, 'A', 'Z') && c != '-' && c != '_' &&
+ c != '.' && c != ':' && c != '@')
+ return 0;
+ }
+ return 1;
+}
+
+int str2nodeid(char *str, uint8_t *id)
+{
+ int len = strlen(str);
+ int i;
+
+ if (len > 32)
+ return -1;
+
+ if (is_name(str, len - 1)) {
+ memcpy(id, str, len);
+ return 0;
+ }
+ if (!is_hex(str, len - 1))
+ return -1;
+
+ str[len] = '0';
+ for (i = 0; i < 16; i++) {
+ if (sscanf(&str[2 * i], "%2hhx", &id[i]) != 1)
+ break;
+ }
+ return 0;
+}
+
+void nodeid2str(uint8_t *id, char *str)
+{
+ int i;
+
+ if (is_name((char *)id, 15)) {
+ memcpy(str, id, 16);
+ return;
+ }
+
+ for (i = 0; i < 16; i++)
+ sprintf(&str[2 * i], "%02x", id[i]);
+
+ for (i = 31; str[i] == '0'; i--)
+ str[i] = 0;
+}
diff --git a/tipc/misc.h b/tipc/misc.h
index 585df74..6e8afdd 100644
--- a/tipc/misc.h
+++ b/tipc/misc.h
@@ -15,5 +15,7 @@
#include <stdint.h>
uint32_t str2addr(char *str);
+int str2nodeid(char *str, uint8_t *id);
+void nodeid2str(uint8_t *id, char *str);
#endif
diff --git a/tipc/node.c b/tipc/node.c
index fe085ae..3ebbe0b 100644
--- a/tipc/node.c
+++ b/tipc/node.c
@@ -131,6 +131,90 @@ static int cmd_node_get_addr(struct nlmsghdr *nlh, const struct cmd *cmd,
return 0;
}
+static int cmd_node_set_nodeid(struct nlmsghdr *nlh, const struct cmd *cmd,
+ struct cmdl *cmdl, void *data)
+{
+ char buf[MNL_SOCKET_BUFFER_SIZE];
+ uint8_t id[16] = {0,};
+ uint64_t *w0 = (uint64_t *) &id[0];
+ uint64_t *w1 = (uint64_t *) &id[8];
+ struct nlattr *nest;
+ char *str;
+
+ if (cmdl->argc != cmdl->optind + 1) {
+ fprintf(stderr, "Usage: %s node set nodeid NODE_ID\n",
+ cmdl->argv[0]);
+ return -EINVAL;
+ }
+
+ str = shift_cmdl(cmdl);
+ if (str2nodeid(str, id)) {
+ fprintf(stderr, "Invalid node identity\n");
+ return -EINVAL;
+ }
+
+ nlh = msg_init(buf, TIPC_NL_NET_SET);
+ if (!nlh) {
+ fprintf(stderr, "error, message initialisation failed\n");
+ return -1;
+ }
+ nest = mnl_attr_nest_start(nlh, TIPC_NLA_NET);
+ mnl_attr_put_u64(nlh, TIPC_NLA_NET_NODEID, *w0);
+ mnl_attr_put_u64(nlh, TIPC_NLA_NET_NODEID_W1, *w1);
+ mnl_attr_nest_end(nlh, nest);
+ return msg_doit(nlh, NULL, NULL);
+}
+
+static int nodeid_get_cb(const struct nlmsghdr *nlh, void *data)
+{
+ struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
+ struct nlattr *info[TIPC_NLA_MAX + 1] = {};
+ struct nlattr *attrs[TIPC_NLA_NET_MAX + 1] = {};
+ char str[33] = {0,};
+ uint8_t id[16] = {0,};
+ uint64_t *w0 = (uint64_t *) &id[0];
+ uint64_t *w1 = (uint64_t *) &id[8];
+ int i;
+
+ mnl_attr_parse(nlh, sizeof(*genl), parse_attrs, info);
+ if (!info[TIPC_NLA_NET])
+ return MNL_CB_ERROR;
+
+ mnl_attr_parse_nested(info[TIPC_NLA_NET], parse_attrs, attrs);
+ if (!attrs[TIPC_NLA_NET_ID])
+ return MNL_CB_ERROR;
+
+ *w0 = mnl_attr_get_u64(attrs[TIPC_NLA_NET_NODEID]);
+ *w1 = mnl_attr_get_u64(attrs[TIPC_NLA_NET_NODEID_W1]);
+ nodeid2str(id, str);
+ printf("Node Identity Hash\n");
+ printf("%s", str);
+ for (i = strlen(str); i <= 33; i++)
+ printf(" ");
+ cmd_node_get_addr(NULL, NULL, NULL, NULL);
+ return MNL_CB_OK;
+}
+
+static int cmd_node_get_nodeid(struct nlmsghdr *nlh, const struct cmd *cmd,
+ struct cmdl *cmdl, void *data)
+{
+ char buf[MNL_SOCKET_BUFFER_SIZE];
+
+ if (help_flag) {
+ (cmd->help)(cmdl);
+ return -EINVAL;
+ }
+
+ nlh = msg_init(buf, TIPC_NL_NET_GET);
+ if (!nlh) {
+ fprintf(stderr, "error, message initialisation failed\n");
+ return -1;
+ }
+
+ return msg_dumpit(nlh, nodeid_get_cb, NULL);
+}
+
+
static int netid_get_cb(const struct nlmsghdr *nlh, void *data)
{
struct genlmsghdr *genl = mnl_nlmsg_get_payload(nlh);
@@ -204,8 +288,8 @@ static void cmd_node_set_help(struct cmdl *cmdl)
fprintf(stderr,
"Usage: %s node set PROPERTY\n\n"
"PROPERTIES\n"
- " address ADDRESS - Set local address\n"
- " netid NETID - Set local netid\n",
+ " identity NODEID - Set node identity\n"
+ " clusterid CLUSTERID - Set local cluster id\n",
cmdl->argv[0]);
}
@@ -213,8 +297,10 @@ static int cmd_node_set(struct nlmsghdr *nlh, const struct cmd *cmd,
struct cmdl *cmdl, void *data)
{
const struct cmd cmds[] = {
- { "address", cmd_node_set_addr, NULL },
+ { "address", cmd_node_set_addr, NULL },
+ { "identity", cmd_node_set_nodeid, NULL },
{ "netid", cmd_node_set_netid, NULL },
+ { "clusterid", cmd_node_set_netid, NULL },
{ NULL }
};
@@ -226,8 +312,8 @@ static void cmd_node_get_help(struct cmdl *cmdl)
fprintf(stderr,
"Usage: %s node get PROPERTY\n\n"
"PROPERTIES\n"
- " address - Get local address\n"
- " netid - Get local netid\n",
+ " identity - Get node identity\n"
+ " clusterid - Get local clusterid\n",
cmdl->argv[0]);
}
@@ -236,7 +322,9 @@ static int cmd_node_get(struct nlmsghdr *nlh, const struct cmd *cmd,
{
const struct cmd cmds[] = {
{ "address", cmd_node_get_addr, NULL },
+ { "identity", cmd_node_get_nodeid, NULL },
{ "netid", cmd_node_get_netid, NULL },
+ { "clusterid", cmd_node_get_netid, NULL },
{ NULL }
};
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply related
* Re: [PATCH] sfp: allow cotsworks modules
From: Russell King - ARM Linux @ 2018-03-28 16:51 UTC (permalink / raw)
To: Joe Perches; +Cc: Andrew Lunn, Florian Fainelli, netdev
In-Reply-To: <1522253941.12357.109.camel@perches.com>
On Wed, Mar 28, 2018 at 09:19:01AM -0700, Joe Perches wrote:
> On Wed, 2018-03-28 at 11:41 +0100, Russell King - ARM Linux wrote:
> > On Wed, Mar 28, 2018 at 03:33:57AM -0700, Joe Perches wrote:
> > > On Wed, 2018-03-28 at 11:18 +0100, Russell King wrote:
> > > > Cotsworks modules fail the checksums - it appears that Cotsworks
> > > > reprograms the EEPROM at the end of production with the final product
> > > > information (serial, date code, and exact part number for module
> > > > options) and fails to update the checksum.
> > >
> > > trivia:
> > >
> > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> > >
> > > []
> > > > @@ -574,23 +575,43 @@ static int sfp_sm_mod_probe(struct sfp *sfp)
> > >
> > > []
> > > > + if (cotsworks) {
> > > > + dev_warn(sfp->dev,
> > > > + "EEPROM base structure checksum failure (0x%02x != 0x%02x)\n",
> > > > + check, id.base.cc_base);
> > > > + } else {
> > > > + dev_err(sfp->dev,
> > > > + "EEPROM base structure checksum failure: 0x%02x != 0x%02x\n",
> > >
> > > It'd be better to move this above the if and
> > > use only a single format string instead of
> > > using 2 slightly different formats.
> >
> > No. I think you've missed the fact that one is a _warning_ the other is
> > an _error_ and they are emitted at the appropriate severity. It's not
> > just that the format strings are slightly different.
>
> Right. Still nicer to use the same formats.
I'll stick a "Warning:" and "Error:" tag before them if you really
want the rest of the message to be identically formatted - otherwise,
when seeing reports from people's dmesg, there will be nothing to
indicate which message was printed.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up
^ permalink raw reply
* Re: [PATCH v7 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-28 16:43 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
rostedt, netdev, kernel-team, linux-api
In-Reply-To: <1074829260.1986.1522244964935.JavaMail.zimbra@efficios.com>
On 3/28/18 6:49 AM, Mathieu Desnoyers wrote:
> ----- On Mar 27, 2018, at 10:11 PM, Alexei Starovoitov ast@fb.com wrote:
>
>> From: Alexei Starovoitov <ast@kernel.org>
>>
>> compute number of arguments passed into tracepoint
>> at compile time and store it as part of 'struct tracepoint'.
>> The number is necessary to check safety of bpf program access that
>> is coming in subsequent patch.
>
>
> Hi Alexei,
>
> Given that only eBPF needs this parameter count, we can move
> it to the struct bpf_raw_event_map newly introduced by Steven,
> right ? This would reduce bloat of struct tracepoint. For instance,
> we don't need to keep this count around when eBPF is configured
> out.
Makes sense. That is indeed cleaner. Will respin.
What I don't like though is 'bloat' argument.
'u32 num_args' padded to 8-byte takes exactly the same amount
of space in 'struct tracepoint' and in 'struct bpf_raw_event_map'
The number of these structures is the same as well
and chances that tracepoints are on while bpf is off are slim.
More so few emails ago you said:
"I'm perfectly fine with adding the "num_args" stuff. I think it's
really useful. It's only the for_each_kernel_tracepoint change for
which I'm trying to understand the rationale."
I'm guessing now you found out that num_args is not useful to lttng
and it bloats its data structures?
It's ok to change an opinion and I'm completely fine using that as
a real reason.
Will repsin, as I said. No problem.
^ permalink raw reply
* Re: [PATCH 07/30] aio: add delayed cancel support
From: Al Viro @ 2018-03-28 16:35 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180328072926.17131-8-hch@lst.de>
On Wed, Mar 28, 2018 at 09:29:03AM +0200, Christoph Hellwig wrote:
> static void aio_fsync_work(struct work_struct *work)
> {
> struct fsync_iocb *req = container_of(work, struct fsync_iocb, work);
> + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, fsync);
> + struct file *file = req->file;
> int ret;
>
> ret = vfs_fsync(req->file, req->datasync);
> - fput(req->file);
> - aio_complete(container_of(req, struct aio_kiocb, fsync), ret, 0);
> + if (aio_complete(iocb, ret, 0, 0))
> + fput(file);
IDGI.
1) can aio_complete() ever return false here?
2) do we ever have aio_kiocb that would not have an associated
struct file * that needs to be dropped on successful aio_complete()? AFAICS,
rw, fsync and poll variants all have one, and I'm not sure what kind of
async IO *could* be done without an opened file.
^ permalink raw reply
* Re: 4.14.29 - tcp_push() - null skb's cb dereference
From: Eric Dumazet @ 2018-03-28 16:33 UTC (permalink / raw)
To: David Miller; +Cc: krzysztof.blaszkowski, netdev, gregkh
In-Reply-To: <20180328.103818.621024177280113028.davem@davemloft.net>
On 03/28/2018 07:38 AM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 28 Mar 2018 06:38:21 -0700
>
>> https://patchwork.ozlabs.org/patch/886324/
>
> I have this in my current -stable submission set, and I'm working
> actively on this right now.
Thanks a lot David.
^ permalink raw reply
* Re: net_tx_action race condition?
From: Eric Dumazet @ 2018-03-28 16:32 UTC (permalink / raw)
To: Saurabh Kr, Angelo Rizzi
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Sarvendra Vikram Singh, Kunal Sharma
In-Reply-To: <BY2PR05MB712796CB064F3202FEFA328B3A30@BY2PR05MB712.namprd05.prod.outlook.com>
On 03/28/2018 12:30 AM, Saurabh Kr wrote:
> Hi Eric/Angelo,
>
> We are seeing the assertion error in linux kernel 2.4.29 “*kernel: KERNEL: assertion (atomic_read(&skb->users) == 0) failed at dev.c(1397)**”.* Based on patch provided (_https://patchwork.kernel.org/patch/5368051/_ ) we merged the changes in linux kernel 2.4.29 but we are still facing the assertion error at dev.c (1397). Please let me know your thoughts.
>
> *Before Merge**(linux 2.4.29)*
> ---------------------------------
>
> static void net_tx_action(struct softirq_action *h)
> {
> int cpu = smp_processor_id();
>
> if (softnet_data[cpu].completion_queue) {
> struct sk_buff *clist;
>
> local_irq_disable();
> clist = softnet_data[cpu].completion_queue; // Existing code
> softnet_data[cpu].completion_queue = NULL;
> local_irq_enable();
>
> while (clist != NULL) {
> struct sk_buff *skb = clist;
> clist = clist->next;
>
> BUG_TRAP(atomic_read(&skb->users) == 0);
> __kfree_skb(skb);
> }
> }
>
> ---------
>
> *After Merge the changes based on available patch**(linux 2.4.29)**:*
> ------------------------------------------------------------------------------
>
> static void net_tx_action(struct softirq_action *h)
> {
> int cpu = smp_processor_id();
>
> if (softnet_data[cpu].completion_queue) {
> struct sk_buff *clist;
>
> local_irq_disable();
> clist = *(volatile typeof(softnet_data[cpu].completion_queue) *)&( softnet_data[cpu].completion_queue); // Modified line based on available patch
> softnet_data[cpu].completion_queue = NULL;
> local_irq_enable();
>
> while (clist != NULL) {
> struct sk_buff *skb = clist;
> clist = clist->next;
>
> BUG_TRAP(atomic_read(&skb->users) == 0);
> __kfree_skb(skb);
> }
> }
> ………….
>
> Thanks & regards,
> Saurabh
>
Thats simply prove (again) that this 'fix' was not the proper one.
I have no idea what is wrong, and there is no way I am going to look at 2.4.29 kernel...
^ permalink raw reply
* Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
From: Andrew Lunn @ 2018-03-28 16:28 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ioana Ciornei, gregkh, Laurentiu Tudor, Linux Kernel Mailing List,
Stuart Yoder, Ruxandra Ioana Ciocoi Radulescu, Razvan Stefanescu,
Roy Pledge, Networking
In-Reply-To: <CAK8P3a1aQCm_r95Ka4x+j8SJLCLFvLuX3A4twK1WJcJs66YwKg@mail.gmail.com>
> I'm still not convinced either way (high-level or low-level
> interface), but I think
> this needs to be discussed with the networking maintainers. Given the examples
> on the github page you linked to, the high-level user space commands
> based on these ioctls
>
> ls-addni # adds a network interface
> ls-addmux # adds a dpdmux
> ls-addsw # adds an l2switch
> ls-listmac # lists MACs and their connections
> ls-listni # lists network interfaces and their connections
>
> and I see that you also support the switchdev interface in
> drivers/staging/fsl-dpaa2, which I think does some of the same
> things, presumably by implementing the switchdev API using
> fsl_mc_command low-level interfaces in the kernel.
Hi Arnd
I agree that switchdev and devlink should be the correct way to handle
this. The low level plumbing of the hardware should all be
hidden. There should not be any user space commands needed other than
the usual network configuration tools and devlink.
Andrew
^ permalink raw reply
* [PATCH bpf-next RFC 2/2] Add MPLS label push/pop functions for EBPF
From: Shrijeet Mukherjee @ 2018-03-28 16:27 UTC (permalink / raw)
To: daniel; +Cc: netdev, roopa, dsa, ast
In-Reply-To: <20180328162715.113-1-shrijeetoss@gmail.com>
From: Shrijeet Mukherjee <shrijeet@gmail.com>
Push: takes an array/size of labels and encaps
Pop : Takes a number and pops the top "n" labels
Signed-off-by: Shrijeet Mukherjee <shm@cumulusnetworks.com>
PS: Will be adding tests as well, refactoring my test into something
smaller.
---
include/linux/bpf.h | 2 +
include/net/mpls.h | 38 +++++++++++++-
include/uapi/linux/bpf.h | 11 +++-
net/core/filter.c | 127 ++++++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 175 insertions(+), 3 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 819229c80eca..b38bfabc1fb5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -672,6 +672,8 @@ extern const struct bpf_func_proto bpf_get_current_uid_gid_proto;
extern const struct bpf_func_proto bpf_get_current_comm_proto;
extern const struct bpf_func_proto bpf_skb_vlan_push_proto;
extern const struct bpf_func_proto bpf_skb_vlan_pop_proto;
+extern const struct bpf_func_proto bpf_skb_mpls_push_proto;
+extern const struct bpf_func_proto bpf_skb_mpls_pop_proto;
extern const struct bpf_func_proto bpf_get_stackid_proto;
extern const struct bpf_func_proto bpf_sock_map_update_proto;
diff --git a/include/net/mpls.h b/include/net/mpls.h
index 2583dbc689b8..3a5b8c00823d 100644
--- a/include/net/mpls.h
+++ b/include/net/mpls.h
@@ -24,14 +24,50 @@ struct mpls_shim_hdr {
__be32 label_stack_entry;
};
+struct mpls_entry_decoded {
+ u32 label;
+ u8 ttl;
+ u8 tc;
+ u8 bos;
+};
+
static inline bool eth_p_mpls(__be16 eth_type)
{
return eth_type == htons(ETH_P_MPLS_UC) ||
eth_type == htons(ETH_P_MPLS_MC);
}
-static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
+static inline struct mpls_shim_hdr *skb_mpls_hdr(const struct sk_buff *skb)
{
return (struct mpls_shim_hdr *)skb_network_header(skb);
}
+
+static inline struct mpls_shim_hdr
+mpls_entry_encode(u32 label, unsigned int ttl, unsigned int tc, bool bos)
+{
+ struct mpls_shim_hdr result;
+
+ result.label_stack_entry =
+ cpu_to_be32((label << MPLS_LS_LABEL_SHIFT)
+ | (tc << MPLS_LS_TC_SHIFT)
+ | (bos ? (1 << MPLS_LS_S_SHIFT) : 0)
+ | (ttl << MPLS_LS_TTL_SHIFT));
+
+ return result;
+}
+
+static inline
+struct mpls_entry_decoded mpls_entry_decode(struct mpls_shim_hdr *hdr)
+{
+ struct mpls_entry_decoded result;
+ unsigned int entry = be32_to_cpu(hdr->label_stack_entry);
+
+ result.label = (entry & MPLS_LS_LABEL_MASK) >> MPLS_LS_LABEL_SHIFT;
+ result.ttl = (entry & MPLS_LS_TTL_MASK) >> MPLS_LS_TTL_SHIFT;
+ result.tc = (entry & MPLS_LS_TC_MASK) >> MPLS_LS_TC_SHIFT;
+ result.bos = (entry & MPLS_LS_S_MASK) >> MPLS_LS_S_SHIFT;
+
+ return result;
+}
+
#endif
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 18b7c510c511..2278548e1f8f 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -439,6 +439,12 @@ union bpf_attr {
* int bpf_skb_vlan_pop(skb)
* Return: 0 on success or negative error
*
+ * int bpf_skb_mpls_push(skb, num_lbls, lbls[])
+ * Return 0 on success or negative error
+ *
+ * int bpf_skb_mpls_pop(skb, num_lbls)
+ * Return number of popped labels, 0 is no-op, deliver packet to current dst
+ *
* int bpf_skb_get_tunnel_key(skb, key, size, flags)
* int bpf_skb_set_tunnel_key(skb, key, size, flags)
* retrieve or populate tunnel metadata
@@ -794,7 +800,10 @@ union bpf_attr {
FN(msg_redirect_map), \
FN(msg_apply_bytes), \
FN(msg_cork_bytes), \
- FN(msg_pull_data),
+ FN(msg_pull_data), \
+ FN(skb_mpls_push), \
+ FN(skb_mpls_pop),
+
/* integer value in 'imm' field of BPF_CALL instruction selects which helper
* function eBPF program intends to call
diff --git a/net/core/filter.c b/net/core/filter.c
index 00f62fafc788..c96ae8ef423d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2522,7 +2522,7 @@ static int bpf_skb_adjust_net(struct sk_buff *skb, s32 len_diff)
}
BPF_CALL_4(bpf_skb_adjust_room, struct sk_buff *, skb, s32, len_diff,
- u32, mode, u64, flags)
+ u32, mode, u64, flags)
{
if (unlikely(flags))
return -EINVAL;
@@ -2542,6 +2542,125 @@ static const struct bpf_func_proto bpf_skb_adjust_room_proto = {
.arg4_type = ARG_ANYTHING,
};
+static int bpf_skb_mpls_net_grow(struct sk_buff *skb, int len_diff)
+{
+ u32 off = skb_mac_header_len(skb); /*LL_RESERVED_SPACE ?? */
+ int ret;
+
+ ret = skb_cow(skb, len_diff);
+ if (unlikely(ret < 0))
+ return ret;
+
+ skb_set_inner_protocol(skb, skb->protocol);
+ skb_reset_inner_network_header(skb);
+
+ ret = bpf_skb_generic_push(skb, off, len_diff);
+ if (unlikely(ret < 0))
+ return ret;
+
+ skb_reset_mac_header(skb);
+ skb_set_network_header(skb, ETH_HLEN);
+ skb->protocol = eth_hdr(skb)->h_proto = htons(ETH_P_MPLS_UC);
+
+ if (skb_is_gso(skb)) {
+/* Due to header grow, MSS needs to be downgraded. */
+ skb_shinfo(skb)->gso_size -= len_diff;
+/* Header must be checked, and gso_segs recomputed. */
+ skb_shinfo(skb)->gso_type |= SKB_GSO_DODGY;
+ skb_shinfo(skb)->gso_segs = 0;
+ }
+
+ bpf_compute_data_pointers(skb);
+ return 0;
+}
+
+BPF_CALL_2(bpf_skb_mpls_pop, struct sk_buff*, skb, u32, num_lbls)
+{
+ u32 i = 0;
+ struct mpls_shim_hdr *hdr;
+ unsigned char *cursor;
+ struct mpls_entry_decoded dec;
+
+ if (num_lbls == 0)
+ return 0;
+
+ cursor = skb_network_header(skb);
+ do {
+ hdr = (struct mpls_shim_hdr *)cursor;
+ dec = mpls_entry_decode(hdr);
+ i++; cursor = cursor + sizeof(struct mpls_shim_hdr);
+ } while (dec.bos != 1 && i < num_lbls);
+
+ bpf_push_mac_rcsum(skb);
+ skb_pull(skb, i * sizeof(struct mpls_shim_hdr));
+ skb_reset_network_header(skb);
+
+ skb->protocol = eth_hdr(skb)->h_proto = htons(ETH_P_MPLS_UC);
+ bpf_pull_mac_rcsum(skb);
+ bpf_compute_data_pointers(skb);
+
+ return i;
+}
+
+const struct bpf_func_proto bpf_skb_mpls_pop_proto = {
+ .func = bpf_skb_mpls_pop,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_ANYTHING,
+};
+EXPORT_SYMBOL_GPL(bpf_skb_mpls_pop_proto);
+
+BPF_CALL_3(bpf_skb_mpls_push, struct sk_buff*, skb,
+ __be32*, lbls, u32, num_lbls)
+{
+ int ret, i;
+ unsigned int new_header_size = num_lbls * sizeof(__be32);
+ unsigned int ttl = 255;
+ struct dst_entry *dst = skb_dst(skb);
+ struct net_device *out_dev = dst->dev;
+ struct mpls_shim_hdr *hdr;
+ bool bos;
+
+ /* Ensure there is enough space for the headers in the skb */
+ ret = bpf_skb_mpls_net_grow(skb, new_header_size);
+ if (ret < 0) {
+ trace_printk("COW was killed\n");
+ bpf_compute_data_pointers(skb);
+ return -ENOMEM;
+ }
+
+ skb->dev = out_dev;
+/* XXX this may need finesse to integrate with
+ * global TTL values for MPLS
+ */
+ if (dst->ops->family == AF_INET)
+ ttl = ip_hdr(skb)->ttl;
+ else if (dst->ops->family == AF_INET6)
+ ttl = ipv6_hdr(skb)->hop_limit;
+
+ /* Push the new labels */
+ hdr = skb_mpls_hdr(skb);
+ bos = true;
+ for (i = num_lbls - 1; i >= 0; i--) {
+ hdr[i] = mpls_entry_encode(lbls[i], ttl, 0, bos);
+ bos = false;
+ }
+
+ bpf_compute_data_pointers(skb);
+ return 0;
+}
+
+const struct bpf_func_proto bpf_skb_mpls_push_proto = {
+ .func = bpf_skb_mpls_push,
+ .gpl_only = false,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_PTR_TO_MEM,
+ .arg3_type = ARG_CONST_SIZE,
+};
+EXPORT_SYMBOL_GPL(bpf_skb_mpls_push_proto);
+
static u32 __bpf_skb_min_len(const struct sk_buff *skb)
{
u32 min_len = skb_network_offset(skb);
@@ -3019,6 +3138,8 @@ bool bpf_helper_changes_pkt_data(void *func)
{
if (func == bpf_skb_vlan_push ||
func == bpf_skb_vlan_pop ||
+ func == bpf_skb_mpls_push ||
+ func == bpf_skb_mpls_pop ||
func == bpf_skb_store_bytes ||
func == bpf_skb_change_proto ||
func == bpf_skb_change_head ||
@@ -3682,6 +3803,10 @@ tc_cls_act_func_proto(enum bpf_func_id func_id)
return &bpf_skb_vlan_push_proto;
case BPF_FUNC_skb_vlan_pop:
return &bpf_skb_vlan_pop_proto;
+ case BPF_FUNC_skb_mpls_push:
+ return &bpf_skb_mpls_push_proto;
+ case BPF_FUNC_skb_mpls_pop:
+ return &bpf_skb_mpls_pop_proto;
case BPF_FUNC_skb_change_proto:
return &bpf_skb_change_proto_proto;
case BPF_FUNC_skb_change_type:
--
2.16.2.windows.1
^ permalink raw reply related
* [PATCH bpf-next RFC 1/2] Organize MPLS headers
From: Shrijeet Mukherjee @ 2018-03-28 16:27 UTC (permalink / raw)
To: daniel; +Cc: netdev, roopa, dsa, ast
From: Shrijeet Mukherjee <shrijeet@gmail.com>
Prepare shared headers for new MPLS label push/pop EBPF helpers
Signed-off-by: Shrijeet Mukherjee <shm@cumulusnetworks.com>
---
include/net/mpls.h | 1 +
net/core/filter.c | 2 ++
net/mpls/af_mpls.c | 8 ++++----
net/mpls/internal.h | 31 -------------------------------
net/mpls/mpls_iptunnel.c | 2 +-
net/openvswitch/actions.c | 12 ++++++------
6 files changed, 14 insertions(+), 42 deletions(-)
diff --git a/include/net/mpls.h b/include/net/mpls.h
index 1dbc669b770e..2583dbc689b8 100644
--- a/include/net/mpls.h
+++ b/include/net/mpls.h
@@ -16,6 +16,7 @@
#include <linux/if_ether.h>
#include <linux/netdevice.h>
+#include <uapi/linux/mpls.h>
#define MPLS_HLEN 4
diff --git a/net/core/filter.c b/net/core/filter.c
index c86f03fd9ea5..00f62fafc788 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -29,6 +29,7 @@
#include <linux/sock_diag.h>
#include <linux/in.h>
#include <linux/inet.h>
+#include <linux/mpls.h>
#include <linux/netdevice.h>
#include <linux/if_packet.h>
#include <linux/if_arp.h>
@@ -56,6 +57,7 @@
#include <net/sock_reuseport.h>
#include <net/busy_poll.h>
#include <net/tcp.h>
+#include <net/mpls.h>
#include <linux/bpf_trace.h>
/**
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index d4a89a8be013..4e05391b77f0 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -170,7 +170,7 @@ static u32 mpls_multipath_hash(struct mpls_route *rt, struct sk_buff *skb)
break;
/* Read and decode the current label */
- hdr = mpls_hdr(skb) + label_index;
+ hdr = skb_mpls_hdr(skb) + label_index;
dec = mpls_entry_decode(hdr);
/* RFC6790 - reserved labels MUST NOT be used as keys
@@ -379,7 +379,7 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
goto err;
/* Read and decode the label */
- hdr = mpls_hdr(skb);
+ hdr = skb_mpls_hdr(skb);
dec = mpls_entry_decode(hdr);
rt = mpls_route_input_rcu(net, dec.label);
@@ -440,7 +440,7 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
skb_push(skb, new_header_size);
skb_reset_network_header(skb);
/* Push the new labels */
- hdr = mpls_hdr(skb);
+ hdr = skb_mpls_hdr(skb);
bos = dec.bos;
for (i = nh->nh_labels - 1; i >= 0; i--) {
hdr[i] = mpls_entry_encode(nh->nh_label[i],
@@ -2203,7 +2203,7 @@ static int mpls_getroute(struct sk_buff *in_skb, struct nlmsghdr *in_nlh,
skb_reset_network_header(skb);
/* Push new labels */
- hdr = mpls_hdr(skb);
+ hdr = skb_mpls_hdr(skb);
bos = true;
for (i = n_labels - 1; i >= 0; i--) {
hdr[i] = mpls_entry_encode(labels[i],
diff --git a/net/mpls/internal.h b/net/mpls/internal.h
index 768a302879b4..cd93cb201fed 100644
--- a/net/mpls/internal.h
+++ b/net/mpls/internal.h
@@ -8,13 +8,6 @@
*/
#define MAX_NEW_LABELS 30
-struct mpls_entry_decoded {
- u32 label;
- u8 ttl;
- u8 tc;
- u8 bos;
-};
-
struct mpls_pcpu_stats {
struct mpls_link_stats stats;
struct u64_stats_sync syncp;
@@ -172,30 +165,6 @@ struct mpls_route { /* next hop label forwarding entry */
#define endfor_nexthops(rt) }
-static inline struct mpls_shim_hdr mpls_entry_encode(u32 label, unsigned ttl, unsigned tc, bool bos)
-{
- struct mpls_shim_hdr result;
- result.label_stack_entry =
- cpu_to_be32((label << MPLS_LS_LABEL_SHIFT) |
- (tc << MPLS_LS_TC_SHIFT) |
- (bos ? (1 << MPLS_LS_S_SHIFT) : 0) |
- (ttl << MPLS_LS_TTL_SHIFT));
- return result;
-}
-
-static inline struct mpls_entry_decoded mpls_entry_decode(struct mpls_shim_hdr *hdr)
-{
- struct mpls_entry_decoded result;
- unsigned entry = be32_to_cpu(hdr->label_stack_entry);
-
- result.label = (entry & MPLS_LS_LABEL_MASK) >> MPLS_LS_LABEL_SHIFT;
- result.ttl = (entry & MPLS_LS_TTL_MASK) >> MPLS_LS_TTL_SHIFT;
- result.tc = (entry & MPLS_LS_TC_MASK) >> MPLS_LS_TC_SHIFT;
- result.bos = (entry & MPLS_LS_S_MASK) >> MPLS_LS_S_SHIFT;
-
- return result;
-}
-
static inline struct mpls_dev *mpls_dev_get(const struct net_device *dev)
{
return rcu_dereference_rtnl(dev->mpls_ptr);
diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
index 6e558a419f60..f8346dfba9c8 100644
--- a/net/mpls/mpls_iptunnel.c
+++ b/net/mpls/mpls_iptunnel.c
@@ -127,7 +127,7 @@ static int mpls_xmit(struct sk_buff *skb)
skb->protocol = htons(ETH_P_MPLS_UC);
/* Push the new labels */
- hdr = mpls_hdr(skb);
+ hdr = skb_mpls_hdr(skb);
bos = true;
for (i = tun_encap_info->labels - 1; i >= 0; i--) {
hdr[i] = mpls_entry_encode(tun_encap_info->label[i],
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 30a5df27116e..c6fdc632e6fb 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -205,7 +205,7 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
skb_reset_mac_header(skb);
skb_set_network_header(skb, skb->mac_len);
- new_mpls_lse = mpls_hdr(skb);
+ new_mpls_lse = skb_mpls_hdr(skb);
new_mpls_lse->label_stack_entry = mpls->mpls_lse;
skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN);
@@ -227,7 +227,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
if (unlikely(err))
return err;
- skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN);
+ skb_postpull_rcsum(skb, skb_mpls_hdr(skb), MPLS_HLEN);
memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
skb->mac_len);
@@ -239,10 +239,10 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
if (ovs_key_mac_proto(key) == MAC_PROTO_ETHERNET) {
struct ethhdr *hdr;
- /* mpls_hdr() is used to locate the ethertype field correctly in the
- * presence of VLAN tags.
+ /* skb_mpls_hdr() is used to locate the ethertype field
+ * correctly in the presence of VLAN tags.
*/
- hdr = (struct ethhdr *)((void *)mpls_hdr(skb) - ETH_HLEN);
+ hdr = (struct ethhdr *)((void *)skb_mpls_hdr(skb) - ETH_HLEN);
update_ethertype(skb, hdr, ethertype);
}
if (eth_p_mpls(skb->protocol))
@@ -263,7 +263,7 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
if (unlikely(err))
return err;
- stack = mpls_hdr(skb);
+ stack = skb_mpls_hdr(skb);
lse = OVS_MASKED(stack->label_stack_entry, *mpls_lse, *mask);
if (skb->ip_summed == CHECKSUM_COMPLETE) {
__be32 diff[] = { ~(stack->label_stack_entry), lse };
--
2.16.2.windows.1
^ permalink raw reply related
* Re: [PATCH net-next] net: bcmgenet: return NULL instead of plain integer
From: Florian Fainelli @ 2018-03-28 16:24 UTC (permalink / raw)
To: Wei Yongjun, Doug Berger; +Cc: netdev, kernel-janitors
In-Reply-To: <1522241479-77691-1-git-send-email-weiyongjun1@huawei.com>
On March 28, 2018 5:51:19 AM PDT, Wei Yongjun <weiyongjun1@huawei.com> wrote:
>Fixes the following sparse warning:
>
>drivers/net/ethernet/broadcom/genet/bcmgenet.c:1351:16: warning:
> Using plain integer as NULL pointer
>
>Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Nicholas Piggin @ 2018-03-28 16:23 UTC (permalink / raw)
To: David Miller
Cc: benh, paulmck, arnd, linux-rdma, linuxppc-dev, linus971,
will.deacon, alexander.duyck, okaya, jgg, David.Laight, oohall,
netdev, alexander.h.duyck, torvalds
In-Reply-To: <20180328.115509.481837809903086401.davem@davemloft.net>
On Wed, 28 Mar 2018 11:55:09 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Thu, 29 Mar 2018 02:13:16 +1100
>
> > Let's fix all archs, it's way easier than fixing all drivers. Half of
> > the archs are unused or dead anyway.
>
> Agreed.
While we're making decrees here, can we do something about mmiowb?
The semantics are basically indecipherable.
This is a variation on the mandatory write barrier that causes writes to weakly
ordered I/O regions to be partially ordered. Its effects may go beyond the
CPU->Hardware interface and actually affect the hardware at some level.
How can a driver writer possibly get that right?
IIRC it was added for some big ia64 system that was really expensive
to implement the proper wmb() semantics on. So wmb() semantics were
quietly downgraded, then the subsequently broken drivers they cared
about were fixed by adding the stronger mmiowb().
What should have happened was wmb and writel remained correct, sane, and
expensive, and they add an mmio_wmb() to order MMIO stores made by the
writel_relaxed accessors, then use that to speed up the few drivers they
care about.
Now that ia64 doesn't matter too much, can we deprecate mmiowb and just
make wmb ordering talk about stores to the device, not to some
intermediate stage of the interconnect where it can be subsequently
reordered wrt the device? Drivers can be converted back to using wmb
or writel gradually.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] test_bpf: Fix NULL vs IS_ERR() check in test_skb_segment()
From: Yonghong Song @ 2018-03-28 16:19 UTC (permalink / raw)
To: Dan Carpenter, Alexei Starovoitov
Cc: Daniel Borkmann, netdev, kernel-janitors
In-Reply-To: <20180328114836.GD29050@mwanda>
On 3/28/18 4:48 AM, Dan Carpenter wrote:
> The skb_segment() function returns error pointers on error. It never
> returns NULL.
>
> Fixes: 76db8087c4c9 ("net: bpf: add a test for skb_segment in test_bpf module")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> index b2badf6b23cd..8e157806df7a 100644
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -6649,7 +6649,7 @@ static __init int test_skb_segment(void)
> }
>
> segs = skb_segment(skb, features);
> - if (segs) {
> + if (!IS_ERR(segs)) {
> kfree_skb_list(segs);
> ret = 0;
> pr_info("%s: success in skb_segment!", __func__);
Oh, my bad. Thanks for the fix!
Reviewed-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH] sfp: allow cotsworks modules
From: Joe Perches @ 2018-03-28 16:19 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: Andrew Lunn, Florian Fainelli, netdev
In-Reply-To: <20180328104147.GO10980@n2100.armlinux.org.uk>
On Wed, 2018-03-28 at 11:41 +0100, Russell King - ARM Linux wrote:
> On Wed, Mar 28, 2018 at 03:33:57AM -0700, Joe Perches wrote:
> > On Wed, 2018-03-28 at 11:18 +0100, Russell King wrote:
> > > Cotsworks modules fail the checksums - it appears that Cotsworks
> > > reprograms the EEPROM at the end of production with the final product
> > > information (serial, date code, and exact part number for module
> > > options) and fails to update the checksum.
> >
> > trivia:
> >
> > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> >
> > []
> > > @@ -574,23 +575,43 @@ static int sfp_sm_mod_probe(struct sfp *sfp)
> >
> > []
> > > + if (cotsworks) {
> > > + dev_warn(sfp->dev,
> > > + "EEPROM base structure checksum failure (0x%02x != 0x%02x)\n",
> > > + check, id.base.cc_base);
> > > + } else {
> > > + dev_err(sfp->dev,
> > > + "EEPROM base structure checksum failure: 0x%02x != 0x%02x\n",
> >
> > It'd be better to move this above the if and
> > use only a single format string instead of
> > using 2 slightly different formats.
>
> No. I think you've missed the fact that one is a _warning_ the other is
> an _error_ and they are emitted at the appropriate severity. It's not
> just that the format strings are slightly different.
Right. Still nicer to use the same formats.
cheers, Joe
^ permalink raw reply
* Re: [PATCH net-next 1/8] net: phy: Add initial support for Microsemi Ocelot internal PHYs.
From: Alexandre Belloni @ 2018-03-28 16:18 UTC (permalink / raw)
To: Florian Fainelli
Cc: David S . Miller, Allan Nielsen, razvan.stefanescu, po.liu,
Thomas Petazzoni, Andrew Lunn, netdev, devicetree, linux-kernel,
linux-mips, Raju Lakkaraju
In-Reply-To: <1caa7359-9ca3-be22-fae0-475daf8b220f@gmail.com>
On 23/03/2018 at 14:08:10 -0700, Florian Fainelli wrote:
> On 03/23/2018 01:11 PM, Alexandre Belloni wrote:
> > Add Microsemi Ocelot internal PHY ids. For now, simply use the genphy
> > functions but more features are available.
> >
> > Cc: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> > ---
> > drivers/net/phy/mscc.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> > index 650c2667d523..e1ab3acd1cdb 100644
> > --- a/drivers/net/phy/mscc.c
> > +++ b/drivers/net/phy/mscc.c
> > @@ -91,6 +91,7 @@ enum rgmii_rx_clock_delay {
> > #define SECURE_ON_PASSWD_LEN_4 0x4000
> >
> > /* Microsemi PHY ID's */
> > +#define PHY_ID_OCELOT 0x00070540
> > #define PHY_ID_VSC8530 0x00070560
> > #define PHY_ID_VSC8531 0x00070570
> > #define PHY_ID_VSC8540 0x00070760
> > @@ -658,6 +659,19 @@ static int vsc85xx_probe(struct phy_device *phydev)
> >
> > /* Microsemi VSC85xx PHYs */
> > static struct phy_driver vsc85xx_driver[] = {
> > +{
> > + .phy_id = PHY_ID_OCELOT,
> > + .name = "Microsemi OCELOT",
> > + .phy_id_mask = 0xfffffff0,
> > + .features = PHY_GBIT_FEATURES,
> > + .soft_reset = &genphy_soft_reset,
> > + .config_init = &genphy_config_init,
> > + .config_aneg = &genphy_config_aneg,
> > + .aneg_done = &genphy_aneg_done,
> > + .read_status = &genphy_read_status,
> > + .suspend = &genphy_suspend,
> > + .resume = &genphy_resume,
>
> With the exception of config_init(), suspend and resume, everything else
> is already the default when you don't provide a callback. To echo to
> what Andrew wrote already, if the purpose is just to show a nice name,
> and do nothing else, consider using the Generic PHY driver (default).
Ok, I'll drop this patch for now, until we handle more features for that
PHY.
--
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* RE: RFC on writel and writel_relaxed
From: David Laight @ 2018-03-28 16:16 UTC (permalink / raw)
To: 'Benjamin Herrenschmidt', Will Deacon
Cc: Linus Torvalds, Alexander Duyck, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <1522249950.21446.23.camel@kernel.crashing.org>
From: Benjamin Herrenschmidt
> Sent: 28 March 2018 16:13
...
> > I've always wondered exactly what the twi;isync were for - always seemed
> > very heavy handed for most mmio reads.
> > Particularly if you are doing mmio reads from a data fifo.
>
> If you do that you should use the "s" version of the accessors. Those
> will only do the above trick at the end of the access series. Also a
> FIFO needs special care about endianness anyway, so you should use
> those accessors regardless. (Hint: you never endian swap a FIFO even on
> BE on a LE device, unless something's been wired very badly in HW).
That was actually a 64 bit wide fifo connected to a 16bit wide PIO interface.
Reading the high address 'clocked' the fifo.
So the first 3 reads could happen in any order, but the 4th had to be last.
This is a small ppc and we shovel a lot of data through that fifo.
Whether it needed byteswapping depended completely on how our hardware people
had built the pcb (not made easy by some docs using the ibm bit numbering).
In fact it didn't....
While that driver only had to run on a very specific small ppc, generic drivers
might have similar issues.
I suspect that writel() is always (or should always be):
barrier_before_writel()
writel_relaxed()
barrier_after_writel()
So if a driver needs to do multiple writes (without strong ordering)
it should be able to repeat the writel_relaxed() with only one set
of barriers.
Similarly for readl().
In addition a lesser barrier is probably enough between a readl_relaxed()
and a writel_relaxed() that is conditional on the read value.
David
^ permalink raw reply
* [PATCH] gfs2: Stop using rhashtable_walk_peek
From: Andreas Gruenbacher @ 2018-03-28 16:00 UTC (permalink / raw)
To: cluster-devel
Cc: Andreas Gruenbacher, netdev, linux-kernel, NeilBrown, Thomas Graf,
Herbert Xu, Tom Herbert
Function rhashtable_walk_peek is problematic because there is no
guarantee that the glock previously returned still exists; when that key
is deleted, rhashtable_walk_peek can end up returning a different key,
which would cause an inconsistent glock dump. So instead of using
rhashtable_walk_peek, keep track of the current glock in the seq file
iterator functions.
Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
fs/gfs2/glock.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 82fb5583445c..f1fc353875d3 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -55,6 +55,7 @@ struct gfs2_glock_iter {
struct gfs2_sbd *sdp; /* incore superblock */
struct rhashtable_iter hti; /* rhashtable iterator */
struct gfs2_glock *gl; /* current glock struct */
+ bool gl_held;
loff_t last_pos; /* last position */
};
@@ -1923,9 +1924,11 @@ void gfs2_glock_exit(void)
static void gfs2_glock_iter_next(struct gfs2_glock_iter *gi, loff_t n)
{
- if (n == 0)
- gi->gl = rhashtable_walk_peek(&gi->hti);
- else {
+ if (n != 0 || !gi->gl) {
+ if (gi->gl_held) {
+ gfs2_glock_queue_put(gi->gl);
+ gi->gl_held = false;
+ }
gi->gl = rhashtable_walk_next(&gi->hti);
n--;
}
@@ -1988,7 +1991,10 @@ static void gfs2_glock_seq_stop(struct seq_file *seq, void *iter_ptr)
{
struct gfs2_glock_iter *gi = seq->private;
- gi->gl = NULL;
+ if (gi->gl) {
+ lockref_get(&gi->gl->gl_lockref);
+ gi->gl_held = true;
+ }
rhashtable_walk_stop(&gi->hti);
}
@@ -2061,6 +2067,7 @@ static int __gfs2_glocks_open(struct inode *inode, struct file *file,
*/
gi->last_pos = -1;
gi->gl = NULL;
+ gi->gl_held = false;
rhashtable_walk_enter(&gl_hash_table, &gi->hti);
}
return ret;
@@ -2076,7 +2083,8 @@ static int gfs2_glocks_release(struct inode *inode, struct file *file)
struct seq_file *seq = file->private_data;
struct gfs2_glock_iter *gi = seq->private;
- gi->gl = NULL;
+ if (gi->gl_held)
+ gfs2_glock_put(gi->gl);
rhashtable_walk_exit(&gi->hti);
return seq_release_private(inode, file);
}
--
2.14.3
^ permalink raw reply related
* Re: RFC on writel and writel_relaxed
From: David Miller @ 2018-03-28 15:55 UTC (permalink / raw)
To: benh
Cc: okaya, torvalds, alexander.duyck, will.deacon, arnd, jgg,
David.Laight, oohall, linuxppc-dev, linux-rdma, alexander.h.duyck,
paulmck, netdev, linus971
In-Reply-To: <1522249996.21446.25.camel@kernel.crashing.org>
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu, 29 Mar 2018 02:13:16 +1100
> Let's fix all archs, it's way easier than fixing all drivers. Half of
> the archs are unused or dead anyway.
Agreed.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 15:13 UTC (permalink / raw)
To: okaya, Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Arnd Bergmann, Jason Gunthorpe,
David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT), linux-rdma,
Alexander Duyck, Paul E. McKenney, netdev, linus971
In-Reply-To: <b02cc752d90c5e5ae2cc8cc4f67429a7@codeaurora.org>
On Wed, 2018-03-28 at 07:41 -0400, okaya@codeaurora.org wrote:
> Yes, we want to get there indeed. It is because of some arch not
> implementing writel properly. Maintainers want to play safe.
>
> That is why I asked if IA64 and other well known archs follow the
> strongly ordered rule at this moment like PPC and ARM.
>
> Or should we go and inform every arch about this before yanking wmb()?
>
> Maintainers are afraid of introducing a regression.
Let's fix all archs, it's way easier than fixing all drivers. Half of
the archs are unused or dead anyway.
> >
> > The above code makes no sense, and just looks stupid to me. It also
> > generates pointlessly bad code on x86, so it's bad there too.
> >
> > Linus
^ permalink raw reply
* Re: [bpf-next PATCH v2 3/4] bpf: sockmap, BPF_F_INGRESS flag for BPF_SK_SKB_STREAM_VERDICT:
From: Daniel Borkmann @ 2018-03-28 15:46 UTC (permalink / raw)
To: John Fastabend, ast; +Cc: netdev, davem
In-Reply-To: <3e7dd92b-a932-d977-2a7e-505b374733df@gmail.com>
On 03/28/2018 05:45 PM, John Fastabend wrote:
> On 03/28/2018 07:21 AM, Daniel Borkmann wrote:
>> On 03/27/2018 07:23 PM, John Fastabend wrote:
>>> Add support for the BPF_F_INGRESS flag in skb redirect helper. To
>>> do this convert skb into a scatterlist and push into ingress queue.
>>> This is the same logic that is used in the sk_msg redirect helper
>>> so it should feel familiar.
>>>
>>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>>> ---
>>> include/linux/filter.h | 1 +
>>> kernel/bpf/sockmap.c | 94 +++++++++++++++++++++++++++++++++++++++---------
>>> net/core/filter.c | 2 +
>>> 3 files changed, 78 insertions(+), 19 deletions(-)
>> [...]
>>> if (!sg->length && md->sg_start == md->sg_end) {
>>> list_del(&md->list);
>>> + if (md->skb)
>>> + consume_skb(md->skb);
>>> kfree(md);
>>> }
>>> }
>>> @@ -1045,27 +1048,72 @@ static int smap_verdict_func(struct smap_psock *psock, struct sk_buff *skb)
>>> __SK_DROP;
>>> }
>>>
>>> +static int smap_do_ingress(struct smap_psock *psock, struct sk_buff *skb)
>>> +{
>>> + struct sock *sk = psock->sock;
>>> + int copied = 0, num_sg;
>>> + struct sk_msg_buff *r;
>>> +
>>> + r = kzalloc(sizeof(struct sk_msg_buff), __GFP_NOWARN | GFP_ATOMIC);
>>> + if (unlikely(!r))
>>> + return -EAGAIN;
>>> +
>>> + if (!sk_rmem_schedule(sk, skb, skb->len)) {
>>> + kfree(r);
>>> + return -EAGAIN;
>>> + }
>>> + sk_mem_charge(sk, skb->len);
>>
>> Usually mem accounting is based on truesize. This is not done here since
>> you need the exact length of the skb for the sg list later on, right?
>
> Correct.
>
>>
>>> + sg_init_table(r->sg_data, MAX_SKB_FRAGS);
>>> + num_sg = skb_to_sgvec(skb, r->sg_data, 0, skb->len);
>>> + if (unlikely(num_sg < 0)) {
>>> + kfree(r);
>>
>> Don't we need to undo the mem charge here in case of error?
>>
>
> Actually, I'll just move the sk_mem_charge() down below this error
> then we don't need to unwind it.
Agree, good point.
^ permalink raw reply
* Re: NFS mounts failing when keytab present on client
From: J. Bruce Fields @ 2018-03-28 15:46 UTC (permalink / raw)
To: Eric Biggers
Cc: Michael Young, Herbert Xu, Jeff Layton, Trond Myklebust,
Anna Schumaker, linux-nfs, netdev, linux-kernel
In-Reply-To: <20180327222950.GB257332@google.com>
On Tue, Mar 27, 2018 at 03:29:50PM -0700, Eric Biggers wrote:
> Hi Michael,
>
> On Tue, Mar 27, 2018 at 11:06:14PM +0100, Michael Young wrote:
> > NFS mounts stopped working on one of my computers after a kernel update from
> > 4.15.3 to 4.15.4. I traced the problem to the commit
> > [46e8d06e423c4f35eac7a8b677b713b3ec9b0684] crypto: hash - prevent using
> > keyed hashes without setting key
> > and a later kernel with this patch reverted works normally.
> >
> > The problem seems to be related to kerberos as the mount fails when the
> > keytab is present, but works if I rename the keytab file. This is true even
> > though the mount is with sec=sys . The mount should also work with sec=krb5
> > but that also fails in the same way. When the mount fails there are errors
> > in dmesg like
> > [ 1232.522816] gss_marshal: gss_get_mic FAILED (851968)
> > [ 1232.522819] RPC: couldn't encode RPC header, exit EIO
> > [ 1232.522856] gss_marshal: gss_get_mic FAILED (851968)
> > [ 1232.522857] RPC: couldn't encode RPC header, exit EIO
> > [ 1232.522863] NFS: nfs4_discover_server_trunking unhandled error -5.
> > Exiting with error EIO
> > [ 1232.525039] gss_marshal: gss_get_mic FAILED (851968)
> > [ 1232.525042] RPC: couldn't encode RPC header, exit EIO
> >
> > Michael Young
>
> Thanks for the bug report. I think the error is coming from
> net/sunrpc/auth_gss/gss_krb5_crypto.c. There are two potential problems I see.
> The first one, which is definitely a bug, is that make_checksum_hmac_md5()
> allocates an HMAC transform and request, then does these crypto API calls:
>
> crypto_ahash_init()
> crypto_ahash_setkey()
> crypto_ahash_digest()
>
> This is wrong because it makes no sense to init() the HMAC request before the
> key has been set, and doubly so when it's calling digest() which is shorthand
> for init() + update() + final(). So I think it just needs to be removed. You
> can test the following patch:
When was this introduced?
3b5cf20cf439 "sunrpc: Use skcipher and ahash/shash"
- probably not, assuming the above was still just as wrong with
crypto_hash_{init,setkey,digest} as it is with
crypto_ahash_{init,setkey,digest}
So I'm guessing it was wrong from the start when it was added by
fffdaef2eb4a "gss_krb5: Add support for rc4-hmac encryption" 8 years
ago. Wonder why it took this long to notice? Did something else
change?
--b.
>
> diff --git a/net/sunrpc/auth_gss/gss_krb5_crypto.c b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> index 12649c9fedab..8654494b4d0a 100644
> --- a/net/sunrpc/auth_gss/gss_krb5_crypto.c
> +++ b/net/sunrpc/auth_gss/gss_krb5_crypto.c
> @@ -237,9 +237,6 @@ make_checksum_hmac_md5(struct krb5_ctx *kctx, char *header, int hdrlen,
>
> ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_SLEEP, NULL, NULL);
>
> - err = crypto_ahash_init(req);
> - if (err)
> - goto out;
> err = crypto_ahash_setkey(hmac_md5, cksumkey, kctx->gk5e->keylength);
> if (err)
> goto out;
>
> If that's not it, it's also possible that the error is coming from the
> crypto_ahash_init() in make_checksum(). That can only happen if 'cksumkey' is
> NULL and the hash algorithm is keyed, which implies a logical error as it
> doesn't make sense to use a keyed hash algorithm without the key. The callers
> do check kctx->gk5e->keyed_cksum which I'd hope would prevent this, though
> perhaps kctx->cksum can be NULL.
>
> Eric
^ permalink raw reply
* Re: [bpf-next PATCH v2 3/4] bpf: sockmap, BPF_F_INGRESS flag for BPF_SK_SKB_STREAM_VERDICT:
From: John Fastabend @ 2018-03-28 15:45 UTC (permalink / raw)
To: Daniel Borkmann, ast; +Cc: netdev, davem
In-Reply-To: <71a13f21-6886-85c3-6911-8ac33c486901@iogearbox.net>
On 03/28/2018 07:21 AM, Daniel Borkmann wrote:
> On 03/27/2018 07:23 PM, John Fastabend wrote:
>> Add support for the BPF_F_INGRESS flag in skb redirect helper. To
>> do this convert skb into a scatterlist and push into ingress queue.
>> This is the same logic that is used in the sk_msg redirect helper
>> so it should feel familiar.
>>
>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>> ---
>> include/linux/filter.h | 1 +
>> kernel/bpf/sockmap.c | 94 +++++++++++++++++++++++++++++++++++++++---------
>> net/core/filter.c | 2 +
>> 3 files changed, 78 insertions(+), 19 deletions(-)
> [...]
>> if (!sg->length && md->sg_start == md->sg_end) {
>> list_del(&md->list);
>> + if (md->skb)
>> + consume_skb(md->skb);
>> kfree(md);
>> }
>> }
>> @@ -1045,27 +1048,72 @@ static int smap_verdict_func(struct smap_psock *psock, struct sk_buff *skb)
>> __SK_DROP;
>> }
>>
>> +static int smap_do_ingress(struct smap_psock *psock, struct sk_buff *skb)
>> +{
>> + struct sock *sk = psock->sock;
>> + int copied = 0, num_sg;
>> + struct sk_msg_buff *r;
>> +
>> + r = kzalloc(sizeof(struct sk_msg_buff), __GFP_NOWARN | GFP_ATOMIC);
>> + if (unlikely(!r))
>> + return -EAGAIN;
>> +
>> + if (!sk_rmem_schedule(sk, skb, skb->len)) {
>> + kfree(r);
>> + return -EAGAIN;
>> + }
>> + sk_mem_charge(sk, skb->len);
>
> Usually mem accounting is based on truesize. This is not done here since
> you need the exact length of the skb for the sg list later on, right?
Correct.
>
>> + sg_init_table(r->sg_data, MAX_SKB_FRAGS);
>> + num_sg = skb_to_sgvec(skb, r->sg_data, 0, skb->len);
>> + if (unlikely(num_sg < 0)) {
>> + kfree(r);
>
> Don't we need to undo the mem charge here in case of error?
>
Actually, I'll just move the sk_mem_charge() down below this error
then we don't need to unwind it.
>> + return num_sg;
>> + }
>> + copied = skb->len;
>> + r->sg_start = 0;
>> + r->sg_end = num_sg == MAX_SKB_FRAGS ? 0 : num_sg;
>> + r->skb = skb;
>> + list_add_tail(&r->list, &psock->ingress);
>> + sk->sk_data_ready(sk);
>> + return copied;
>> +}
^ permalink raw reply
* Re: [PATCH v3 2/4] bus: fsl-mc: add restool userspace support
From: Arnd Bergmann @ 2018-03-28 15:43 UTC (permalink / raw)
To: Ioana Ciornei
Cc: gregkh, Laurentiu Tudor, Linux Kernel Mailing List, Stuart Yoder,
Ruxandra Ioana Ciocoi Radulescu, Razvan Stefanescu, Roy Pledge,
Networking
In-Reply-To: <HE1PR04MB32126A07A4B4CC7225E661E4E0A30@HE1PR04MB3212.eurprd04.prod.outlook.com>
On Wed, Mar 28, 2018 at 4:27 PM, Ioana Ciornei <ioana.ciornei@nxp.com> wrote:
> Hi,
>
>>
>> Hi Ioana,
>>
>> So this driver is a direct passthrough to your hardware for passing fixed-
>> length command/response pairs. Have you considered using a higher-level
>> interface instead?
>>
>> Can you list some of the commands that are passed here as clarification, and
>> explain what the tradeoffs are that have led to adopting a low-level interface
>> instead of a high-level interface?
>>
>> The main downside of the direct passthrough obviously is that you tie your
>> user space to a particular hardware implementation, while a high-level
>> abstraction could in principle work across a wider range of hardware revisions
>> or even across multiple vendors implementing the same concept by different
>> means.
>
> If by "higher-level" you mean an implementation where commands are created by the kernel at userspace's request, then I believe this approach is not really viable because of the sheer number of possible commands that would bloat the driver.
>
> For example, a DPNI (Data Path Network Interface) can be created using a command that has the following structure:
>
> struct dpni_cmd_create {
> uint32_t options;
> uint8_t num_queues;
> uint8_t num_tcs;
> uint8_t mac_filter_entries;
> uint8_t pad1;
> uint8_t vlan_filter_entries;
> uint8_t pad2;
> uint8_t qos_entries;
> uint8_t pad3;
> uint16_t fs_entries;
> };
>
> In the above structure, each field has a meaning that the end-user might want to be able to change according to their particular use-case (not much is left at its default value).
> The same level of complexity is encountered for all the commands that interact with Data Path objects such as DPBP(buffer pools), DPRC(Resource Container) etc.
> You can find more examples of commands in restool's repo: https://github.com/qoriq-open-source/restool/tree/integration/mc_v10
>
> In my opinion, an in-kernel implementation that is equivalent in terms of flexibility will turn
> into a giant ioctl parser, all while also exposing an userspace API that is not as simple/easy to use.
(adding the netdev list)
The command you list there seems to be networking related, so instead of
an ioctl based interface, a high-lever interface would likely use netlink
for consistency with other drivers. Are all commands for networking
or are there some that are talking to the device to do something unrelated?
Obviously creating a high-level interface would be a lot of work in the kernel,
and it only pays off if there are multiple independent users, we wouldn't do
that for just one driver.
I'm still not convinced either way (high-level or low-level
interface), but I think
this needs to be discussed with the networking maintainers. Given the examples
on the github page you linked to, the high-level user space commands
based on these ioctls
ls-addni # adds a network interface
ls-addmux # adds a dpdmux
ls-addsw # adds an l2switch
ls-listmac # lists MACs and their connections
ls-listni # lists network interfaces and their connections
and I see that you also support the switchdev interface in
drivers/staging/fsl-dpaa2, which I think does some of the same
things, presumably by implementing the switchdev API using
fsl_mc_command low-level interfaces in the kernel.
Is that a correct interpretation? If yes, could we extend switchdev
or other networking interfaces to fill in whatever those don't handle
yet?
>> > @@ -14,10 +14,18 @@
>> > * struct fsl_mc_command - Management Complex (MC) command
>> structure
>> > * @header: MC command header
>> > * @params: MC command parameters
>> > + *
>> > + * Used by RESTOOL_SEND_MC_COMMAND
>> > */
>> > struct fsl_mc_command {
>> > __u64 header;
>> > __u64 params[MC_CMD_NUM_OF_PARAMS]; };
>> >
>> > +#define RESTOOL_IOCTL_TYPE 'R'
>> > +#define RESTOOL_IOCTL_SEQ 0xE0
>>
>> I tried to follow the code path into the hardware and am a bit confused about
>> the semantics of the 'header' field and the data. Both are accessed passed to
>> the hardware using
>>
>> writeq(le64_to_cpu(cmd->header))
>>
>> which would indicate a fixed byte layout on the user structure and that it
>> should really be a '__le64' instead of '__u64', or possibly should be
>> represented as '__u8 header[8]' to clarify that the byte ordering is supposed
>> to match the order of the byte addresses of the register.
>>
>
> Indeed, the struct fsl_mc_command should use '__le64' instead of '__u64', so that the UAPI header file clearly states what endianness should the userspace prepare.
> The writeq appeared as a consequence of a previous mail thread https://lkml.org/lkml/2017/7/17/415, because the endianness of the command is handled by the user and the bus should leave the binary blob intact.
Ok. However, with the dpni_cmd_create structure you list above, it
seems that it doesn't actually contain a set of __le64 but rather
an array of bytes, so it might be best to
>> However, the in-kernel usage of that field suggests that we treat it as a 64-bit
>> cpu-endian number, for which the hardware needs to know the endianess of
>> the currently running kernel and user space.
>>
>
> I might have not understood what you are trying to say, but the in-kernel code treats the fsl_mc_command as little endian by using cpu_to_leXX and leXX_to_cpu in order to setup the command structure.
One function that seems to have a problem is this one:
static enum mc_cmd_status mc_cmd_hdr_read_status(struct fsl_mc_command *cmd)
{
struct mc_cmd_header *hdr = (struct mc_cmd_header *)&cmd->header;
return (enum mc_cmd_status)hdr->status;
}
If cmd->header is little-endian here, then the result is not
an 'enum mc_cmd_status'. I think it would be good to change the
types for fsl_mc_command to __le64 first and run everything through
'sparse' with 'make C=1' to find any remaining endianess problems.
Arnd
^ 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