Netdev List
 help / color / mirror / Atom feed
* [PATCH iproute2-next] devlink: add support for updating device flash
From: Jakub Kicinski @ 2019-02-26 20:20 UTC (permalink / raw)
  To: dsahern, jiri, stephen; +Cc: oss-drivers, netdev, Jakub Kicinski

Add new command for updating flash of devices via devlink API.
Example:

$ cp flash-boot.bin /lib/firmware/
$ devlink dev flash pci/0000:05:00.0 file flash-boot.bin

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 devlink/devlink.c      | 54 ++++++++++++++++++++++++++++++++++++++++++
 man/man8/devlink-dev.8 | 32 +++++++++++++++++++++++++
 2 files changed, 86 insertions(+)

diff --git a/devlink/devlink.c b/devlink/devlink.c
index 960cdda99b5b..5c6cac1f76dd 100644
--- a/devlink/devlink.c
+++ b/devlink/devlink.c
@@ -199,6 +199,8 @@ static void ifname_map_free(struct ifname_map *ifname_map)
 #define DL_OPT_REGION_SNAPSHOT_ID	BIT(22)
 #define DL_OPT_REGION_ADDRESS		BIT(23)
 #define DL_OPT_REGION_LENGTH		BIT(24)
+#define DL_OPT_FLASH_FILE_NAME	BIT(25)
+#define DL_OPT_FLASH_COMPONENT	BIT(26)
 
 struct dl_opts {
 	uint32_t present; /* flags of present items */
@@ -230,6 +232,8 @@ struct dl_opts {
 	uint32_t region_snapshot_id;
 	uint64_t region_address;
 	uint64_t region_length;
+	const char *flash_file_name;
+	const char *flash_component;
 };
 
 struct dl {
@@ -1185,6 +1189,20 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
 			if (err)
 				return err;
 			o_found |= DL_OPT_REGION_LENGTH;
+		} else if (dl_argv_match(dl, "file") &&
+			   (o_all & DL_OPT_FLASH_FILE_NAME)) {
+			dl_arg_inc(dl);
+			err = dl_argv_str(dl, &opts->flash_file_name);
+			if (err)
+				return err;
+			o_found |= DL_OPT_FLASH_FILE_NAME;
+		} else if (dl_argv_match(dl, "component") &&
+			   (o_all & DL_OPT_FLASH_COMPONENT)) {
+			dl_arg_inc(dl);
+			err = dl_argv_str(dl, &opts->flash_component);
+			if (err)
+				return err;
+			o_found |= DL_OPT_FLASH_COMPONENT;
 		} else {
 			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
 			return -EINVAL;
@@ -1389,6 +1407,12 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
 	if (opts->present & DL_OPT_REGION_LENGTH)
 		mnl_attr_put_u64(nlh, DEVLINK_ATTR_REGION_CHUNK_LEN,
 				 opts->region_length);
+	if (opts->present & DL_OPT_FLASH_FILE_NAME)
+		mnl_attr_put_strz(nlh, DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,
+				  opts->flash_file_name);
+	if (opts->present & DL_OPT_FLASH_COMPONENT)
+		mnl_attr_put_strz(nlh, DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,
+				  opts->flash_component);
 }
 
 static int dl_argv_parse_put(struct nlmsghdr *nlh, struct dl *dl,
@@ -1451,6 +1475,7 @@ static void cmd_dev_help(void)
 	pr_err("       devlink dev param show [DEV name PARAMETER]\n");
 	pr_err("       devlink dev reload DEV\n");
 	pr_err("       devlink dev info [ DEV ]\n");
+	pr_err("       devlink dev flash DEV file PATH [ component NAME ]\n");
 }
 
 static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
@@ -2583,6 +2608,32 @@ static int cmd_dev_info(struct dl *dl)
 	return err;
 }
 
+static void cmd_dev_flash_help(void)
+{
+	pr_err("Usage: devlink dev flash DEV file PATH [ component NAME ]\n");
+}
+
+static int cmd_dev_flash(struct dl *dl)
+{
+	struct nlmsghdr *nlh;
+	int err;
+
+	if (dl_argv_match(dl, "help") || dl_no_arg(dl)) {
+		cmd_dev_flash_help();
+		return 0;
+	}
+
+	nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_FLASH_UPDATE,
+			       NLM_F_REQUEST | NLM_F_ACK);
+
+	err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE | DL_OPT_FLASH_FILE_NAME,
+				DL_OPT_FLASH_COMPONENT);
+	if (err)
+		return err;
+
+	return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
+}
+
 static int cmd_dev(struct dl *dl)
 {
 	if (dl_argv_match(dl, "help")) {
@@ -2604,6 +2655,9 @@ static int cmd_dev(struct dl *dl)
 	} else if (dl_argv_match(dl, "info")) {
 		dl_arg_inc(dl);
 		return cmd_dev_info(dl);
+	} else if (dl_argv_match(dl, "flash")) {
+		dl_arg_inc(dl);
+		return cmd_dev_flash(dl);
 	}
 	pr_err("Command \"%s\" not found\n", dl_argv(dl));
 	return -ENOENT;
diff --git a/man/man8/devlink-dev.8 b/man/man8/devlink-dev.8
index 47838371fecd..1804463b2321 100644
--- a/man/man8/devlink-dev.8
+++ b/man/man8/devlink-dev.8
@@ -69,6 +69,16 @@ devlink-dev \- devlink device configuration
 .IR DEV
 .RI "]"
 
+.ti -8
+.BR "devlink dev flash"
+.IR DEV
+.BR file
+.IR PATH
+.RI "["
+.BR target
+.IR ID
+.RI "]"
+
 .SH "DESCRIPTION"
 .SS devlink dev show - display devlink device attributes
 
@@ -177,6 +187,28 @@ versions may differ after flash has been updated, but before reboot.
 - specifies the devlink device to show.
 If this argument is omitted all devices are listed.
 
+.SS devlink dev flash - write device's non-volatile memory.
+
+.PP
+.I "DEV"
+- specifies the devlink device to write to.
+
+.BR file
+.I PATH
+- Path to the file which will be written into device's flash. The path needs
+to be relative to one of the directories searched by the kernel firmware loaded,
+such as /lib/firmware.
+
+.BR component
+.I NAME
+- If device stores multiple firmware images in non-volatile memory, this
+parameter may be used to indicate which firmware image should be written.
+The value of
+.I NAME
+should match the component names from
+.B "devlink dev info"
+and may be driver-dependent.
+
 .SH "EXAMPLES"
 .PP
 devlink dev show
-- 
2.19.2


^ permalink raw reply related

* Re: [PATCH 0/3] soc: fsl: dpio: enable and configure cache stashing
From: David Miller @ 2019-02-26 20:31 UTC (permalink / raw)
  To: leoyang.li
  Cc: ioana.ciornei, roy.pledge, ruxandra.radulescu, laurentiu.tudor,
	horia.geanta, brouer, netdev, linux-kernel
In-Reply-To: <CADRPPNSRpbD_dUZYJC4-SwudNk2JTPCug3T6g4dTYMBDQeeu7w@mail.gmail.com>

From: Li Yang <leoyang.li@nxp.com>
Date: Tue, 26 Feb 2019 13:03:17 -0600

> On Mon, Feb 25, 2019 at 1:01 AM Ioana Ciornei <ioana.ciornei@nxp.com> wrote:
>>
>> > Subject: Re: [PATCH 0/3] soc: fsl: dpio: enable and configure cache stashing
>> >
>> > From: Ioana Ciornei <ioana.ciornei@nxp.com>
>> > Date: Sat, 23 Feb 2019 08:48:42 +0000
>> >
>> > > The first two patches enable cache stashing and configure the core
>> > > cluster destination per software portal while the third patch is the
>> > > one configuring the amount of stashing on a queue.
>> >
>> > Should I merge this series in via my networking tree?
>>
>> Even though it would be really good to have this on the networking tree as soon as possible, I am afraid that patch 2/3 is not going to apply cleanly because it touches the same areas of code that some other patches on Leo's tree are.
> 
> Hi David,
> 
> What shall we do if we want to get this in the 5.1?  Since the change
> on the ethernet driver is pretty small, can I get your ACK on that
> patch and merge the series through my tree?

Sure.

Acked-by: David S. Miller <davem@davemloft.net>

^ permalink raw reply

* Re: [PATCH net 4/4] tls: Fix tls_device receive
From: Dave Watson @ 2019-02-26 20:34 UTC (permalink / raw)
  To: Boris Pismenny
  Cc: aviadye@mellanox.com, john.fastabend@gmail.com,
	daniel@iogearbox.net, vakul.garg@nxp.com, netdev@vger.kernel.org,
	eranbe@mellanox.com
In-Reply-To: <20190226121235.20784-5-borisp@mellanox.com>

On 02/26/19 02:12 PM, Boris Pismenny wrote:
> Currently, the receive function fails to handle records already
> decrypted by the device due to the commit mentioned below.
> 
> This commit advances the TLS record sequence number and prepares the context
> to handle the next record.
> 
> Fixes: fedf201e1296 ("net: tls: Refactor control message handling on recv")
> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com>
> ---
>  net/tls/tls_sw.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index f515cd7e984e..85da10182d8d 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
>  
>  			return err;
>  		}
> -
> -		rxm->full_len -= padding_length(ctx, tls_ctx, skb);
> -
> -		rxm->offset += prot->prepend_size;
> -		rxm->full_len -= prot->overhead_size;
> -		tls_advance_record_sn(sk, &tls_ctx->rx, version);
> -		ctx->decrypted = true;
> -		ctx->saved_data_ready(sk);
>  	} else {
>  		*zc = false;
>  	}
>  
> +	rxm->full_len -= padding_length(ctx, tls_ctx, skb);
> +	rxm->offset += prot->prepend_size;
> +	rxm->full_len -= prot->overhead_size;
> +	tls_advance_record_sn(sk, &tls_ctx->rx, version);
> +	ctx->decrypted = true;
> +	ctx->saved_data_ready(sk);
> +
>  	return err;
>  }

