Re: [PATCH v3 net-next] sctp: Add support to per-association statistics via a new SCTP_GET_ASSOC_STA

[Michele Baldessari <michele@acksyn.org>]

Hi Vlad,

On Thu, Nov 29, 2012 at 12:31:17PM -0500, Vlad Yasevich wrote:
> On 11/28/2012 02:39 PM, Michele Baldessari wrote:
> 
> >diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c
> >index 397296f..7edc89d 100644
> >--- a/net/sctp/inqueue.c
> >+++ b/net/sctp/inqueue.c
> >@@ -105,6 +105,9 @@ void sctp_inq_push(struct sctp_inq *q, struct sctp_chunk *chunk)
> >  	 */
> >  	list_add_tail(&chunk->list, &q->in_chunk_list);
> >  	q->immediate.func(&q->immediate);
> >+
> >+	if (chunk->asoc)
> >+		chunk->asoc->stats.ipackets++;
> 
> you may want to consider putting this before the immediate.func()
> call, so that you record an incoming packet before it's fully
> processed.  This
> would mimic the behavior of the rest of the stats you are collecting, as
> you typically increment the stat and then process the data.

ok, makes sense.

> >  }
> >
> >  /* Peek at the next chunk on the inqeue. */
> 
> >
> >+/*
> >+ * SCTP_GET_ASSOC_STATS
> >+ *
> >+ * This option retrieves local per endpoint statistics. It is modeled
> >+ * after OpenSolaris' implementation
> >+ */
> >+static int sctp_getsockopt_assoc_stats(struct sock *sk, int len,
> >+				       char __user *optval,
> >+				       int __user *optlen)
> >+{
> >+	struct sctp_assoc_stats sas;
> >+	struct sctp_association *asoc = NULL;
> >+
> >+	/* User must provide at least the assoc id */
> >+	if (len < sizeof(sctp_assoc_t))
> >+		return -EINVAL;
> >+
> >+	if (copy_from_user(&sas, optval, len))
> >+		return -EFAULT;
> >+
> >+	asoc = sctp_id2assoc(sk, sas.sas_assoc_id);
> >+	if (!asoc)
> >+		return -EINVAL;
> >+
> >+	sas.sas_rtxchunks = asoc->stats.rtxchunks;
> >+	sas.sas_gapcnt = asoc->stats.gapcnt;
> >+	sas.sas_outofseqtsns = asoc->stats.outofseqtsns;
> >+	sas.sas_osacks = asoc->stats.osacks;
> >+	sas.sas_isacks = asoc->stats.isacks;
> >+	sas.sas_octrlchunks = asoc->stats.octrlchunks;
> >+	sas.sas_ictrlchunks = asoc->stats.ictrlchunks;
> >+	sas.sas_oodchunks = asoc->stats.oodchunks;
> >+	sas.sas_iodchunks = asoc->stats.iodchunks;
> >+	sas.sas_ouodchunks = asoc->stats.ouodchunks;
> >+	sas.sas_iuodchunks = asoc->stats.iuodchunks;
> >+	sas.sas_idupchunks = asoc->stats.idupchunks;
> >+	sas.sas_opackets = asoc->stats.opackets;
> >+	sas.sas_ipackets = asoc->stats.ipackets;
> >+
> >+	/* New high max rto observed, will return 0 if not a single
> >+	 * RTO update took place. obs_rto_ipaddr will be bogus
> >+	 * in such a case
> >+	 */
> >+	sas.sas_maxrto = asoc->stats.max_obs_rto;
> >+	memcpy(&sas.sas_obs_rto_ipaddr, &asoc->stats.obs_rto_ipaddr,
> >+		sizeof(struct sockaddr_storage));
> >+
> >+	/* Mark beginning of a new observation period */
> >+	asoc->stats.max_obs_rto = 0;
> 
> Ok.  That's better.  If there are 2 fast queries you get 0 on the
> second, but that still feels a bit strange to me.  I think resetting
> max_obs_rto to rto_min might make more sense.  rto_min is the low bound
> and rto can't go below that.  So, on the next rto measurement, that'll
> be the smallest value of max_obs_rto.  IMO, it might be better to reset
> the max_obs_rto to rto_min, so that
>  1) We always see a valid rto value in the field.
>  2) We can more easily see the variances in rto over time.
> 
> What do you think?

I agree, it makes more sense (will send an updated version shortly)

thanks,
Michele
-- 
Michele Baldessari            <michele@acksyn.org>
C2A5 9DA3 9961 4FFB E01B  D0BC DDD4 DCCB 7515 5C6D