[dpdk-dev] [PATCH v15 3/7] ethdev: add validation to offloads set by PMD

[Andrew Rybchenko]

On 11/3/19 9:57 AM, Matan Azrad wrote:
> Hi
>
> From: Andrew Rybchenko
>> On 10/31/19 7:33 PM, Pavan Nikhilesh Bhagavatula wrote:
>>>> From: Pavan Nikhilesh Bhagavatula
>>>>> Hi Matan,
>>>>>
>>>>>> Hi Pavan
>>>>>>
>>>>>> From: Pavan Nikhilesh <pbhagavatula at marvell.com>
>>>>>>> Some PMDs cannot work when certain offloads are enable/disabled,
>>>>>>> as a workaround PMDs auto enable/disable offloads internally and
>>>>>>> expose it through dev->data->dev_conf.rxmode.offloads.
>>>>>>>
>>>>>>> After device specific dev_configure is called compare the
>>>>>>> requested offloads to the offloads exposed by the PMD and, if the
>>>>>>> PMD failed to enable a given offload then log it and return
>>>>>>> -EINVAL from rte_eth_dev_configure, else if the PMD failed to
>>>>>>> disable a given offload log and continue with rte_eth_dev_configure.
>>>>>>>
>>>>>> rte_eth_dev_configure can be called more than 1 time in the device
>>>>>> life time, How can you know what is the minimum offload
>>>>>> configurations required by the port after the first call?
>>>>>> Maybe putting it in dev info is better, what do you think?
>>>>>>
>>>>> We only return -EINVAL in the case where we enable an offload
>>>>> advertised by dev_info and the port still fails to enable it.
>>>> Are you sure it is ok that devices may disable\enable offloads under
>>>> the hood without user notification?
>>> Some devices already do it. The above check adds validation for the same.
>> The problem is that some offloads cannot be disabled.
> Yes, I understand it.
>
>> If application does not request Rx checksum offload since it does use it, it is
>> not a problem to report it.
> Yes, for RX checksum I tend to agree that application doesn't care if the PMD will calculate the checksum in spite of the offload is disabled.
>
> But what's about other offloads:
> For example in RX: LRO, CRC_KEEP, VLAN_STRIP, JUMBO
> If the PMD will stay them on while the app is disabling it, It can cause a problems to the application (affects the packet length).

Yes, I agree that some offloads are critical to be disabled, but RSS_HASH
discussed in the changeset is not critical.

> For example in TX: TSO, VLAN, MULTI_SEG.....

Tx is not that critical since application should not request
these offloads per-packet. Tx offloads are mainly required
to ensure that application may request the offload per packet
and it will be done.

>> Of course, it could be a problem if the offload is
>> used, but application wants to disable it, for example, for debugging
>> purposes.
>> In this case, the solution is to mask offloads on application level, which is not
>> ideal as well.
> Why not ideal?

It eats CPU cycles.

> If application can know the limitation of offloads disabling (for example to read capability on it)
> The application has all information to take decisions.
>
>> Anyway, the patch just tries to highlight difference of applied from
>> requested. So, it is a step forward.
>> Also, the patch will fail configure if an offload is requested, but not enabled.
>>
>>>> Can't it break applications?
>>>> Why does the device expose unsupported offloads in dev info?
>>>> Does it update the running offload usynchronically? Race?
>>>> Can you explain also your specific use case?
>>>>
>>>>
>>>>>> Matan