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 dccp" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
