Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Shahaf Shuler <shahafs@mellanox.com>]

Tuesday, September 5, 2017 11:10 AM, 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 user 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.

We should have only one place were application set offloads and this is currently on dev_configure,
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 and reconfigure the PMD.
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 traffic flows.


> > > > > So, I think at least these 3 flags need to be remained on a port 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.

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 will return error.
    Such error can be reported on device start. The PMD will traverse the queues and check for conflicts.

I will focus on the cons, since both achieve the goal:

Cons of #1:
- Two places to configure offloads.
- Like Thomas mentioned - what about offloads per device? This direction leads to more places to configure the offloads.

Cons of #2:
- Late error reporting - on device start and not on queue setup.

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 setup.
> > > >
> > > > > 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() 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.