Re: [dpdk-dev] [PATCH v15 1/7] ethdev: add set ptype function

[Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>]

>29/10/2019 16:37, pbhagavatula@marvell.com:
>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> --- a/doc/guides/rel_notes/release_19_11.rst
>> +++ b/doc/guides/rel_notes/release_19_11.rst
>> @@ -231,6 +231,14 @@ New Features
>>    * Added a console command to testpmd app, ``show port (port_id)
>ptypes`` which
>>      gives ability to print port supported ptypes in different protocol
>layers.
>>
>> +* **Added ethdev API to set supported packet types**
>> +
>> +  *  Added new API ``rte_eth_dev_set_supported_ptypes`` that
>allows an
>> +     application to inform PMD about packet types classification the
>application
>> +     is interested in
>> +  *  This scheme will allow PMDs to avoid lookup to internal ptype
>table on Rx
>> +     and thereby improve Rx performance if application wishes do so.
>
>You just added or rebased this paragraph at the end.
>As mentioned in the release notes files (top of the section),
>there is an order for presenting features.

Will fix in v16.

>
>>  Removed Items
>>  -------------
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * Inform Ethernet device of the packet types classification the
>recipient is
>> + * interested in.
>
>This is a bit convoluted.
>What about this?
>"Optimize driver handling of packet types by reducing its range."

@arybchenko@solarflare.com Thoughts?

>
>> + *
>> + * Application can use this function to set only specific ptypes that it's
>> + * interested. This information can be used by the PMD to optimize
>Rx path.
>> + *
>> + * The function accepts an array `set_ptypes` allocated by the caller
>to
>> + * store the packet types set by the driver, the last element of the
>array
>> + * is set to RTE_PTYPE_UNKNOWN. The size of the `set_ptype` array
>should be
>> + * `rte_eth_dev_get_supported_ptypes() + 1` else it might only be
>filled
>> + * partially.
>> + *
>> + * @param port_id
>> + *   The port identifier of the Ethernet device.
>> + * @param ptype_mask
>> + *   The ptype family that application is interested in should be
>bitwise OR of
>> + *   RTE_PTYPE_*_MASK or 0.
>> + * @param set_ptypes
>> + *   An array pointer to store set packet types, allocated by caller. The
>> + *   function marks the end of array with RTE_PTYPE_UNKNOWN.
>> + * @param num
>> + *   Size of the array pointed by param ptypes.
>> + *   Should be rte_eth_dev_get_supported_ptypes() + 1 to
>accommodate the
>> + *   set ptypes.
>> + * @return
>> + *   - (0) if Success.
>> + *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EINVAL) if *ptype_mask* is invalid (or) set_ptypes is NULL and
>> + *     num > 0.
>> + */
>
>John, please you check the English wording?
>
>> +__rte_experimental
>> +int rte_eth_dev_set_supported_ptypes(uint16_t port_id, uint32_t
>ptype_mask,
>> +				     uint32_t *set_ptypes, unsigned int
>num);
>
>I don't like the name of the function because they are
>not "supported" packet types but "requested".
>What about replacing "set_supported" with "set_allowed", or
>"white_list"?

"white_list" seems ok but hope it doesn't call for blacklisting API.

>