David S. Miller a écrit :
From: Andi Kleen <[EMAIL PROTECTED]>
Date: Thu, 18 Aug 2005 03:05:25 +0200


I would just set the ra pointer to a single global structure if the
allocation fails. Then you can avoid all the other checks. It will
slow down things and trash some state, but not fail and nobody
should expect good performance after out of memory anyways. The only
check still needed would be on freeing.


I would think twice about that due to repeatability concerns.  Yes, we
should care less when memory is so low, but if we can avoid this kind
of scenerio easily we should.

Having said that, I would like to recommend looking into a scheme
where the path leading to the filp allocation states whether the
read-ahead bits are needed or not.  This has two benefits:

1) Repeatability, and error signalling at the correct place
   should the memory allocation fail.

2) We can avoid the pointer dereference overhead.  The read-ahead
   state is always at (filp + 1).  Macro'ized or static inline
   function'ized interfaces for this access can make it look
   clean and perhaps even implement debugging of the case where
   we try to get at the read-ahead state for a non-read-ahead
   filp.

I do really think that would be a better approach.  A quick glance
shows that it should be easy to propagate the "need_read_ahead"
state, just by passing a boolean to get_unused_fd() via
sock_map_fd().

Thanks David and Andi

Hum... get_unused_fd() allocates a file descriptor, not a 'struct file *'
Maybe you meant get_empty_filp(void), that we might change to 
get_empty_filp(int need_read_ahead)

After reading your suggestions, I understand we still need two slabs.
One (filp_cachep) without the readahead data, the other one (filp_ra_cachep) 
with it.

static inline struct file_ra_state *get_ra_state(struct file *f)
{
#ifdef CONFIG_DEBUG_READAHEAD
        BUG_ON(filp_ra_cachep != GET_PAGE_CACHE(virt_to_page(f)));
#endif
        return (struct file_ra_state *)(f+1);
}


struct file *get_empty_filp(int need_read_ahead)
{
        struct file *f;
        ...
        f = kmem_cache_alloc(need_read_ahead ? filp_ra_cachep : filp_cachep, 
GFP_KERNEL);
        if (f == NULL)
                goto fail;
        memset(f, 0, need_read_ahead ? sizeof(struct file) : sizeof(struct 
file) + sizeof(struct file_ra_state));
        ...
}

file_free() then call kfree() that should find the right kmem_cache_t *

static inline void file_free(struct file *f)
{
        kfree(f);
}


I will provide a new version of the patch in a later mail

Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to