Re: Prioritized protocol processing project announcement

* Takashi Ikebe ([email protected]) wrote:
> Hello, all.
> NTT announces "prioratus" open source project.
> The prioratus project provides "PRF.33.5 Prioritized protocol
> processing"  GPL implementation.
> First version of implementation is available on the web.
> Prioratus provides high-priority protocol processing mechanism in
> addition to ksoftirq, and makes high priority packets deriver low-latency.
> 
> URL is below.
> http://prioratus.sourceforge.net/

Nice to see you've released this work.  Can you help me understand what
is missing in current combinations of packet prioritzation and scheduler
priorities?  I don't quite see the justification for this work yet.

Some comments on the patch itself (no comment on the functionality yet,
I'm still understanding it):

include/net/cong_ctrl.h

+#include <linux/in.h>
+#include <linux/errno.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>

These aren't necessary.


main/init.c
+#ifdef CONFIG_NET_CONG_CTRL
+       extern int spawn_netif_dequeue_low_pri_skb_thread(void);
+       spawn_netif_dequeue_low_pri_skb_thread();
+#endif /* CONFIG_NET_CONG_CTRL */

Can't mix the declaration there.  And why there, can't it be done from
networking init (sock_init())?  It looks like you want it to be on the
bootstrap processor only?

net/Kconfig
+config NET_CONG_CTRL
+       bool "Congestion control (EXPERIMENTAL)"
+       default y

Should probably be default n.

+       depends on EXPERIMENTAL
+       ---help---
+         Congestion control.

The help doesn't help explain the feature very well.  Similar with all
the config items.  Also, there are way too many config items.  For the
min/max features, please use a nice default, and if you need to
reconfigure to it dynamically via a userspace request (write to sysctl,
sysfs, proc, whatever you're using for other userspace interaction.


net/core/cong_ctrl.c 

+static int cong_init_stat = CONG_CTRL_INIT_NO_ERROR;
+static u64 prev_idle    = 0ULL;
+static u64 prev_busy    = 0ULL;
+static u64 prev_irq     = 0ULL;
+static u64 prev_softirq = 0ULL;
+static unsigned int cpu_high_load = 0;
+static unsigned int load_chg_count = 0;

All this is unnecessary.  BSS data is guaranteed to be 0 initialized

+static struct high_pri_table active_table[CONG_HIGH_PRI_MAXPROTOCOLS] ;

Extra space before ';' should be removed.

+#ifdef CONFIG_NET_CONG_CTRL_DEBUG_DETAIL_LOG
+#if 0
<snip>
+#endif
+#endif

Please don't do this.  This code will never compile into .o anyway.  It
should either be removed, or you should use a macro (like pr_debug)
which you can code directly into function body, and it will only be
emitted if DEBUG is defined.  The code is hard to read otherwise.

I think you have unprotected __get_cpu() calls because of this style of
coding (couldn't been preempted and switched cpus).

net/dev/core.c
+#ifdef CONFIG_NET_CONG_CTRL
+int netif_receive_skb_org(struct sk_buff *skb)
+#else
 int netif_receive_skb(struct sk_buff *skb)
+#endif /* CONFIG_NET_CONG_CTRL */

Sorry, you can't do that.  It will never be acceptable to mainline.
If you absolutely have to overwrite the netif_receive_skb function (looks
like a very bad idea), then at least hide it in the proper header file.

include/linux/netdevice.h
#ifdef CONFIG_NET_CONG_CTRL
static inline int netif_receive_skb(struct sk_buff *skb)
{
	if (cong_ctl_cpu_high_load())
		return netif_check_high_pri_skb();
	return __netif_receive_skb();
}
#else
#define netif_receive_skb __netif_receive_skb
...
extern int	__netif_receive_skb();

I would highly recommend using TC Action instead.  That's its purpose.
It already does egress scheduling, best would be a way to make it do
ingress scheduling.  Then you could reuse all the existing infrastructure.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

_______________________________________________
cgl_discussion mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/cgl_discussion