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