netdev - Re: [DCCP] [RFC] [Patchv2 1/1]: Queuing policies -- reworked version of Tomasz's patch set

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, 15 Apr 2008 01:45:05 +0200
From:	Tomasz Grobelny <tomasz@...belny.oswiecenia.net>
To:	Gerrit Renker <gerrit@....abdn.ac.uk>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	dccp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [DCCP] [RFC] [Patchv2 1/1]: Queuing policies -- reworked version of Tomasz's patch set

Dnia Monday 14 of April 2008, Gerrit Renker napisał:
> [DCCP] [RFC]: Reworked queueing policy patch set
>
> This is a re-worked version of Tomasz's priority-queueing patch set for
> DCCP. The essential internals of his patch set have been left untouched,
> in particular the prio/simple algorithms are the same as before.
>
> The changes for which I am asking for comments are:
>
>   Given that each file has less than 100 lines, it seemed better to put
> them all into one file and export the declarations in dccp.h, the result is
> now:
>
>   With less than 150 lines, there is still room for a few more policies.
>
For now the files are small but I would expect at least qpolicy_prio.c to grow 
a little bit in the future. What comes to mind are get/setsockopts (for 
example statistics about packets dropped by qpolicy), and per packet expiry 
times. I didn't look how big the files are but wanted to have clear 
separation between specific policies and common code. But if you think that 
this organization is better I have no problem with it.

> 2. Let pop() return result
> --------------------------
>   * at least for the forseeable future, the underlying queue is
> sk_write_queue; * an skb can not be on two lists at the same time;
>   * thus skb_unlink will always be part of implementing pop();
>   * having pop() return the skb is more versatile (compare Assembler
> pop()).
>
That should be fine...

>   * makes tx_qlen a socket member of dccp_sock (there was a bug in the
>     implementation -- tx_qlen was shared by all sockets using the same
> policy); 
Haven't tested it thoroughly but it shouldn't be so. After all qpol_txqlen was 
part of struct dccp_qpolicy which was in turn part of dccp_sock.

> +/*
> + * Wrappers for methods provided by policy currently in use
> + */
> +void qpolicy_push(struct sock *sk, struct sk_buff *skb, struct msghdr
> *msg) +{
> +	struct dccp_packet_info *dcp = NULL;
> +
> +	if (msg->msg_control != NULL && msg->msg_controllen == sizeof(*dcp))
> +		dcp = (struct dccp_packet_info *)msg->msg_control;
> +
> +	qpol_table[dccp_sk(sk)->dccps_qpolicy].push(sk, skb, dcp);
> +}
> +
What happens if application is compiled with dccp_packet_info containing only 
one field (priority) and kernel containing two fields (for example priority 
and later added expiry time)? Wouldn't that implementation make extensions to 
dccp_packet_info impossible?

> @@ -501,6 +519,8 @@ struct dccp_sock {
>  	struct ccid			*dccps_hc_rx_ccid;
>  	struct ccid			*dccps_hc_tx_ccid;
>  	struct dccp_options_received	dccps_options_received;
> +	__u8				dccps_qpolicy;
> +	__u32				dccps_tx_qlen;
>  	enum dccp_role			dccps_role:2;
>  	__u8				dccps_hc_rx_insert_options:1;
>  	__u8				dccps_hc_tx_insert_options:1;
I know that currently none of the policies has any per-socket data. But if it 
had were should it go?

> +	case DCCP_SOCKOPT_QPOLICY_ID:
> +		if (sk->sk_state != DCCP_CLOSED)
> +			err = -EISCONN;
What about DCCP_LISTENING state?

> --- a/Documentation/networking/dccp.txt
> +++ b/Documentation/networking/dccp.txt
> @@ -45,6 +45,17 @@ http://linux-net.osdl.org/index.php/DCCP
>
> +DCCP_SOCKOPT_QPOLICY_TXQLEN sets the maximum TX queue length. This is
> relevant only for +those TX dequeueing policies that honour this upper
> bound (currently only the +simple/default policy does this). This socket
> option overrides the default value +of the sysctl
> /proc/sys/net/dccp/default/tx_qlen.
>
In my implementation prio policy did honour DCCP_SOCKOPT_QPOLICY_TXQLEN 
setting. At least it was meant to do so.

Having said "ok" here and there or nothing in other points I haven't tried 
applying this patch nor tested your rework.
-- 
Regards,
Tomasz Grobelny
--
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