DPDK patches and discussions
 help / color / mirror / Atom feed
From: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
To: Harman Kalra <hkalra@marvell.com>
Cc: <dev@dpdk.org>, Ray Kinsella <mdr@ashroe.eu>
Subject: Re: [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement get set APIs
Date: Sun, 3 Oct 2021 21:05:05 +0300
Message-ID: <20211003210505.1c852bd4@sovereign> (raw)
In-Reply-To: <20210903124102.47425-3-hkalra@marvell.com>

2021-09-03 18:10 (UTC+0530), Harman Kalra:
> [...]
> diff --git a/lib/eal/common/eal_common_interrupts.c b/lib/eal/common/eal_common_interrupts.c
> new file mode 100644
> index 0000000000..2e4fed96f0
> --- /dev/null
> +++ b/lib/eal/common/eal_common_interrupts.c
> @@ -0,0 +1,506 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(C) 2021 Marvell.
> + */
> +
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <rte_errno.h>
> +#include <rte_log.h>
> +#include <rte_malloc.h>
> +
> +#include <rte_interrupts.h>
> +
> +
> +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size,
> +						       bool from_hugepage)

Since the purpose of the series is to reduce future ABI breakages,
how about making the second parameter "flags" to have some spare bits?
(If not removing it completely per David's suggestion.)

> +{
> +	struct rte_intr_handle *intr_handle;
> +	int i;
> +
> +	if (from_hugepage)
> +		intr_handle = rte_zmalloc(NULL,
> +					  size * sizeof(struct rte_intr_handle),
> +					  0);
> +	else
> +		intr_handle = calloc(1, size * sizeof(struct rte_intr_handle));
> +	if (!intr_handle) {
> +		RTE_LOG(ERR, EAL, "Fail to allocate intr_handle\n");
> +		rte_errno = ENOMEM;
> +		return NULL;
> +	}
> +
> +	for (i = 0; i < size; i++) {
> +		intr_handle[i].nb_intr = RTE_MAX_RXTX_INTR_VEC_ID;
> +		intr_handle[i].alloc_from_hugepage = from_hugepage;
> +	}
> +
> +	return intr_handle;
> +}
> +
> +struct rte_intr_handle *rte_intr_handle_instance_index_get(
> +				struct rte_intr_handle *intr_handle, int index)

If rte_intr_handle_instance_alloc() returns a pointer to an array,
this function is useless since the user can simply manipulate a pointer.
If we want to make a distinction between a single struct rte_intr_handle and a
commonly allocated bunch of such (but why?), then they should be represented
by distinct types.

> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOMEM;

Why it's sometimes ENOMEM and sometimes ENOTSUP when the handle is not
allocated?

> +		return NULL;
> +	}
> +
> +	return &intr_handle[index];
> +}
> +
> +int rte_intr_handle_instance_index_set(struct rte_intr_handle *intr_handle,
> +				       const struct rte_intr_handle *src,
> +				       int index)

See above regarding the "index" parameter. If it can be removed, a better name
for this function would be rte_intr_handle_copy().

> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}
> +
> +	if (src == NULL) {
> +		RTE_LOG(ERR, EAL, "Source interrupt instance unallocated\n");
> +		rte_errno = EINVAL;
> +		goto fail;
> +	}
> +
> +	if (index < 0) {
> +		RTE_LOG(ERR, EAL, "Index cany be negative");
> +		rte_errno = EINVAL;
> +		goto fail;
> +	}

How about making this parameter "size_t"?

> +
> +	intr_handle[index].fd = src->fd;
> +	intr_handle[index].vfio_dev_fd = src->vfio_dev_fd;
> +	intr_handle[index].type = src->type;
> +	intr_handle[index].max_intr = src->max_intr;
> +	intr_handle[index].nb_efd = src->nb_efd;
> +	intr_handle[index].efd_counter_size = src->efd_counter_size;
> +
> +	memcpy(intr_handle[index].efds, src->efds, src->nb_intr);
> +	memcpy(intr_handle[index].elist, src->elist, src->nb_intr);
> +
> +	return 0;
> +fail:
> +	return rte_errno;

Should be (-rte_errno) per documentation.
Please check all functions in this file that return an "int" status.

> [...]
> +int rte_intr_handle_dev_fd_get(const struct rte_intr_handle *intr_handle)
> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}
> +
> +	return intr_handle->vfio_dev_fd;
> +fail:
> +	return rte_errno;
> +}

