On 11/8/19 1:29 PM, Matan Azrad wrote: > > > From: Andrew Rybchenko >> Sent: Friday, November 8, 2019 12:12 PM >> To: Matan Azrad <ma...@mellanox.com>; Pavan Nikhilesh Bhagavatula >> <pbhagavat...@marvell.com>; ferruh.yi...@intel.com; Jerin Jacob >> Kollanukkaran <jer...@marvell.com>; Thomas Monjalon >> <tho...@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 >>>>>>>>>>>>>>>> <pbhagavat...@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.
