From: Ye Xiaolong <xiaolong.ye@intel.com>
To: Ciara Loftus <ciara.loftus@intel.com>
Cc: dev@dpdk.org, kevin.laatz@intel.com, bruce.richardson@intel.com
Subject: Re: [dpdk-dev] [PATCH 2/3] net/af_xdp: support pinning of IRQs
Date: Tue, 24 Sep 2019 22:12:33 +0800 [thread overview]
Message-ID: <20190924141233.GA67866@intel.com> (raw)
In-Reply-To: <20190919141520.4227-3-ciara.loftus@intel.com>
On 09/19, Ciara Loftus wrote:
[snip]
>+/* drivers supported for the queue_irq option */
>+enum {I40E_DRIVER, IXGBE_DRIVER, MLX5_DRIVER, NUM_DRIVERS};
Minor nit, how about using below format for readability and align with other
enum type definition in DPDK?
enum supported_driver {
I40E_DRIVER,
IXGBE_DRIVER,
MLX5_DRIVER,
NUM_DRIVERS
};
>+char driver_array[NUM_DRIVERS][NAME_MAX] = {"i40e", "ixgbe", "mlx5_core"};
[snip]
>+ * function for getting the index into driver_handlers array that corresponds
>+ * to 'driver'
>+ */
>+static int
>+get_driver_idx(char *driver)
const char *driver
>+{
>+ for (int i = 0; i < NUM_DRIVERS; i++) {
>+ if (strncmp(driver, driver_array[i], strlen(driver_array[i])))
>+ continue;
>+ return i;
>+ }
>+
>+ return -1;
>+}
>+
>+/** generate /proc/interrupts search regex based on driver type */
>+static int
>+generate_search_regex(const char *driver, struct pmd_internals *internals,
>+ uint16_t netdev_qid, regex_t *r)
I'd prefer put *internals as the first parameter.
>+{
>+ char iface_regex_str[128];
>+ int ret = -1;
>+ char *driver_dup = strdup(driver);
>+ int idx = get_driver_idx(driver_dup);
Why not using driver directly?
>+
>+ if (idx == -1) {
>+ AF_XDP_LOG(ERR, "Error getting driver index for %s\n",
>+ internals->if_name);
>+ goto out;
>+ }
>+
>+ if (driver_handlers[idx](iface_regex_str, internals, netdev_qid)) {
>+ AF_XDP_LOG(ERR, "Error getting regex string for %s\n",
>+ internals->if_name);
>+ goto out;
>+ }
Need to check whether driver_handlers[idex] exists.
>+
>+ if (regcomp(r, iface_regex_str, 0)) {
>+ AF_XDP_LOG(ERR, "Error computing regex %s\n", iface_regex_str);
>+ goto out;
>+ }
>+
>+ ret = 0;
>+
>+out:
>+ free(driver_dup);
>+ return ret;
>+}
>+
>+/** get interrupt number associated with the given interface qid */
>+static int
>+get_interrupt_number(regex_t *r, int *interrupt,
>+ struct pmd_internals *internals)
Better to put the input before output in parameter list, I'd prefer
get_interrupt_number(struct pmd_internals *internals,
regex_t *r, int *interrupt)
>+{
>+ FILE *f_int_proc;
>+ int found = 0;
>+ char line[4096];
>+ int ret = 0;
>+
>+ f_int_proc = fopen("/proc/interrupts", "r");
>+ if (f_int_proc == NULL) {
>+ AF_XDP_LOG(ERR, "Failed to open /proc/interrupts.\n");
>+ return -1;
>+ }
>+
>+ while (!feof(f_int_proc) && !found) {
>+ /* Make sure to read a full line at a time */
>+ if (fgets(line, sizeof(line), f_int_proc) == NULL ||
>+ line[strlen(line) - 1] != '\n') {
>+ AF_XDP_LOG(ERR, "Error reading from interrupts file\n");
>+ ret = -1;
>+ break;
>+ }
>+
>+ /* Extract interrupt number from line */
>+ if (regexec(r, line, 0, NULL, 0) == 0) {
>+ *interrupt = atoi(line);
>+ found = true;
>+ AF_XDP_LOG(INFO, "Got interrupt %d for %s\n",
>+ *interrupt, internals->if_name);
>+ }
>+ }
>+
>+ fclose(f_int_proc);
>+
>+ return ret;
>+}
>+
>+/** affinitise interrupts for the given qid to the given coreid */
>+static int
>+set_irq_affinity(int coreid, struct pmd_internals *internals,
>+ uint16_t rx_queue_id, uint16_t netdev_qid, int interrupt)
Prefer to put *internals in the beginning.
>+{
>+ char bitmask[128];
>+ char smp_affinity_filename[NAME_MAX];
>+ FILE *f_int_smp_affinity;
>+ int i;
>+
>+ /* Create affinity bitmask. Every 32 bits are separated by a comma */
>+ snprintf(bitmask, sizeof(bitmask), "%x", 1 << (coreid % 32));
>+ for (i = 0; i < coreid / 32; i++)
>+ strlcat(bitmask, ",00000000", sizeof(bitmask));
>+
>+ /* Write the new affinity bitmask */
>+ snprintf(smp_affinity_filename, sizeof(smp_affinity_filename),
>+ "/proc/irq/%d/smp_affinity", interrupt);
>+ f_int_smp_affinity = fopen(smp_affinity_filename, "w");
>+ if (f_int_smp_affinity == NULL) {
>+ AF_XDP_LOG(ERR, "Error opening %s\n", smp_affinity_filename);
>+ return -1;
>+ }
>+ fwrite(bitmask, strlen(bitmask), 1, f_int_smp_affinity);
Need to check the return value of fwrite, otherwise coverity may complain.
>+ fclose(f_int_smp_affinity);
>+ AF_XDP_LOG(INFO, "IRQs for %s ethdev queue %i (netdev queue %i)"
>+ " affinitised to core %i\n",
>+ internals->if_name, rx_queue_id,
>+ netdev_qid, coreid);
>+
>+ return 0;
>+}
>+
>+static void
>+configure_irqs(struct pmd_internals *internals, uint16_t rx_queue_id)
>+{
>+ int coreid = internals->queue_irqs[rx_queue_id];
>+ char driver[NAME_MAX];
>+ uint16_t netdev_qid = rx_queue_id + internals->start_queue_idx;
>+ regex_t r;
>+ int interrupt;
>+
>+ if (coreid < 0)
>+ return;
>+
>+ if (coreid > (get_nprocs() - 1)) {
>+ AF_XDP_LOG(ERR, "Affinitisation failed - invalid coreid %i\n",
>+ coreid);
>+ return;
>+ }
I think we can combine above 2 sanity checks together.
>+
>+ if (get_driver_name(internals, driver)) {
>+ AF_XDP_LOG(ERR, "Error retrieving driver name for %s\n",
>+ internals->if_name);
>+ return;
>+ }
>+
>+ if (generate_search_regex(driver, internals, netdev_qid, &r)) {
>+ AF_XDP_LOG(ERR, "Error generating search regex for %s\n",
>+ internals->if_name);
>+ return;
>+ }
>+
>+ if (get_interrupt_number(&r, &interrupt, internals)) {
>+ AF_XDP_LOG(ERR, "Error getting interrupt number for %s\n",
>+ internals->if_name);
>+ return;
>+ }
>+
>+ if (set_irq_affinity(coreid, internals, rx_queue_id, netdev_qid,
>+ interrupt)) {
>+ AF_XDP_LOG(ERR, "Error setting interrupt affinity for %s\n",
>+ internals->if_name);
>+ return;
>+ }
>+}
>+
> static int
> eth_rx_queue_setup(struct rte_eth_dev *dev,
> uint16_t rx_queue_id,
>@@ -697,6 +996,8 @@ eth_rx_queue_setup(struct rte_eth_dev *dev,
> goto err;
> }
>
>+ configure_irqs(internals, rx_queue_id);
>+
> rxq->fds[0].fd = xsk_socket__fd(rxq->xsk);
> rxq->fds[0].events = POLLIN;
>
>@@ -834,6 +1135,39 @@ parse_name_arg(const char *key __rte_unused,
> return 0;
> }
>
>+/** parse queue irq argument */
>+static int
>+parse_queue_irq_arg(const char *key __rte_unused,
>+ const char *value, void *extra_args)
>+{
>+ int (*queue_irqs)[RTE_MAX_QUEUES_PER_PORT] = extra_args;
>+ char *parse_str = strdup(value);
>+ char delimiter[] = ":";
>+ char *queue_str;
>+
>+ queue_str = strtok(parse_str, delimiter);
>+ if (queue_str != NULL && strncmp(queue_str, value, strlen(value))) {
>+ char *end;
>+ long queue = strtol(queue_str, &end, 10);
>+
>+ if (*end == '\0' && queue >= 0 &&
>+ queue < RTE_MAX_QUEUES_PER_PORT) {
>+ char *core_str = strtok(NULL, delimiter);
>+ long core = strtol(core_str, &end, 10);
>+
>+ if (*end == '\0' && core >= 0 && core < get_nprocs()) {
>+ (*queue_irqs)[queue] = core;
>+ free(parse_str);
>+ return 0;
>+ }
>+ }
>+ }
>+
>+ AF_XDP_LOG(ERR, "Invalid queue_irq argument.\n");
>+ free(parse_str);
>+ return -1;
>+}
>+
> static int
> xdp_get_channels_info(const char *if_name, int *max_queues,
> int *combined_queues)
>@@ -877,7 +1211,8 @@ xdp_get_channels_info(const char *if_name, int *max_queues,
>
> static int
> parse_parameters(struct rte_kvargs *kvlist, char *if_name, int *start_queue,
>- int *queue_cnt, int *pmd_zc)
>+ int *queue_cnt, int *pmd_zc,
>+ int (*queue_irqs)[RTE_MAX_QUEUES_PER_PORT])
> {
> int ret;
>
>@@ -903,6 +1238,11 @@ parse_parameters(struct rte_kvargs *kvlist, char *if_name, int *start_queue,
> if (ret < 0)
> goto free_kvlist;
>
>+ ret = rte_kvargs_process(kvlist, ETH_AF_XDP_QUEUE_IRQ_ARG,
>+ &parse_queue_irq_arg, queue_irqs);
>+ if (ret < 0)
>+ goto free_kvlist;
>+
> free_kvlist:
> rte_kvargs_free(kvlist);
> return ret;
>@@ -940,7 +1280,8 @@ get_iface_info(const char *if_name,
>
> static struct rte_eth_dev *
> init_internals(struct rte_vdev_device *dev, const char *if_name,
>- int start_queue_idx, int queue_cnt, int pmd_zc)
>+ int start_queue_idx, int queue_cnt, int pmd_zc,
>+ int queue_irqs[RTE_MAX_QUEUES_PER_PORT])
> {
> const char *name = rte_vdev_device_name(dev);
> const unsigned int numa_node = dev->device.numa_node;
>@@ -957,6 +1298,8 @@ init_internals(struct rte_vdev_device *dev, const char *if_name,
> internals->queue_cnt = queue_cnt;
> internals->pmd_zc = pmd_zc;
> strlcpy(internals->if_name, if_name, IFNAMSIZ);
>+ memcpy(internals->queue_irqs, queue_irqs,
Use rte_memcpy instead.
Thanks,
Xiaolong
>+ sizeof(int) * RTE_MAX_QUEUES_PER_PORT);
>
> if (xdp_get_channels_info(if_name, &internals->max_queue_cnt,
> &internals->combined_queue_cnt)) {
>@@ -1035,6 +1378,9 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device *dev)
> struct rte_eth_dev *eth_dev = NULL;
> const char *name;
> int pmd_zc = 0;
>+ int queue_irqs[RTE_MAX_QUEUES_PER_PORT];
>+
>+ memset(queue_irqs, -1, sizeof(int) * RTE_MAX_QUEUES_PER_PORT);
>
> AF_XDP_LOG(INFO, "Initializing pmd_af_xdp for %s\n",
> rte_vdev_device_name(dev));
>@@ -1062,7 +1408,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device *dev)
> dev->device.numa_node = rte_socket_id();
>
> if (parse_parameters(kvlist, if_name, &xsk_start_queue_idx,
>- &xsk_queue_cnt, &pmd_zc) < 0) {
>+ &xsk_queue_cnt, &pmd_zc, &queue_irqs) < 0) {
> AF_XDP_LOG(ERR, "Invalid kvargs value\n");
> return -EINVAL;
> }
>@@ -1073,7 +1419,7 @@ rte_pmd_af_xdp_probe(struct rte_vdev_device *dev)
> }
>
> eth_dev = init_internals(dev, if_name, xsk_start_queue_idx,
>- xsk_queue_cnt, pmd_zc);
>+ xsk_queue_cnt, pmd_zc, queue_irqs);
> if (eth_dev == NULL) {
> AF_XDP_LOG(ERR, "Failed to init internals\n");
> return -1;
>@@ -1117,7 +1463,8 @@ RTE_PMD_REGISTER_PARAM_STRING(net_af_xdp,
> "iface=<string> "
> "start_queue=<int> "
> "queue_count=<int> "
>- "pmd_zero_copy=<0|1>");
>+ "pmd_zero_copy=<0|1> "
>+ "queue_irq=<int>:<int>");
>
> RTE_INIT(af_xdp_init_log)
> {
>--
>2.17.1
>
next prev parent reply other threads:[~2019-09-24 14:14 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-19 14:15 [dpdk-dev] [PATCH 0/3] AF_XDP tx halt fix, IRQ pinning and unaligned chunks Ciara Loftus
2019-09-19 14:15 ` [dpdk-dev] [PATCH 1/3] net/af_xdp: fix Tx halt when no recv packets Ciara Loftus
2019-09-19 14:15 ` [dpdk-dev] [PATCH 2/3] net/af_xdp: support pinning of IRQs Ciara Loftus
2019-09-24 14:12 ` Ye Xiaolong [this message]
2019-09-27 13:21 ` Loftus, Ciara
2019-09-27 14:06 ` Ye Xiaolong
2019-09-24 16:42 ` Stephen Hemminger
2019-09-19 14:15 ` [dpdk-dev] [PATCH 3/3] net/af_xdp: enable support for unaligned umem chunks Ciara Loftus
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=20190924141233.GA67866@intel.com \
--to=xiaolong.ye@intel.com \
--cc=bruce.richardson@intel.com \
--cc=ciara.loftus@intel.com \
--cc=dev@dpdk.org \
--cc=kevin.laatz@intel.com \
/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).