On 6/30/23 10:00, Robin Jarry wrote:
> Hi Ilya,
>
> Ilya Maximets, Jun 29, 2023 at 23:57:
>>> + if (flags && ovsrcu_get(void *, &netdev->hw_info.offload_data)) {
>>
>> We probbaly shouldn't use the offload_data outside of netdev-offload
>> modules. Simply checking netdev_is_flow_api_enabled() should be
>> enough. Since both features require rte_flow oflload it's unlikely
>> that rx-steering can work if flow api is enabled globally. And
>> netdev-offload-dpdk doesn't really check device capabilities.
>
> OK, I'll change that for v13.
>
>>> static int
>>> netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args,
>>> char **errp)
>>
>> Aside from the set_config, we also need a get_config callback update.
>> get_config() should return everything that set_config() set. i.e.
>> we should return the "rx-steering": "rss+lacp", if user configured it.
>> The output will be visible in the dpif/show.
>
> Hmm, I had missed the get_config callback. I have added something for
> get status but I assume it is different.
Right. 'status' is anything the netdev provider wants to report in
a free form. 'config' is something that users can put into a database
to get a configuration equivalent to a current one.
get_config() in netdev-dpdk currently reports weird stuff and needs
fixing. If you want an example, look at n_rxq_desc reporting, it is
more or less correctly done.
Having some extra information in the status is fine.
>
>>> + memset(reta_conf, 0, sizeof(reta_conf));
>>> + for (uint16_t i = 0; i < info.reta_size; i++) {
>>> + uint16_t idx = i / RTE_ETH_RETA_GROUP_SIZE;
>>> + uint16_t shift = i % RTE_ETH_RETA_GROUP_SIZE;
>>
>> An empty line here.
>>
>>> + reta_conf[idx].mask |= 1ULL << shift;
>>> + reta_conf[idx].reta[shift] = i % rss_n_rxq;
>>> + }
>>
>> And here.
>
> Ack.
>
>>> + for (int i = 0; i < dev->rx_steer_flows_num; i++) {
>>> + set_error(&error, RTE_FLOW_ERROR_TYPE_NONE);
>>> + if (rte_flow_destroy(dev->port_id, dev->rx_steer_flows[i],
>>> &error)) {
>>> + VLOG_DBG("%s: rx-steering: failed to destroy flow: %s",
>>
>> Maybe a WARN ?
>
> Will change that.
>
>> So, this function destroys all the flows, OK.
>> But we also need to restore the redirection table, right? Or is it handled
>> somehow in the driver when number of queues changed? From my experience,
>> drivers tend to not restore this kind of configuration even on device detach
>> and it gets preserved even across OVS restarts.
>
> From what I saw during testing, the RETA is reset every time the device
> is reset. In the first versions of this patch, I did reset the table but
> I seem to remember some discussions with Kevin and/or David where we
> concluded that it was redundant. I will check again to make sure before
> sending a respin.
OK. Thanks!
Was just checking that this part wasn't overlooked. Maybe add a comment
to a function on why it doesn't restore the redirection table, so the
code readers don't have extra questions?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
