changing dev->needed_headroom/needed_tailroom?

changing dev->needed_headroom/needed_tailroom?

[Johannes Berg]

Does it seem reasonable to change dev->needed_headroom and
dev->needed_tailroom on the fly?

We currently set needed_headroom to the max of what we need, but we
could do better like making it depend on the interface type (e.g. only
asking for mesh space on mesh interfaces). This would be done only when
the interface isn't connected, I can't promise it would be down but the
carrier would be off.

Another thing we might want to do is change it according to the
currently configured crypto (this would also affect
dev->needed_tailroom) since we actually only need tailroom when TKIP is
used. This could might be done on the fly, but could also be done when
the carrier is still down during connection establishment (which would
not be a complete optimisation but still be better than what we have
now)

Thoughts?

johannes

Re: changing dev->needed_headroom/needed_tailroom?

[Ben Hutchings]

On Fri, 2013-07-26 at 16:50 +0200, Johannes Berg wrote:
> Does it seem reasonable to change dev->needed_headroom and
> dev->needed_tailroom on the fly?
> 
> We currently set needed_headroom to the max of what we need, but we
> could do better like making it depend on the interface type (e.g. only
> asking for mesh space on mesh interfaces). This would be done only when
> the interface isn't connected, I can't promise it would be down but the
> carrier would be off.
[...]

I don't think this is safe when the interface is running (even if
carrier is off).  Some functions may read dev->needed_headroom twice and
rely on getting the same value each time.

Also, stacked devices would need to adjust their own needed_headroom
accordingly.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: changing dev->needed_headroom/needed_tailroom?

[Eric Dumazet]

On Fri, 2013-08-02 at 10:55 +0200, Ben Hutchings wrote:

> I don't think this is safe when the interface is running (even if
> carrier is off).  Some functions may read dev->needed_headroom twice and
> rely on getting the same value each time.

It should be no problem. Remaining unsafe places should be fixed.

We already had this discussion in the past, and some patches were
issued. Check commit ae641949df01b85117845bec45328eab6d6fada1
("net: Remove all uses of LL_ALLOCATED_SPACE")

Re: changing dev->needed_headroom/needed_tailroom?

[Johannes Berg]

On Fri, 2013-08-02 at 06:11 -0700, Eric Dumazet wrote:
> On Fri, 2013-08-02 at 10:55 +0200, Ben Hutchings wrote:
> 
> > I don't think this is safe when the interface is running (even if
> > carrier is off).  Some functions may read dev->needed_headroom twice and
> > rely on getting the same value each time.
> 
> It should be no problem. Remaining unsafe places should be fixed.

Most interesting would be stack devs, which I hadn't even considered. In
any case, since I can't completely _rely_ on it, it's an optimisation,
the only bugs would be around the double-access and then running
over/under the SKB or so?

> We already had this discussion in the past, and some patches were
> issued. Check commit ae641949df01b85117845bec45328eab6d6fada1
> ("net: Remove all uses of LL_ALLOCATED_SPACE")

That would have addressed some of that, I guess.


I'm asking because some of the crypto stuff we do has fairly large
head/tailroom requirements and it seems I may need to add more. But if
you don't have crypto, it would be much smaller, so I figured we could
switch it.

johannes

Re: changing dev->needed_headroom/needed_tailroom?

[Florian Fainelli]

2013/8/5 Johannes Berg <johannes@sipsolutions.net>:
> On Fri, 2013-08-02 at 06:11 -0700, Eric Dumazet wrote:
>> On Fri, 2013-08-02 at 10:55 +0200, Ben Hutchings wrote:
>>
>> > I don't think this is safe when the interface is running (even if
>> > carrier is off).  Some functions may read dev->needed_headroom twice and
>> > rely on getting the same value each time.
>>
>> It should be no problem. Remaining unsafe places should be fixed.
>
> Most interesting would be stack devs, which I hadn't even considered. In
> any case, since I can't completely _rely_ on it, it's an optimisation,
> the only bugs would be around the double-access and then running
> over/under the SKB or so?

As far as I could test this with an Ethernet driver which adjusted its
needed_headroom by 64 bytes whenever some hardware feature was
enabled/disabled, this expectedly broke bridge and vlans at least.
Bridge code does not use the slave ports needed_headroom values, and
VLAN devices get the parent device needed_headroom only when creating
the vlan device. The good thing is since the needed_headroom space you
need is most likely fixed for a given configuration type, you should
see a pretty "stable" corruption of your SKB head.

