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

[Matan Azrad <matan@mellanox.com>]

From: 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@mellanox.com>; Pavan Nikhilesh Bhagavatula
> >> <pbhagavatula@marvell.com>; ferruh.yigit@intel.com; Jerin Jacob
> >> Kollanukkaran <jerinj@marvell.com>; Thomas Monjalon
> >> <thomas@monjalon.net>
> >> Cc: dev@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@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.

Agree with your summary.

My problem in this patch is part 3:
This part implies by a formal way(ethdev) that there are 2 configurations:
1. The application configuration.
2. The actual device configuration.

I think that formally if there is a difference between both, an error must be returned.
Informally, PMD can do whatever it want with good documentation in its doc file.   


Matan.