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 <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.
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.
