Re: [lng-odp] [PATCH v3] linux-gen: pktio: ignore num_queues parameter if classifier enabled

[Github ODP bot]

muvarov replied on github web page:

platform/linux-generic/odp_packet_io.c
@@ -1229,7 +1229,12 @@ int odp_pktin_queue_config(odp_pktio_t pktio,
if (mode == ODP_PKTIN_MODE_DISABLED)
return 0;
 
-   num_queues = param->num_queues;
+   if (param->classifier_enable) {
+   ODP_DBG("num_queues ignored if classifier is enabled\n");
+   num_queues = 1;
+   } else {
+   num_queues = param->num_queues;
+   }


Comment:
@Bill-Fischofer-Linaro  I was faster updating this PR :) Please review.

> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Good point, though that could be optimized to:
> ```
>   for (i = 0; i < num_queues; i++) {
>   if (mode == ODP_PKTIN_MODE_QUEUE ||
>   mode == ODP_PKTIN_MODE_SCHED) {
>   odp_queue_param_t queue_param;
>   char name[ODP_QUEUE_NAME_LEN];
>   int pktio_id = pktio_to_id(pktio);
> 
>   snprintf(name, sizeof(name), "odp-pktin-%i-%i",
>pktio_id, i);
> 
> - memcpy(_param, >queue_param,
> -sizeof(odp_queue_param_t));
> -
> + if (param->classifier_enable)
> + odp_queue_param_init(_param);
> +   else
> +  memcpy(_param, >queue_param,
> +  sizeof(odp_queue_param_t));
>   queue_param.type = ODP_QUEUE_TYPE_PLAIN;
> ```


>> lironhimi wrote
>> you should also ignore the input 'param->queue_param'. consider the 
>> following:
>> ```
>>  for (i = 0; i < num_queues; i++) {
>>  if (mode == ODP_PKTIN_MODE_QUEUE ||
>>  mode == ODP_PKTIN_MODE_SCHED) {
>>  odp_queue_param_t queue_param;
>>  char name[ODP_QUEUE_NAME_LEN];
>>  int pktio_id = pktio_to_id(pktio);
>> 
>>  snprintf(name, sizeof(name), "odp-pktin-%i-%i",
>>   pktio_id, i);
>> 
>>  memcpy(_param, >queue_param,
>> sizeof(odp_queue_param_t));
>> 
>> +if (param->classifier_enable)
>> +odp_queue_param_init(_param);
>>  queue_param.type = ODP_QUEUE_TYPE_PLAIN;
>> '''


>>> muvarov wrote
>>> yes, that will work. Will send v2.


 Bill Fischofer(Bill-Fischofer-Linaro) wrote:
 Ok, that check may be too strict. If we're going to ignore it then the 
 test should be changed to:
 ```
 if (!param->classifier_enable && param->num_queues == 0) {
 ODP_DBG("invalid num_queues for operation mode\n");
 return -1;
 }
 
 num_queues = param->classifier_enable ? 1 : param->num_queues;
 ```


> muvarov wrote
> API says " When classifier is enabled in odp_pktin_queue_config() this 
> value is ignored".  Should we also correct API statement to "When 
> classifier is enabled num_queues has to be 0" ?.


>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> The requirement is that `num_queues` should be zero if and only if 
>> `param->classifier_enable` is true, so a more complete check would be:
>> ```
>> if (param->classifier_enable ^ param->num_queues == 0) {
>> ODP_DBG("invalid num_queues for operation mode\n");
>> return -1;
>> }
>> 
>> num_queues = param->classifier_enable ? 1 : param->num_queues;
>> ```


https://github.com/Linaro/odp/pull/240#discussion_r145979242
updated_at 2017-10-20 14:33:38

Re: [lng-odp] [PATCH v3] linux-gen: pktio: ignore num_queues parameter if classifier enabled

[Github ODP bot]

Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:

platform/linux-generic/odp_packet_io.c
@@ -1229,7 +1229,12 @@ int odp_pktin_queue_config(odp_pktio_t pktio,
if (mode == ODP_PKTIN_MODE_DISABLED)
return 0;
 
-   num_queues = param->num_queues;
+   if (param->classifier_enable) {
+   ODP_DBG("num_queues ignored if classifier is enabled\n");
+   num_queues = 1;
+   } else {
+   num_queues = param->num_queues;
+   }


Comment:
Good point, though that could be optimized to:
```
for (i = 0; i < num_queues; i++) {
if (mode == ODP_PKTIN_MODE_QUEUE ||
mode == ODP_PKTIN_MODE_SCHED) {
odp_queue_param_t queue_param;
char name[ODP_QUEUE_NAME_LEN];
int pktio_id = pktio_to_id(pktio);

snprintf(name, sizeof(name), "odp-pktin-%i-%i",
 pktio_id, i);

-   memcpy(_param, >queue_param,
-  sizeof(odp_queue_param_t));
-
+   if (param->classifier_enable)
+   odp_queue_param_init(_param);
+   else
+  memcpy(_param, >queue_param,
+  sizeof(odp_queue_param_t));
queue_param.type = ODP_QUEUE_TYPE_PLAIN;
```

> lironhimi wrote
> you should also ignore the input 'param->queue_param'. consider the following:
> ```
>   for (i = 0; i < num_queues; i++) {
>   if (mode == ODP_PKTIN_MODE_QUEUE ||
>   mode == ODP_PKTIN_MODE_SCHED) {
>   odp_queue_param_t queue_param;
>   char name[ODP_QUEUE_NAME_LEN];
>   int pktio_id = pktio_to_id(pktio);
> 
>   snprintf(name, sizeof(name), "odp-pktin-%i-%i",
>pktio_id, i);
> 
>   memcpy(_param, >queue_param,
>  sizeof(odp_queue_param_t));
> 
> + if (param->classifier_enable)
> + odp_queue_param_init(_param);
>   queue_param.type = ODP_QUEUE_TYPE_PLAIN;
> '''


>> muvarov wrote
>> yes, that will work. Will send v2.


>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Ok, that check may be too strict. If we're going to ignore it then the test 
>>> should be changed to:
>>> ```
>>> if (!param->classifier_enable && param->num_queues == 0) {
>>> ODP_DBG("invalid num_queues for operation mode\n");
>>> return -1;
>>> }
>>> 
>>> num_queues = param->classifier_enable ? 1 : param->num_queues;
>>> ```


 muvarov wrote
 API says " When classifier is enabled in odp_pktin_queue_config() this 
 value is ignored".  Should we also correct API statement to "When 
 classifier is enabled num_queues has to be 0" ?.


> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The requirement is that `num_queues` should be zero if and only if 
> `param->classifier_enable` is true, so a more complete check would be:
> ```
> if (param->classifier_enable ^ param->num_queues == 0) {
> ODP_DBG("invalid num_queues for operation mode\n");
> return -1;
> }
> 
> num_queues = param->classifier_enable ? 1 : param->num_queues;
> ```


https://github.com/Linaro/odp/pull/240#discussion_r145978457
updated_at 2017-10-20 14:31:40