ethdev: prefer offload names in logs

  Displaying a bitmask is terrible for users.
Prefer offload names when refusing some offloads in
rte_eth_dev_configure.

Before:
Ethdev port_id=0 requested Rx offloads 0x621 doesn't match Rx offloads
	capabilities 0x0 in rte_eth_dev_configure()

After:
Ethdev port_id=0 requested Rx offloads 'VLAN_STRIP,QINQ_STRIP,VLAN_FILTER,
	VLAN_EXTEND' in rte_eth_dev_configure(). Device supports '' Rx
	offloads but does not support 'VLAN_STRIP,QINQ_STRIP,VLAN_FILTER,
	VLAN_EXTEND'.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/ethdev/rte_ethdev.c | 80 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 68 insertions(+), 12 deletions(-)
  

On Thu,  9 Mar 2023 09:16:33 +0100
David Marchand <david.marchand@redhat.com> wrote:

> +		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u requested Rx offloads '%s' in %s(). "
> +			"Device supports '%s' Rx offloads but does not support '%s'.\n",
> +			port_id, requested != NULL ? requested : "N/A", __func__,
> +			available != NULL ? available : "N/A",
> +			unavailable != NULL ? unavailable : "N/A");
> +		free(requested);

Please shorten message and make sure it does not cross line boundaries.
Best to allow users to do simple search for message.
  

On 3/9/2023 4:21 PM, Stephen Hemminger wrote:
> On Thu,  9 Mar 2023 09:16:33 +0100
> David Marchand <david.marchand@redhat.com> wrote:
> 
>> +		RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u requested Rx offloads '%s' in %s(). "
>> +			"Device supports '%s' Rx offloads but does not support '%s'.\n",
>> +			port_id, requested != NULL ? requested : "N/A", __func__,
>> +			available != NULL ? available : "N/A",
>> +			unavailable != NULL ? unavailable : "N/A");
>> +		free(requested);
> 
> Please shorten message and make sure it does not cross line boundaries.
> Best to allow users to do simple search for message.

Agree that using offload names are more user friendly.

To keep the log more reasonable length, what would you think to split
into two, one in ERR level other is in info/debug:
ERR, "Ethdev port_id=%u does not support '%s'.\n", unavailable
DEBUG, "Ethdev port_id=%u requested Rx offloads '%s', device supports
'%s'.\n", requested, available

And I think we can drop __func__, we don't use in many other logs anyway.



Other option can be to provide APIs to print all offloads (similar to
'rte_eth_dev_tx_offload_name()'), so application does its own logging,
and ethdev just prints 'unavailable' part of the log.
  

On 3/9/2023 8:16 AM, David Marchand wrote:
> +static char *
> +eth_dev_offload_names(uint64_t bitmask, const char *(*offload_name)(uint64_t))
> +{
> +	uint64_t offload;
> +	char *names;
> +
> +	if (bitmask == 0) {
> +		names = strdup("");
> +		goto out;
> +	}
> +
> +	offload = RTE_BIT64(__builtin_ctzll(bitmask));
> +	names = strdup(offload_name(offload));
> +	if (names == NULL)
> +		goto out;
> +
> +	bitmask &= ~offload;
> +	while (bitmask != 0) {
> +		char *old = names;
> +		int ret;
> +
> +		offload = RTE_BIT64(__builtin_ctzll(bitmask));
> +		ret = asprintf(&names, "%s,%s", old, offload_name(offload));
> +		free(old);
> +		if (ret == -1) {
> +			names = NULL;
> +			goto out;
> +		}
> +
> +		bitmask &= ~offload;
> +	}
> +
> +out:
> +	return names;
> +}
> +

Above works fine, not a strong opinion but just a comment,
this function is just for logging and output string will be short lived,
but it does lots of memory alloc and free, and expose lot of failure points.

To simplify, why not just alloc a big enough buffer, fill it in a loop
and never return NULL?
(Can always append "%s," and drop final ',' at the end.)
  

On Wed, May 17, 2023 at 4:53 PM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 3/9/2023 4:21 PM, Stephen Hemminger wrote:
> > On Thu,  9 Mar 2023 09:16:33 +0100
> > David Marchand <david.marchand@redhat.com> wrote:
> >
> >> +            RTE_ETHDEV_LOG(ERR, "Ethdev port_id=%u requested Rx offloads '%s' in %s(). "
> >> +                    "Device supports '%s' Rx offloads but does not support '%s'.\n",
> >> +                    port_id, requested != NULL ? requested : "N/A", __func__,
> >> +                    available != NULL ? available : "N/A",
> >> +                    unavailable != NULL ? unavailable : "N/A");
> >> +            free(requested);
> >
> > Please shorten message and make sure it does not cross line boundaries.
> > Best to allow users to do simple search for message.
>
> Agree that using offload names are more user friendly.
>
> To keep the log more reasonable length, what would you think to split
> into two, one in ERR level other is in info/debug:
> ERR, "Ethdev port_id=%u does not support '%s'.\n", unavailable
> DEBUG, "Ethdev port_id=%u requested Rx offloads '%s', device supports
> '%s'.\n", requested, available
>
> And I think we can drop __func__, we don't use in many other logs anyway.

Splitting seems the simpler and won't require an application involvement.
I would even split the debug message in two (after all, if we have two
logs, why not three :-)).

I'll also revisit the patch wrt allocations.

>
>
>
> Other option can be to provide APIs to print all offloads (similar to
> 'rte_eth_dev_tx_offload_name()'), so application does its own logging,
> and ethdev just prints 'unavailable' part of the log.
>