From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id E6ADA237 for ; Mon, 18 Sep 2017 06:54:32 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga105.fm.intel.com with ESMTP; 17 Sep 2017 21:54:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,411,1500966000"; d="scan'208";a="1015470817" Received: from nikhilr-mobl.amr.corp.intel.com (HELO [10.106.152.71]) ([10.106.152.71]) by orsmga003.jf.intel.com with ESMTP; 17 Sep 2017 21:54:26 -0700 To: Nipun Gupta , "jerin.jacob@caviumnetworks.com" , "bruce.richardson@intel.com" Cc: "gage.eads@intel.com" , "dev@dpdk.org" , "thomas@monjalon.net" , "harry.van.haaren@intel.com" , Hemant Agrawal , "narender.vangati@intel.com" , "erik.g.carrillo@intel.com" , "abhinandan.gujjar@intel.com" References: <04bcb240-51fb-50dc-833c-60c33a420d6f@intel.com> <1505328781-23456-1-git-send-email-nikhil.rao@intel.com> From: "Rao, Nikhil" Message-ID: <4b5c1d2f-84d6-4e99-d57d-128a71005993@intel.com> Date: Mon, 18 Sep 2017 10:24:25 +0530 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 3/4] eventdev: Add eventdev ethernet Rx adapter 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: , X-List-Received-Date: Mon, 18 Sep 2017 04:54:33 -0000 On 9/15/2017 11:37 AM, Nipun Gupta wrote: > > >> -----Original Message----- >> From: Nikhil Rao [mailto:nikhil.rao@intel.com] >> +static inline void >> +fill_event_buffer(struct rte_event_eth_rx_adapter *rx_adapter, >> + uint8_t dev_id, >> + uint16_t rx_queue_id, >> + struct rte_mbuf **mbufs, >> + uint16_t num) >> +{ >> + uint32_t i; >> + struct eth_device_info *eth_device_info = >> + &rx_adapter->eth_devices[dev_id]; >> + struct eth_rx_queue_info *eth_rx_queue_info = >> + ð_device_info- >>> rx_queue[rx_queue_id]; >> + >> + int32_t qid = eth_rx_queue_info->event_queue_id; >> + uint8_t sched_type = eth_rx_queue_info->sched_type; >> + uint8_t priority = eth_rx_queue_info->priority; >> + uint32_t flow_id; >> + struct rte_event events[BATCH_SIZE]; >> + struct rte_mbuf *m = mbufs[0]; >> + uint32_t rss_mask; >> + uint32_t rss; >> + int do_rss; >> + >> + /* 0xffff ffff if PKT_RX_RSS_HASH is set, otherwise 0 */ >> + rss_mask = ~(((m->ol_flags & PKT_RX_RSS_HASH) != 0) - 1); >> + do_rss = !rss_mask && !eth_rx_queue_info->flow_id_mask; >> + >> + for (i = 0; i < num; i++) { >> + m = mbufs[i]; >> + struct rte_event *ev = &events[i]; >> + >> + rss = do_rss ? do_softrss(m) : m->hash.rss; >> + flow_id = >> + eth_rx_queue_info->flow_id & >> + eth_rx_queue_info->flow_id_mask; >> + flow_id |= rss & ~eth_rx_queue_info->flow_id_mask; >> + >> + ev->flow_id = flow_id; >> + ev->op = RTE_EVENT_OP_NEW; >> + ev->sched_type = sched_type; >> + ev->queue_id = qid; >> + ev->event_type = RTE_EVENT_TYPE_ETHDEV; >> + ev->sub_event_type = 0; >> + ev->priority = priority; >> + ev->mbuf = m; > > How will the application get to know about the queue and eth_port from where the > packet is received by the eventdev? Is it the user responsibility to have that information > in the flow_id? queue: If the application needs to know the queue ID, it would have to set the flow ID to the queue ID at the time of adding the queue. eth port: input port contained in in the mbuf. >> + >> + buf_event_enqueue(rx_adapter, ev); >> + } >> +} >> + > > > >> +int >> +rte_event_eth_rx_adapter_queue_add(uint8_t id, >> + uint8_t eth_dev_id, >> + int32_t rx_queue_id, >> + const struct rte_event_eth_rx_adapter_queue_conf >> *queue_conf) >> +{ >> + int ret; >> + uint32_t rx_adapter_cap; >> + struct rte_event_eth_rx_adapter *rx_adapter; >> + struct rte_eventdev *dev; >> + struct eth_device_info *dev_info; >> + int start_service = 0; >> + >> + RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL); >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(eth_dev_id, -EINVAL); >> + >> + rx_adapter = id_to_rx_adapter(id); >> + if (!rx_adapter || !queue_conf) >> + return -EINVAL; >> + >> + dev = &rte_eventdevs[rx_adapter->eventdev_id]; >> + ret = (*dev->dev_ops->eth_rx_adapter_caps_get)(dev, eth_dev_id, >> + &rx_adapter_cap); >> + if (ret) { >> + RTE_EDEV_LOG_ERR("Failed to get adapter caps edev %" PRIu8 >> + "eth port %" PRIu8, id, eth_dev_id); >> + return ret; >> + } >> + >> + if (!(rx_adapter_cap & RTE_EVENT_ETH_RX_ADAPTER_CAP_FLOW_ID) >> && >> + !(queue_conf->rx_queue_flags & >> + >> RTE_EVENT_ETH_RX_ADAPTER_QUEUE_FLOW_ID_VALID)) { >> + RTE_EDEV_LOG_ERR("Flow ID required for configuration," >> + " eth port: %" PRIu8 " adapter id: %" PRIu8, >> + eth_dev_id, id); >> + return -EINVAL; >> + } >> + >> + if ((rx_adapter_cap & >> RTE_EVENT_ETH_RX_ADAPTER_CAP_SINGLE_EVENTQ) && >> + (rx_queue_id != -1)) { >> + RTE_EDEV_LOG_ERR("Rx queues can only be connected to >> single " >> + "event queue id %u eth port %u", id, eth_dev_id); >> + return -EINVAL; >> + } >> + >> + if (rx_queue_id != -1 && (uint16_t)rx_queue_id >= >> + rte_eth_devices[eth_dev_id].data->nb_rx_queues) { >> + RTE_EDEV_LOG_ERR("Invalid rx queue_id %" PRIu16, >> + (uint16_t)rx_queue_id); >> + return -EINVAL; >> + } >> + >> + start_service = 0; >> + dev_info = &rx_adapter->eth_devices[eth_dev_id]; >> + >> + if (rx_adapter_cap & >> RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT) { >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops- >>> eth_rx_adapter_queue_add, >> + -ENOTSUP); >> + if (!dev_info->rx_queue) { >> + dev_info->rx_queue = >> + rte_zmalloc_socket(rx_adapter->mem_name, >> + dev_info->dev->data->nb_rx_queues * >> + sizeof(struct eth_rx_queue_info), 0, >> + rx_adapter->socket_id); >> + if (!dev_info->rx_queue) >> + return -ENOMEM; >> + } >> + >> + ret = (*dev->dev_ops->eth_rx_adapter_queue_add)(dev, >> eth_dev_id, >> + rx_queue_id, queue_conf); > > IMO, Instead of eth_dev_id rte_eth_dev parameter should be passed to the underlying driver. > Otherwise the underlying driver needs to use rte_eth_devices[] global variable to fetch the device. > This does not seems very right to me. Agree, passing struct rte_eth_dev to the PMD is a cleaner approach. > >> + if (!ret) { >> + update_queue_info(rx_adapter, >> + &rx_adapter- >>> eth_devices[eth_dev_id], >> + rx_queue_id, >> + 1); >> + } >> + } else { >> + rte_spinlock_lock(&rx_adapter->rx_lock); >> + ret = init_service(rx_adapter, id); >> + if (!ret) >> + ret = add_rx_queue(rx_adapter, eth_dev_id, >> rx_queue_id, >> + queue_conf); >> + rte_spinlock_unlock(&rx_adapter->rx_lock); >> + if (!ret) >> + start_service = >> !!sw_rx_adapter_queue_count(rx_adapter); >> + } >> + >> + if (ret) >> + return ret; >> + >> + if (start_service) >> + rte_service_component_runstate_set(rx_adapter->service_id, >> 1); >> + >> + return 0; >> +} >> + >> +int >> +rte_event_eth_rx_adapter_queue_del(uint8_t id, uint8_t eth_dev_id, >> + int32_t rx_queue_id) >> +{ >> + int ret = 0; >> + struct rte_eventdev *dev; >> + struct rte_event_eth_rx_adapter *rx_adapter; >> + struct eth_device_info *dev_info; >> + uint32_t rx_adapter_cap; >> + uint16_t i; >> + >> + RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL); >> + RTE_ETH_VALID_PORTID_OR_ERR_RET(eth_dev_id, -EINVAL); >> + >> + rx_adapter = id_to_rx_adapter(id); >> + if (!rx_adapter) >> + return -EINVAL; >> + >> + dev = &rte_eventdevs[rx_adapter->eventdev_id]; >> + ret = dev->dev_ops->eth_rx_adapter_caps_get(dev, eth_dev_id, >> + &rx_adapter_cap); >> + if (ret) >> + return ret; >> + >> + if (rx_queue_id != -1 && (uint16_t)rx_queue_id >= >> + rte_eth_devices[eth_dev_id].data->nb_rx_queues) { >> + RTE_EDEV_LOG_ERR("Invalid rx queue_id %" PRIu16, >> + (uint16_t)rx_queue_id); >> + return -EINVAL; >> + } >> + >> + dev_info = &rx_adapter->eth_devices[eth_dev_id]; >> + >> + if (rx_adapter_cap & >> RTE_EVENT_ETH_RX_ADAPTER_CAP_INTERNAL_PORT) { >> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops- >>> eth_rx_adapter_queue_del, >> + -ENOTSUP); >> + ret = (*dev->dev_ops->eth_rx_adapter_queue_del)(dev, >> eth_dev_id, >> + rx_queue_id); > > Here too rte_eth_device shall be passed. OK. > >> + if (!ret) { >> + update_queue_info(rx_adapter, >> + &rx_adapter- >>> eth_devices[eth_dev_id], >> + rx_queue_id, >> + 0); >> + if (!dev_info->nb_dev_queues) { >> + rte_free(dev_info->rx_queue); >> + dev_info->rx_queue = NULL; >> + } >> + } >> + } else { >> + int rc; >> + rte_spinlock_lock(&rx_adapter->rx_lock); >> + if (rx_queue_id == -1) { >> + for (i = 0; i < dev_info->dev->data->nb_rx_queues; i++) >> + >> _rte_event_eth_rx_adapter_queue_del(rx_adapter, >> + dev_info, >> + i); >> + } else { >> + _rte_event_eth_rx_adapter_queue_del(rx_adapter, >> + dev_info, >> + >> (uint16_t)rx_queue_id); >> + } >> + >> + rc = eth_poll_wrr_calc(rx_adapter); >> + if (rc) >> + RTE_EDEV_LOG_ERR("WRR recalculation failed %" >> PRId32, >> + rc); >> + >> + if (!dev_info->nb_dev_queues) { >> + rte_free(dev_info->rx_queue); >> + dev_info->rx_queue = NULL; >> + } >> + >> + rte_spinlock_unlock(&rx_adapter->rx_lock); >> + rte_service_component_runstate_set(rx_adapter->service_id, >> + sw_rx_adapter_queue_count(rx_adapter)); >> + } >> + >> + return ret; >> +} >> + >> + >> +int >> +rte_event_eth_rx_adapter_start(uint8_t id) >> +{ >> + return rx_adapter_ctrl(id, 1); >> +} >> + >> +int >> +rte_event_eth_rx_adapter_stop(uint8_t id) >> +{ >> + return rx_adapter_ctrl(id, 0); >> +} >> + >> +int >> +rte_event_eth_rx_adapter_stats_get(uint8_t id, >> + struct rte_event_eth_rx_adapter_stats *stats) >> +{ >> + struct rte_event_eth_rx_adapter *rx_adapter; >> + struct rte_event_eth_rx_adapter_stats dev_stats_sum = { 0 }; >> + struct rte_event_eth_rx_adapter_stats dev_stats; >> + struct rte_eventdev *dev; >> + struct eth_device_info *dev_info; >> + uint32_t i; >> + int ret; >> + >> + RTE_EVENT_ETH_RX_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL); >> + >> + rx_adapter = id_to_rx_adapter(id); >> + if (!rx_adapter || !stats) >> + return -EINVAL; >> + >> + dev = &rte_eventdevs[rx_adapter->eventdev_id]; >> + memset(stats, 0, sizeof(*stats)); >> + for (i = 0; i < rte_eth_dev_count(); i++) { >> + dev_info = &rx_adapter->eth_devices[i]; >> + if (!dev_info->internal_event_port) >> + continue; >> + ret = (*dev->dev_ops->eth_rx_adapter_stats_get)(dev, i, >> + &dev_stats); > > Preferable to check if the adapter stats get and stats set API are registered from the driver. > OK. Thanks for the review, I will make the changes and post a v4. Nikhil > Regards, > Nipun >