Returning a errno value instead of an FD is very error-prone.
Probably returning (-1) is both safe and convenient?

> +
> +int rte_intr_handle_max_intr_set(struct rte_intr_handle *intr_handle,
> +				 int max_intr)
> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}
> +
> +	if (max_intr > intr_handle->nb_intr) {
> +		RTE_LOG(ERR, EAL, "Max_intr=%d greater than PLT_MAX_RXTX_INTR_VEC_ID=%d",

Seems like this common/cnxk name leaked here by mistake?

> +			max_intr, intr_handle->nb_intr);
> +		rte_errno = ERANGE;
> +		goto fail;
> +	}
> +
> +	intr_handle->max_intr = max_intr;
> +
> +	return 0;
> +fail:
> +	return rte_errno;
> +}
> +
> +int rte_intr_handle_max_intr_get(const struct rte_intr_handle *intr_handle)
> +{
> +	if (intr_handle == NULL) {
> +		RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n");
> +		rte_errno = ENOTSUP;
> +		goto fail;
> +	}
> +
> +	return intr_handle->max_intr;
> +fail:
> +	return rte_errno;
> +}

Should be negative per documentation and to avoid returning a positive value
that cannot be distinguished from a successful return.
Please also check other functions in this file returning an "int" result
(not status).

  parent reply	other threads:[~2021-10-03 18:05 UTC|newest]

