Re: [PATCH ipsec-next v8 10/16] xfrm: iptfs: add fragmenting of larger than MTU user packets

[Christian Hopps <chopps@chopps.org>]

Florian Westphal <fw@strlen.de> writes:

> Christian Hopps <chopps@chopps.org> wrote:
>> > > > > +	if (!l3resv) {
>> > > > > +		resv = XFRM_IPTFS_MIN_L2HEADROOM;
>> > > > > +	} else {
>> > > > > +		resv = skb_headroom(tpl);
>> > > > > +		if (resv < XFRM_IPTFS_MIN_L3HEADROOM)
>> > > > > +			resv = XFRM_IPTFS_MIN_L3HEADROOM;
>> > > > > +	}
>> > > > > +
>> > > > > +	skb = alloc_skb(len + resv, GFP_ATOMIC);
>> > > > > +	if (!skb) {
>> > > > > +		XFRM_INC_STATS(dev_net(tpl->dev), LINUX_MIB_XFRMNOSKBERROR);
>> > > >
>> > > > Hmpf, so we've gone from incrementing the wrong counter to
>> > > > incrementing a new counter that doesn't have a precise meaning.
>> > >
>> > > The new "No SKB" counter is supposed to mean "couldn't get an SKB",
>> > > given plenty of other errors are logged under "OutErr" or "InErr"
>> > > i'm not sure what level of precision you're looking for here. :)
>> >
>> > OutErr and InErr would be better than that new counter IMO.
>>
>> Why?
>>
>> My counter tracks the SKB depletion failure that is actually happening. Would
>> you have me now pass in the direction argument just so I can tick the correct
>> overly general MIB counter that provides less value to the user in identifying
>> the actual problem? How is that good design?
>>
>> I'm inclined to just delete the thing altogether rather than block on this thing that will almost never happen.
>
> Makes sense to me, skb allocation failure is transient anyway, there is
> no action that could be taken if this error counter is incrementing.
>
> You might want to pass GFP_ATOMIC | __GFP_NOWARN to alloc_skb() to avoid
> any splats given this is a high-volume allocation.

Will do.

Thanks,
Chris.