On Tue, 2009-08-25 at 00:02 +0200, Eric Dumazet wrote:
quoted text
> Christoph Lameter a écrit :
> > On Mon, 17 Aug 2009, Sridhar Samudrala wrote:
> > 
> >> So it is possible that there is some other place in the stack where the packets
> >> are gettting dropped but not counted.
> > 
> > Such a deed occurs in ip_push_pending_frames():
> > 
> >         /* Netfilter gets whole the not fragmented skb. */
> >         err = ip_local_out(skb);
> >         if (err) {
> >                 if (err > 0)
> >                         err = inet->recverr ? net_xmit_errno(err) : 0;
> > 			^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >                 if (err)
> >                         goto error;
> >         }
> > 
> > out:
> >         ip_cork_release(inet);
> >         return err;
> > 
> > error:
> >         IP_INC_STATS(net, IPSTATS_MIB_OUTDISCARDS);
> >         goto out;
> > 
> > 
> > So if ip_local_out returns NET_XMIT_DROP then its simply going to be
> > replaced by 0. Then we check err again and there is no error!!!!

Christoph,

So are you hitting this case with your workload and does this account for all the
packet losses you are seeing? 

If we are dropping the packet and returing NET_XMIT_DROP, should
we also increment qdisc drop stats (sch->qstats.drops)?

In dev_queue_xmit(), we have

        if (q->enqueue) {
                spinlock_t *root_lock = qdisc_lock(q);

                spin_lock(root_lock);

                if (unlikely(test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
                        kfree_skb(skb);
                        rc = NET_XMIT_DROP;
                } else {
                        rc = qdisc_enqueue_root(skb, q);
                        qdisc_run(q);
                }
                spin_unlock(root_lock);

                goto out;
        }

Here, if QDISC_STATE_DEACTIVATED is true, the skb is dropped and NET_XMIT_DROP
is returned, but not accounted in qdisc drop stats.
However it is incremented when NET_XMIT_DROP is returned via qdisc_drop().

If we count these drops as qdisc drops, should we also count them as IP OUTDISCARDS?

Thanks
Sridhar

quoted text
> > 
> NET_XMIT_CN strikes again :)
> 
> Well, if ip_local_out() returns a negative error (say -EPERM for example),
>  your patch disables OUTDISCARDS increments.
> 
> Maybe a simpler patch like this one ?
> 
> [PATCH] net: correctly updates OUTDISCARDS in ip_push_pending_frames()
> 
> ip_push_pending_frames() can fail to send a frame because of a congestioned
> device. In this case, we increment SNMP OUTDISCARDS only if user set
> IP_RECVERR, which is not RFC conformant.
> 
> Only case where we should not update OUTDISCARDS is when
> ip_local_output() return value is NET_XMIT_CN (meaning
> skb was xmitted but future frames might be dropped)
> 
> Signed-off-by: Christoph Lameter <cl@linux-foundation.org>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index 7d08210..27a5b79 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1301,19 +1301,15 @@ int ip_push_pending_frames(struct sock *sk)
>  	/* Netfilter gets whole the not fragmented skb. */
>  	err = ip_local_out(skb);
>  	if (err) {
> +		if (err != NET_XMIT_CN)
> +			IP_INC_STATS(net, IPSTATS_MIB_OUTDISCARDS);
>  		if (err > 0)
>  			err = inet->recverr ? net_xmit_errno(err) : 0;
> -		if (err)
> -			goto error;
>  	}
> 
>  out:
>  	ip_cork_release(inet);
>  	return err;
> -
> -error:
> -	IP_INC_STATS(net, IPSTATS_MIB_OUTDISCARDS);
> -	goto out;
>  }
> 
>  /*


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html