From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id A6137A0613 for ; Tue, 24 Sep 2019 16:14:45 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 69FBD343C; Tue, 24 Sep 2019 16:14:45 +0200 (CEST) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by dpdk.org (Postfix) with ESMTP id 6F5E12BE6 for ; Tue, 24 Sep 2019 16:14:43 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Sep 2019 07:14:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,544,1559545200"; d="scan'208";a="213700155" Received: from yexl-server.sh.intel.com (HELO localhost) ([10.67.117.17]) by fmsmga004.fm.intel.com with ESMTP; 24 Sep 2019 07:14:40 -0700 Date: Tue, 24 Sep 2019 22:12:33 +0800 From: Ye Xiaolong To: Ciara Loftus Cc: dev@dpdk.org, kevin.laatz@intel.com, bruce.richardson@intel.com Message-ID: <20190924141233.GA67866@intel.com> References: <20190919141520.4227-1-ciara.loftus@intel.com> <20190919141520.4227-3-ciara.loftus@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190919141520.4227-3-ciara.loftus@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH 2/3] net/af_xdp: support pinning of IRQs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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= " > "start_queue= " > "queue_count= " >- "pmd_zero_copy=<0|1>"); >+ "pmd_zero_copy=<0|1> " >+ "queue_irq=:"); > > RTE_INIT(af_xdp_init_log) > { >-- >2.17.1 >