From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 84E70293B for ; Tue, 5 Sep 2017 17:31:14 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 05 Sep 2017 08:31:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,480,1498546800"; d="scan'208";a="1191742775" Received: from irsmsx107.ger.corp.intel.com ([163.33.3.99]) by fmsmga001.fm.intel.com with ESMTP; 05 Sep 2017 08:31:11 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.75]) by IRSMSX107.ger.corp.intel.com ([169.254.10.65]) with mapi id 14.03.0319.002; Tue, 5 Sep 2017 16:31:11 +0100 From: "Ananyev, Konstantin" To: Shahaf Shuler , Thomas Monjalon CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API Thread-Index: AQHTJYU8DF/tpFi9okyg3kyCdEiysqKkwPgQgAEZ2gCAABLvoIAAIFYAgAATnAA= Date: Tue, 5 Sep 2017 15:31:10 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772584F246B5D@irsmsx105.ger.corp.intel.com> References: <2327783.H4uO08xLcu@xps> <2601191342CEEE43887BDE71AB9772584F2460F1@irsmsx105.ger.corp.intel.com> <2334939.YzL2ADl2XU@xps> <2601191342CEEE43887BDE71AB9772584F246819@irsmsx105.ger.corp.intel.com> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API 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: Tue, 05 Sep 2017 15:31:15 -0000 >=20 > > > > > > In fact, right now it is possible to query/change these 3 vlan > > > > > > offload flags on the fly (after dev_start) on port basis by > > rte_eth_dev_(get|set)_vlan_offload API. >=20 > Regarding this API from ethdev. >=20 > So this seems like a hack on ethdev. Currently there are 2 ways for user = to set Rx vlan offloads. > One is through dev_configure which require the ports to be stopped. The o= ther is this API which can set even if the port is started. Yes there is an ability to enable/disable VLAN offloads without stop/reconf= igure the device. Though I wouldn't call it 'a hack'. >>From my perspective - it is a useful feature.=20 Same as it is possible in some cases to change MTU without stopping device,= etc. >=20 > We should have only one place were application set offloads and this is c= urrently on dev_configure, Hmm, if HW supports the ability to do things at runtime why we have to stop= users from using that ability? > And future to be on rx_queue_setup. >=20 > I would say that this API should be removed as well. > Application which wants to change those offloads will stop the ports and = reconfigure the PMD. I wouldn't agree - see above. > Am quite sure that there are PMDs which need to re-create the Rxq based o= n vlan offloads changing and this cannot be done while the > traffic flows. That's an optional API - PMD can choose does it want to support it or not. >=20 >=20 > > > > > > So, I think at least these 3 flags need to be remained on a por= t basis. > > > > > > > > > > I don't understand how it helps to be able to configure the same > > > > > thing in 2 places. > > > > > > > > Because some offloads are per device, another - per queue. > > > > Configuring on a device basis would allow most users to conjure all > > > > queues in the same manner by default. > > > > Those users who would need more fine-grained setup (per queue) wil= l > > > > be able to overwrite it by rx_queue_setup(). > > > > > > Those users can set the same config for all queues. > > > > > > > > > I think you are just describing a limitation of these HW: some > > > > > offloads must be the same for all queues. > > > > > > > > As I said above - on some devices some offloads might also affect > > > > queues that belong to VFs (to another ports in DPDK words). > > > > You might never invoke rx_queue_setup() for these queues per your > > app. > > > > But you still want to enable this offload on that device. > > > > I am ok with having per-port and per-queue offload configuration. > > My concern is that after that patch only per-queue offload configuratio= n will > > remain. > > I think we need both. >=20 > So looks like we all agree PMDs should report as part of the rte_eth_dev_= info_get which offloads are per port and which are per queue. Yep. >=20 > Regarding the offloads configuration by application I see 2 options: > 1. have an API to set offloads per port as part of device configure and A= PI to set offloads per queue as part of queue setup > 2. set all offloads as part of queue configuration (per port offloads wil= l be set equally for all queues). In case of a mixed configuration for > port offloads PMD will return error. > Such error can be reported on device start. The PMD will traverse the= queues and check for conflicts. >=20 > I will focus on the cons, since both achieve the goal: >=20 > Cons of #1: > - Two places to configure offloads. Yes, but why is that a problem? > - Like Thomas mentioned - what about offloads per device? This direction = leads to more places to configure the offloads. As you said above - there would be 2 places: per port and per queue. Could you explain - what other places you are talking about?=20 >=20 > Cons of #2: > - Late error reporting - on device start and not on queue setup. Consider scenario when PF has a corresponding VFs (PF is controlled by DPDK) Right now (at least with Intel HW) it is possible to: struct rte_eth_conf dev_conf; dev_conf. rxmode.hw_vlan_filter =3D 1; ... rte_eth_dev_configure(pf_port_id, 0, 0, &dev_conf); rte_eth_dev_start(pf_port_id); In that scenario I don't have any RX/TX queues configured. Though I still able to enable vlan filter, and it would work correctly for = VFs. Same for other per-port offloads. With approach #2 it simply wouldn't work. So my preference is still #1. Konstantin >=20 > I would go with #2. >=20 > > Konstantin > > > > > > > > You are advocating for per-port configuration API because some > > > settings must be the same on all the ports of your hardware? > > > So there is a big trouble. You don't need per-port settings, but > > > per-hw-device settings. > > > Or would you accept more fine-grained per-port settings? > > > If yes, you can accept even finer grained per-queues settings. > > > > > > > > > It does not prevent from configuring them in the per-queue setup. > > > > > > > > > > > In fact, why can't we have both per port and per queue RX offlo= ad: > > > > > > - dev_configure() will accept RX_OFFLOAD_* flags and apply them= on > > a port basis. > > > > > > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and appl= y > > them on a queue basis. > > > > > > - if particular RX_OFFLOAD flag for that device couldn't be set= up on a > > queue basis - > > > > > > rx_queue_setup() will return an error. > > > > > > > > > > The queue setup can work while the value is the same for every > > queues. > > > > > > > > Ok, and how people would know that? > > > > That for device N offload X has to be the same for all queues, and > > > > for device M offload X can be differs for different queues. > > > > > > We can know the hardware limitations by filling this information at > > > PMD init. > > > > > > > Again, if we don't allow to enable/disable offloads for particular > > > > queue, why to bother with updating rx_queue_setup() API at all? > > > > > > I do not understand this question. > > > > > > > > > - rte_eth_rxq_info can be extended to provide information which > > RX_OFFLOADs > > > > > > can be configured on a per queue basis. > > > > > > > > > > Yes the PMD should advertise its limitations like being forced to > > > > > apply the same configuration to all its queues. > > > > > > > > Didn't get your last sentence. > > > > > > I agree that the hardware limitations must be written in an ethdev > > structure.