On 7/7/24 22:09, Adrian Moreno wrote: > When an action set ends in a an OFP_SAMPLE action, it means the packet > will be dropped and sampled. Make the drop explicit in this case so that > drop statistics remain accurate. > > This could be done outside of the sample action, i.e: "sample(...),drop" > but datapaths optimize sample actions that are found in the last > position. So, taking into account that datapaths already report when the > last sample probability fails, it is safe to put the drop inside the > sample, i.e: "sample(...,drop)". > > Signed-off-by: Adrian Moreno <[email protected]> > --- > ofproto/ofproto-dpif-xlate.c | 16 +++++++-- > tests/drop-stats.at | 65 ++++++++++++++++++++++++++++++++++++ > tests/ofproto-dpif.at | 44 ++++++++++++++++++++++++ > 3 files changed, 123 insertions(+), 2 deletions(-)
Hi, Adrian. As we discussed before on the kernel list, I'm not sure this is a right change to make for a fwe reasons: 1. We do not know the user's intentions. We do not know if they wanted to drop these packets or their goal was to sample them and it just happened to be the last action in the list, because they put it after the output action and not before. 2. This patch doesn't cover cases where sampling is not actually the last action, but further actions do not yield any datapath actions. E.g. if you have a register resoration afterwards, which happens in automatically built pipelines like in OVN. Or if the last action after sampling is learn(). And things are getting more complicated if we take into account resubmits and processing in different bridges via patch ports. 3. If someone is sampling the drops specifically, they know where to find the packets, because they configured the collector. 4. Packets are not actually dropped, they are delivered to userspace or psample. It might make sense though to drop with a reson in case the upcall fails or psample fails to deliver to any entity and it is the last action in the datapath, but that's a kernel change to make. 5. Drop reporting in either datapath implementation is not 100% accurate anyway and requires users to know the kernel internals. Some of that also applies to the next patch in the set. For the patch itself, you should also check that the action set is actually empty (not the action list) after the sample action translation. There is a chance that the clone action is disabled and sample is used in place of a clone, I'm not sure this case is covered. And the formatting in tests seems very inconsistent. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
