Hi Peter,
On Mon, 2008-07-28 at 12:17 +0200, Peter Zijlstra wrote:
quoted text > On Mon, 2008-07-28 at 13:06 +0300, Pekka Enberg wrote:
> > Hi Peter,
> >
> > On Thu, 2008-07-24 at 16:00 +0200, Peter Zijlstra wrote:
> > > +/*
> > > + * alloc wrappers
> > > + */
> > > +
> >
> > Hmm, I'm not sure I like the use of __kmalloc_track_caller() (even
> > though you do add the wrappers for SLUB). The functions really are SLAB
> > internals so I'd prefer to see kmalloc_reserve() moved to the
> > allocators.
>
> See below..
>
> > > +void *___kmalloc_reserve(size_t size, gfp_t flags, int node, void *ip,
> > > + struct mem_reserve *res, int *emerg)
> > > +{
> >
> > This function could use some comments...
>
> Yes, my latest does have those.. let me paste the relevant bit:
>
> +void *___kmalloc_reserve(size_t size, gfp_t flags, int node, void *ip,
> + struct mem_reserve *res, int *emerg)
> +{
> + void *obj;
> + gfp_t gfp;
> +
> + /*
> + * Try a regular allocation, when that fails and we're not entitled
> + * to the reserves, fail.
> + */
> + gfp = flags | __GFP_NOMEMALLOC | __GFP_NOWARN;
> + obj = __kmalloc_node_track_caller(size, gfp, node, ip);
> +
> + if (obj || !(gfp_to_alloc_flags(flags) & ALLOC_NO_WATERMARKS))
> + goto out;
> +
> + /*
> + * If we were given a reserve to charge against, try that.
> + */
> + if (res && !mem_reserve_kmalloc_charge(res, size)) {
> + /*
> + * If we failed to charge and we're not allowed to wait for
> + * it to succeed, bail.
> + */
> + if (!(flags & __GFP_WAIT))
> + goto out;
> +
> + /*
> + * Wait for a successfull charge against the reserve. All
> + * uncharge operations against this reserve will wake us up.
> + */
> + wait_event(res->waitqueue,
> + mem_reserve_kmalloc_charge(res, size));
> +
> + /*
> + * After waiting for it, again try a regular allocation.
> + * Pressure could have lifted during our sleep. If this
> + * succeeds, uncharge the reserve.
> + */
> + obj = __kmalloc_node_track_caller(size, gfp, node, ip);
> + if (obj) {
> + mem_reserve_kmalloc_charge(res, -size);
> + goto out;
> + }
> + }
> +
> + /*
> + * Regular allocation failed, and we've successfully charged our
> + * requested usage against the reserve. Do the emergency allocation.
> + */
> + obj = __kmalloc_node_track_caller(size, flags, node, ip);
> + WARN_ON(!obj);
> + if (emerg)
> + *emerg |= 1;
> +
> +out:
> + return obj;
> +}
Heh, indeed, looks much better :-).
quoted text >
> > > + void *obj;
> > > + gfp_t gfp;
> > > +
> > > + gfp = flags | __GFP_NOMEMALLOC | __GFP_NOWARN;
> > > + obj = __kmalloc_node_track_caller(size, gfp, node, ip);
> > > +
> > > + if (obj || !(gfp_to_alloc_flags(flags) & ALLOC_NO_WATERMARKS))
> > > + goto out;
> > > +
> > > + if (res && !mem_reserve_kmalloc_charge(res, size)) {
> > > + if (!(flags & __GFP_WAIT))
> > > + goto out;
> > > +
> > > + wait_event(res->waitqueue,
> > > + mem_reserve_kmalloc_charge(res, size));
> > > +
> > > + obj = __kmalloc_node_track_caller(size, gfp, node, ip);
> > > + if (obj) {
> > > + mem_reserve_kmalloc_charge(res, -size);
> >
> > Why do we discharge here?
>
> because a regular allocation succeeded.
>
> > > + goto out;
> > > + }
> >
> > If the allocation fails, we try again (but nothing has changed, right?).
> > Why?
>
> Note the different allocation flags for the two allocations.
Uhm, yeah. I missed that.
quoted text > > > + }
> > > +
> > > + obj = __kmalloc_node_track_caller(size, flags, node, ip);
> > > + WARN_ON(!obj);
> >
> > Why don't we discharge from the reserve here if !obj?
>
> Well, this allocation should never fail:
> - we reserved memory
> - we accounted/throttle its usage
>
> Thus this allocation should always succeed.
But if it *does* fail, it doesn't help that we mess up the reservation
counts, no?
quoted text > > > + if (emerg)
> > > + *emerg |= 1;
> > > +
> > > +out:
> > > + return obj;
> > > +}
> > > +
> > > +void __kfree_reserve(void *obj, struct mem_reserve *res, int emerg)
> >
> > I don't see 'emerg' used anywhere.
>
> Patch 19/30 has:
>
> - data = kmalloc_node_track_caller(size + sizeof(struct skb_shared_info),
> - gfp_mask, node);
> + data = kmalloc_reserve(size + sizeof(struct skb_shared_info),
> + gfp_mask, node, &net_skb_reserve, &emergency);
> if (!data)
> goto nodata;
>
> @@ -205,6 +211,7 @@ struct sk_buff *__alloc_skb(unsigned int
> * the tail pointer in struct sk_buff!
> */
> memset(skb, 0, offsetof(struct sk_buff, tail));
> + skb->emergency = emergency;
> skb->truesize = size + sizeof(struct sk_buff);
> atomic_set(&skb->users, 1);
> skb->head = data;
>
> > > +{
> > > + size_t size = ksize(obj);
> > > +
> > > + kfree(obj);
> >
> > We're trying to get rid of kfree() so I'd __kfree_reserve() could to
> > mm/sl?b.c. Matt, thoughts?
>
> My issue with moving these helpers into mm/sl?b.c is that it would
> require replicating all this code 3 times. Even though the functionality
> is (or should) be invariant to the actual slab implementation.
Right, I guess we could just rename ksize() to something else then and
keep it internal to mm/.
quoted text > > > + /*
> > > + * ksize gives the full allocated size vs the requested size we used to
> > > + * charge; however since we round up to the nearest power of two, this
> > > + * should all work nicely.
> > > + */
> > > + mem_reserve_kmalloc_charge(res, -size);
> > > +}
> > >
> >
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to
majordomo@vger.kernel.org
More majordomo info at
http://vger.kernel.org/majordomo-info.html
