On 2/4/06, Ian McDonald <[EMAIL PROTECTED]> wrote:
> Initial implementation of transmit buffering. Tested using netem with
> delay and loss also.
>
> Signed off by: Ian McDonald <[EMAIL PROTECTED]>
> ----
> diff --git a/include/linux/dccp.h b/include/linux/dccp.h
> index 088529f..4a2f845 100644
> --- a/include/linux/dccp.h
> +++ b/include/linux/dccp.h
> @@ -411,6 +411,8 @@ struct dccp_ackvec;
> * @dccps_role - Role of this sock, one of %dccp_role
> * @dccps_ndp_count - number of Non Data Packets since last data packet
> * @dccps_hc_rx_ackvec - rx half connection ack vector
> + * @dccps_xmit_timer - timer for when CCID is not ready to send
> + * @dccps_xmit_lock - lock for transmitting
> */
> struct dccp_sock {
> /* inet_connection_sock has to be the first member of dccp_sock */
> @@ -443,6 +445,8 @@ struct dccp_sock {
> enum dccp_role dccps_role:2;
> __u8 dccps_hc_rx_insert_options:1;
> __u8 dccps_hc_tx_insert_options:1;
> + struct timer_list dccps_xmit_timer;
> + spinlock_t dccps_xmit_lock;
See the comments below, I guess we don't need yet another lock :-)
> };
>
> static inline struct dccp_sock *dccp_sk(const struct sock *sk)
> diff --git a/net/dccp/dccp.h b/net/dccp/dccp.h
> index 93f26dd..82431c0 100644
> --- a/net/dccp/dccp.h
> +++ b/net/dccp/dccp.h
> @@ -5,7 +5,7 @@
> *
> * An implementation of the DCCP protocol
> * Copyright (c) 2005 Arnaldo Carvalho de Melo <[EMAIL PROTECTED]>
> - * Copyright (c) 2005 Ian McDonald <[EMAIL PROTECTED]>
> + * Copyright (c) 2005-6 Ian McDonald <[EMAIL PROTECTED]>
> *
> * This program is free software; you can redistribute it and/or modify
> it
> * under the terms of the GNU General Public License version 2 as
> @@ -126,7 +126,7 @@ extern void dccp_send_delayed_ack(struct
> extern void dccp_send_sync(struct sock *sk, const u64 seq,
> const enum dccp_pkt_type pkt_type);
>
> -extern int dccp_write_xmit(struct sock *sk, struct sk_buff *skb, long
> *timeo);
> +void dccp_write_xmit(struct sock *sk, int block);
> extern void dccp_write_space(struct sock *sk);
Please leave the "extern"
> extern void dccp_init_xmit_timers(struct sock *sk);
> diff --git a/net/dccp/output.c b/net/dccp/output.c
> index efd7ffb..90b802d 100644
> --- a/net/dccp/output.c
> +++ b/net/dccp/output.c
> @@ -2,7 +2,8 @@
> * net/dccp/output.c
> *
> * An implementation of the DCCP protocol
> - * Arnaldo Carvalho de Melo <[EMAIL PROTECTED]>
> + * Copyright (c) 2005 Arnaldo Carvalho de Melo <[EMAIL PROTECTED]>
> + * Copyright (c) 2006 Ian McDonald <[EMAIL PROTECTED]>
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> @@ -191,7 +192,7 @@ static int dccp_wait_for_ccid(struct soc
> while (1) {
> prepare_to_wait(sk->sk_sleep, &wait, TASK_INTERRUPTIBLE);
>
> - if (sk->sk_err || (sk->sk_shutdown & SEND_SHUTDOWN))
> + if (sk->sk_err)
Humm, have to check this one...
> goto do_error;
> if (!*timeo)
> goto do_nonblock;
> @@ -207,9 +208,11 @@ static int dccp_wait_for_ccid(struct soc
> goto do_nonblock;
>
> sk->sk_write_pending++;
> + spin_unlock(&dp->dccps_xmit_lock);
> release_sock(sk);
See? two locks being released...
> *timeo -= schedule_timeout(delay);
> lock_sock(sk);
> + spin_lock(&dp->dccps_xmit_lock);
and being grabbed again...
> sk->sk_write_pending--;
> }
> out:
> @@ -227,37 +230,56 @@ do_interrupted:
> goto out;
> }
>
> -int dccp_write_xmit(struct sock *sk, struct sk_buff *skb, long *timeo)
> -{
> - const struct dccp_sock *dp = dccp_sk(sk);
> - int err = ccid_hc_tx_send_packet(dp->dccps_hc_tx_ccid, sk, skb,
> - skb->len);
> +static void dccp_write_xmit_timer(unsigned long sk) {
> + dccp_write_xmit((struct sock *)sk,0);
> +}
I guess we don't need this lock, but instead use bh_lock_sock, check if
there are users, look at ccid3_hc_tx_no_feedback_timer & ccid2_hc_tx_rto_expire
(I left a comment even having fixed the problem, duh will fix).
> - if (err > 0)
> - err = dccp_wait_for_ccid(sk, skb, timeo);
> +void dccp_write_xmit(struct sock *sk, int block)
> +{
> + struct dccp_sock *dp = dccp_sk(sk);
> + struct sk_buff *skb;
> + long timeo = 2000; /* FIXME imcdnzl - 2 second default */
>
> - if (err == 0) {
> - struct dccp_skb_cb *dcb = DCCP_SKB_CB(skb);
> - const int len = skb->len;
> + spin_lock(&dp->dccps_xmit_lock);
We're already protected by lock_sock() and the timer will just use bh_lock_sock
> +
> + while ((skb = skb_peek(&sk->sk_write_queue))) {
> + int err = ccid_hc_tx_send_packet(dp->dccps_hc_tx_ccid, sk,
> skb,
> + skb->len);
> +
> + if (err > 0) {
> + if (likely(!block)) {
> + mod_timer(&dp->dccps_xmit_timer,
> +
> msecs_to_jiffies(err)+jiffies);
sk_reset_timer() please, so that we make sure we have a reference count
for the sock as its going to be on a timer
> + goto xmit_out;
> + } else {
> + err = dccp_wait_for_ccid(sk, skb, &timeo);
> + }
No need for the {}, just one statement
> + }
>
> - if (sk->sk_state == DCCP_PARTOPEN) {
> - /* See 8.1.5. Handshake Completion */
> - inet_csk_schedule_ack(sk);
> - inet_csk_reset_xmit_timer(sk, ICSK_TIME_DACK,
> + skb_dequeue(&sk->sk_write_queue);
sk_eat_skb() later?
> + if (err == 0) {
> + struct dccp_skb_cb *dcb = DCCP_SKB_CB(skb);
> + const int len = skb->len;
> +
> + if (sk->sk_state == DCCP_PARTOPEN) {
> + /* See 8.1.5. Handshake Completion */
> + inet_csk_schedule_ack(sk);
> + inet_csk_reset_xmit_timer(sk, ICSK_TIME_DACK,
> inet_csk(sk)->icsk_rto,
> DCCP_RTO_MAX);
> - dcb->dccpd_type = DCCP_PKT_DATAACK;
> - } else if (dccp_ack_pending(sk))
> - dcb->dccpd_type = DCCP_PKT_DATAACK;
> - else
> - dcb->dccpd_type = DCCP_PKT_DATA;
> -
> - err = dccp_transmit_skb(sk, skb);
> - ccid_hc_tx_packet_sent(dp->dccps_hc_tx_ccid, sk, 0, len);
> - } else
> - kfree_skb(skb);
> -
> - return err;
> + dcb->dccpd_type = DCCP_PKT_DATAACK;
> + } else if (dccp_ack_pending(sk))
> + dcb->dccpd_type = DCCP_PKT_DATAACK;
> + else
> + dcb->dccpd_type = DCCP_PKT_DATA;
> +
> + err = dccp_transmit_skb(sk, skb);
> + ccid_hc_tx_packet_sent(dp->dccps_hc_tx_ccid, sk, 0,
> len);
> + } else
> + kfree(skb);
Like at the end, i.e. we dequeue and free (sk_eat_skb())
> + }
> +xmit_out:
> + spin_unlock(&dp->dccps_xmit_lock);
> }
>
> int dccp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
> @@ -396,6 +418,10 @@ static inline void dccp_connect_init(str
> dccp_set_seqno(&dp->dccps_awl, max48(dp->dccps_awl, dp->dccps_iss));
>
> icsk->icsk_retransmits = 0;
> + init_timer(&dp->dccps_xmit_timer);
> + dp->dccps_xmit_timer.data = (unsigned long)sk;
> + dp->dccps_xmit_timer.function = dccp_write_xmit_timer;
Perhaps we use one of icsk timers? Have to check
> + dp->dccps_xmit_lock = SPIN_LOCK_UNLOCKED;
Not needed
> }
>
> int dccp_connect(struct sock *sk)
> @@ -527,8 +553,11 @@ void dccp_send_close(struct sock *sk, co
> DCCP_PKT_CLOSE : DCCP_PKT_CLOSEREQ;
>
> if (active) {
> + dccp_write_xmit(sk, 1);
> dccp_skb_entail(sk, skb);
> dccp_transmit_skb(sk, skb_clone(skb, prio));
> - } else
> + /* FIXME do we need a retransmit timer here? */
> + } else {
> dccp_transmit_skb(sk, skb);
> + }
No need for {}, just one statement
> }
> diff --git a/net/dccp/proto.c b/net/dccp/proto.c
> index 65b11ea..119ecc5 100644
> --- a/net/dccp/proto.c
> +++ b/net/dccp/proto.c
> @@ -385,6 +385,7 @@ int dccp_sendmsg(struct kiocb *iocb, str
>
> lock_sock(sk);
> timeo = sock_sndtimeo(sk, noblock);
> + /* FIXME imcdnzl - we should be using timeo for memory allocation */
>
> /*
> * We have to use sk_stream_wait_connect here to set sk_write_pending,
> @@ -407,19 +408,13 @@ int dccp_sendmsg(struct kiocb *iocb, str
> if (rc != 0)
> goto out_discard;
>
> - rc = dccp_write_xmit(sk, skb, &timeo);
> - /*
> - * XXX we don't use sk_write_queue, so just discard the packet.
> - * Current plan however is to _use_ sk_write_queue with
> - * an algorith similar to tcp_sendmsg, where the main difference
> - * is that in DCCP we have to respect packet boundaries, so
> - * no coalescing of skbs.
> - *
> - * This bug was _quickly_ found & fixed by just looking at an
> OSTRA
> - * generated callgraph 8) -acme
> - */
> + skb_queue_tail(&sk->sk_write_queue, skb);
> + release_sock(sk);
> + dccp_write_xmit(sk,0);
> + goto out_end;
> out_release:
> release_sock(sk);
> +out_end:
> return rc ? : len;
> out_discard:
> kfree_skb(skb);
> @@ -591,6 +586,7 @@ static int dccp_close_state(struct sock
>
> void dccp_close(struct sock *sk, long timeout)
> {
> + struct dccp_sock *dp = dccp_sk(sk);
> struct sk_buff *skb;
>
> lock_sock(sk);
> @@ -606,6 +602,8 @@ void dccp_close(struct sock *sk, long ti
> goto adjudge_to_death;
> }
>
> + del_timer_sync(&dp->dccps_xmit_timer);
> +
sk_stop_timer please, to drop the refcount we grabbed with sk_reset_timer()
elsewhere.
- Arnaldo
-
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
