From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tariq Toukan Subject: Re: [bpf-next V1-RFC PATCH 02/14] xdp/mlx5: setup xdp_rxq_info and extend with qtype Date: Wed, 13 Dec 2017 14:27:08 +0200 Message-ID: References: <151316391502.14967.13292358380181773729.stgit@firesoul> <151316397109.14967.10661226040285486466.stgit@firesoul> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, dsahern@gmail.com, Matan Barak , Saeed Mahameed , gospo@broadcom.com, bjorn.topel@intel.com, michael.chan@broadcom.com, Tariq Toukan To: Jesper Dangaard Brouer , Daniel Borkmann , Alexei Starovoitov Return-path: Received: from mail-eopbgr30052.outbound.protection.outlook.com ([40.107.3.52]:65131 "EHLO EUR03-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751910AbdLMM1R (ORCPT ); Wed, 13 Dec 2017 07:27:17 -0500 In-Reply-To: <151316397109.14967.10661226040285486466.stgit@firesoul> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: Hi Jesper, Thanks for taking care of the drop RQ. In general, mlx5 part looks ok to me. Find a few comments below. Mostly pointing out some typos. On 13/12/2017 1:19 PM, Jesper Dangaard Brouer wrote: > The mlx5 driver have a special drop-RQ queue (one per interface) that > simply drops all incoming traffic. It helps driver keep other HW > objects (flow steering) alive upon down/up operations. It is > temporarily pointed by flow steering objects during the interface > setup, and when interface is down. It lacks many fields that are set > in a regular RQ (for example its state is never switched to > MLX5_RQC_STATE_RDY). (Thanks to Tariq Toukan for explaination). typo: explanation > > The XDP RX-queue info API is extended with a queue-type, and mlx5 uses > this kind of drop/sink-type (RXQ_TYPE_SINK) for this kind of sink queue. > > Driver hook points for xdp_rxq_info: > * init+reg: mlx5e_alloc_rq() > * init+reg: mlx5e_alloc_drop_rq() > * unreg : mlx5e_free_rq() > > Tested on actual hardware with samples/bpf program > > Cc: Saeed Mahameed > Cc: Matan Barak > Cc: Tariq Toukan > Signed-off-by: Jesper Dangaard Brouer > --- > drivers/net/ethernet/mellanox/mlx5/core/en.h | 4 ++++ > drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 14 +++++++++++++ > drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 1 + > include/net/xdp.h | 23 +++++++++++++++++++++ > net/core/xdp.c | 6 +++++ > 5 files changed, 48 insertions(+) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h > index c0872b3284cb..fe10a042783b 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h > @@ -46,6 +46,7 @@ > #include > #include > #include > +#include > #include "wq.h" > #include "mlx5_core.h" > #include "en_stats.h" > @@ -568,6 +569,9 @@ struct mlx5e_rq { > u32 rqn; > struct mlx5_core_dev *mdev; > struct mlx5_core_mkey umr_mkey; > + > + /* XDP read-mostly */ > + struct xdp_rxq_info xdp_rxq; > } ____cacheline_aligned_in_smp; > > struct mlx5e_channel { > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > index 0f5c012de52e..ea44b5f25e11 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c > @@ -582,6 +582,12 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c, > rq->ix = c->ix; > rq->mdev = mdev; > > + /* XDP RX-queue info */ > + xdp_rxq_info_init(&rq->xdp_rxq); > + rq->xdp_rxq.dev = rq->netdev; > + rq->xdp_rxq.queue_index = rq->ix; > + xdp_rxq_info_reg(&rq->xdp_rxq); > + You don't set type here. This is ok as long as the following hold: 1) RXQ_TYPE_DEFAULT is zero 2) xdp_rxq is zalloc'ed. > rq->xdp_prog = params->xdp_prog ? bpf_prog_inc(params->xdp_prog) : NULL; > if (IS_ERR(rq->xdp_prog)) { > err = PTR_ERR(rq->xdp_prog); > @@ -695,6 +701,7 @@ static int mlx5e_alloc_rq(struct mlx5e_channel *c, > err_rq_wq_destroy: > if (rq->xdp_prog) > bpf_prog_put(rq->xdp_prog); > + xdp_rxq_info_unreg(&rq->xdp_rxq); > mlx5_wq_destroy(&rq->wq_ctrl); > > return err; > @@ -707,6 +714,8 @@ static void mlx5e_free_rq(struct mlx5e_rq *rq) > if (rq->xdp_prog) > bpf_prog_put(rq->xdp_prog); > > + xdp_rxq_info_unreg(&rq->xdp_rxq); > + > switch (rq->wq_type) { > case MLX5_WQ_TYPE_LINKED_LIST_STRIDING_RQ: > mlx5e_rq_free_mpwqe_info(rq); > @@ -2768,6 +2777,11 @@ static int mlx5e_alloc_drop_rq(struct mlx5_core_dev *mdev, > if (err) > return err; > > + /* XDP RX-queue info for "Drop-RQ", packets never reach XDP */ > + xdp_rxq_info_init(&rq->xdp_rxq); > + xdp_rxq_info_type(&rq->xdp_rxq, RXQ_TYPE_SINK); > + xdp_rxq_info_reg(&rq->xdp_rxq); > + > rq->mdev = mdev; > > return 0; > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > index 5b499c7a698f..7b38480811d4 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c > @@ -812,6 +812,7 @@ static inline int mlx5e_xdp_handle(struct mlx5e_rq *rq, > xdp_set_data_meta_invalid(&xdp); > xdp.data_end = xdp.data + *len; > xdp.data_hard_start = va; > + xdp.rxq = &rq->xdp_rxq; > > act = bpf_prog_run_xdp(prog, &xdp); > switch (act) { > diff --git a/include/net/xdp.h b/include/net/xdp.h > index e4acd198fd60..5be560d943e1 100644 > --- a/include/net/xdp.h > +++ b/include/net/xdp.h > @@ -36,10 +36,33 @@ struct xdp_rxq_info { > struct net_device *dev; > u32 queue_index; > u32 reg_state; > + u32 qtype; > } ____cacheline_aligned; /* perf critical, avoid false-sharing */ > > void xdp_rxq_info_init(struct xdp_rxq_info *xdp_rxq); > void xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq); > void xdp_rxq_info_unreg(struct xdp_rxq_info *xdp_rxq); > > +/** > + * DOC: XDP RX-queue type > + * > + * The XDP RX-queue info can have associated a type. > + * > + * @RXQ_TYPE_DEFAULT: default no specifik queue type need to be specified typo: specific > + * > + * @RXQ_TYPE_SINK: indicate a fake queue that never reach XDP RX > + * code. Some drivers have a need to maintain a lower layer > + * RX-queue as a sink queue, while reconfiguring other RX-queues. > + */ > +#define RXQ_TYPE_DEFAULT 0 > +#define RXQ_TYPE_SINK 1 > +#define RXQ_TYPE_MAX RXQ_TYPE_SINK Definitions of incremental numbers, enum might be best here, you can give them some enum type and use it in xdp_rxq_info->qtype. > + > +static inline > +void xdp_rxq_info_type(struct xdp_rxq_info *xdp_rxq, u32 qtype) > +{ > + BUILD_BUG_ON(qtype > RXQ_TYPE_MAX); > + xdp_rxq->qtype = qtype; > +} > + > #endif /* __LINUX_NET_XDP_H__ */ > diff --git a/net/core/xdp.c b/net/core/xdp.c > index a9d2dd7b1ede..2a111f5987f6 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -32,8 +32,14 @@ EXPORT_SYMBOL_GPL(xdp_rxq_info_init); > > void xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq) > { > + if (xdp_rxq->qtype == RXQ_TYPE_SINK) > + goto skip_content_check; > + > + /* Check information setup by driver code */ > WARN(!xdp_rxq->dev, "Missing net_device from driver"); > WARN(xdp_rxq->queue_index == U32_MAX, "Miss queue_index from driver"); > + > +skip_content_check: > WARN(!(xdp_rxq->reg_state == REG_STATE_NEW),"API violation, miss init"); > xdp_rxq->reg_state = REG_STATE_REGISTRED; typo: REGISTERED (introduced in a previous patch) > } >