This breaks the tls.control_msg test:

  [ RUN      ] tls.control_msg
  tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len) (18446744073709551614) == 0 (0)
  tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len) (18446744073709551614) == 0 (0)
  tls.control_msg: Test failed at step #8

So either control message handling needs to only call
decrypt_skb_update once, or we need a new flag or something to handle
the device case

^ permalink raw reply

* RE: [RFC v1 10/19] RDMA/irdma: Add connection manager
From: Saleem, Shiraz @ 2019-02-26 21:07 UTC (permalink / raw)
  To: 'Jason Gunthorpe', Gal Pressman
  Cc: dledford@redhat.com, davem@davemloft.net,
	linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
	Ismail, Mustafa, Kirsher, Jeffrey T
In-Reply-To: <20190225184629.GC21863@ziepe.ca>

>Subject: Re: [RFC v1 10/19] RDMA/irdma: Add connection manager
>
>On Sun, Feb 24, 2019 at 01:21:16PM +0200, Gal Pressman wrote:
>> On 15-Feb-19 19:10, Shiraz Saleem wrote:
>> > +/**
>> > + * irdma_cm_teardown_connections - teardown QPs
>> > + * @iwdev: device pointer
>> > + * @ipaddr: Pointer to IPv4 or IPv6 address
>> > + * @ipv4: flag indicating IPv4 when true
>>
>> There is no ipv4 parameter.
>
>Be sure to run code through make W=1 - it runs stuff that checks the kdocs.

OK. Will do. Thanks! We have a few hits here.

>
>> > +	INIT_LIST_HEAD(&teardown_list);
>> > +	for (i = 0; i < IRDMA_MAX_USER_PRIORITY; i++) {
>> > +		spin_lock_irqsave(&vsi->qos[i].lock, flags);
>> > +		list_for_each_safe(list_node, list_core_temp, &vsi->qos[i].qplist) {
>> > +			u32 qp_ip[4];
>> > +
>> > +			sc_qp = container_of(list_node, struct irdma_sc_qp, list);
>> > +			if (sc_qp->qp_type != IRDMA_QP_TYPE_ROCE_RC)
>> > +				continue;
>> > +
>> > +			qp = sc_qp->back_qp;
>> > +			if (!disconnect_all) {
>> > +				if (nfo->ipv4)
>> > +					qp_ip[0] = qp->udp_info.local_ipaddr3;
>> > +				else
>> > +					memcpy(qp_ip,
>> > +					       &qp->udp_info.local_ipaddr0,
>> > +					       sizeof(qp_ip));
>> > +			}
>> > +
>> > +			if (disconnect_all ||
>> > +			    (nfo->vlan_id == qp->udp_info.vlan_tag &&
>> > +			    !memcmp(qp_ip, ipaddr, nfo->ipv4 ? 4 : 16))) {
>> > +				spin_lock_irqsave(&iwdev->rf->qptable_lock,
>flags);
>>
>> You should use different 'flags' here.
>
>If irqs are already proven disabled it is just spin_lock, right?
>
Correct.

^ permalink raw reply

* RE: [RFC v1 04/19] RDMA/irdma: Add driver framework definitions
From: Saleem, Shiraz @ 2019-02-26 21:08 UTC (permalink / raw)
  To: 'Gal Pressman', dledford@redhat.com, jgg@ziepe.ca,
	davem@davemloft.net
  Cc: linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
	Ismail, Mustafa, Kirsher, Jeffrey T
In-Reply-To: <5215c158-2935-ef45-db49-44caa84e07d3@amazon.com>

>Subject: Re: [RFC v1 04/19] RDMA/irdma: Add driver framework definitions
>
>On 15-Feb-19 19:10, Shiraz Saleem wrote:
>> +/* client interface functions */
>> +static const struct i40e_client_ops i40e_ops = {
>> +	.open = i40iw_open,
>> +	.close = i40iw_close,
>> +	.l2_param_change = i40iw_l2param_change,
>> +	.virtchnl_receive = NULL,
>> +	.vf_reset = NULL,
>> +	.vf_enable = NULL,
>> +	.vf_capable = NULL
>
>NULL assignments are redundant.
>
>> +};
>> +
>> diff --git a/drivers/infiniband/hw/irdma/irdma_if.c
>> b/drivers/infiniband/hw/irdma/irdma_if.c
>> new file mode 100644
>> index 0000000..f7b89e9
>> --- /dev/null
>> +++ b/drivers/infiniband/hw/irdma/irdma_if.c
>> @@ -0,0 +1,430 @@
>> +// SPDX-License-Identifier: GPL-2.0 or Linux-OpenIB
>> +/* Copyright (c) 2019, Intel Corporation. */
>> +
>> +#include <linux/module.h>
>> +#include <linux/moduleparam.h>
>> +#include <ice_idc.h>
>> +#include "main.h"
>> +#include "ws.h"
>> +#include "icrdma_hw.h"
>> +
>> +void irdma_add_dev_ref(struct irdma_sc_dev *dev) {
>> +	try_module_get(THIS_MODULE);
>> +}
>> +
>> +void irdma_put_dev_ref(struct irdma_sc_dev *dev) {
>> +	module_put(THIS_MODULE);
>> +}
>
>What are these used for?
>
>> +
>> +/**
>> + * irdma_find_iwdev - find a vsi device given a name
>> + * @name: name of iwdev
>> + */
>
>Can't find uses of this function as well.
>
>> +struct irdma_device *irdma_find_iwdev(const char *name) {
>> +	struct irdma_handler *hdl;
>> +	struct list_head *pos;
>> +	struct list_head *tmp;
>> +	struct irdma_device *iwdev;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&irdma_handler_lock, flags);
>> +	list_for_each_entry(hdl, &irdma_handlers, list) {
>> +		list_for_each_safe(pos, tmp, &hdl->rf.vsi_dev_list) {
>> +			iwdev = container_of(pos, struct irdma_device, list);
>> +			if (!strcmp(name, iwdev->iwibdev->ibdev.name)) {
>> +				spin_unlock_irqrestore(&irdma_handler_lock,
>> +						       flags);
>> +				return iwdev;
>> +			}
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&irdma_handler_lock, flags);
>> +
>> +	return NULL;
>> +}
>> +

Will all fix 3 comments. Thanks!

^ permalink raw reply

* RE: [RFC v1 12/19] RDMA/irdma: Implement device supported verb APIs
From: Saleem, Shiraz @ 2019-02-26 21:09 UTC (permalink / raw)
  To: Gal Pressman, dledford@redhat.com, jgg@ziepe.ca,
	davem@davemloft.net
  Cc: linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
	Ismail, Mustafa, Kirsher, Jeffrey T, Yossi Leybovich
In-Reply-To: <01b0d571-81d8-ed6c-77b7-e83ee0ab9caa@amazon.com>

