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]

Message-ID: <20080415151403.GA20735@gerrit.erg.abdn.ac.uk>
Date:	Tue, 15 Apr 2008 16:14:03 +0100
From:	Gerrit Renker <gerrit@....abdn.ac.uk>
To:	Tomasz Grobelny <tomasz@...belny.oswiecenia.net>
Cc:	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

| >   * 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.
| 
I have to apologise here -- this was my oversight. The bug I assumed to be there 
does not exist, since the tx_qlen was indeed part of the socket. Sorry.

| > +/*
| > + * 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?
Had not looked at it from this point of view. So you are suggesting

	if (msg->msg_control != NULL && msg->msg_controllen >= sizeof(*dcp))
		dcp = (struct dccp_packet_info *)msg->msg_control;

instead? Agreed, point #1 to change in the patch.

| 
| > @@ -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?
| 
I can't see anything wrong with putting everything into dccp_sock. To do it well, 
we could consider inserting documentation such as "this section is only for
queueing policies" (as is done very well for struct tcp_sock).

I see several advantages of this:
 * by just storing the u8 of the policy ID, the socket as a whole gets smaller,
 * when not using nested structs, the elements can be optimised for minimal space,
 * the internals of the function pointer table can be hidden,
 * the socket carries the same information as is passed on via socket options,
   which simplifies querying.

| > +	case DCCP_SOCKOPT_QPOLICY_ID:
| > +		if (sk->sk_state != DCCP_CLOSED)
| > +			err = -EISCONN;
| What about DCCP_LISTENING state?
| 
There is a problem with allowing this, considering for example:
 * socket is created, and listen() is called,
 * socket enters with default policy,
 * now user decides to change policy from default ("simple") to "prio",
 * before he can do that, the first packet arrives on the incoming
   queue, policy is inherited to child socket,
 * so this connection ends up with "simple" policy instead of "prio".

| > +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.
| 
The documentation certainly needs more work, I just sketched something
but it is not very good. So this is point #2 which needs to change.

Ideally, it would have a little description of what each policy does and
for which use case it is best suited.

Please take your time and check through the other parts of the patch as
well. I would like to accumulate the issues and then address each of the
points. And also, do some testing.I haven't had time to write test code for
prio policy -- is there something in your SVN repository?

Gerrit

The University of Aberdeen is a charity registered in Scotland, No SC013683.

--
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