From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 3ABD02965 for ; Thu, 29 Mar 2018 11:13:54 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Mar 2018 02:13:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,376,1517904000"; d="scan'208";a="27952511" Received: from dwdohert-mobl.ger.corp.intel.com (HELO [10.252.2.125]) ([10.252.2.125]) by fmsmga007.fm.intel.com with ESMTP; 29 Mar 2018 02:13:49 -0700 To: Shahaf Shuler , "dev@dpdk.org" Cc: Alex Rosenbaum , Ferruh Yigit , Thomas Monjalon , Qi Zhang , Alejandro Lucero , Andrew Rybchenko , Mohammad Abdul Awal , Remy Horton , John McNamara , Rony Efraim , Wu , Jingjing , Lu , Wenzhuo , Vincent JArdin , Yuanhan Liu , Richardson , Bruce , Ananyev , Konstantin , Wang , Zhihong References: <20180328135433.20203-1-declan.doherty@intel.com> <20180328135433.20203-3-declan.doherty@intel.com> From: "Doherty, Declan" Message-ID: Date: Thu, 29 Mar 2018 10:13:49 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.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 v6 2/8] ethdev: add switch identifier parameter to port 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: Thu, 29 Mar 2018 09:13:56 -0000 On 29/03/2018 7:13 AM, Shahaf Shuler wrote: > Hi Declan, > > Thanks for the series! See some comments below > > Wednesday, March 28, 2018 4:54 PM, Declan Doherty: >> Subject: [dpdk-dev][PATCH v6 2/8] ethdev: add switch identifier parameter >> to port >> >> Introduces a new port attribute to ethdev port's which denotes the switch >> domain a port belongs to. By default all port's switch identifiers are the their >> port_id. Ports which share a common switch domain are configured with the >> same switch id. >> >> Signed-off-by: Declan Doherty >> --- >> app/test-pmd/config.c | 1 + >> lib/librte_ether/rte_ethdev.c | 3 +++ >> lib/librte_ether/rte_ethdev.h | 1 + >> lib/librte_ether/rte_ethdev_core.h | 1 + >> 4 files changed, 6 insertions(+) >> >> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index >> 4bb255c62..e12f8c515 100644 >> --- a/app/test-pmd/config.c >> +++ b/app/test-pmd/config.c >> @@ -517,6 +517,7 @@ port_infos_display(portid_t port_id) >> printf("Min possible number of TXDs per queue: %hu\n", >> dev_info.tx_desc_lim.nb_min); >> printf("TXDs number alignment: %hu\n", >> dev_info.tx_desc_lim.nb_align); >> + printf("Switch Id: %u\n", dev_info.switch_id); >> } >> >> void >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c >> index 23857c91f..f32d18cad 100644 >> --- a/lib/librte_ether/rte_ethdev.c >> +++ b/lib/librte_ether/rte_ethdev.c >> @@ -290,6 +290,8 @@ rte_eth_dev_allocate(const char *name) >> eth_dev = eth_dev_get(port_id); >> snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), >> "%s", name); >> eth_dev->data->port_id = port_id; >> + eth_dev->data->switch_id = port_id; >> + /**< Default switch_id is the port_id of the device */ > > Why such default is needed? Why not let the PMD to set it always? I saw this a simple way to have a consistent default value (the port_id) for all PMDs, without the need to modify existing PMD which don't currently have any concept of a switch domain. Also taking the approach of just leaving it up to the PMD to decide the value would mean that some form of synchronisation would be required so that two device don't select the switch domain identifier. > >> eth_dev->data->mtu = ETHER_MTU; >> >> unlock: >> @@ -2395,6 +2397,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct >> rte_eth_dev_info *dev_info) >> dev_info->driver_name = dev->device->driver->name; >> dev_info->nb_rx_queues = dev->data->nb_rx_queues; >> dev_info->nb_tx_queues = dev->data->nb_tx_queues; >> + dev_info->switch_id = dev->data->switch_id; > > Why there is a need to keep the switch_id on device data? > I think PMD to store it on its private structure and report it in dev_info is enough. > That way would require every PMD to be modified to maintain a switch_id structure, which I know isn't a big deal, as only the device which need to support it would need it, but we would need to change the dev_info->switch_id from being a uint16_t to being a signed value so we could have a default of -1 for device which don't support switch domain. I thought the former approach was cleaner. >> } >> >> int >> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h >> index 036153306..dced4fc41 100644 >> --- a/lib/librte_ether/rte_ethdev.h >> +++ b/lib/librte_ether/rte_ethdev.h >> @@ -1029,6 +1029,7 @@ struct rte_eth_dev_info { >> /** Configured number of rx/tx queues */ >> uint16_t nb_rx_queues; /**< Number of RX queues. */ >> uint16_t nb_tx_queues; /**< Number of TX queues. */ >> + uint16_t switch_id; /**< Switch Domain Id */ >> }; >> >> /** >> diff --git a/lib/librte_ether/rte_ethdev_core.h >> b/lib/librte_ether/rte_ethdev_core.h >> index e5681e466..caed7a4e6 100644 >> --- a/lib/librte_ether/rte_ethdev_core.h >> +++ b/lib/librte_ether/rte_ethdev_core.h >> @@ -585,6 +585,7 @@ struct rte_eth_dev_data { >> struct ether_addr* hash_mac_addrs; >> /** Device Ethernet MAC addresses of hash filtering. */ >> uint16_t port_id; /**< Device [external] port identifier. */ >> + uint16_t switch_id; /**< Switch which port is associated with >> */ >> __extension__ >> uint8_t promiscuous : 1, /**< RX promiscuous mode ON(1) / OFF(0). >> */ >> scattered_rx : 1, /**< RX of scattered packets is ON(1) / >> OFF(0) */ >> -- >> 2.14.3 >