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.
> > + 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.
Thanks for the review.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