>
>> We already had this discussion in the past, and some patches were
>> issued. Check commit ae641949df01b85117845bec45328eab6d6fada1
>> ("net: Remove all uses of LL_ALLOCATED_SPACE")
>
> That would have addressed some of that, I guess.
>
>
> I'm asking because some of the crypto stuff we do has fairly large
> head/tailroom requirements and it seems I may need to add more. But if
> you don't have crypto, it would be much smaller, so I figured we could
> switch it.

We could probably do it via a pair of new NETDEV_* notify event to
signal new needed_headroom/tailroom values to stacked devices. Would
that be acceptable?
-- 
Florian

Re: changing dev->needed_headroom/needed_tailroom?

[Stephen Hemminger]

On Tue, 20 Aug 2013 11:00:18 +0100
Florian Fainelli <f.fainelli@gmail.com> wrote:

> 2013/8/5 Johannes Berg <johannes@sipsolutions.net>:
> > On Fri, 2013-08-02 at 06:11 -0700, Eric Dumazet wrote:
> >> On Fri, 2013-08-02 at 10:55 +0200, Ben Hutchings wrote:
> >>
> >> > I don't think this is safe when the interface is running (even if
> >> > carrier is off).  Some functions may read dev->needed_headroom twice and
> >> > rely on getting the same value each time.
> >>
> >> It should be no problem. Remaining unsafe places should be fixed.
> >
> > Most interesting would be stack devs, which I hadn't even considered. In
> > any case, since I can't completely _rely_ on it, it's an optimisation,
> > the only bugs would be around the double-access and then running
> > over/under the SKB or so?
> 
> As far as I could test this with an Ethernet driver which adjusted its
> needed_headroom by 64 bytes whenever some hardware feature was
> enabled/disabled, this expectedly broke bridge and vlans at least.
> Bridge code does not use the slave ports needed_headroom values, and
> VLAN devices get the parent device needed_headroom only when creating
> the vlan device. The good thing is since the needed_headroom space you
> need is most likely fixed for a given configuration type, you should
> see a pretty "stable" corruption of your SKB head.

All code must check for needed headroom first, and copy packet
if space is not available. Since excess headroom is always safe,
most devices just always use the same worst case headroom.

Even with your changes this will still be necessary since packets will
be still in flight while features change.

Re: changing dev->needed_headroom/needed_tailroom?

[Johannes Berg]

On Tue, 2013-08-20 at 09:20 -0700, Stephen Hemminger wrote:

> All code must check for needed headroom first, and copy packet
> if space is not available. Since excess headroom is always safe,
> most devices just always use the same worst case headroom.
> 
> Even with your changes this will still be necessary since packets will
> be still in flight while features change.

I agree, it will always be necessary. I guess the question boils down to
whether/what N bytes headroom more or less actually make a difference.

The wireless stack currently sets needed_headroom to 34 or so I think,
but I will need to increase by 8 which was why I had this question to
start with. However, those 34 are an absolute worst case - in most cases
it's much less...

So I suppose the other question we should ask is will increasing from
~34 to ~42 make a significant different that it's worth thinking about
avoiding it in the common cases at all?

johannes

Re: changing dev->needed_headroom/needed_tailroom?

[Stephen Hemminger]

On Tue, 20 Aug 2013 18:24:57 +0200
Johannes Berg <johannes@sipsolutions.net> wrote:

> On Tue, 2013-08-20 at 09:20 -0700, Stephen Hemminger wrote:
> 
> > All code must check for needed headroom first, and copy packet
> > if space is not available. Since excess headroom is always safe,
> > most devices just always use the same worst case headroom.
> > 
> > Even with your changes this will still be necessary since packets will
> > be still in flight while features change.
> 
> I agree, it will always be necessary. I guess the question boils down to
> whether/what N bytes headroom more or less actually make a difference.
> 
> The wireless stack currently sets needed_headroom to 34 or so I think,
> but I will need to increase by 8 which was why I had this question to
> start with. However, those 34 are an absolute worst case - in most cases
> it's much less...
> 
> So I suppose the other question we should ask is will increasing from
> ~34 to ~42 make a significant different that it's worth thinking about
> avoiding it in the common cases at all?
> 
> johannes
> 

The only thing that might matter is alignment. but +/- 8 bytes isn't going
to change that.