From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shahaf Shuler Subject: Re: [PATCH 4/4] ethdev: add helpers to move to the new offloads API Date: Wed, 6 Sep 2017 06:01:43 +0000 Message-ID: References: <2327783.H4uO08xLcu@xps> <2601191342CEEE43887BDE71AB9772584F2460F1@irsmsx105.ger.corp.intel.com> <2334939.YzL2ADl2XU@xps> <2601191342CEEE43887BDE71AB9772584F246819@irsmsx105.ger.corp.intel.com> <2601191342CEEE43887BDE71AB9772584F246B5D@irsmsx105.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: "Ananyev, Konstantin" , Thomas Monjalon Return-path: Received: from EUR01-DB5-obe.outbound.protection.outlook.com (mail-db5eur01on0061.outbound.protection.outlook.com [104.47.2.61]) by dpdk.org (Postfix) with ESMTP id 03B3F3DC for ; Wed, 6 Sep 2017 08:01:45 +0200 (CEST) In-Reply-To: <2601191342CEEE43887BDE71AB9772584F246B5D@irsmsx105.ger.corp.intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Tuesday, September 5, 2017 6:31 PM, Ananyev, Konstantin: > > > > > > > > > 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. > > > > Regarding this API from ethdev. > > > > So this seems like a hack on ethdev. Currently there are 2 ways for use= r to > set Rx vlan offloads. > > One is through dev_configure which require the ports to be stopped. The > other is this API which can set even if the port is started. >=20 > Yes there is an ability to enable/disable VLAN offloads without > stop/reconfigure the device. > Though I wouldn't call it 'a hack'. > From my perspective - it is a useful feature. > Same as it is possible in some cases to change MTU without stopping devic= e, > etc. >=20 > > > > We should have only one place were application set offloads and this > > is currently on dev_configure, >=20 > Hmm, if HW supports the ability to do things at runtime why we have to st= op > users from using that ability? >=20 > > And future to be on rx_queue_setup. > > > > I would say that this API should be removed as well. > > Application which wants to change those offloads will stop the ports an= d > reconfigure the PMD. >=20 > I wouldn't agree - see above. >=20 > > Am quite sure that there are PMDs which need to re-create the Rxq > > based on vlan offloads changing and this cannot be done while the traff= ic > flows. >=20 > That's an optional API - PMD can choose does it want to support it or not= . >=20 > > > > > > > > > > > So, I think at least these 3 flags need to be remained on a p= ort > 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) > > > > > will 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 > > > configuration will remain. > > > I think we need both. > > > > 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. >=20 > 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 API to set offloads per queue as part of queue setup 2. set all > > offloads as part of queue configuration (per port offloads will be set = equally > for all queues). In case of a mixed configuration for port offloads PMD w= ill > return error. > > Such error can be reported on device start. The PMD will traverse t= he > queues and check for conflicts. > > > > I will focus on the cons, since both achieve the goal: > > > > Cons of #1: > > - Two places to configure offloads. >=20 > Yes, but why is that a problem? If we could make the offloads API to set the offloads in a single place it = would be much cleaner and less error prune. There is one flow which change the offloads configuration.=20 Later on when we want to change/expend it will be much simpler, as all modi= fication can happen in a single place only. >=20 > > - Like Thomas mentioned - what about offloads per device? This directio= n > leads to more places to configure the offloads. >=20 > As you said above - there would be 2 places: per port and per queue. > Could you explain - what other places you are talking about? In fact, the vlan filter offload for PF is a *per device* offload and not p= er port. Since the corresponding VF has it just by the fact the PF set it o= n dev_configure. So to be exact, such offload should be set on a new offload section called = "per device offloads". Currently you compromise on setting it in the *per port* offload section, w= ith proper explanation on the VF limitation in intel. >=20 > > > > Cons of #2: > > - Late error reporting - on device start and not on queue setup. >=20 > Consider scenario when PF has a corresponding VFs (PF is controlled by > DPDK) Right now (at least with Intel HW) it is possible to: >=20 > 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); >=20 > 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 fo= r VFs. > Same for other per-port offloads. For the PF - enabling vlan filtering without any queues means nothing. The = PF can receive no traffic, what different does it makes the vlan filtering = is set? For the VF - I assume it will have queues, therefore for it vlan filtering = has a meaning. However as I said above, the VF has the vlan filter because = in intel this is per-device offload, so this is not a good example.=20 Which other per-port offloads you refer to? I don't understand what is the meaning of setting per-port offloads without= opening any Tx/Rx queues.=20 > With approach #2 it simply wouldn't work. Yes for vlan filtering it will not work on intel, and this may be enough to= move to suggestion #1. Thomas? >=20 > So my preference is still #1. >=20 > Konstantin >=20 > > > > I would go with #2. > > > > > 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 setu= p. > > > > > > > > > > > > > In fact, why can't we have both per port and per queue RX > offload: > > > > > > > - dev_configure() will accept RX_OFFLOAD_* flags and apply > > > > > > > them on > > > a port basis. > > > > > > > - rx_queue_setup() will also accept RX_OFFLOAD_* flags and > > > > > > > apply > > > them on a queue basis. > > > > > > > - if particular RX_OFFLOAD flag for that device couldn't be > > > > > > > setup 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() AP= I > 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.