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

[Paul Menzel <pmenzel@molgen.mpg.de>]

[Remove Kiran from Cc; and correct s/lieber/dear/]


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.
> 
>> 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?
> 
>> +         * 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.
> 
>>       if (v_retval) {
>>           switch (v_opcode) {
>>           case VIRTCHNL_OP_ADD_VLAN:
> 
> 
> Kind regards,
> 
> Paul
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan