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