Re: [dpdk-dev] [PATCH V5 2/2] net/tap: use new Rx offloads API

[Shahaf Shuler <shahafs@mellanox.com>]

Thursday, March 15, 2018 12:41 AM, Ferruh Yigit:
> On 3/14/2018 5:49 AM, Shahaf Shuler wrote:
> > Tuesday, March 13, 2018 1:57 PM, Ferruh Yigit:
> >>>
> >>> Again - the application should follow the API which currently
> >>> dictates how
> >> to set port offload. It is not depends on the rx_queue_offloads
> capabilities.
> >>> For example, PMD which don't support queue offloads can still have
> >> verification for the API that each port offload is set also on the
> >> queue offloads.
> >>
> >> I am not agree with this part, why to dictate application to set
> >> queue offloads if it already knows device doesn't support queue specific
> offloads?
> >
> > I agree we can make a small change in the API to not force the application
> to set the port offloads in the queue configuration. It makes sense.
> > The change will be:
> > "port offloads should be set on the port configuration. Queue offloads
> should be set on the queue configuration"
> 
> I am OK to this one, this is more reasonable for devices that support only port
> level offloads.
> 
> This looks like same as option #2 mentioned in the previous mails.
> 
> >
> >>
> >> In some of the existing PMD patches, to switch to new offloading API,
> >> PMD sets [rt]x_queue_offload_capa as same as [rt]x_offload_capa,
> >
> > Well this is just wrong. Unless those PMDs support all the offloads in a
> queue level.
> >
> > The logic is "every queue offload can be counted as port offload", because
> such offload can be set on each and every queue.
> > The other way around is not correct, port offload cannot be counted as
> queue offload.
> >
> > So if such PMDs has offloads which are supported only on the port level
> they cannot be declared as queue offloads.
> 
> Thanks for confirming, it would be great if you can help on the PMD new
> offload API patch reviews, to catch these kind of issues.

Sure, have me Cc in the patches so It can pass through my mailbox filters. 

> 
> >
> >
> >> in that case
> >> application can't know if queue specific offloads are supported or
> >> not and application may try to set queue offloads, this forces PMD to
> verify them.
> >>
> >> You confirmed [rt]x_queue_offload_capa is the way for application to
> >> know if device supports queue specific offloads or not. If these
> >> values always set to [rt]x_offload_capa, application losts this capability.
> >>
> >> Instead:
> >> - PMD that doesn't support queue specific offloads should set
> >> [rt]x_queue_offload_capa to 0
> >> - When [rt]x_queue_offload_capa is 0, application should be free to
> >> set queue offloads whatever it wants
> >
> > I don't agree, when queue_offload_capa is 0 the expected behavior from
> application is not to set any offload (if we do the change in the API that you
> are pushing to).
> > PMDs can verify it or not, but if capability is not set the application should
> not set the offload. This is how the API should be defined.
> 
> OK for this one.
> 
> >
> >> - When [rt]x_queue_offload_capa is 0, PMD should be free to verify
> >> queue offloads but most probably shouldn't verify them since we don't
> >> know what application will send.
> >>
> >> - When [rt]x_queue_offload_capa is != 0, applications should set
> >> queue offloads at least "[rt]x_queue_offload = [rt]x_offload"
> >
> > If we do the change you are pushing it is not needed.
> > Application will set the port offload in the port configuration, and the
> queue offload in the queue configuration.
> > No need to make special treatment based on the offloads_capa.
> 
> Right.
> 
> >
> >> - When [rt]x_queue_offload_capa is != 0, PMD should verify the queue
> >> offloads
> >>
> 
> 
> Back to initial question J, is tap supports queue level offloads?
> If not it shouldn't be reporting or checking queue offloads.
> 
> 
> Although it will be changed after above suggested change in API, I think
> check in existing tap queue_setup, also same in mlx5, is wrong.
> 
> tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) {
> 
>         uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
>         uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
>         uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
> 
> 
> <...>
>         if ((port_offloads ^ offloads) & port_supp_offloads)
>                return false;
>         return true;
> 
> }
> 
> 
> take the example:
> port_supp_offloads = 11111
> port_offloads = 111
> queue_supp_offloads = 1111
> offloads = 1111
> 
> (port_offloads ^ offloads) & port_supp_offloads = 1000 Which will return
> false.
> 
> This only works if "port_offloads == offloads" which is practically only
> supporting port level offloads.

For mlx5, the port_supp_offloads is internal function which returns **only** the pure port offloads (the port offloads in dev_info are rx_offload_get_queue_capa() | rx_offload_get_port_capa())
That is, offload cannot be in both port and queue offload. So the scenario above is not feasible.