From: Maxim Mikityanskiy <maximmi@mellanox.com>
To: Magnus Karlsson <magnus.karlsson@intel.com>
Cc: bjorn.topel@intel.com, ast@kernel.org, daniel@iogearbox.net,
netdev@vger.kernel.org, jonathan.lemon@gmail.com,
bpf@vger.kernel.org, jeffrey.t.kirsher@intel.com,
anthony.l.nguyen@intel.com, maciej.fijalkowski@intel.com,
maciejromanfijalkowski@gmail.com, cristian.dumitrescu@intel.com
Subject: Re: [PATCH bpf-next v4 08/14] xsk: enable sharing of dma mappings
Date: Tue, 28 Jul 2020 11:59:07 +0300 [thread overview]
Message-ID: <11a4e8cb-1c88-ada8-7534-8f32d16e729d@mellanox.com> (raw)
In-Reply-To: <1595307848-20719-9-git-send-email-magnus.karlsson@intel.com>
On 2020-07-21 08:04, Magnus Karlsson wrote:
> Enable the sharing of dma mappings by moving them out from the buffer
> pool. Instead we put each dma mapped umem region in a list in the umem
> structure. If dma has already been mapped for this umem and device, it
> is not mapped again and the existing dma mappings are reused.
>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
> include/net/xdp_sock.h | 1 +
> include/net/xsk_buff_pool.h | 7 +++
> net/xdp/xdp_umem.c | 1 +
> net/xdp/xsk_buff_pool.c | 112 ++++++++++++++++++++++++++++++++++++--------
> 4 files changed, 102 insertions(+), 19 deletions(-)
>
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index 126d243..282aeba 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -30,6 +30,7 @@ struct xdp_umem {
> u8 flags;
> int id;
> bool zc;
> + struct list_head xsk_dma_list;
> };
>
> struct xsk_map {
> diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> index 83f100c..8f1dc4c 100644
> --- a/include/net/xsk_buff_pool.h
> +++ b/include/net/xsk_buff_pool.h
> @@ -28,6 +28,13 @@ struct xdp_buff_xsk {
> struct list_head free_list_node;
> };
>
> +struct xsk_dma_map {
> + dma_addr_t *dma_pages;
> + struct net_device *dev;
> + refcount_t users;
> + struct list_head list; /* Protected by the RTNL_LOCK */
> +};
> +
> struct xsk_buff_pool {
> struct xsk_queue *fq;
> struct xsk_queue *cq;
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index 372998d..cf27249 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -199,6 +199,7 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
> umem->user = NULL;
> umem->flags = mr->flags;
>
> + INIT_LIST_HEAD(&umem->xsk_dma_list);
> refcount_set(&umem->users, 1);
>
> err = xdp_umem_account_pages(umem);
> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index c563874..ca74a3e 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -104,6 +104,25 @@ void xp_set_rxq_info(struct xsk_buff_pool *pool, struct xdp_rxq_info *rxq)
> }
> EXPORT_SYMBOL(xp_set_rxq_info);
>
> +static void xp_disable_drv_zc(struct xsk_buff_pool *pool)
> +{
> + struct netdev_bpf bpf;
> + int err;
> +
> + ASSERT_RTNL();
> +
> + if (pool->umem->zc) {
> + bpf.command = XDP_SETUP_XSK_POOL;
> + bpf.xsk.pool = NULL;
> + bpf.xsk.queue_id = pool->queue_id;
> +
> + err = pool->netdev->netdev_ops->ndo_bpf(pool->netdev, &bpf);
> +
> + if (err)
> + WARN(1, "Failed to disable zero-copy!\n");
> + }
> +}
> +
> int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *netdev,
> u16 queue_id, u16 flags)
> {
> @@ -122,6 +141,8 @@ int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *netdev,
> if (xsk_get_pool_from_qid(netdev, queue_id))
> return -EBUSY;
>
> + pool->netdev = netdev;
> + pool->queue_id = queue_id;
> err = xsk_reg_pool_at_qid(netdev, pool, queue_id);
> if (err)
> return err;
> @@ -155,11 +176,15 @@ int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *netdev,
> if (err)
> goto err_unreg_pool;
>
> - pool->netdev = netdev;
> - pool->queue_id = queue_id;
> + if (!pool->dma_pages) {
> + WARN(1, "Driver did not DMA map zero-copy buffers");
> + goto err_unreg_xsk;
> + }
> pool->umem->zc = true;
> return 0;
>
> +err_unreg_xsk:
> + xp_disable_drv_zc(pool);
> err_unreg_pool:
> if (!force_zc)
> err = 0; /* fallback to copy mode */
> @@ -170,25 +195,10 @@ int xp_assign_dev(struct xsk_buff_pool *pool, struct net_device *netdev,
>
> void xp_clear_dev(struct xsk_buff_pool *pool)
> {
> - struct netdev_bpf bpf;
> - int err;
> -
> - ASSERT_RTNL();
> -
> if (!pool->netdev)
> return;
>
> - if (pool->umem->zc) {
> - bpf.command = XDP_SETUP_XSK_POOL;
> - bpf.xsk.pool = NULL;
> - bpf.xsk.queue_id = pool->queue_id;
> -
> - err = pool->netdev->netdev_ops->ndo_bpf(pool->netdev, &bpf);
> -
> - if (err)
> - WARN(1, "Failed to disable zero-copy!\n");
> - }
> -
> + xp_disable_drv_zc(pool);
> xsk_clear_pool_at_qid(pool->netdev, pool->queue_id);
> dev_put(pool->netdev);
> pool->netdev = NULL;
> @@ -233,14 +243,61 @@ void xp_put_pool(struct xsk_buff_pool *pool)
> }
> }
>
> +static struct xsk_dma_map *xp_find_dma_map(struct xsk_buff_pool *pool)
> +{
> + struct xsk_dma_map *dma_map;
> +
> + list_for_each_entry(dma_map, &pool->umem->xsk_dma_list, list) {
> + if (dma_map->dev == pool->netdev)
> + return dma_map;
> + }
> +
> + return NULL;
> +}
> +
> +static void xp_destroy_dma_map(struct xsk_dma_map *dma_map)
> +{
> + list_del(&dma_map->list);
> + kfree(dma_map);
> +}
> +
> +static void xp_put_dma_map(struct xsk_dma_map *dma_map)
> +{
> + if (!refcount_dec_and_test(&dma_map->users))
> + return;
> +
> + xp_destroy_dma_map(dma_map);
> +}
> +
> +static struct xsk_dma_map *xp_create_dma_map(struct xsk_buff_pool *pool)
> +{
> + struct xsk_dma_map *dma_map;
> +
> + dma_map = kzalloc(sizeof(*dma_map), GFP_KERNEL);
> + if (!dma_map)
> + return NULL;
> +
> + dma_map->dev = pool->netdev;
> + refcount_set(&dma_map->users, 1);
> + list_add(&dma_map->list, &pool->umem->xsk_dma_list);
> + return dma_map;
> +}
> +
> void xp_dma_unmap(struct xsk_buff_pool *pool, unsigned long attrs)
> {
> + struct xsk_dma_map *dma_map;
> dma_addr_t *dma;
> u32 i;
>
> if (pool->dma_pages_cnt == 0)
> return;
>
> + dma_map = xp_find_dma_map(pool);
> + if (!dma_map) {
> + WARN(1, "Could not find dma_map for device");
> + return;
> + }
> +
> for (i = 0; i < pool->dma_pages_cnt; i++) {
> dma = &pool->dma_pages[i];
> if (*dma) {
> @@ -250,6 +307,7 @@ void xp_dma_unmap(struct xsk_buff_pool *pool, unsigned long attrs)
> }
> }
>
> + xp_put_dma_map(dma_map);
I believe that the logic in this function is not correct. Basically, the
driver calls xp_dma_[un]map when a socket is enabled/disabled on a given
queue of a given netdev. On xp_dma_map, if the UMEM is already mapped
for that netdev, we skip all the logic, but on xp_dma_unmap you unmap
the pages unconditionally, only after that you check the refcount. This
is not symmetric, and the pages will be unmapped when the first socket
is closed, rendering the rest of sockets unusable.
> kvfree(pool->dma_pages);
> pool->dma_pages_cnt = 0;
> pool->dev = NULL;
> @@ -271,14 +329,29 @@ static void xp_check_dma_contiguity(struct xsk_buff_pool *pool)
> int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
> unsigned long attrs, struct page **pages, u32 nr_pages)
> {
> + struct xsk_dma_map *dma_map;
> dma_addr_t dma;
> u32 i;
>
> + dma_map = xp_find_dma_map(pool);
> + if (dma_map) {
> + pool->dma_pages = dma_map->dma_pages;
> + refcount_inc(&dma_map->users);
> + return 0;
> + }
> +
> + dma_map = xp_create_dma_map(pool);
> + if (!dma_map)
> + return -ENOMEM;
> +
> pool->dma_pages = kvcalloc(nr_pages, sizeof(*pool->dma_pages),
> GFP_KERNEL);
> - if (!pool->dma_pages)
> + if (!pool->dma_pages) {
> + xp_destroy_dma_map(dma_map);
> return -ENOMEM;
> + }
>
> + dma_map->dma_pages = pool->dma_pages;
> pool->dev = dev;
> pool->dma_pages_cnt = nr_pages;
> pool->dma_need_sync = false;
> @@ -288,6 +361,7 @@ int xp_dma_map(struct xsk_buff_pool *pool, struct device *dev,
> DMA_BIDIRECTIONAL, attrs);
> if (dma_mapping_error(dev, dma)) {
> xp_dma_unmap(pool, attrs);
> + xp_destroy_dma_map(dma_map);
> return -ENOMEM;
> }
> if (dma_need_sync(dev, dma))
>
next prev parent reply other threads:[~2020-07-28 8:59 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-21 5:03 [PATCH bpf-next v4 00/14] xsk: support shared umems between devices and queues Magnus Karlsson
2020-07-21 5:03 ` [PATCH bpf-next v4 01/14] xsk: i40e: ice: ixgbe: mlx5: pass buffer pool to driver instead of umem Magnus Karlsson
2020-07-28 7:04 ` Björn Töpel
2020-07-21 5:03 ` [PATCH bpf-next v4 02/14] xsk: i40e: ice: ixgbe: mlx5: rename xsk zero-copy driver interfaces Magnus Karlsson
2020-07-28 7:04 ` Björn Töpel
2020-07-21 5:03 ` [PATCH bpf-next v4 03/14] xsk: create and free buffer pool independently from umem Magnus Karlsson
2020-07-28 7:05 ` Björn Töpel
2020-07-21 5:03 ` [PATCH bpf-next v4 04/14] xsk: move fill and completion rings to buffer pool Magnus Karlsson
2020-07-28 7:05 ` Björn Töpel
2020-07-21 5:03 ` [PATCH bpf-next v4 05/14] xsk: move queue_id, dev and need_wakeup " Magnus Karlsson
2020-07-28 7:09 ` Björn Töpel
2020-07-29 13:20 ` Magnus Karlsson
2020-07-28 9:21 ` Maxim Mikityanskiy
2020-07-29 13:21 ` Magnus Karlsson
2020-07-21 5:04 ` [PATCH bpf-next v4 06/14] xsk: move xsk_tx_list and its lock " Magnus Karlsson
2020-07-28 7:10 ` Björn Töpel
2020-07-21 5:04 ` [PATCH bpf-next v4 07/14] xsk: move addrs from buffer pool to umem Magnus Karlsson
2020-07-28 7:11 ` Björn Töpel
2020-07-21 5:04 ` [PATCH bpf-next v4 08/14] xsk: enable sharing of dma mappings Magnus Karlsson
2020-07-28 7:14 ` Björn Töpel
2020-07-28 8:59 ` Maxim Mikityanskiy [this message]
2020-07-29 13:22 ` Magnus Karlsson
2020-07-21 5:04 ` [PATCH bpf-next v4 09/14] xsk: rearrange internal structs for better performance Magnus Karlsson
2020-07-28 7:14 ` Björn Töpel
2020-07-21 5:04 ` [PATCH bpf-next v4 10/14] xsk: add shared umem support between queue ids Magnus Karlsson
2020-07-28 7:15 ` Björn Töpel
2020-07-21 5:04 ` [PATCH bpf-next v4 11/14] xsk: add shared umem support between devices Magnus Karlsson
2020-07-28 7:15 ` Björn Töpel
2020-07-21 5:04 ` [PATCH bpf-next v4 12/14] libbpf: support shared umems between queues and devices Magnus Karlsson
2020-07-28 7:18 ` Björn Töpel
2020-07-21 5:04 ` [PATCH bpf-next v4 13/14] samples/bpf: add new sample xsk_fwd.c Magnus Karlsson
2020-07-28 7:18 ` Björn Töpel
2020-07-21 5:04 ` [PATCH bpf-next v4 14/14] xsk: documentation for XDP_SHARED_UMEM between queues and netdevs Magnus Karlsson
2020-07-28 7:18 ` Björn Töpel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=11a4e8cb-1c88-ada8-7534-8f32d16e729d@mellanox.com \
--to=maximmi@mellanox.com \
--cc=anthony.l.nguyen@intel.com \
--cc=ast@kernel.org \
--cc=bjorn.topel@intel.com \
--cc=bpf@vger.kernel.org \
--cc=cristian.dumitrescu@intel.com \
--cc=daniel@iogearbox.net \
--cc=jeffrey.t.kirsher@intel.com \
--cc=jonathan.lemon@gmail.com \
--cc=maciej.fijalkowski@intel.com \
--cc=maciejromanfijalkowski@gmail.com \
--cc=magnus.karlsson@intel.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).