Thread overview: 152+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-26 14:57 [dpdk-dev] [RFC 0/7] make rte_intr_handle internal Harman Kalra
2021-08-26 14:57 ` [dpdk-dev] [RFC 1/7] eal: interrupt handle API prototypes Harman Kalra
2021-08-31 15:52   ` Kinsella, Ray
2021-08-26 14:57 ` [dpdk-dev] [RFC 2/7] eal/interrupts: implement get set APIs Harman Kalra
2021-08-31 15:53   ` Kinsella, Ray
2021-08-26 14:57 ` [dpdk-dev] [RFC 3/7] eal/interrupts: avoid direct access to interrupt handle Harman Kalra
2021-08-26 14:57 ` [dpdk-dev] [RFC 4/7] test/interrupt: apply get set interrupt handle APIs Harman Kalra
2021-08-26 14:57 ` [dpdk-dev] [RFC 5/7] drivers: remove direct access to interrupt handle fields Harman Kalra
2021-08-26 14:57 ` [dpdk-dev] [RFC 6/7] eal/interrupts: make interrupt handle structure opaque Harman Kalra
2021-08-26 14:57 ` [dpdk-dev] [RFC 7/7] eal/alarm: introduce alarm fini routine Harman Kalra
2021-09-03 12:40 ` [dpdk-dev] [PATCH v1 0/7] make rte_intr_handle internal Harman Kalra
2021-09-03 12:40   ` [dpdk-dev] [PATCH v1 1/7] eal: interrupt handle API prototypes Harman Kalra
2021-09-03 12:40   ` [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement get set APIs Harman Kalra
2021-09-28 15:46     ` David Marchand
2021-10-04  8:51       ` [dpdk-dev] [EXT] " Harman Kalra
2021-10-04  9:57         ` David Marchand
2021-10-12 15:22           ` Thomas Monjalon
2021-10-13 17:54             ` Harman Kalra
2021-10-13 17:57               ` Harman Kalra
2021-10-13 18:52                 ` Thomas Monjalon
2021-10-14  8:22                   ` Thomas Monjalon
2021-10-14  9:31                     ` Harman Kalra
2021-10-14  9:37                       ` David Marchand
2021-10-14  9:41                       ` Thomas Monjalon
2021-10-14 10:31                         ` Harman Kalra
2021-10-14 10:35                           ` Thomas Monjalon
2021-10-14 10:44                             ` Harman Kalra
2021-10-14 12:04                               ` Thomas Monjalon
2021-10-14 10:25                       ` Dmitry Kozlyuk
2021-10-03 18:05     ` Dmitry Kozlyuk [this message]
2021-10-04 10:37       ` Harman Kalra
2021-10-04 11:18         ` Dmitry Kozlyuk
2021-10-04 14:03           ` Harman Kalra
2021-09-03 12:40   ` [dpdk-dev] [PATCH v1 3/7] eal/interrupts: avoid direct access to interrupt handle Harman Kalra
2021-09-03 12:40   ` [dpdk-dev] [PATCH v1 4/7] test/interrupt: apply get set interrupt handle APIs Harman Kalra
2021-09-03 12:41   ` [dpdk-dev] [PATCH v1 5/7] drivers: remove direct access to interrupt handle fields Harman Kalra
2021-09-03 12:41   ` [dpdk-dev] [PATCH v1 6/7] eal/interrupts: make interrupt handle structure opaque Harman Kalra
2021-10-03 18:16     ` Dmitry Kozlyuk
2021-10-04 14:09       ` [dpdk-dev] [EXT] " Harman Kalra
2021-09-03 12:41   ` [dpdk-dev] [PATCH v1 7/7] eal/alarm: introduce alarm fini routine Harman Kalra
2021-09-15 14:13   ` [dpdk-dev] [PATCH v1 0/7] make rte_intr_handle internal Harman Kalra
2021-09-23  8:20   ` David Marchand
2021-10-05 12:14 ` [dpdk-dev] [PATCH v2 0/6] " Harman Kalra
2021-10-05 12:14   ` [dpdk-dev] [PATCH v2 1/6] eal/interrupts: implement get set APIs Harman Kalra
2021-10-14  0:58     ` Dmitry Kozlyuk
2021-10-14 17:15       ` [dpdk-dev] [EXT] " Harman Kalra
2021-10-14 17:53         ` Dmitry Kozlyuk
2021-10-15  7:53           ` Thomas Monjalon
2021-10-14  7:31     ` [dpdk-dev] " David Marchand
2021-10-05 12:14   ` [dpdk-dev] [PATCH v2 2/6] eal/interrupts: avoid direct access to interrupt handle Harman Kalra
2021-10-14  0:59     ` Dmitry Kozlyuk
2021-10-14 17:31       ` [dpdk-dev] [EXT] " Harman Kalra
2021-10-14 17:53         ` Dmitry Kozlyuk
2021-10-05 12:14   ` [dpdk-dev] [PATCH v2 3/6] test/interrupt: apply get set interrupt handle APIs Harman Kalra
2021-10-05 12:15   ` [dpdk-dev] [PATCH v2 4/6] drivers: remove direct access to interrupt handle Harman Kalra
2021-10-05 12:15   ` [dpdk-dev] [PATCH v2 5/6] eal/interrupts: make interrupt handle structure opaque Harman Kalra
2021-10-05 12:15   ` [dpdk-dev] [PATCH v2 6/6] eal/alarm: introduce alarm fini routine Harman Kalra
2021-10-05 16:07 ` [dpdk-dev] [RFC 0/7] make rte_intr_handle internal Stephen Hemminger
2021-10-07 10:57   ` [dpdk-dev] [EXT] " Harman Kalra
2021-10-18 19:37 ` [dpdk-dev] [PATCH v3 " Harman Kalra
2021-10-18 19:37   ` [dpdk-dev] [PATCH v3 1/7] malloc: introduce malloc is ready API Harman Kalra
2021-10-19 15:53     ` Thomas Monjalon
2021-10-18 19:37   ` [dpdk-dev] [PATCH v3 2/7] eal/interrupts: implement get set APIs Harman Kalra
2021-10-18 22:07     ` Dmitry Kozlyuk
2021-10-19  8:50       ` [dpdk-dev] [EXT] " Harman Kalra
2021-10-19 18:44         ` Harman Kalra
2021-10-18 22:56     ` [dpdk-dev] " Stephen Hemminger
2021-10-19  8:32       ` [dpdk-dev] [EXT] " Harman Kalra
2021-10-19 15:58         ` Thomas Monjalon
2021-10-20 15:30         ` Dmitry Kozlyuk
2021-10-21  9:16           ` Harman Kalra
2021-10-21 12:33             ` Dmitry Kozlyuk
2021-10-21 13:32               ` David Marchand
2021-10-21 16:05                 ` Harman Kalra
2021-10-18 19:37   ` [dpdk-dev] [PATCH v3 3/7] eal/interrupts: avoid direct access to interrupt handle Harman Kalra
2021-10-18 19:37   ` [dpdk-dev] [PATCH v3 4/7] test/interrupt: apply get set interrupt handle APIs Harman Kalra
2021-10-18 19:37   ` [dpdk-dev] [PATCH v3 5/7] drivers: remove direct access to interrupt handle Harman Kalra
2021-10-18 19:37   ` [dpdk-dev] [PATCH v3 6/7] eal/interrupts: make interrupt handle structure opaque Harman Kalra
2021-10-18 19:37   ` [dpdk-dev] [PATCH v3 7/7] eal/alarm: introduce alarm fini routine Harman Kalra
2021-10-19 18:35 ` [dpdk-dev] [PATCH v4 0/7] make rte_intr_handle internal Harman Kalra
2021-10-19 18:35   ` [dpdk-dev] [PATCH v4 1/7] malloc: introduce malloc is ready API Harman Kalra
2021-10-19 22:01     ` Dmitry Kozlyuk
2021-10-19 22:04       ` Dmitry Kozlyuk
2021-10-20  9:01         ` [dpdk-dev] [EXT] " Harman Kalra
2021-10-19 18:35   ` [dpdk-dev] [PATCH v4 2/7] eal/interrupts: implement get set APIs Harman Kalra
2021-10-20  6:14     ` David Marchand
2021-10-20 14:29       ` Dmitry Kozlyuk
2021-10-20 16:15     ` Dmitry Kozlyuk
2021-10-19 18:35   ` [dpdk-dev] [PATCH v4 3/7] eal/interrupts: avoid direct access to interrupt handle Harman Kalra
2021-10-19 21:27     ` Dmitry Kozlyuk
2021-10-20  9:25       ` [dpdk-dev] [EXT] " Harman Kalra
2021-10-20  9:52         ` Dmitry Kozlyuk
2021-10-19 18:35   ` [dpdk-dev] [PATCH v4 4/7] test/interrupt: apply get set interrupt handle APIs Harman Kalra
2021-10-19 18:35   ` [dpdk-dev] [PATCH v4 5/7] drivers: remove direct access to interrupt handle Harman Kalra
2021-10-20  1:57     ` Hyong Youb Kim (hyonkim)
2021-10-19 18:35   ` [dpdk-dev] [PATCH v4 6/7] eal/interrupts: make interrupt handle structure opaque Harman Kalra
2021-10-19 18:35   ` [dpdk-dev] [PATCH v4 7/7] eal/alarm: introduce alarm fini routine Harman Kalra
2021-10-19 21:39     ` Dmitry Kozlyuk
2021-10-22 20:49 ` [dpdk-dev] [PATCH v5 0/6] make rte_intr_handle internal Harman Kalra
2021-10-22 20:49   ` [dpdk-dev] [PATCH v5 1/6] eal/interrupts: implement get set APIs Harman Kalra
2021-10-22 23:33     ` Dmitry Kozlyuk
2021-10-22 20:49   ` [dpdk-dev] [PATCH v5 2/6] eal/interrupts: avoid direct access to interrupt handle Harman Kalra
2021-10-22 23:33     ` Dmitry Kozlyuk
2021-10-22 20:49   ` [dpdk-dev] [PATCH v5 3/6] test/interrupt: apply get set interrupt handle APIs Harman Kalra
2021-10-22 20:49   ` [dpdk-dev] [PATCH v5 4/6] drivers: remove direct access to interrupt handle Harman Kalra
2021-10-22 20:49   ` [dpdk-dev] [PATCH v5 5/6] eal/interrupts: make interrupt handle structure opaque Harman Kalra
2021-10-22 23:33     ` Dmitry Kozlyuk
2021-10-22 20:49   ` [dpdk-dev] [PATCH v5 6/6] eal/alarm: introduce alarm fini routine Harman Kalra
2021-10-22 23:33     ` Dmitry Kozlyuk
2021-10-22 23:37       ` Dmitry Kozlyuk
2021-10-24 20:04   ` [dpdk-dev] [PATCH v6 0/9] make rte_intr_handle internal David Marchand
2021-10-24 20:04     ` [dpdk-dev] [PATCH v6 1/9] interrupts: add allocator and accessors David Marchand
2021-10-24 20:04     ` [dpdk-dev] [PATCH v6 2/9] interrupts: remove direct access to interrupt handle David Marchand
2021-10-25  6:57       ` David Marchand
2021-10-24 20:04     ` [dpdk-dev] [PATCH v6 3/9] test/interrupts: " David Marchand
2021-10-24 20:04     ` [dpdk-dev] [PATCH v6 4/9] alarm: " David Marchand
2021-10-25 10:49       ` Dmitry Kozlyuk
2021-10-25 11:09         ` David Marchand
2021-10-24 20:04     ` [dpdk-dev] [PATCH v6 5/9] lib: " David Marchand
2021-10-24 20:04     ` [dpdk-dev] [PATCH v6 6/9] drivers: " David Marchand
2021-10-24 20:04     ` [dpdk-dev] [PATCH v6 7/9] interrupts: make interrupt handle structure opaque David Marchand
2021-10-24 20:04     ` [dpdk-dev] [PATCH v6 8/9] interrupts: rename device specific file descriptor David Marchand
2021-10-24 20:04     ` [dpdk-dev] [PATCH v6 9/9] interrupts: extend event list David Marchand
2021-10-25 10:49       ` Dmitry Kozlyuk
2021-10-25 11:11         ` David Marchand
2021-10-25 13:04   ` [dpdk-dev] [PATCH v5 0/6] make rte_intr_handle internal Raslan Darawsheh
2021-10-25 13:09     ` David Marchand
2021-10-25 13:34   ` [dpdk-dev] [PATCH v7 0/9] " David Marchand
2021-10-25 13:34     ` [dpdk-dev] [PATCH v7 1/9] interrupts: add allocator and accessors David Marchand
2021-10-25 13:34     ` [dpdk-dev] [PATCH v7 2/9] interrupts: remove direct access to interrupt handle David Marchand
2021-10-25 13:34     ` [dpdk-dev] [PATCH v7 3/9] test/interrupts: " David Marchand
2021-10-25 13:34     ` [dpdk-dev] [PATCH v7 4/9] alarm: " David Marchand
2021-10-25 13:34     ` [dpdk-dev] [PATCH v7 5/9] lib: " David Marchand
2021-10-28  6:14       ` Jiang, YuX
2021-10-25 13:34     ` [dpdk-dev] [PATCH v7 6/9] drivers: " David Marchand
2021-10-25 13:34     ` [dpdk-dev] [PATCH v7 7/9] interrupts: make interrupt handle structure opaque David Marchand
2021-10-25 13:34     ` [dpdk-dev] [PATCH v7 8/9] interrupts: rename device specific file descriptor David Marchand
2021-10-25 13:34     ` [dpdk-dev] [PATCH v7 9/9] interrupts: extend event list David Marchand
2021-10-25 14:27   ` [dpdk-dev] [PATCH v8 0/9] make rte_intr_handle internal David Marchand
2021-10-25 14:27     ` [dpdk-dev] [PATCH v8 1/9] interrupts: add allocator and accessors David Marchand
2021-10-25 14:27     ` [dpdk-dev] [PATCH v8 2/9] interrupts: remove direct access to interrupt handle David Marchand
2021-10-25 14:27     ` [dpdk-dev] [PATCH v8 3/9] test/interrupts: " David Marchand
2021-10-25 14:27     ` [dpdk-dev] [PATCH v8 4/9] alarm: " David Marchand
2021-10-25 14:27     ` [dpdk-dev] [PATCH v8 5/9] lib: " David Marchand
2021-10-25 14:27     ` [dpdk-dev] [PATCH v8 6/9] drivers: " David Marchand
2021-10-25 14:27     ` [dpdk-dev] [PATCH v8 7/9] interrupts: make interrupt handle structure opaque David Marchand
2021-10-25 14:27     ` [dpdk-dev] [PATCH v8 8/9] interrupts: rename device specific file descriptor David Marchand
2021-10-25 14:27     ` [dpdk-dev] [PATCH v8 9/9] interrupts: extend event list David Marchand
2021-10-28 15:58       ` Ji, Kai
2021-10-28 17:16         ` David Marchand
2021-10-25 14:32     ` [dpdk-dev] [PATCH v8 0/9] make rte_intr_handle internal Raslan Darawsheh
2021-10-25 19:24     ` David Marchand

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=20211003210505.1c852bd4@sovereign \
    --to=dmitry.kozliuk@gmail.com \
    --cc=dev@dpdk.org \
    --cc=hkalra@marvell.com \
    --cc=mdr@ashroe.eu \
    /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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git