[v15,3/7] ethdev: add validation to offloads set by PMD
Checks
Commit Message
From: Pavan Nikhilesh <pbhagavatula@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.
Suggested-by: Andrew Rybchenko <arybchenko@solarflare.com>
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
--
2.17.1
Comments
On 10/29/19 6:37 PM, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@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.
>
> Suggested-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Few minor notes below, many thanks
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 3f45b9e9c..8c58da91c 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1135,6 +1135,41 @@ rte_eth_dev_tx_offload_name(uint64_t offload)
> return name;
> }
>
> +static int
> +_rte_eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads,
I'm not sure about underscore in function name. May be Thomas or
Ferruh can comment.
> + uint64_t set_offloads,
> + const char *(*f)(uint64_t))
> +{
> + uint64_t offloads_diff = req_offloads ^ set_offloads;
> + uint64_t offloads_req_diff, offloads_set_diff;
> + uint64_t offload;
> + uint8_t err = 0;
> +
> + /* Check if any offload is advertised but not enabled. */
> + offloads_req_diff = offloads_diff & req_offloads;
> + while (offloads_req_diff) {
> + offload = 1ULL << __builtin_ctzll(offloads_req_diff);
> + offloads_req_diff &= ~offload;
> + RTE_ETHDEV_LOG(ERR, "Port %u failed to enable %s offload",
Offload name does not include Rx/Tx, so IPV4_CKSUM is identical
and it is required to log if it is Rx or Tx offload separately.
Sounds like one more parameter.
> + port_id, f(offload));
> + err = 1;
> + }
> +
> + if (err)
> + return -EINVAL;
> +
> + /* Chech if any offload couldn't be disabled. */
> + offloads_set_diff = offloads_diff & set_offloads;
> + while (offloads_set_diff) {
> + offload = 1ULL << __builtin_ctzll(offloads_set_diff);
> + offloads_set_diff &= ~offload;
> + RTE_ETHDEV_LOG(INFO, "Port %u failed to disable %s offload",
> + port_id, f(offload));
> + }
Consider to do it in one loop, something like:
int rc = 0;
while (offloads_diff != 0) {
offload = 1ULL << __builtin_ctzll(offloads_diff);
offloads_diff &= ~offload;
if (offload & req_offload) {
RTE_ETHDEV_LOG(INFO,
"Port %u failed to enable %s %s offload",
port_id, f(offload), rxtx);
rc = -EINVAL;
} else {
RTE_ETHDEV_LOG(INFO,
"Port %u failed to disable %s %s offload",
port_id, f(offload), rxtx);
}
}
return rc;
BTW, I'm not sure that -EINVAL is good here, but right now
can't suggest anything better.
> +
> + return 0;
> +}
> +
> int
> rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> const struct rte_eth_conf *dev_conf)
> @@ -1358,6 +1393,30 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> goto rollback;
> }
>
> + /* Validate Rx offloads. */
> + diag = _rte_eth_dev_validate_offloads(port_id,
> + dev_conf->rxmode.offloads,
> + dev->data->dev_conf.rxmode.offloads,
> + rte_eth_dev_rx_offload_name);
> + if (diag != 0) {
> + rte_eth_dev_rx_queue_config(dev, 0);
> + rte_eth_dev_tx_queue_config(dev, 0);
May be it is a good moment to make one more rollback
label with queues release and avoid duplicating it.
> + ret = diag;
> + goto rollback;
> + }
> +
> + /* Validate Tx offloads. */
> + diag = _rte_eth_dev_validate_offloads(port_id,
> + dev_conf->txmode.offloads,
> + dev->data->dev_conf.txmode.offloads,
> + rte_eth_dev_tx_offload_name);
> + if (diag != 0) {
> + rte_eth_dev_rx_queue_config(dev, 0);
> + rte_eth_dev_tx_queue_config(dev, 0);
> + ret = diag;
> + goto rollback;
> + }
> +
> return 0;
>
> rollback:
> --
> 2.17.1
>
>>lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 59 insertions(+)
>>
>>diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>index 3f45b9e9c..8c58da91c 100644
>>--- a/lib/librte_ethdev/rte_ethdev.c
>>+++ b/lib/librte_ethdev/rte_ethdev.c
>>@@ -1135,6 +1135,41 @@ rte_eth_dev_tx_offload_name(uint64_t offload)
>> return name;
>> }
>>
>>+static int
>>+_rte_eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads,
>
>I'm not sure about underscore in function name. May be Thomas or
>Ferruh can comment.
>
>
>>+ uint64_t set_offloads,
>>+ const char *(*f)(uint64_t))
>>+{
>>+ uint64_t offloads_diff = req_offloads ^ set_offloads;
>>+ uint64_t offloads_req_diff, offloads_set_diff;
>>+ uint64_t offload;
>>+ uint8_t err = 0;
>>+
>>+ /* Check if any offload is advertised but not enabled. */
>>+ offloads_req_diff = offloads_diff & req_offloads;
>>+ while (offloads_req_diff) {
>>+ offload = 1ULL << __builtin_ctzll(offloads_req_diff);
>>+ offloads_req_diff &= ~offload;
>>+ RTE_ETHDEV_LOG(ERR, "Port %u failed to enable %s offload",
>>
>Offload name does not include Rx/Tx, so IPV4_CKSUM is identical
>and it is required to log if it is Rx or Tx offload separately.
>Sounds like one more parameter.
>
Yes, I guess we can pass "RX"/"Tx" as another parameter.
>
>>+ port_id, f(offload));
>>+ err = 1;
>>+ }
>>+
>>+ if (err)
>>+ return -EINVAL;
>>+
>>+ /* Chech if any offload couldn't be disabled. */
>>+ offloads_set_diff = offloads_diff & set_offloads;
>>+ while (offloads_set_diff) {
>>+ offload = 1ULL << __builtin_ctzll(offloads_set_diff);
>>+ offloads_set_diff &= ~offload;
>>+ RTE_ETHDEV_LOG(INFO, "Port %u failed to disable %s offload",
>>+ port_id, f(offload));
>>+ }
>>
>Consider to do it in one loop, something like:
> int rc = 0;
> while (offloads_diff != 0) {
> offload = 1ULL << __builtin_ctzll(offloads_diff);
> offloads_diff &= ~offload;
> if (offload & req_offload) {
> RTE_ETHDEV_LOG(INFO,
> "Port %u failed to enable %s %s offload",
> port_id, f(offload), rxtx);
> rc = -EINVAL;
> } else {
> RTE_ETHDEV_LOG(INFO,
> "Port %u failed to disable %s %s offload",
> port_id, f(offload), rxtx);
> }
> }
>
> return rc;
>
We could merge the loops I though that the differentiating them would
be informative. No strong opinions we could merge them in v16.
>BTW, I'm not sure that -EINVAL is good here, but right now
>can't suggest anything better.
>
>
>>+
>>+ return 0;
>>+}
>>+
>> int
>> rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>> const struct rte_eth_conf *dev_conf)
>>@@ -1358,6 +1393,30 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>> goto rollback;
>> }
>>
>>+ /* Validate Rx offloads. */
>>+ diag = _rte_eth_dev_validate_offloads(port_id,
>>+ dev_conf->rxmode.offloads,
>>+ dev->data->dev_conf.rxmode.offloads,
>>+ rte_eth_dev_rx_offload_name);
>>+ if (diag != 0) {
>>+ rte_eth_dev_rx_queue_config(dev, 0);
>>+ rte_eth_dev_tx_queue_config(dev, 0);
>
>May be it is a good moment to make one more rollback
>label with queues release and avoid duplicating it.
>
Yeah we could put them under a new label.
>
>>+ ret = diag;
>>+ goto rollback;
>>+ }
>>+
>>+ /* Validate Tx offloads. */
>>+ diag = _rte_eth_dev_validate_offloads(port_id,
>>+ dev_conf->txmode.offloads,
>>+ dev->data->dev_conf.txmode.offloads,
>>+ rte_eth_dev_tx_offload_name);
>>+ if (diag != 0) {
>>+ rte_eth_dev_rx_queue_config(dev, 0);
>>+ rte_eth_dev_tx_queue_config(dev, 0);
>>+ ret = diag;
>>+ goto rollback;
>>+ }
>>+
>> return 0;
>>
>> rollback:
>>--
>>2.17.1
29/10/2019 17:53, Andrew Rybchenko:
> On 10/29/19 6:37 PM, pbhagavatula@marvell.com wrote:
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > +static int
> > +_rte_eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads,
>
> I'm not sure about underscore in function name. May be Thomas or
> Ferruh can comment.
If it is private, don't use the rte_ prefix at all.
This function can be simply named "validate_offloads".
29/10/2019 16:37, pbhagavatula@marvell.com:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> +static int
> +_rte_eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads,
> + uint64_t set_offloads,
> + const char *(*f)(uint64_t))
Please do not call "f" a function parameter.
This function has a purpose, please name it.
Overall, I feel it would be easier to understand this function
with a comment on top, explaining each parameter. Thanks
> +{
> + uint64_t offloads_diff = req_offloads ^ set_offloads;
> + uint64_t offloads_req_diff, offloads_set_diff;
> + uint64_t offload;
> + uint8_t err = 0;
> +
> + /* Check if any offload is advertised but not enabled. */
Not sure "advertised" is the right word here.
Hi Pavan
From: Pavan Nikhilesh <pbhagavatula@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?
Matan
Hi Matan,
>Hi Pavan
>
>From: Pavan Nikhilesh <pbhagavatula@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.
>Matan
From: Pavan Nikhilesh Bhagavatula
> Hi Matan,
>
> >Hi Pavan
> >
> >From: Pavan Nikhilesh <pbhagavatula@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?
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
>From: Pavan Nikhilesh Bhagavatula
>> Hi Matan,
>>
>> >Hi Pavan
>> >
>> >From: Pavan Nikhilesh <pbhagavatula@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.
>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
>29/10/2019 16:37, pbhagavatula@marvell.com:
>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> +static int
>> +_rte_eth_dev_validate_offloads(uint16_t port_id, uint64_t
>req_offloads,
>> + uint64_t set_offloads,
>> + const char *(*f)(uint64_t))
>
>Please do not call "f" a function parameter.
>This function has a purpose, please name it.
>
>Overall, I feel it would be easier to understand this function
>with a comment on top, explaining each parameter. Thanks
Will fix in v16.
>
>> +{
>> + uint64_t offloads_diff = req_offloads ^ set_offloads;
>> + uint64_t offloads_req_diff, offloads_set_diff;
>> + uint64_t offload;
>> + uint8_t err = 0;
>> +
>> + /* Check if any offload is advertised but not enabled. */
>
>Not sure "advertised" is the right word here.
Don't PMDs advertise their capabilities/offloads through dev_info?.
>
>
On 10/31/19 7:33 PM, Pavan Nikhilesh Bhagavatula wrote:
>> From: Pavan Nikhilesh Bhagavatula
>>> Hi Matan,
>>>
>>>> Hi Pavan
>>>>
>>>> From: Pavan Nikhilesh <pbhagavatula@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.
If application does not request Rx checksum offload since it
does use it, it is not a problem to report it. 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.
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
31/10/2019 17:44, Pavan Nikhilesh Bhagavatula:
> >29/10/2019 16:37, pbhagavatula@marvell.com:
> >> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >> +{
> >> + uint64_t offloads_diff = req_offloads ^ set_offloads;
> >> + uint64_t offloads_req_diff, offloads_set_diff;
> >> + uint64_t offload;
> >> + uint8_t err = 0;
> >> +
> >> + /* Check if any offload is advertised but not enabled. */
> >
> >Not sure "advertised" is the right word here.
>
> Don't PMDs advertise their capabilities/offloads through dev_info?.
Yes
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 <pbhagavatula@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).
For example in TX: TSO, VLAN, MULTI_SEG.....
> 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?
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
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 <pbhagavatula@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.
> 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.
>> 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.
> 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
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 <pbhagavatula@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?
It even be more problematic if the dynamic offload field in mbuf is not exist at all.
>
> > 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 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
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 <pbhagavatula@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.
> It even be more problematic if the dynamic offload field in mbuf is not exist at all.
>
>>
>>> 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).
>>> 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
>
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 <pbhagavatula@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).
> > It even be more problematic if the dynamic offload field in mbuf is not exist
> at all.
Any 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?
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.
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.
> >>> 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
> >
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 <pbhagavatula@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. 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.
>>> It even be more problematic if the dynamic offload field in mbuf is not exist at all.
>
> Any answer here?
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.
> 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.
> 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
>>>
>
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 <pbhagavatula@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
> >>>
> >
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
>>>>>>>>>>>> <pbhagavatula@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
>>>>>
>>>
>
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
> >>>>>>>>>>>> <pbhagavatula@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
> >>>>>
> >>>
> >
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
>>>>>>>>>>>>>> <pbhagavatula@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.
>>>> 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.
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.
>>> 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).
See above.
>>>>> 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
>>>>>>>
>>>>>
>>>
>
From: Andrew Rybchenko
> Sent: Friday, November 8, 2019 12:12 PM
> To: Matan Azrad <matan@mellanox.com>; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; ferruh.yigit@intel.com; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; Thomas Monjalon
> <thomas@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
> >>>>>>>>>>>>>> <pbhagavatula@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.
> >>> 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).
>
> See above.
>
> >>>>> 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
> >>>>>>>
> >>>>>
> >>>
> >
On 11/8/19 1:29 PM, Matan Azrad wrote:
>
>
> From: Andrew Rybchenko
>> Sent: Friday, November 8, 2019 12:12 PM
>> To: Matan Azrad <matan@mellanox.com>; Pavan Nikhilesh Bhagavatula
>> <pbhagavatula@marvell.com>; ferruh.yigit@intel.com; Jerin Jacob
>> Kollanukkaran <jerinj@marvell.com>; Thomas Monjalon
>> <thomas@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
>>>>>>>>>>>>>>>> <pbhagavatula@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.
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 <matan@mellanox.com>; Pavan Nikhilesh Bhagavatula
> >> <pbhagavatula@marvell.com>; ferruh.yigit@intel.com; Jerin Jacob
> >> Kollanukkaran <jerinj@marvell.com>; Thomas Monjalon
> >> <thomas@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
> >>>>>>>>>>>>>>>> <pbhagavatula@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.
On 11/8/19 2:48 PM, Matan Azrad wrote:
>
>
> 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 <matan@mellanox.com>; Pavan Nikhilesh Bhagavatula
>>>> <pbhagavatula@marvell.com>; ferruh.yigit@intel.com; Jerin Jacob
>>>> Kollanukkaran <jerinj@marvell.com>; Thomas Monjalon
>>>> <thomas@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
>>>>>>>>>>>>>>>>>> <pbhagavatula@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.
OK I see. The problem that you'd like make (3) stronger
check to log and return error.
Above discussion cover it. We can discuss it later, but
right now it cannot be done (at least from my point of
view) as I said above.
Many thanks.
08/11/2019 13:09, Andrew Rybchenko:
> On 11/8/19 2:48 PM, Matan Azrad wrote:
> > From: Andrew Rybchenko
> >> 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.
>
> OK I see. The problem that you'd like make (3) stronger
> check to log and return error.
>
> Above discussion cover it. We can discuss it later, but
> right now it cannot be done (at least from my point of
> view) as I said above.
Yes, making checks stronger can always be done later.
It looks a step is done in this patch.
In general, the gap between app needs and PMD support
needs to be better handled in ethdev.
Matan, I suggest you submit a patch for next release,
so it will be a base for continuing this discussion.
@@ -1135,6 +1135,41 @@ rte_eth_dev_tx_offload_name(uint64_t offload)
return name;
}
+static int
+_rte_eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads,
+ uint64_t set_offloads,
+ const char *(*f)(uint64_t))
+{
+ uint64_t offloads_diff = req_offloads ^ set_offloads;
+ uint64_t offloads_req_diff, offloads_set_diff;
+ uint64_t offload;
+ uint8_t err = 0;
+
+ /* Check if any offload is advertised but not enabled. */
+ offloads_req_diff = offloads_diff & req_offloads;
+ while (offloads_req_diff) {
+ offload = 1ULL << __builtin_ctzll(offloads_req_diff);
+ offloads_req_diff &= ~offload;
+ RTE_ETHDEV_LOG(ERR, "Port %u failed to enable %s offload",
+ port_id, f(offload));
+ err = 1;
+ }
+
+ if (err)
+ return -EINVAL;
+
+ /* Chech if any offload couldn't be disabled. */
+ offloads_set_diff = offloads_diff & set_offloads;
+ while (offloads_set_diff) {
+ offload = 1ULL << __builtin_ctzll(offloads_set_diff);
+ offloads_set_diff &= ~offload;
+ RTE_ETHDEV_LOG(INFO, "Port %u failed to disable %s offload",
+ port_id, f(offload));
+ }
+
+ return 0;
+}
+
int
rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
const struct rte_eth_conf *dev_conf)
@@ -1358,6 +1393,30 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
goto rollback;
}
+ /* Validate Rx offloads. */
+ diag = _rte_eth_dev_validate_offloads(port_id,
+ dev_conf->rxmode.offloads,
+ dev->data->dev_conf.rxmode.offloads,
+ rte_eth_dev_rx_offload_name);
+ if (diag != 0) {
+ rte_eth_dev_rx_queue_config(dev, 0);
+ rte_eth_dev_tx_queue_config(dev, 0);
+ ret = diag;
+ goto rollback;
+ }
+
+ /* Validate Tx offloads. */
+ diag = _rte_eth_dev_validate_offloads(port_id,
+ dev_conf->txmode.offloads,
+ dev->data->dev_conf.txmode.offloads,
+ rte_eth_dev_tx_offload_name);
+ if (diag != 0) {
+ rte_eth_dev_rx_queue_config(dev, 0);
+ rte_eth_dev_tx_queue_config(dev, 0);
+ ret = diag;
+ goto rollback;
+ }
+
return 0;
rollback: