On Mon, Jun 24, 2024 at 07:43:32PM GMT, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 04:05:36PM GMT, Ilya Maximets wrote: > > On 6/24/24 15:19, Adrián Moreno wrote: > > > On Mon, Jun 24, 2024 at 01:37:44PM GMT, Ilya Maximets wrote: > > >> On 6/5/24 22:23, Adrian Moreno wrote: > > >>> Use the newly added emit_sample to implement OpenFlow sample() actions > > >>> with local sampling configuration. > > >>> > > >>> Signed-off-by: Adrian Moreno <[email protected]> > > >>> --- > > >>> ofproto/ofproto-dpif-lsample.c | 17 ++++ > > >>> ofproto/ofproto-dpif-lsample.h | 6 ++ > > >>> ofproto/ofproto-dpif-xlate.c | 163 ++++++++++++++++++++++++--------- > > >>> ofproto/ofproto-dpif-xlate.h | 3 +- > > >>> ofproto/ofproto-dpif.c | 2 +- > > >>> 5 files changed, 144 insertions(+), 47 deletions(-) > > >> > > >> Not a full review, just a note that this patch needs tests in > > >> ofproto-dpif.at > > >> that would check that we generate correct datapath flows. > > >> > > > > > > I thought about that, but ofproto-dpif.at is based on dummy datapat > > > (userspace) which does not support this action. The configuration will > > > be ignored and the datapath actions will not be generated. That's why I > > > relied on system-traffic.at tests. Do you see any way around this? > > > > We don't need actual datapath support if we're not sending any actual > > trafic. You should be able to just turn on the capability and check > > that ofproto/trace generates correct actions. > > > > We test kernel tunnels this way, for example. See the macro > > OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP. And some other similar checks. > > >
Tried it but it doesn't seem to work. I now remember I hit this issue originally :-). We cannot enable a capability beyond the datapath's "boot time" capabilities. If you try to run "ovs-appctl dpif/set-dp-features br0 emit_sample true", the command fails with: "Can not enable features not supported by the datapth" The safeguard does make sense in general (prevents users from knee-capping themselves) but for testing maybe we want to force it? OTOH, marking this feature as supported in the userspace datapath would not be catastrophic, we could just warn the action is not supported in odp_execute(), or consider VLOG_DBG_RL as current implementation until we come up with a good one. However, I cannot of a good way of of properly reporting this to the user beforehand so I think it could still cause a lot of confusion. Thoughts? Thanks. Adrián _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
