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. > >> 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. > >>>>>> > >>>>>>> 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. > > And If the meaning is the PMD configuration set (which weirdly can be > > different from what application want) I think it should be an error - > > because app doesn't follow the API. > > Which app? Which API? App - the dpdk application which configures an offload that cannot be masked. API - The Rx offload field in the ethdev data (which weirdly means what was configured by the PMD). > > >>> It looks me like PMD limitation which can be solved by 2 > >>> options: 1. Capability information which say to the app what offload > >>> may not be disabled. > >>> 2. Add limitation in the PMD documentation and print warning\error > >>> massage from the PMD. > >> > >> Yes, right now we are going way (2). > >> > >>>>>>> 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 > >>>>> > >>> > >