>Subject: Re: [RFC v1 12/19] RDMA/irdma: Implement device supported verb APIs
>
>On 15-Feb-19 19:10, Shiraz Saleem wrote:
>> /**
>>  * irdma_dealloc_ucontext - deallocate the user context data structure
>>  * @context: user context created during alloc  */ static int
>> irdma_dealloc_ucontext(struct ib_ucontext *context) {
>> 	struct irdma_ucontext *ucontext = to_ucontext(context);
>> 	unsigned long flags;
>>
>> 	spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags);
>> 	if (!list_empty(&ucontext->cq_reg_mem_list)) {
>> 		spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
>> 		return -EBUSY;
>> 	}
>> 	spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
>>
>> 	spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
>> 	if (!list_empty(&ucontext->qp_reg_mem_list)) {
>> 		spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
>> 		return -EBUSY;
>
>Drivers are not permitted to fail dealloc_ucontext.

This is fixed in RFC v1 submission. Maybe this was pasted from the v0 ver?

[..]

>> +/**
>> + * irdma_alloc_pd - allocate protection domain
>> + * @pd: PD pointer
>> + * @context: user context created during alloc
>> + * @udata: user data
>> + */
>> +static int irdma_alloc_pd(struct ib_pd *pd,
>> +			  struct ib_ucontext *context,
>> +			  struct ib_udata *udata)
>> +{
>> +	struct irdma_pd *iwpd = to_iwpd(pd);
>> +	struct irdma_device *iwdev = to_iwdev(pd->device);
>> +	struct irdma_sc_dev *dev = &iwdev->rf->sc_dev;
>> +	struct irdma_pci_f *rf = iwdev->rf;
>> +	struct irdma_alloc_pd_resp uresp = {};
>> +	struct irdma_sc_pd *sc_pd;
>> +	struct irdma_ucontext *ucontext;
>> +	u32 pd_id = 0;
>> +	int err;
>> +
>> +	if (iwdev->closing)
>> +		return -ENODEV;
>> +
>> +	err = irdma_alloc_rsrc(rf, rf->allocated_pds, rf->max_pd, &pd_id,
>> +			       &rf->next_pd);
>> +	if (err)
>> +		return err;
>> +
>> +	sc_pd = &iwpd->sc_pd;
>> +	if (context) {
>
>I think this should be 'if (udata)', this applies to many other places in this driver.

That’s right. Will fix it.

>
>> +		ucontext = to_ucontext(context);
>> +		dev->iw_pd_ops->pd_init(dev, sc_pd, pd_id, ucontext->abi_ver);
>> +		uresp.pd_id = pd_id;
>> +		if (ib_copy_to_udata(udata, &uresp, sizeof(uresp))) {
>> +			err = -EFAULT;
>> +			goto error;
>> +		}
>> +	} else {
>> +		dev->iw_pd_ops->pd_init(dev, sc_pd, pd_id, -1);
>> +	}
>> +
>> +	irdma_add_pdusecount(iwpd);
>> +
>> +	return 0;
>> +error:
>> +	irdma_free_rsrc(rf, rf->allocated_pds, pd_id);
>> +
>> +	return err;
>> +}
>> +/**
>> + * irdma_create_qp - create qp
>> + * @ibpd: ptr of pd
>> + * @init_attr: attributes for qp
>> + * @udata: user data for create qp
>> + */
>> +static struct ib_qp *irdma_create_qp(struct ib_pd *ibpd,
>> +				     struct ib_qp_init_attr *init_attr,
>> +				     struct ib_udata *udata)
>> +{
>> +	struct irdma_pd *iwpd = to_iwpd(ibpd);
>> +	struct irdma_device *iwdev = to_iwdev(ibpd->device);
>> +	struct irdma_pci_f *rf = iwdev->rf;
>> +	struct irdma_cqp *iwcqp = &rf->cqp;
>> +	struct irdma_qp *iwqp;
>> +	struct irdma_ucontext *ucontext;
>> +	struct irdma_create_qp_req req;
>> +	struct irdma_create_qp_resp uresp = {};
>> +	struct i40iw_create_qp_resp uresp_gen1 = {};
>> +	u32 qp_num = 0;
>> +	void *mem;
>> +	enum irdma_status_code ret;
>> +	int err_code = 0;
>> +	int sq_size;
>> +	int rq_size;
>> +	struct irdma_sc_qp *qp;
>> +	struct irdma_sc_dev *dev = &rf->sc_dev;
>> +	struct irdma_qp_init_info init_info = {};
>> +	struct irdma_create_qp_info *qp_info;
>> +	struct irdma_cqp_request *cqp_request;
>> +	struct cqp_cmds_info *cqp_info;
>> +	struct irdma_qp_host_ctx_info *ctx_info;
>> +	struct irdma_iwarp_offload_info *iwarp_info;
>> +	struct irdma_roce_offload_info *roce_info;
>> +	struct irdma_udp_offload_info *udp_info;
>> +	unsigned long flags;
>> +
>> +	if (iwdev->closing)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	if (init_attr->create_flags)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	if (init_attr->cap.max_inline_data > dev->hw_attrs.max_hw_inline)
>> +		init_attr->cap.max_inline_data = dev->hw_attrs.max_hw_inline;
>> +
>> +	if (init_attr->cap.max_send_sge > dev->hw_attrs.max_hw_wq_frags)
>> +		init_attr->cap.max_send_sge = dev-
>>hw_attrs.max_hw_wq_frags;
>> +
>> +	if (init_attr->cap.max_recv_sge > dev->hw_attrs.max_hw_wq_frags)
>> +		init_attr->cap.max_recv_sge = dev->hw_attrs.max_hw_wq_frags;
>
>AFAIK, you can change the requested values to be greater than or equal to the
>values requested. I don't think you can change them to something smaller.

Hmm...This is a sanity check to make sure we don’t exceed the device supported values.
But we should fail the call.

[..]

>> +	mem = kzalloc(sizeof(*iwqp), GFP_KERNEL);
>> +	if (!mem)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	iwqp = (struct irdma_qp *)mem;
>> +	iwqp->allocated_buf = mem;
>
>'allocated_buf' feels redundant. Why is iwqp not sufficient?

I agree.
[..]

>> +	if (udata) {
>> +		err_code = ib_copy_from_udata(&req, udata, sizeof(req));
>
>Perhaps ib_copy_from_udata(&req, udata, min(sizeof(req), udata->inlen)?
>Applies to other call sites of ib_copy_from/to_udata as well.
>

It’s a good idea.

>> + * irdma_query - query qp attributes
>> + * @ibqp: qp pointer
>> + * @attr: attributes pointer
>> + * @attr_mask: Not used
>> + * @init_attr: qp attributes to return  */ static int
>> +irdma_query_qp(struct ib_qp *ibqp,
>> +			  struct ib_qp_attr *attr,
>> +			  int attr_mask,
>> +			  struct ib_qp_init_attr *init_attr) {
>> +	struct irdma_qp *iwqp = to_iwqp(ibqp);
>> +	struct irdma_sc_qp *qp = &iwqp->sc_qp;
>> +
>> +	attr->qp_state = iwqp->ibqp_state;
>> +	attr->cur_qp_state = iwqp->ibqp_state;
>> +	attr->qp_access_flags = 0;
>> +	attr->cap.max_send_wr = qp->qp_uk.sq_size - 1;
>> +	attr->cap.max_recv_wr = qp->qp_uk.rq_size - 1;
>
>Why -1?

It's reserved for HW. But the equation should be 
(sqdepth - I40IW_SQ_RSVD) >> sqshift.

[....]
>
>> +	attr->cap.max_inline_data = qp->qp_uk.max_inline_data;
>> +	attr->cap.max_send_sge = qp->qp_uk.max_sq_frag_cnt;
>> +	attr->cap.max_recv_sge = qp->qp_uk.max_rq_frag_cnt;
>> +	attr->qkey = iwqp->roce_info.qkey;
>> +
>> +	init_attr->event_handler = iwqp->ibqp.event_handler;
>> +	init_attr->qp_context = iwqp->ibqp.qp_context;
>> +	init_attr->send_cq = iwqp->ibqp.send_cq;
>> +	init_attr->recv_cq = iwqp->ibqp.recv_cq;
>> +	init_attr->srq = iwqp->ibqp.srq;
>> +	init_attr->cap = attr->cap;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * irdma_destroy_cq - destroy cq
>> + * @ib_cq: cq pointer
>> + */
>> +static int irdma_destroy_cq(struct ib_cq *ib_cq) {
>> +	struct irdma_cq *iwcq;
>> +	struct irdma_device *iwdev;
>> +	struct irdma_sc_cq *cq;
>> +
>> +	if (!ib_cq) {
>> +		irdma_pr_err("ib_cq == NULL\n");
>> +		return 0;
>> +	}
>
>Is this really needed? Which caller can pass NULL pointer?

Not needed.

>> +
>> +/**
>> + * board_id_show
>> + */
>> +static ssize_t board_id_show(struct device *dev,
>> +			     struct device_attribute *attr,
>> +			     char *buf)
>> +{
>> +	return sprintf(buf, "%.*s\n", 32, "IRDMA Board ID");
>
>That doesn't add much information.

Will fix.

>
>> +}
>> +
>> +static DEVICE_ATTR_RO(hw_rev);
>> +static DEVICE_ATTR_RO(hca_type);
>> +static DEVICE_ATTR_RO(board_id);
>> +
>> +static struct attribute *irdma_dev_attributes[] = {
>> +	&dev_attr_hw_rev.attr,
>> +	&dev_attr_hca_type.attr,
>> +	&dev_attr_board_id.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group irdma_attr_group = {
>> +	.attrs = irdma_dev_attributes,
>> +};
>> +
>> +/**
>> + * irdma_modify_port  Modify port properties
>> + * @ibdev: device pointer from stack
>> + * @port: port number
>> + * @port_modify_mask: mask for port modifications
>> + * @props: port properties
>> + */
>> +static int irdma_modify_port(struct ib_device *ibdev,
>> +			     u8 port,
>> +			     int port_modify_mask,
>> +			     struct ib_port_modify *props) {
>> +	return 0;
>> +}
>
>Same question as disacossiate_ucontext.

This was likely added during early dev. and can be removed.

>
>> +
>> +/**
>> + * irdma_query_gid_roce - Query port GID for Roce
>> + * @ibdev: device pointer from stack
>> + * @port: port number
>> + * @index: Entry index
>> + * @gid: Global ID
>> + */
>> +static int irdma_query_gid_roce(struct ib_device *ibdev,
>> +				u8 port,
>> +				int index,
>> +				union ib_gid *gid)
>> +{
>> +	int ret;
>> +
>> +	ret = rdma_query_gid(ibdev, port, index, gid);
>> +	if (ret == -EAGAIN) {
>
>I can't see a path where rdma_query_gid returns -EAGAIN.

This function can be removed now. It's only applicable to non-Roce providers.

>
>> +		memcpy(gid, &zgid, sizeof(*gid));
>> +		return 0;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>
>> +/**
>> + * irdma_create_ah - create address handle
>> + * @ibpd: ptr to protection domain
>> + * @ah_attr: address handle attributes
>
>'ah_attr' -> 'attr', missing flags and udata.

Will fix all these hits in the driver.

[..]
>> + */
>> +static int irdma_destroy_ah(struct ib_ah *ibah, u32 flags) {
>> +	struct irdma_device *iwdev = to_iwdev(ibah->device);
>> +	struct irdma_ah *ah = to_iwah(ibah);
>> +	int err;
>> +
>> +	if (!ah->sc_ah.ah_info.ah_valid)
>> +		return -EINVAL;
>> +
>> +	err = irdma_ah_cqp_op(iwdev->rf, &ah->sc_ah,
>IRDMA_OP_AH_DESTROY,
>> +			      flags & RDMA_DESTROY_AH_SLEEPABLE,
>> +			      irdma_destroy_ah_cb, ah);
>> +	if (!err)
>> +		return 0;
>
>Why are the rest of the cleanups only in case of error?

On success, the cleanup is done in the callback, irdma_destroy_ah_cb.

[...]


>> +static __be64 irdma_mac_to_guid(struct net_device *ndev) {
>> +	unsigned char *mac = ndev->dev_addr;
>> +	__be64 guid;
>> +	unsigned char *dst = (unsigned char *)&guid;
>> +
>> +	dst[0] = mac[0] ^ 2;
>> +	dst[1] = mac[1];
>> +	dst[2] = mac[2];
>> +	dst[3] = 0xff;
>> +	dst[4] = 0xfe;
>> +	dst[5] = mac[3];
>> +	dst[6] = mac[4];
>> +	dst[7] = mac[5];
>> +
>> +	return guid;
>> +}
>
>There's a variant of this function in irdma, bnxt_re, ocrdma and qedr.
>Maybe it's time to provide it in common code?

Agreed.

^ permalink raw reply

* RE: [RFC v1 12/19] RDMA/irdma: Implement device supported verb APIs
From: Saleem, Shiraz @ 2019-02-26 21:09 UTC (permalink / raw)
  To: 'Jason Gunthorpe', Gal Pressman
  Cc: dledford@redhat.com, davem@davemloft.net,
	linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
	Ismail, Mustafa, Kirsher, Jeffrey T, Yossi Leybovich
In-Reply-To: <20190225185015.GD21863@ziepe.ca>

>Subject: Re: [RFC v1 12/19] RDMA/irdma: Implement device supported verb APIs
>

[..]

>> > +	ret = irdma_alloc_rsrc(iwdev->rf,
>> > +			       iwdev->rf->allocated_mrs, iwdev->rf->max_mr,
>> > +			       &stag_index, &next_stag_index);
>> > +	if (!ret) {
>> > +		stag = stag_index << IRDMA_CQPSQ_STAG_IDX_S;
>> > +		stag |= driver_key;
>> > +		stag += (u32)consumer_key;
>> > +		irdma_add_devusecount(iwdev);
>> > +	}
>>
>> This is confusing IMHO, better to test for 'if (ret)' and keep the
>> main flow unindented.
>
>Yes please follow the standard 'success oriented flow'
>
OK.

^ permalink raw reply

* Re: [RFC] nasty corner case in unix_dgram_sendmsg()
From: Jason Baron @ 2019-02-26 20:35 UTC (permalink / raw)
  To: Al Viro, Rainer Weikusat; +Cc: netdev
In-Reply-To: <20190226190316.GJ2217@ZenIV.linux.org.uk>



On 2/26/19 2:03 PM, Al Viro wrote:
> On Tue, Feb 26, 2019 at 03:31:32PM +0000, Rainer Weikusat wrote:
>> Al Viro <viro@zeniv.linux.org.uk> writes:
>>> On Tue, Feb 26, 2019 at 06:28:17AM +0000, Al Viro wrote:
>>
>> [...]
>>
>>
>>>> 	* if after relocking we see that unix_peer(sk) now
>>>> is equal to other, we arrange for wakeup forwarding from other's
>>>> peer_wait *and* if that has (likely) succeeded we fail with -EAGAIN.
>>>> Huh?
>>
>> This returns 1 if sending isn't possible at the moment, ie, if the
>> process which tries to send has to wait.
> 
> Except that in _this_ case we won't be waiting at all - we'll just
> return -EAGAIN (as one could expect, what with no timeout given/left).
> So what's the point of forwarding wakeups?  IOW, what is it that we
> expect to be waiting on sk_sleep(sk)?  Note that it won't be this
> call of sendmsg(2) (it'll bugger off without any further waiting).
> It won't be subsequent calls of sendmsg(2) either - they either
> sleep on skb allocation (which has nothing to do with destination)
> _or_ they sleep directly on other->peer_wait.  And poll(), while it
> will be sleeping on sk_sleep(sk), will make sure to set the forwarding 
> up.
> 
> I understand what the unix_dgram_peer_wake_me() is doing; I understand
> what unix_dgram_poll() is using it for.  What I do not understand is
> what's the point of doing that in unix_dgram_sendmsg()...
> 

Hi,

So the unix_dgram_peer_wake_me() in unix_dgram_sendmsg() is there for
epoll in edge-triggered mode. In that case, we want to ensure that if
-EAGAIN is returned a subsequent epoll_wait() is not stuck indefinitely.
Probably could use a comment...

Thanks,

-Jason

^ permalink raw reply

* Re: [PATCH net-next 0/5] tcp: cleanups for linux-5.1
From: David Miller @ 2019-02-26 21:16 UTC (permalink / raw)
  To: edumazet; +Cc: netdev, eric.dumazet
In-Reply-To: <20190226174913.18824-1-edumazet@google.com>

From: Eric Dumazet <edumazet@google.com>
Date: Tue, 26 Feb 2019 09:49:08 -0800

> This small patch series cleanups few things, and add a small
> timewait optimization for hosts not using md5.

Series applied, thanks Eric.

^ permalink raw reply

* Re: [PATCH RFC] net: Validate size of non-TSO packets in validate_xmit_skb().
From: David Miller @ 2019-02-26 21:16 UTC (permalink / raw)
  To: michael.chan; +Cc: maheshb, edumazet, netdev, dja
In-Reply-To: <CACKFLim51U_NGEU9nH5ZPWiup==aDLQM+x3p_O0-3EerZQ74hg@mail.gmail.com>

From: Michael Chan <michael.chan@broadcom.com>
Date: Tue, 26 Feb 2019 10:22:42 -0800

> On Tue, Feb 26, 2019 at 9:13 AM David Miller <davem@davemloft.net> wrote:
>>
>> From: Michael Chan <michael.chan@broadcom.com>
>> Date: Tue, 26 Feb 2019 05:56:41 -0500
>>
>> > There have been reports of oversize UDP packets being sent to the
>> > driver to be transmitted, causing error conditions.  The issue is
>> > likely caused by the dst of the SKB switching between 'lo' with
>> > 64K MTU and the hardware device with a smaller MTU.  Patches are
>> > being proposed by Mahesh Bandewar <maheshb@google.com> to fix the
>> > issue.
>> >
>> > Separately, we should add a length check in validate_xmit_skb()
>> > to drop these oversize packets before they reach the driver.
>> > This patch only validates non-TSO packets.  Complete validation
>> > of segmented TSO packet size will probably be too slow.
>> >
>> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>>
>> Anything which changes the dst of an SKB really is responsible for
>> fixing up whatever became "incompatible" in the new path.
>>
>> So like Eric I want to see this out of the fast path.
> 
> Ok.  In the meantime, will you take a 2-line bnxt_en patch that will
> prevent this issue in kernel 5.0?

Sure, but we will have to remember to remove it when it is no longer
necessary...

^ permalink raw reply

* Re: [PATCH net 0/3] net: Fail route add with unsupported nexthop attribute
From: David Miller @ 2019-02-26 22:27 UTC (permalink / raw)
  To: dsahern; +Cc: netdev, dsahern
In-Reply-To: <20190226170004.4535-1-dsahern@kernel.org>

From: David Ahern <dsahern@kernel.org>
Date: Tue, 26 Feb 2019 09:00:01 -0800

> From: David Ahern <dsahern@gmail.com>
> 
> RTA_VIA was added for MPLS as a way of specifying a gateway from a
> different address family. IPv4 and IPv6 do not currently support RTA_VIA
> so using it leads to routes that are not what the user intended. Catch
> and fail - returning a proper error message.
> 
> MPLS on the other hand does not support RTA_GATEWAY since it does not
> make sense to have a nexthop from the MPLS address family. Similarly,
> catch and fail - returning a proper error message.

Series applied and queued up for -stable.

^ permalink raw reply

* [PATCH bpf-next 1/3] bpf: add bpf_progenyof helper
From: Javier Honduvilla Coto @ 2019-02-26 22:36 UTC (permalink / raw)
  To: netdev; +Cc: yhs, kernel-team
In-Reply-To: <20190226223651.3166820-1-javierhonduco@fb.com>

This patch adds the bpf_progenyof helper which receives a PID and returns
1 if the process currently being executed is in the process hierarchy
including itself or 0 if not.

This is very useful in tracing programs when we want to filter by a
given PID and all the children it might spawn. The current workarounds
most people implement for this purpose have issues:

- Attaching to process spawning syscalls and dynamically add those PIDs
  to some bpf map that would be used to filter is cumbersome and
potentially racy.
- Unrolling some loop to perform what this helper is doing consumes lots
  of instructions. That and the impossibility to jump backwards makes it
really hard to be correct in really large process chains.

Signed-off-by: Javier Honduvilla Coto <javierhonduco@fb.com>
---
 include/linux/bpf.h      |  1 +
 include/uapi/linux/bpf.h |  3 ++-
 kernel/bpf/core.c        |  1 +
 kernel/bpf/helpers.c     | 29 +++++++++++++++++++++++++++++
 kernel/trace/bpf_trace.c |  2 ++
 5 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index de18227b3d95..447395ba202b 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -921,6 +921,7 @@ extern const struct bpf_func_proto bpf_sk_redirect_map_proto;
 extern const struct bpf_func_proto bpf_spin_lock_proto;
 extern const struct bpf_func_proto bpf_spin_unlock_proto;
 extern const struct bpf_func_proto bpf_get_local_storage_proto;
+extern const struct bpf_func_proto bpf_progenyof_proto;
 
 /* Shared helpers among cBPF and eBPF. */
 void bpf_user_rnd_init_once(void);
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bcdd2474eee7..804e4218eb28 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2457,7 +2457,8 @@ union bpf_attr {
 	FN(spin_lock),			\
 	FN(spin_unlock),		\
 	FN(sk_fullsock),		\
-	FN(tcp_sock),
+	FN(tcp_sock),			\
+	FN(progenyof),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ef88b167959d..69e209fbd128 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -2015,6 +2015,7 @@ const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
 const struct bpf_func_proto bpf_get_current_comm_proto __weak;
 const struct bpf_func_proto bpf_get_current_cgroup_id_proto __weak;
 const struct bpf_func_proto bpf_get_local_storage_proto __weak;
+const struct bpf_func_proto bpf_progenyof_proto __weak;
 
 const struct bpf_func_proto * __weak bpf_get_trace_printk_proto(void)
 {
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index a411fc17d265..3899787e8dbf 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -18,6 +18,7 @@
 #include <linux/sched.h>
 #include <linux/uidgid.h>
 #include <linux/filter.h>
+#include <linux/init_task.h>
 
 /* If kernel subsystem is allowing eBPF programs to call this function,
  * inside its own verifier_ops->get_func_proto() callback it should return
@@ -364,3 +365,31 @@ const struct bpf_func_proto bpf_get_local_storage_proto = {
 };
 #endif
 #endif
+
+BPF_CALL_1(bpf_progenyof, int, pid)
+{
+	int result = 0;
+	struct task_struct *task = current;
+
+	if (unlikely(!task))
+		return -EINVAL;
+
+	rcu_read_lock();
+	while (task != &init_task) {
+		if (task->pid == pid) {
+			result = 1;
+			break;
+		}
+		task = rcu_dereference(task->real_parent);
+	}
+	rcu_read_unlock();
+
+	return result;
+}
+
+const struct bpf_func_proto bpf_progenyof_proto = {
+	.func		= bpf_progenyof,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_ANYTHING,
+};
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index f1a86a0d881d..8602ae83c799 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -600,6 +600,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_get_prandom_u32_proto;
 	case BPF_FUNC_probe_read_str:
 		return &bpf_probe_read_str_proto;
+	case BPF_FUNC_progenyof:
+		return &bpf_progenyof_proto;
 #ifdef CONFIG_CGROUPS
 	case BPF_FUNC_get_current_cgroup_id:
 		return &bpf_get_current_cgroup_id_proto;
-- 
2.17.1


^ permalink raw reply related

* [PATCH bpf-next 0/3] bpf: add progenyof helper
From: Javier Honduvilla Coto @ 2019-02-26 22:36 UTC (permalink / raw)
  To: netdev; +Cc: yhs, kernel-team

Hi all,

This patch add the bpf_progenyof helper which receives a PID and returns
1 if the process currently being executed is in the process hierarchy,
including itself or 0 if not.

This is very useful in tracing programs when we want to filter by a
given PID and all the children it might have. The current workarounds
most people implement for this purpose have issues:

- Attaching to process spawning syscalls and dynamically add those PIDs
  to  some bpf map that would be used to filter is cumbersome and
potentially racy.
- Unrolling some loop to perform what this helper is doing consumes lots
  of instructions. That and the impossibility to jump backwards makes it
really hard to be correct in really large process chains.

Let me know what do you think!

Thanks,

Javier Honduvilla Coto (3):
  bpf: add bpf_progenyof helper
  bpf: sync kernel uapi headers
  bpf: add tests for bpf_progenyof

 include/linux/bpf.h                           |   1 +
 include/uapi/linux/bpf.h                      |   3 +-
 kernel/bpf/core.c                             |   1 +
 kernel/bpf/helpers.c                          |  29 ++
 kernel/trace/bpf_trace.c                      |   2 +
 tools/include/uapi/linux/bpf.h                |  11 +-
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |   2 +-
 tools/testing/selftests/bpf/bpf_helpers.h     |   1 +
 .../selftests/bpf/progs/test_progenyof_kern.c |  46 ++++
 .../selftests/bpf/test_progenyof_user.c       | 249 ++++++++++++++++++
 11 files changed, 343 insertions(+), 3 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_progenyof_kern.c
 create mode 100644 tools/testing/selftests/bpf/test_progenyof_user.c

-- 
2.17.1


^ permalink raw reply

* [PATCH bpf-next 3/3] bpf: add tests for bpf_progenyof
From: Javier Honduvilla Coto @ 2019-02-26 22:36 UTC (permalink / raw)
  To: netdev; +Cc: yhs, kernel-team
In-Reply-To: <20190226223651.3166820-1-javierhonduco@fb.com>

Adding the following test cases:

- progenyof(current->pid) == 1
- progenyof(current->real_parent->pid) == 1
- progenyof(1) == 1
- progenyof(0) == 0
- progenyof(current->children[0]->pid) == 0

Signed-off-by: Javier Honduvilla Coto <javierhonduco@fb.com>
---
 tools/testing/selftests/bpf/.gitignore        |   1 +
 tools/testing/selftests/bpf/Makefile          |   2 +-
 tools/testing/selftests/bpf/bpf_helpers.h     |   1 +
 .../selftests/bpf/progs/test_progenyof_kern.c |  46 ++++
 .../selftests/bpf/test_progenyof_user.c       | 249 ++++++++++++++++++
 5 files changed, 298 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_progenyof_kern.c
 create mode 100644 tools/testing/selftests/bpf/test_progenyof_user.c

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index e47168d1257d..677be3b05cd2 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -30,3 +30,4 @@ test_section_names
 test_tcpnotify_user
 test_libbpf
 alu32
+test_progenyof_user
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index ccffaa0a0787..8a8d4b2a5b74 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -23,7 +23,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
 	test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
 	test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
 	test_socket_cookie test_cgroup_storage test_select_reuseport test_section_names \
-	test_netcnt test_tcpnotify_user test_sock_fields
+	test_netcnt test_tcpnotify_user test_sock_fields test_progenyof_user
 
 BPF_OBJ_FILES = $(patsubst %.c,%.o, $(notdir $(wildcard progs/*.c)))
 TEST_GEN_FILES = $(BPF_OBJ_FILES)
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index d9999f1ed1d2..1f3e0d1ad159 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -180,6 +180,7 @@ static struct bpf_sock *(*bpf_sk_fullsock)(struct bpf_sock *sk) =
 	(void *) BPF_FUNC_sk_fullsock;
 static struct bpf_tcp_sock *(*bpf_tcp_sock)(struct bpf_sock *sk) =
 	(void *) BPF_FUNC_tcp_sock;
+static int (*bpf_progenyof)(int pid) = (void *) BPF_FUNC_progenyof;
 
 /* llvm builtin functions that eBPF C program may use to
  * emit BPF_LD_ABS and BPF_LD_IND instructions
diff --git a/tools/testing/selftests/bpf/progs/test_progenyof_kern.c b/tools/testing/selftests/bpf/progs/test_progenyof_kern.c
new file mode 100644
index 000000000000..c4be2c794d83
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_progenyof_kern.c
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+struct bpf_map_def SEC("maps") pidmap = {
+	.type = BPF_MAP_TYPE_ARRAY,
+	.key_size = sizeof(__u32),
+	.value_size = sizeof(__u32),
+	.max_entries = 2,
+};
+
+struct bpf_map_def SEC("maps") resultmap = {
+	.type = BPF_MAP_TYPE_ARRAY,
+	.key_size = sizeof(__u32),
+	.value_size = sizeof(__u32),
+	.max_entries = 1,
+};
+
+SEC("tracepoint/syscalls/sys_enter_open")
+int trace(void *ctx)
+{
+	__u32 pid = bpf_get_current_pid_tgid();
+	__u32 current_key = 0, ancestor_key = 1, *expected_pid, *ancestor_pid;
+	__u32 *val;
+
+	expected_pid = bpf_map_lookup_elem(&pidmap, &current_key);
+	if (!expected_pid || *expected_pid != pid)
+		return 0;
+
+	ancestor_pid = bpf_map_lookup_elem(&pidmap, &ancestor_key);
+	if (!ancestor_pid)
+		return 0;
+
+	if (!bpf_progenyof(*ancestor_pid))
+		return 0;
+
+	val = bpf_map_lookup_elem(&resultmap, &current_key);
+	if (val)
+		*val = *ancestor_pid;
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
+__u32 _version SEC("version") = 1;
diff --git a/tools/testing/selftests/bpf/test_progenyof_user.c b/tools/testing/selftests/bpf/test_progenyof_user.c
new file mode 100644
index 000000000000..0f9572986a11
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_progenyof_user.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <syscall.h>
+#include <unistd.h>
+#include <linux/perf_event.h>
+#include <sys/ioctl.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#define CHECK(condition, tag, format...)                                       \
+	({                                                                     \
+		int __ret = !!(condition);                                     \
+		if (__ret) {                                                   \
+			printf("%s:FAIL:%s ", __func__, tag);                  \
+			printf(format);                                        \
+		} else {                                                       \
+			printf("%s:PASS:%s\n", __func__, tag);                 \
+		}                                                              \
+		__ret;                                                         \
+	})
+
+static int bpf_find_map(const char *test, struct bpf_object *obj,
+			const char *name)
+{
+	struct bpf_map *map;
+
+	map = bpf_object__find_map_by_name(obj, name);
+	if (!map)
+		return -1;
+	return bpf_map__fd(map);
+}
+
+int main(int argc, char **argv)
+{
+	const char *probe_name = "syscalls/sys_enter_open";
+	const char *file = "test_progenyof_kern.o";
+	int err, bytes, efd, prog_fd, pmu_fd;
+	int resultmap_fd, pidmap_fd;
+	struct perf_event_attr attr = {};
+	struct bpf_object *obj;
+	__u32 retrieved_ancestor_pid = 0;
+	__u32 key = 0, pid;
+	int exit_code = EXIT_FAILURE;
+	char buf[256];
+
+	int child_pid, ancestor_pid, root_fd;
+	__u32 ancestor_key = 1;
+	int pipefd[2];
+	char marker[1];
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
+	if (CHECK(err, "bpf_prog_load", "err %d errno %d\n", err, errno))
+		goto fail;
+
+	resultmap_fd = bpf_find_map(__func__, obj, "resultmap");
+	if (CHECK(resultmap_fd < 0, "bpf_find_map", "err %d errno %d\n",
+		  resultmap_fd, errno))
+		goto close_prog;
+
+	pidmap_fd = bpf_find_map(__func__, obj, "pidmap");
+	if (CHECK(pidmap_fd < 0, "bpf_find_map", "err %d errno %d\n", pidmap_fd,
+		  errno))
+		goto close_prog;
+
+	pid = getpid();
+	bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
+	bpf_map_update_elem(pidmap_fd, &ancestor_key, &pid, 0);
+
+	snprintf(buf, sizeof(buf), "/sys/kernel/debug/tracing/events/%s/id",
+		 probe_name);
+	efd = open(buf, O_RDONLY, 0);
+	if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
+		goto close_prog;
+	bytes = read(efd, buf, sizeof(buf));
+	close(efd);
+	if (CHECK(bytes <= 0 || bytes >= sizeof(buf), "read",
+		  "bytes %d errno %d\n", bytes, errno))
+		goto close_prog;
+
+	attr.config = strtol(buf, NULL, 0);
+	attr.type = PERF_TYPE_TRACEPOINT;
+	attr.sample_type = PERF_SAMPLE_RAW;
+	attr.sample_period = 1;
+	attr.wakeup_events = 1;
+
+	pmu_fd = syscall(__NR_perf_event_open, &attr, getpid(), -1, -1, 0);
+	if (CHECK(pmu_fd < 0, "perf_event_open", "err %d errno %d\n", pmu_fd,
+		  errno))
+		goto close_prog;
+
+	err = ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
+	if (CHECK(err, "perf_event_ioc_enable", "err %d errno %d\n", err,
+		  errno))
+		goto close_pmu;
+
+	err = ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd);
+	if (CHECK(err, "perf_event_ioc_set_bpf", "err %d errno %d\n", err,
+		  errno))
+		goto close_pmu;
+
+	// Test on itself: progenyof(current->pid) is true
+	bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
+	bpf_map_update_elem(pidmap_fd, &ancestor_key, &pid, 0);
+	bpf_map_update_elem(resultmap_fd, &key, &key, 0);
+
+	root_fd = open("/", O_RDONLY);
+	if (CHECK(efd < 0, "open", "errno %d\n", errno))
+		goto close_prog;
+	close(root_fd);
+
+	err = bpf_map_lookup_elem(resultmap_fd, &key, &retrieved_ancestor_pid);
+	if (CHECK(err, "bpf_map_lookup_elem", "err %d errno %d\n", err, errno))
+		goto close_pmu;
+	if (CHECK(retrieved_ancestor_pid != pid,
+		  "progenyof is true with same pid", "%d == %d\n",
+		  retrieved_ancestor_pid, pid))
+		goto close_pmu;
+
+	// Test that PID 1 is among our progeny
+	bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
+	ancestor_pid = 1;
+	bpf_map_update_elem(pidmap_fd, &ancestor_key, &ancestor_pid, 0);
+	bpf_map_update_elem(resultmap_fd, &key, &key, 0);
+
+	root_fd = open("/", O_RDONLY);
+	if (CHECK(efd < 0, "open", "errno %d\n", errno))
+		goto close_prog;
+	close(root_fd);
+
+	err = bpf_map_lookup_elem(resultmap_fd, &key, &retrieved_ancestor_pid);
+	if (CHECK(err, "bpf_map_lookup_elem", "err %d errno %d\n", err, errno))
+		goto close_pmu;
+	if (CHECK(retrieved_ancestor_pid != ancestor_pid,
+		  "progenyof reaches init", "%d == %d\n",
+		  retrieved_ancestor_pid, ancestor_pid))
+		goto close_pmu;
+
+	// Test we don't go over PID 1
+	bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
+	ancestor_pid = 0;
+	bpf_map_update_elem(pidmap_fd, &ancestor_key, &ancestor_pid, 0);
+	bpf_map_update_elem(resultmap_fd, &key, &key, 0);
+
+	root_fd = open("/", O_RDONLY);
+	if (CHECK(efd < 0, "open", "errno %d\n", errno))
+		goto close_prog;
+	close(root_fd);
+
+	err = bpf_map_lookup_elem(resultmap_fd, &key, &retrieved_ancestor_pid);
+	if (CHECK(err, "bpf_map_lookup_elem", "err %d errno %d\n", err, errno))
+		goto close_pmu;
+	if (CHECK(retrieved_ancestor_pid != 0,
+		  "progenyof does not go over init", "%d == %d\n",
+		  retrieved_ancestor_pid, 0))
+		goto close_pmu;
+
+	// Test that we are the progeny of our child
+	pipe(pipefd);
+	child_pid = fork();
+	if (child_pid == -1) {
+		printf("fork failed\n");
+		goto close_pmu;
+	} else if (child_pid == 0) {
+		close(pipefd[1]);
+		read(pipefd[0], &marker, 1);
+
+		root_fd = open("/", O_RDONLY);
+		if (CHECK(efd < 0, "open", "errno %d\n", errno))
+			goto close_prog;
+		close(root_fd);
+
+		close(pipefd[0]);
+		_exit(EXIT_SUCCESS);
+	} else {
+		close(pipefd[0]);
+		bpf_map_update_elem(resultmap_fd, &key, &key, 0);
+		bpf_map_update_elem(pidmap_fd, &key, &child_pid, 0);
+		bpf_map_update_elem(pidmap_fd, &ancestor_key, &pid, 0);
+
+		write(pipefd[1], &marker, 1);
+		wait(NULL);
+		close(pipefd[1]);
+
+		err = bpf_map_lookup_elem(resultmap_fd, &key,
+					  &retrieved_ancestor_pid);
+		if (CHECK(err, "bpf_map_lookup_elem", "err %d errno %d\n", err,
+			  errno))
+			goto close_pmu;
+		if (CHECK(retrieved_ancestor_pid != pid, "progenyof of parent",
+			  "%d == %d\n", retrieved_ancestor_pid, pid))
+			goto close_pmu;
+	}
+
+	// Test that a child of ours doesn't belong to our progeny
+	bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
+	bpf_map_update_elem(resultmap_fd, &key, &key, 0);
+
+	pipe(pipefd);
+	child_pid = fork();
+	if (child_pid == -1) {
+		printf("fork failed\n");
+		goto close_pmu;
+	} else if (child_pid == 0) {
+		close(pipefd[1]);
+		read(pipefd[0], marker, 1);
+		close(pipefd[0]);
+		_exit(EXIT_SUCCESS);
+	} else {
+		close(pipefd[0]);
+
+		bpf_map_update_elem(pidmap_fd, &ancestor_key, &child_pid, 0);
+
+		root_fd = open("/", O_RDONLY);
+		if (CHECK(efd < 0, "open", "errno %d\n", errno))
+			goto close_prog;
+		close(root_fd);
+
+		write(pipefd[1], marker, 1);
+		wait(NULL);
+		close(pipefd[1]);
+
+		err = bpf_map_lookup_elem(resultmap_fd, &key,
+					  &retrieved_ancestor_pid);
+		if (CHECK(err, "bpf_map_lookup_elem", "err %d errno %d\n", err,
+			  errno))
+			goto close_pmu;
+		if (CHECK(retrieved_ancestor_pid != 0, "progenyof of child",
+			  "%d == %d\n", retrieved_ancestor_pid, 0))
+			goto close_pmu;
+	}
+
+	exit_code = EXIT_SUCCESS;
+	printf("%s:PASS\n", argv[0]);
+
+close_pmu:
+	close(pmu_fd);
+close_prog:
+	bpf_object__close(obj);
+fail:
+	return exit_code;
+}
-- 
2.17.1


^ permalink raw reply related

* [PATCH bpf-next 2/3] bpf: sync kernel uapi headers
From: Javier Honduvilla Coto @ 2019-02-26 22:36 UTC (permalink / raw)
  To: netdev; +Cc: yhs, kernel-team
In-Reply-To: <20190226223651.3166820-1-javierhonduco@fb.com>

Sync kernel uapi headers.

Signed-off-by: Javier Honduvilla Coto <javierhonduco@fb.com>
---
 tools/include/uapi/linux/bpf.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index bcdd2474eee7..354ec295864c 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2359,6 +2359,14 @@ union bpf_attr {
  *	Return
  *		A **struct bpf_tcp_sock** pointer on success, or NULL in
  *		case of failure.
+ *
+ * int bpf_progenyof(int pid)
+ *	Description
+ *		This helper is useful in programs that want to filter events
+ *		happening to a pid of any of its descendants.
+ *	Return
+ *		1 if the currently executing process' pid is in the process
+ *		hierarchy of the passed pid. 0 Otherwise.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2457,7 +2465,8 @@ union bpf_attr {
 	FN(spin_lock),			\
 	FN(spin_unlock),		\
 	FN(sk_fullsock),		\
-	FN(tcp_sock),
+	FN(tcp_sock),			\
+	FN(progenyof),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
-- 
2.17.1


^ permalink raw reply related

* Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: Cong Wang @ 2019-02-26 22:38 UTC (permalink / raw)
  To: Vlad Buslov
  Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
	David Miller
In-Reply-To: <vbf4l8qmvt4.fsf@mellanox.com>

On Tue, Feb 26, 2019 at 7:08 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> >> at least once is unreliable. Some classifiers set 'stop' flag on error
> >> before calling walker callback, other classifiers used to call it with NULL
> >> filter pointer when empty. In order to prevent further regressions, extend
> >> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> >> tcf_walker->fn() implementation that is used to check if classifier has
> >> filters configured.
> >
> >
> > So, after this patch commits like 31a998487641 ("net: sched: fw: don't
> > set arg->stop in fw_walk() when empty") can be reverted??
>
> Yes, it is safe now to revert following commits:
>
> 3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
> 31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")

Yeah, and probably commit d66022cd1623
("net: sched: matchall: verify that filter is not NULL in mall_walk()").

Please send a patch to revert them all.

Thanks.

^ permalink raw reply

* Re: [PATCH net-next v2 1/5] net/mlx5e: Return -EOPNOTSUPP when modify header action zero
From: Or Gerlitz @ 2019-02-26 22:41 UTC (permalink / raw)
  To: Tonghao Zhang; +Cc: Saeed Mahameed, Linux Netdev List
In-Reply-To: <1551091207-10366-2-git-send-email-xiangxia.m.yue@gmail.com>

On Mon, Feb 25, 2019 at 1:06 PM <xiangxia.m.yue@gmail.com> wrote:

> When max modify header action is zero, we return -EOPNOTSUPP
> directly. In this way, we can ignore wrong message info (e.g.
> "mlx5: parsed 0 pedit actions, can't do more").
>
> This happens when offloading pedit actions on mlx VFs.

this command should work, we support header re-write (pedit offload)
for tc NIC rules

Is this CX5 VF? if yes, something broke

> For example:
> $ tc filter add dev mlx5_vf parent ffff: protocol ip prio 1 \
>         flower skip_sw dst_mac 00:10:56:fb:64:e8 \
>         dst_ip 1.1.1.100 src_ip 1.1.1.200 \
>         action pedit ex munge eth src set 00:10:56:b4:5d:20

^ permalink raw reply

* Re: [PATCH net-next v2 5/5] net/mlx5e: Return -EOPNOTSUPP when attempting to offload an unsupported action
From: Or Gerlitz @ 2019-02-26 22:42 UTC (permalink / raw)
  To: Tonghao Zhang; +Cc: Saeed Mahameed, Linux Netdev List
In-Reply-To: <1551091207-10366-6-git-send-email-xiangxia.m.yue@gmail.com>

On Mon, Feb 25, 2019 at 1:07 PM <xiangxia.m.yue@gmail.com> wrote:

> The encapsulation is not supported for mlx5 VFs. When we try to
> offload that action, the -EINVAL is returned, but not -EOPNOTSUPP.
> This patch changes the returned value and ignore to confuse user.

FWIW, note that this changes the behavior towards user-space, I don't see
concrete harm done here but we should realize that

> For example: (p2p1_0 is VF net device)
> tc filter add dev p2p1_0 protocol ip  parent ffff: prio 1 flower skip_sw \
>         src_mac e4:11:22:33:44:01       \
>         action tunnel_key set           \
>         src_ip 1.1.1.100                \
>         dst_ip 1.1.1.200                \
>         dst_port 4789 id 100            \
>         action mirred egress redirect dev vxlan0
>
> "RTNETLINK answers: Invalid argument"

^ permalink raw reply

* Re: [PATCH hyperv-fixes] hv_netvsc: Fix IP header checksum for coalesced packets
From: David Miller @ 2019-02-26 22:45 UTC (permalink / raw)
  To: haiyangz, haiyangz
  Cc: sashal, linux-hyperv, kys, sthemmin, olaf, vkuznets, netdev,
	linux-kernel
In-Reply-To: <20190222182503.12160-1-haiyangz@linuxonhyperv.com>

From: Haiyang Zhang <haiyangz@linuxonhyperv.com>
Date: Fri, 22 Feb 2019 18:25:03 +0000

> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> Incoming packets may have IP header checksum verified by the host.
> They may not have IP header checksum computed after coalescing.
> This patch re-compute the checksum when necessary, otherwise the
> packets may be dropped, because Linux network stack always checks it.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

Applied and queued up for -stable.

^ permalink raw reply

* Re: [PATCH][net-next] net: Use RCU_POINTER_INITIALIZER() to init static variable
From: David Miller @ 2019-02-26 22:52 UTC (permalink / raw)
  To: lirongqing; +Cc: netdev
In-Reply-To: <1551062586-5658-1-git-send-email-lirongqing@baidu.com>

From: Li RongQing <lirongqing@baidu.com>
Date: Mon, 25 Feb 2019 10:43:06 +0800

> This pointer is RCU protected, so proper primitives should be used.
> 
> Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

Applied.

^ permalink raw reply

* Re: [net v3 1/1] tipc: fix race condition causing hung sendto
From: David Miller @ 2019-02-26 22:52 UTC (permalink / raw)
  To: tung.q.nguyen; +Cc: netdev, tipc-discussion
In-Reply-To: <20190225035720.5175-1-tung.q.nguyen@dektech.com.au>

From: Tung Nguyen <tung.q.nguyen@dektech.com.au>
Date: Mon, 25 Feb 2019 10:57:20 +0700

> When sending multicast messages via blocking socket,
> if sending link is congested (tsk->cong_link_cnt is set to 1),
> the sending thread will be put into sleeping state. However,
> tipc_sk_filter_rcv() is called under socket spin lock but
> tipc_wait_for_cond() is not. So, there is no guarantee that
> the setting of tsk->cong_link_cnt to 0 in tipc_sk_proto_rcv() in
> CPU-1 will be perceived by CPU-0. If that is the case, the sending
> thread in CPU-0 after being waken up, will continue to see
> tsk->cong_link_cnt as 1 and put the sending thread into sleeping
> state again. The sending thread will sleep forever.
...
> This commit fixes it by adding memory barrier to tipc_sk_proto_rcv()
> and tipc_wait_for_cond().
> 
> Acked-by: Jon Maloy <jon.maloy@ericsson.com>
> Signed-off-by: Tung Nguyen <tung.q.nguyen@dektech.com.au>

Applied and queued up for -stable.

^ permalink raw reply

* [PATCH internal pre-review] bpf: add missing entries to bpf_helpers.h
From: Willem de Bruijn @ 2019-02-26 22:55 UTC (permalink / raw)
  To: netdev, sdf; +Cc: soheil, Willem de Bruijn

From: Willem de Bruijn <willemb@google.com>

Signed-off-by: Willem de Bruijn <willemb@google.com>
---
 tools/testing/selftests/bpf/bpf_helpers.h | 29 +++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index d9999f1ed1d2a..fcd11839903c7 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -232,6 +232,35 @@ static int (*bpf_skb_change_head)(void *, int len, int flags) =
 	(void *) BPF_FUNC_skb_change_head;
 static int (*bpf_skb_pull_data)(void *, int len) =
 	(void *) BPF_FUNC_skb_pull_data;
+static unsigned long long (*bpf_get_cgroup_classid)(void *ctx) =
+	(void *) BPF_FUNC_get_cgroup_classid;
+static unsigned long long (*bpf_get_route_realm)(void *ctx) =
+	(void *) BPF_FUNC_get_route_realm;
+static int (*bpf_skb_change_proto)(void *ctx, __be16 proto, __u64 flags) =
+	(void *) BPF_FUNC_skb_change_proto;
+static int (*bpf_skb_change_type)(void *ctx, __u32 type) =
+	(void *) BPF_FUNC_skb_change_type;
+static unsigned long long (*bpf_get_hash_recalc)(void *ctx) =
+	(void *) BPF_FUNC_get_hash_recalc;
+static unsigned long long (*bpf_get_current_task)(void *ctx) =
+	(void *) BPF_FUNC_get_current_task;
+static int (*bpf_skb_change_tail)(void *ctx, __u32 len, __u64 flags) =
+	(void *) BPF_FUNC_skb_change_tail;
+static long long (*bpf_csum_update)(void *ctx, __u32 csum) =
+	(void *) BPF_FUNC_csum_update;
+static void (*bpf_set_hash_invalid)(void *ctx) =
+	(void *) BPF_FUNC_set_hash_invalid;
+static int (*bpf_get_numa_node_id)(void) =
+	(void *) BPF_FUNC_get_numa_node_id;
+static int (*bpf_probe_read_str)(void *ctx, int size, const void *unsafe_ptr) =
+	(void *) BPF_FUNC_probe_read_str;
+static unsigned int (*bpf_get_socket_uid)(void *ctx) =
+	(void *) BPF_FUNC_get_socket_uid;
+static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
+	(void *) BPF_FUNC_set_hash;
+static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
+				  unsigned long long flags) =
+	(void *) BPF_FUNC_skb_adjust_room;
 
 /* Scan the ARCH passed in from ARCH env variable (see Makefile) */
 #if defined(__TARGET_ARCH_x86)
-- 
2.21.0.rc2.261.ga7da99ff1b-goog


^ permalink raw reply related

* Re: [PATCH internal pre-review] bpf: add missing entries to bpf_helpers.h
From: Soheil Hassas Yeganeh @ 2019-02-26 22:59 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: netdev, Stanislav Fomichev, Willem de Bruijn
In-Reply-To: <20190226225553.214360-1-willemdebruijn.kernel@gmail.com>

On Tue, Feb 26, 2019 at 5:55 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

Acked-by: Soheil Hassas Yeganeh <soheil@google.com>

> ---
>  tools/testing/selftests/bpf/bpf_helpers.h | 29 +++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index d9999f1ed1d2a..fcd11839903c7 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -232,6 +232,35 @@ static int (*bpf_skb_change_head)(void *, int len, int flags) =
>         (void *) BPF_FUNC_skb_change_head;
>  static int (*bpf_skb_pull_data)(void *, int len) =
>         (void *) BPF_FUNC_skb_pull_data;
> +static unsigned long long (*bpf_get_cgroup_classid)(void *ctx) =
> +       (void *) BPF_FUNC_get_cgroup_classid;
> +static unsigned long long (*bpf_get_route_realm)(void *ctx) =
> +       (void *) BPF_FUNC_get_route_realm;
> +static int (*bpf_skb_change_proto)(void *ctx, __be16 proto, __u64 flags) =
> +       (void *) BPF_FUNC_skb_change_proto;
> +static int (*bpf_skb_change_type)(void *ctx, __u32 type) =
> +       (void *) BPF_FUNC_skb_change_type;
> +static unsigned long long (*bpf_get_hash_recalc)(void *ctx) =
> +       (void *) BPF_FUNC_get_hash_recalc;
> +static unsigned long long (*bpf_get_current_task)(void *ctx) =
> +       (void *) BPF_FUNC_get_current_task;
> +static int (*bpf_skb_change_tail)(void *ctx, __u32 len, __u64 flags) =
> +       (void *) BPF_FUNC_skb_change_tail;
> +static long long (*bpf_csum_update)(void *ctx, __u32 csum) =
> +       (void *) BPF_FUNC_csum_update;
> +static void (*bpf_set_hash_invalid)(void *ctx) =
> +       (void *) BPF_FUNC_set_hash_invalid;
> +static int (*bpf_get_numa_node_id)(void) =
> +       (void *) BPF_FUNC_get_numa_node_id;
> +static int (*bpf_probe_read_str)(void *ctx, int size, const void *unsafe_ptr) =
> +       (void *) BPF_FUNC_probe_read_str;
> +static unsigned int (*bpf_get_socket_uid)(void *ctx) =
> +       (void *) BPF_FUNC_get_socket_uid;
> +static unsigned int (*bpf_set_hash)(void *ctx, __u32 hash) =
> +       (void *) BPF_FUNC_set_hash;
> +static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> +                                 unsigned long long flags) =
> +       (void *) BPF_FUNC_skb_adjust_room;
>
>  /* Scan the ARCH passed in from ARCH env variable (see Makefile) */
>  #if defined(__TARGET_ARCH_x86)
> --
> 2.21.0.rc2.261.ga7da99ff1b-goog
>

^ permalink raw reply

* Re: [PATCH internal pre-review] bpf: add missing entries to bpf_helpers.h
From: Willem de Bruijn @ 2019-02-26 23:00 UTC (permalink / raw)
  To: Network Development, Stanislav Fomichev
  Cc: Soheil Hassas Yeganeh, Willem de Bruijn
In-Reply-To: <20190226225553.214360-1-willemdebruijn.kernel@gmail.com>

On Tue, Feb 26, 2019 at 5:55 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> From: Willem de Bruijn <willemb@google.com>
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>

As the topic indicated I did not intend to send for review to netdev@
just yet. Apologies for the spam.

For one, this commit lacks a commit message.

That said. It is out there, so any comments welcome, thanks.

^ permalink raw reply

* Re: [BUG] net/sched : qlen can not really be per cpu ?
From: Eric Dumazet @ 2019-02-26 23:19 UTC (permalink / raw)
  To: Eric Dumazet, John Fastabend, Networking, Jamal Hadi Salim,
	Cong Wang, Jiri Pirko
In-Reply-To: <5fb87db3-d5bd-714b-213d-6821f4bf37da@gmail.com>



On 02/25/2019 10:42 PM, Eric Dumazet wrote:
> HTB + pfifo_fast as a leaf qdisc hits badly the following warning in htb_activate() :
> 
> WARN_ON(cl->level || !cl->leaf.q || !cl->leaf.q->q.qlen);
> 
> This is because pfifo_fast does not update sch->q.qlen, but per cpu counters.
> So cl->leaf.q->q.qlen is zero.
> 
> HFSC, CBQ, DRR, QFQ  have the same problem.
> 
> Any ideas how we can fix this ?

What about something simple for stable ?
( I yet have to boot/test this )

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 9481f2c142e26ee1174653d673e6134edd9851da..3a9e442fcaaf2ea48ae65bc87ee95f59cd7100c8 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -51,7 +51,10 @@ struct qdisc_size_table {
 struct qdisc_skb_head {
        struct sk_buff  *head;
        struct sk_buff  *tail;
-       __u32           qlen;
+       union {
+               __u32           qlen;
+               atomic_t        atomic_qlen;
+       };
        spinlock_t      lock;
 };
 
@@ -408,27 +411,19 @@ static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
        BUILD_BUG_ON(sizeof(qcb->data) < sz);
 }
 
-static inline int qdisc_qlen_cpu(const struct Qdisc *q)
-{
-       return this_cpu_ptr(q->cpu_qstats)->qlen;
-}
-
 static inline int qdisc_qlen(const struct Qdisc *q)
 {
        return q->q.qlen;
 }
 
-static inline int qdisc_qlen_sum(const struct Qdisc *q)
+static inline u32 qdisc_qlen_sum(const struct Qdisc *q)
 {
-       __u32 qlen = q->qstats.qlen;
-       int i;
+       u32 qlen = q->qstats.qlen;
 
-       if (q->flags & TCQ_F_NOLOCK) {
-               for_each_possible_cpu(i)
-                       qlen += per_cpu_ptr(q->cpu_qstats, i)->qlen;
-       } else {
+       if (q->flags & TCQ_F_NOLOCK)
+               qlen += atomic_read(&q->q.atomic_qlen);
+       else
                qlen += q->q.qlen;
-       }
 
        return qlen;
 }
@@ -827,12 +822,12 @@ static inline void qdisc_qstats_cpu_backlog_inc(struct Qdisc *sch,
 
 static inline void qdisc_qstats_cpu_qlen_inc(struct Qdisc *sch)
 {
-       this_cpu_inc(sch->cpu_qstats->qlen);
+       atomic_inc(&sch->q.atomic_qlen);
 }
 
 static inline void qdisc_qstats_cpu_qlen_dec(struct Qdisc *sch)
 {
-       this_cpu_dec(sch->cpu_qstats->qlen);
+       atomic_dec(&sch->q.atomic_qlen);
 }
 
 static inline void qdisc_qstats_cpu_requeues_inc(struct Qdisc *sch)



^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox