netdev - Re: [PATCHv2 1/3] NET_SCHED: PSPacer qdisc module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 Nov 2007 20:08:03 +0900 (JST)
From:	TAKANO Ryousei <takano@...-inc.co.jp>
To:	kaber@...sh.net
Cc:	takano-ryousei@...t.go.jp, netdev@...r.kernel.org,
	shemminger@...ux-foundation.org, dada1@...mosbay.com,
	t.kudoh@...t.go.jp
Subject: Re: [PATCHv2 1/3] NET_SCHED: PSPacer qdisc module

Hi Patrick,

I appreciate your comments.
I will update and resend patches.

> > +typedef long long gapclock_t;
> 
> It seems you only add to this, does it need to be signed?
> How about using a fixed size type (u64) and getting rid
> of the typedef?
> 
OK. I will use u64 instead of gapclock_t, and remove the typedef.

> > +struct tc_psp_qopt
> > +{
> > +	__u32	defcls;
> > +	__u32	rate;
> > +};
> 
> 
> What unit is rate measured in?
> 
'rate' is the transmission rate in bytes per sec.

> > +struct tc_psp_xstats
> > +{
> > +	__u32	bytes;		/* gap packet statistics */
> > +	__u32	packets;
> > +};
> 
> How about using gnet_stats_basic for this?
> 
OK. I will use gnet_stats_basic.

> > +	if (!skb)
> > +		return NULL;
> > +
> > +	memset(skb->data, 0xff, size);
> 
> memsetting the header portion seems unnecessary since you overwrite
> it again directly below.
> 
The size of a gap packet is variable, so it fills a whole gap packet
with 0xff, where 'size' indicates the interface MTU size.

> > +	skb_put(skb, size);
> 
> This is usually done before putting data in the packet.
> 
Therefore, skb_put() is needed.

> > +static int recalc_gapsize(struct sk_buff *skb, struct Qdisc *sch)
> > +{
> > +	struct psp_sched_data *q = qdisc_priv(sch);
> > +	struct psp_class *cl;
> > +	unsigned int len = skb->len;
> > +	int gapsize = 0;
> > +	int err;
> > +
> > +	cl = psp_classify(skb, sch, &err);
> > +	switch (cl->mode) {
> > +	case MODE_STATIC:
> > +		gapsize = cl->gapsize;
> > +		if (len < q->mtu) /* workaround for overflow */
> > +			gapsize = (int)((long long)gapsize * len / q->mtu);
> 
> No need to cast to the LHS type. Shouldn't this also be rounded up
> like the other gapsize calculation?
> 
How about this?

	u64 gapsize = 0;
	:
	case MODE_STATIC:
		gapsize = (u64)cl->gapsize * len / q->mtu;
		gapsize = min_t(u64, gapsize, UINT_MAX);
		break;
	}

> > +static struct psp_class *lookup_next_class(struct Qdisc *sch, int *gapsize)
> > +{
> > +	struct psp_sched_data *q = qdisc_priv(sch);
> > +	struct psp_class *cl, *next = NULL;
> > +	gapclock_t diff;
> > +	int shortest;
> > +
> > +	/* pacing class */
> > +	shortest = q->mtu;
> > +	list_for_each_entry(cl, &q->pacing_list, plist) {
> > +		diff = cl->clock - q->clock;
> > +		if (diff > 0) {
> > +			if ((gapclock_t)shortest > diff)
> > +				shortest = (int)diff;
> 
> Is diff guaranteed not to exceed an int?
> 
No. But actually, 'diff' is much smaller than INT_MAX.
How about this?

	u64 diff, shortest;
	:
		if (q->clock < cl->clock) {
			diff = cl->clock - q->clock;
			if (shortest > diff)
				shortest = diff;
			continue;
		}


> > +static void psp_destroy(struct Qdisc *sch)
> > +{
> > +	struct psp_sched_data *q = qdisc_priv(sch);
> > +
> > +	tcf_destroy_chain(q->filter_list);
> > +
> > +	while (!list_empty(&q->root))
> > +		psp_destroy_class(sch, list_entry(q->root.next,
> > +						  struct psp_class, sibling));
> 
> list_for_each_entry_safe.
> 
I think it works well. Should I need to use list_for_each_entry_safe?

Best regards,
Ryousei Takano
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html