Re: [Intel-wired-lan] [PATCH net v1] iavf: Fix 'tc qdisc show' list too many queues

["Jagielski, Jedrzej" <jedrzej.jagielski@intel.com>]

Hello Paul,

>Am 15.06.22 um 11:16 schrieb Paul Menzel:
>> Dear Jedrzej, dear Przemyslaw,
>> 
>> 
>> Am 15.06.22 um 10:33 schrieb Jedrzej Jagielski:
>>> From: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
>> 
>> In the summary:
>> 
>> s/list/listing/
>> 
>>> Fix tc qdisc show dev <ethX> root displaying too many fq_codel
>>> qdiscs.
>>> tc_modify_qdisc, which is caller of ndo_setup_tc, expects
>>> driver to call netif_set_real_num_tx_queues, which prepares
>>> qdiscs.
>>> Without this patch, fq_codel qdiscs would not be adjusted to
>>> number of queues on VF.
>> 
>> Please reflow for 75 characters per line.

Sure, I will.

>> 
>>> e.g.:
>>> tc qdisc show dev <ethX>
>>> qdisc mq 0: root
>>> qdisc fq_codel 0: parent :4 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>> qdisc fq_codel 0: parent :3 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>> qdisc fq_codel 0: parent :2 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>> qdisc fq_codel 0: parent :1 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>> tc qdisc add dev <ethX> root mqprio num_tc 2 map 1 0 0 0 0 0 0 0 queues 1@0 1@1 hw 1 mode channel shaper bw_rlimit max_rate 5000Mbit 150Mbit
>>> tc qdisc show dev <ethX>
>>> qdisc mqprio 8003: root tc 2 map 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
>>>               queues:(0:0) (1:1)
>>>               mode:channel
>>>               shaper:bw_rlimit   max_rate:5Gbit 150Mbit
>>> qdisc fq_codel 0: parent 8003:4 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>> qdisc fq_codel 0: parent 8003:3 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>> qdisc fq_codel 0: parent 8003:2 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>> qdisc fq_codel 0: parent 8003:1 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>>
>>> While after fix:
>>> tc qdisc add dev <ethX> root mqprio num_tc 2 map 1 0 0 0 0 0 0 0 
>>> queues 1@0 1@1 hw 1 mode channel shaper bw_rlimit max_rate 5000Mbit 
>>> 150Mbit
>>> tc qdisc show dev <ethX> #should show 2, shows 4
>>> qdisc mqprio 8004: root tc 2 map 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
>>>               queues:(0:0) (1:1)
>>>               mode:channel
>>>               shaper:bw_rlimit   max_rate:5Gbit 150Mbit
>>> qdisc fq_codel 0: parent 8004:2 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>> qdisc fq_codel 0: parent 8004:1 limit 10240p flows 1024 quantum 1514 target 5ms interval 100ms memory_limit 32Mb ecn drop_batch 64
>>>
>>> Fixes: d5b33d024496 ("i40evf: add ndo_setup_tc callback to i40evf")
>>> Signed-off-by: Przemyslaw Patynowski <przemyslawx.patynowski@intel.com>
>>> Signed-off-by: Grzegorz Szczurek <grzegorzx.szczurek@intel.com>
>>> Signed-off-by: Kiran Patil <kiran.patil@intel.com>
>>> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
>>> ---
>>>   drivers/net/ethernet/intel/iavf/iavf.h        |  5 +++++
>>>   drivers/net/ethernet/intel/iavf/iavf_main.c   | 20 +++++++++++++++++++
>>>   .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  1 +
>>>   3 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h 
>>> b/drivers/net/ethernet/intel/iavf/iavf.h
>>> index 49aed3e506a6..05cd2dd5bd36 100644
>>> --- a/drivers/net/ethernet/intel/iavf/iavf.h
>>> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
>>> @@ -427,6 +427,11 @@ struct iavf_adapter {
>>>       /* lock to protect access to the cloud filter list */
>>>       spinlock_t cloud_filter_list_lock;
>>>       u16 num_cloud_filters;
>>> +    /* snapshot of "num_active_queues" before setup_tc for qdisc add
>>> +     * is invoked. This information is useful during qdisc del flow,
>>> +     * to restore correct number of queues
>>> +     */
>>> +    int orig_num_active_queues;
>>>   #define IAVF_MAX_FDIR_FILTERS 128    /* max allowed Flow Director 
>>> filters */
>>>       u16 fdir_active_fltr;
>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c 
>>> b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> index f3ecb3bca33d..d2220043fd48 100644
>>> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
>>> @@ -3411,6 +3411,7 @@ static int __iavf_setup_tc(struct net_device 
>>> *netdev, void *type_data)
>>>               netif_tx_disable(netdev);
>>>               iavf_del_all_cloud_filters(adapter);
>>>               adapter->aq_required = IAVF_FLAG_AQ_DISABLE_CHANNELS;
>>> +            total_qps = adapter->orig_num_active_queues;
>>>               goto exit;
>>>           } else {
>>>               return -EINVAL;
>>> @@ -3454,7 +3455,20 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
>>>                   adapter->ch_config.ch_info[i].offset = 0;
>>>               }
>>>           }
>>> +
>>> +        /* Take snapshot of original config such as "num_active_queues"
>>> +         * It is used later when delete ADQ flow is exercised, so that
>>> +         * once delete ADQ flow completes, VF shall go back to its
>>> +         * original queue configuration
>>> +         */
>>> +
>>> +        adapter->orig_num_active_queues = adapter->num_active_queues;
>> 
>> It’d move the blank line below.
>> 
>>> +        /* Store queue info based on TC so that, VF gets configured
>> 
>> Remove the comma?

Sure.

>> 
>>> +         * with correct number of queues when VF completes ADQ config
>>> +         * flow
>>> +         */
>>>           adapter->ch_config.total_qps = total_qps;
>>> +
>>>           netif_tx_stop_all_queues(netdev);
>>>           netif_tx_disable(netdev);
>>>           adapter->aq_required |= IAVF_FLAG_AQ_ENABLE_CHANNELS;
>>> @@ -3471,6 +3485,12 @@ static int __iavf_setup_tc(struct net_device *netdev, void *type_data)
>>>           }
>>>       }
>>>   exit:
>>> +    if (test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
>>> +        return 0;
>>> +
>>> +    netif_set_real_num_rx_queues(netdev, total_qps);
>>> +    netif_set_real_num_tx_queues(netdev, total_qps);
>>> +
>>>       return ret;
>>>   }
>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c 
>>> b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
>>> index 782450d5c12f..8d5f1d5b49cd 100644
>>> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
>>> @@ -1920,6 +1920,7 @@ void iavf_virtchnl_completion(struct 
>>> iavf_adapter *adapter,
>>>           }
>>>           return;
>>>       }
>>> +
>> 
>> Unrelated.

Oh I didn't notice that. Thanks for pointing it out.

>> 
>>>       if (v_retval) {
>>>           switch (v_opcode) {
>>>           case VIRTCHNL_OP_ADD_VLAN:
>> 
>> 
>> Kind regards,
>> 
>> Paul

Thanks for suggestions.

Best Regards,
Jedrzej
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan