From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Abeni <pabeni@redhat.com>
Subject: Re: [RFC PATCH v2 02/10] udp: implement GRO for plain UDP sockets.
Date: Mon, 22 Oct 2018 15:41:57 +0200
Message-ID: <b69077c93ec047845d4b22a57fa6f89b63c0639c.camel@redhat.com>
References: <cover.1539957909.git.pabeni@redhat.com>
         <3fa3822651e29b8484d598b10ae61b0efde6b14f.1539957909.git.pabeni@redhat.com>
         <20181022112451.GH3823@gauss3.secunet.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, Willem de Bruijn <willemb@google.com>
To: Steffen Klassert <steffen.klassert@secunet.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:49444 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1727210AbeJVWAi (ORCPT <rfc822;netdev@vger.kernel.org>);
        Mon, 22 Oct 2018 18:00:38 -0400
In-Reply-To: <20181022112451.GH3823@gauss3.secunet.de>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Mon, 2018-10-22 at 13:24 +0200, Steffen Klassert wrote:
> On Fri, Oct 19, 2018 at 04:25:12PM +0200, Paolo Abeni wrote:
> >  
> > +#define UDO_GRO_CNT_MAX 64
> 
> Maybe better UDP_GRO_CNT_MAX?

Oops, typo. Yes, sure, will address in the next iteration.

> Btw. do we really need this explicit limit?
> We should not get more than 64 packets during
> one napi poll cycle.

With HZ >= 1000, gro_flush happens at most once per jiffies: we can
have much more than 64 packets per segment, with appropriate pkt len.

> 
> > +static struct sk_buff *udp_gro_receive_segment(struct list_head *head,
> > +					       struct sk_buff *skb)
> > +{
> > +	struct udphdr *uh = udp_hdr(skb);
> > +	struct sk_buff *pp = NULL;
> > +	struct udphdr *uh2;
> > +	struct sk_buff *p;
> > +
> > +	/* requires non zero csum, for simmetry with GSO */
> > +	if (!uh->check) {
> > +		NAPI_GRO_CB(skb)->flush = 1;
> > +		return NULL;
> > +	}
> 
> Why is the requirement of checksums different than in 
> udp_gro_receive? It's not that I care much about UDP
> packets without a checksum, but you would not need
> to implement your own loop if the requirement could
> be the same as in udp_gro_receive.

uhm.... 
AFAIU, we need to generated aggregated packets that UDP GSO is able to
process/segment. I was unable to get a nocsum packet segment (possibly
PEBKAC) so I enforced that condition on the rx path.

@Willem: did I see ghost here? is UDP_SEGMENT fine with no checksum
segment?

> > +
> > +	/* pull encapsulating udp header */
> > +	skb_gro_pull(skb, sizeof(struct udphdr));
> > +	skb_gro_postpull_rcsum(skb, uh, sizeof(struct udphdr));
> > +
> > +	list_for_each_entry(p, head, list) {
> > +		if (!NAPI_GRO_CB(p)->same_flow)
> > +			continue;
> > +
> > +		uh2 = udp_hdr(p);
> > +
> > +		/* Match ports only, as csum is always non zero */
> > +		if ((*(u32 *)&uh->source != *(u32 *)&uh2->source)) {
> > +			NAPI_GRO_CB(p)->same_flow = 0;
> > +			continue;
> > +		}
> > +
> > +		/* Terminate the flow on len mismatch or if it grow "too much".
> > +		 * Under small packet flood GRO count could elsewhere grow a lot
> > +		 * leading to execessive truesize values
> > +		 */
> > +		if (!skb_gro_receive(p, skb) &&
> > +		    NAPI_GRO_CB(p)->count > UDO_GRO_CNT_MAX)
> 
> This allows to merge UDO_GRO_CNT_MAX + 1 packets.

Thanks, will address in the next iteration.

Cheers,

Paolo