From 4656ba585c3b61307262fca3158bb6d7712a374c Mon Sep 17 00:00:00 2001
From: Neil McKee <neil.mckee@inmon.com>
Date: Tue, 8 Oct 2024 16:14:07 -0700
Subject: [PATCH] bugfix: some off-by-one corrections to sampling logic and
 pool counter.

---
 sflow/node.c  | 19 +++++++++++++------
 sflow/sflow.h |  5 ++++-
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/sflow/node.c b/sflow/node.c
index f1c0015..04df492 100644
--- a/sflow/node.c
+++ b/sflow/node.c
@@ -84,16 +84,19 @@ VLIB_NODE_FN (sflow_node) (vlib_main_t * vm,
   uword thread_index = os_get_thread_index();
   sflow_per_thread_data_t *sfwk = vec_elt_at_index(smp->per_thread_data, thread_index);
 
+  /* note that sfwk->skip==1 means "take the next packet",
+     so we never see sfwk->skip==0. */
+
   u32 pkts = n_left_from;
-  if(PREDICT_TRUE(sfwk->skip >= pkts)) {
+  if(PREDICT_TRUE(sfwk->skip > pkts)) {
     /* skip the whole frame-vector */
     sfwk->skip -= pkts;
     sfwk->pool += pkts;
   }
   else {
-    while(pkts > sfwk->skip) {
+    while(pkts >= sfwk->skip) {
       /* reach in to get the one we want. */
-      vlib_buffer_t *bN = vlib_get_buffer (vm, from[sfwk->skip]);
+      vlib_buffer_t *bN = vlib_get_buffer (vm, from[sfwk->skip - 1]);
 
       /* Sample this packet header. */
       u32 hdr = bN->current_length;
@@ -135,14 +138,18 @@ VLIB_NODE_FN (sflow_node) (vlib_main_t * vm,
       sfwk->pool += sfwk->skip;
       sfwk->skip = sflow_next_random_skip(sfwk);
     }
+    /* We took a sample (or several) from this frame-vector, but now we are
+       skipping the rest. */
+    sfwk->skip -= pkts;
+    sfwk->pool += pkts;
   }
 
   /* the rest of this is boilerplate code just to make sure
    * that packets are passed on the same way as they would
    * have been if this node were not enabled.
-   *
-   * Not sure at all if this is right.
-   * There has got to be an easier way?
+   * TODO: If there is ever a way to do this in one step
+   * (i.e. pass on the whole frame-vector unchanged) then it
+   * might help performance.
    */
   
   next_index = node->cached_next_index;
diff --git a/sflow/sflow.h b/sflow/sflow.h
index e8b8588..6e97891 100644
--- a/sflow/sflow.h
+++ b/sflow/sflow.h
@@ -171,9 +171,12 @@ extern sflow_main_t sflow_main;
 extern vlib_node_registration_t sflow_node;
 
 static inline u32 sflow_next_random_skip(sflow_per_thread_data_t *sfwk) {
+  /* skip==1 means "take the next packet" so this
+     fn must never return 0 */
   if(sfwk->smpN <= 1)
     return 1;
-  return (random_u32(&sfwk->seed) % (2 * sfwk->smpN) - 1) + 1;
+  u32 lim = (2 * sfwk->smpN) - 1;
+  return (random_u32(&sfwk->seed) % lim) + 1;
 }
 
 #endif /* __included_sflow_h__ */