Re: [PATCH 00/28] Swap over NFS -v16

Neil Brown
2008-02-27T05:51:12Z

Hi Peter,

On Tuesday February 26, a.p.zijlstra@chello.nl wrote:
> Hi Neil,
>
> Thanks for taking a look, and giving such elaborate feedback. I'll try
> and address these issues asap, but first let me reply to a few points
> here.

Thanks... the tree thing is starting to make sense, and I'm not
confused my __mem_reserve_add any more :-)

I've been having a closer read of some of the code that I skimmed over
before and I have some more questions.

1/ I note there is no way to tell if memory returned by kmalloc is
from the emergency reserve - which contrasts with alloc_page
which does make that information available through page->reserve.
This seems a slightly unfortunate aspect of the interface.

It seems to me that __alloc_skb could be simpler if this
information was available. It currently tries the allocation
normally, then if that fails it retries with __GFP_MEMALLOC set and
if that works it assume it was from the emergency pool ... which it
might not be, though the assumption is safe enough.

It would seem to be nicer if you could just make the one alloc call,
setting GFP_MEMALLOC if that might be appropriate. Then if the
memory came from the emergency reserve, flag it as an emergency skb.

However doing that would have issues with reservations.... the
mem_reserve would need to be passed to kmalloc :-(

2/ It doesn't appear to be possible to wait on a reservation. i.e. if
mem_reserve_*_charge fails, we might sometimes want to wait until
it will succeed. This feature is an integral part of how mempools
work and are used. If you want reservations to (be able to)
replace mempools, then waiting really is needed.

It seems that waiting would work best if it was done quite low down
inside kmalloc. That would require kmalloc to know which
'mem_reserve' you are using which it currently doesn't.

If it did, then it could choose to use emergency pages if the
mem_reserve allowed it more space, otherwise require a regular page.
And if __GFP_WAIT is set then each time around the loop it could
revise whether to use an emergency page or not, based on whether it
can successfully charge the reservation.

Of course, having a mem_reserve available for PF_MEMALLOC
allocations would require changing every kmalloc call in the
protected region, which might not be ideal, but might not be a
major hassle, and would ensure that you find all kmalloc calls that
might be made while in PF_MALLOC state.

3/ Thinking about the tree structure a bit more: Your motivation
seems to be that it allows you to make two separate reservations,
and then charge some memory usage again either-one-of-the-other.
This seems to go against a key attribute of reservations. I would
have thought that an important rule about reservations is that
no-one else can use memory reserved for a particular purpose.
So if IPv6 reserves some memory, and the IPv4 uses it, that doesn't
seem like something we want to encourage...

4/ __netdev_alloc_page is bothering me a bit.
This is used to allocate memory for incoming fragments in a
(potentially multi-fragment) packet. But we only rx_emergency_get
for each page as it arrives rather than all at once at the start.
So you could have a situation where two very large packets are
arriving at the same time and there is enough reserve to hold
either of them but not both. The current code will hand out that
reservation a little (well, one page) at a time to each packet and
will run out before either packet has been fully received. This
seems like a bad thing. Is it?

Is it possible to do the rx_emergency_get just once of the whole
packet I wonder?

NeilBrown