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
