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

[Andrew Rybchenko]

On 11/8/19 1:29 PM, Matan Azrad wrote:
> 
> 
> From: Andrew Rybchenko
>> Sent: Friday, November 8, 2019 12:12 PM
>> To: Matan Azrad <matan at mellanox.com>; Pavan Nikhilesh Bhagavatula
>> <pbhagavatula at marvell.com>; ferruh.yigit at intel.com; Jerin Jacob
>> Kollanukkaran <jerinj at marvell.com>; Thomas Monjalon
>> <thomas at monjalon.net>
>> Cc: dev at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v15 3/7] ethdev: add validation to offloads
>> set by PMD
>>
>> On 11/7/19 9:56 AM, Matan Azrad wrote:
>>> Hi
>>>
>>> From: Andrew Rybchenko
>>>> On 11/6/19 9:58 AM, Matan Azrad wrote:
>>>>>
>>>>>
>>>>> From: Andrew Rybchenko
>>>>>> On 11/5/19 5:05 PM, Matan Azrad wrote:
>>>>>>> From: Andrew Rybchenko
>>>>>>>> On 11/3/19 6:16 PM, Matan Azrad wrote
>>>>>>>>> From: 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.
>>>>>>>>>
>>>>>>>>> So, are you agree It should not be checked globally for all the
>>>>>>>>> offloads in
>>>>>>>> ethdev layer?
>>>>>>>>
>>>>>>>> If offload is not requested, but enabled (since PMD cannot
>>>>>>>> disable it), right not it will not fail configure, but warn about
>>>>>>>> it in logs.
>>>>>>>>
>>>>>>>
>>>>>>> In this case warning print is not enough since it can be critical
>>>>>>> for the
>>>>>> application for some offloads.
>>>>>>> It can be very weird for the application to see that some offload
>>>>>>> are on
>>>>>> while the application doesn't expect them to be on.
>>>>>>> it even can cause app crash(at least for the RX offload I wrote
>>>>>>> above).
>>>>>>
>>>>>> The patch improves the situation. Earlier it was silent, now it
>>>>>> will be at least visible.
>>>>>
>>>>> We can do it visible inside the limited PMDs.
>>>>
>>>> Why?
>>>
>>> Because this is not according to what application should understand from
>> the ethdev API.
>>
>> It does not answer why it should be inside the limited PMDs instead of
>> ethdev layer.
> 
> Why not?
> Application doesn't expect to it and it may affect it.
> 
>>>>>> I'm afraid that in 19.11 release cycle we cannot change it to fail
>>>>>> dev_configure. I think it will be too destructive. Future
>>>>>> improvement should be discussed separately.
>>>>>
>>>>> So we can remove this ethdev patch now and let the PMD to do it
>>>>> until we will find better solution later.
>>>>
>>>> Sorry, but I don't think so.
>>>>
>>>>>>>>> It even be more problematic if the dynamic offload field in mbuf
>>>>>>>>> is not exist at all.
>>>>>>>
>>>>>>> Any answer here?
>>>>>
>>>>> A Rx offload requires dynamic mbuf field cannot stay visible while
>>>>> the app disabling it. Because the dynamic mbuf field probably is not
>>>>> set in the mbuf. May cause problems.
>>>>>
>>>>>> Please, clarify the question.
>>>>>>
>>>
>>> No answer here.
>>
>> Sorry, but I don't understand the problem.
>> If there is no dynamic field, it will not be set.
> Why not? The offload is enabled for the PMD perspective.
> 
>> If there is dynamic field, it is the same as regular fields.
>>
>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> yes, you right, In TX it looks less critical (for now).
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> Sorry, I don't understand your use case here.
>>>>>>>>
>>>>>>>> If application wants to try code path without, for example, Rx
>>>>>>>> checksum offload, it could be insufficient to disable the offload
>>>>>>>> right now, but also required to cleanup offload results flags in
>>>>>>>> each mbuf (if PMD does not support the offload disabling).
>>>>>>>
>>>>>>> What is "right now"? Configuration time?
>>>>>>
>>>>>> Right now is the current state of some drivers in DPDK tree.
>>>>>>
>>>>>
>>>>> OK. I think the offload configuration is in configuration time. No
>>>>> data-path.
>>>>>
>>>>>>> If application will know that PMD cannot disable the rx-checksum
>>>>>>> in configuration time, It can plan to not clean this flag in mbuf
>>>>>>> for each rx
>>>>>> mbuf.
>>>>>>
>>>>>> Yes and application has a way to know it - take a look at
>>>>>> dev->data->dev_conf.rxmode.offloads.
>>>>>
>>>>> As I understand, before this patch, this field used for ethdev layer
>>>>> knowledge to track on the application Rx offload configuration. Am I
>>>>> wrong?
>>>>
>>>> I think it is just Rx offloads configuration.
>>>> It is better to have real offloads here since it is used on Rx queue
>>>> setup to mask already enabled offloads.
>>>
>>> And in DPDK or any SW management controls a device, the configuration
>> must be set by the user.
>>> So, it should reflect the user configuration as is.
>>
>> It is ideal world which is unfortunately too far from real life.
>> There is always a trade off. It is possible to define too restrictive interface
>> which will enforce complicated implementation with bad performance
>> characteristics for no real value.
>>
>> In any case, the patch simply makes the difference visible.
>> It does not enforce any rules except to fail configure if requested offload is
>> not enabled which is a strong violation of the interface. If you don't like it, we
>> can discuss the point. In the area of not requested but enabled offloads, it
>> just adds logs. No changes in behaviour. I'm strongly against making it hard
>> failure in 19.11 since it is too late for the decision. We can discuss it later
>> separately from the patch.
>>
> While I don't agree with the patch and the idea here,  we can continue discuss later.
> I think we understand the ideas of both of us and we can dig to it later. 

I agree that it is time to wrap the discussion.
Could you make it clear what you don't like in the patch and why.

It does 3 things for all port at the end of rte_eth_dev_configure()
based on dev->data->dev_conf (if PMD updates it in the case of
violations).

1. If requested offload is not enabled, log error message.

2. If requested offload is not enabled, fail dev_configure by
   returning error and deconfiguring all queues.

3. If not requested offload is enabled, log info message.

Above is done for Rx and Tx offloads.