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: <200804152138.41077.tomasz@grobelny.oswiecenia.net>
Date:	Tue, 15 Apr 2008 21:38:40 +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 Tuesday 15 of April 2008, Gerrit Renker napisał:
> | > @@ -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).
>
Let me remind you your comment made on 18/03/2008 on dccp ml to my first 
patch:
 --- START --- 
@@ -545,6 +549,8 @@ struct dccp_sock {
        __u8                            dccps_hc_tx_insert_options:1;
        __u8                            dccps_server_timewait:1;
        struct timer_list               dccps_xmit_timer;
+       struct queuing_policy           *dccps_policy;
+       void                            *dccps_policy_data;
 };

I think this should be just one field for the policy, and the
policy_data can be an internal field of `struct queueing_policy'
(compare with struct ackvec or struct ccid).
 --- END --- 

> I see several advantages of this:
>  * by just storing the u8 of the policy ID, the socket as a whole gets
> smaller,
>  * the internals of the function pointer table can be hidden,
> 
That's ok with me. 

> * when not using nested structs, the elements can be optimised for 
> minimal space,
I have nothing against it. In fact that's what I did in the first try.

> | > +	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:
> (...)
Ok, you are right. If before DCCP_LISTENING there is always DCCP_CLOSED then 
there is no need to allow changing qpolicy in DCCP_LISTENING state.

> 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
If I didn't mention some parts of the patch you can assume that I'm ok with 
them.

> 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?
>
Have a look at http://dccp.one.pl/svn/userspace/test/ And I'll also do some 
testing of your patch.

One more question (probably more for Arnaldo): is there any timeframe to push 
changes from test tree upstream?
-- 
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