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(&queue_param, &param->queue_param,
> -                            sizeof(odp_queue_param_t));
> -
> +                     if (param->classifier_enable)
> +                             odp_queue_param_init(&queue_param);
> +                       else
> +                              memcpy(&queue_param, &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(&queue_param, &param->queue_param,
>>                             sizeof(odp_queue_param_t));
>> 
>> +                    if (param->classifier_enable)
>> +                            odp_queue_param_init(&queue_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