[LU-6587] refactor OBD_ALLOC_LARGE to always do kmalloc first

• Type: Bug
• Resolution: Fixed
• Priority: Minor
• Fix Version/s: Lustre 2.8.0
• Severity: 3
• Assignee: Yang Sheng
• Reporter: Oleg Drokin

Description

Since vmalloc allocations are notoriously unscalable and slow, we should do what other filesystems are doing (and what we converted upstream kernel client to do) and for large allocations we should always try kmalloc first (with NOWARN flag) and then if that fails, retry with vmalloc.

Issue Links

• is related to: LU-7666 llog_cat_new_log() should use chunk size when freeing header

Comments

[James A Simmons]

Doesn't OBD_ALLOC_LARGE always try a kmalloc first then if it fails try a vmalloc? I don't think we can just drop the is_vmalloc_addr so easily. I agree with you Andreas that the vmalloc fallbacks are bad news but instead of trying to modify OBD_FREE_LARGE why don't we ripe off the band aid and start using vmalloc directly when it really needed. Note the upstream client has function wrapper libcfs_kvzalloc() and libcfs_kvzalloc_cpt() in libcfs to replace all those macros. Honestly I like to just use vmalloc directly.

[Andreas Dilger]

Sure there is little overhead to check is_vmalloc_addr() for every single allocation, but not zero, so why would we use it everywhere when it is not needed. Also, in the upstream kernel these macros are gone and we don't want to have the vmalloc(( fallback and calls to is_vmalloc_addr() check everywhere in the code simply because we don't know anymore which allocations might be large and which ones are not. We'd be taking a huge step backwards in terms of code quality, and a patch like that would never be accepted upstream, so why would we do it in master just because it can hide inside a macro?

Having huge allocations that require vmalloc() is something we should be very aware of and make a conscious decision to do, as otherwise we might be wasting a lot of memory accidentally.

[Oleg Drokin]

static inline int is_vmalloc_addr(const void *x)
{
#ifdef CONFIG_MMU
        unsigned long addr = (unsigned long)x;

        return addr >= VMALLOC_START && addr < VMALLOC_END;
#else
        return 0;
#endif
}

So I believe there's next to no overhead in is_vmalloc_addr esp. compared to other stuff we do in OBD_FREE()

I'd be instead inclined to drop OBD_ALLOC_LARGE and just always do OBD_ALLOC that would fallbacl to vmalloc if regular alloc failed?

[Gerrit Updater]

Andreas Dilger ([e-mail]) uploaded a new patch: http://review.whamcloud.com/18034
Subject: LU-6587 obdclass: use OBD_FREE_LARGE with OBD_ALLOC_LARGE
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: a7ac09582c72dad0be99db2072971dd34d6d6165

[Andreas Dilger]

Since this is only a few lines of change, it would be good to get this fixed before 2.9 if possible.

[Andreas Dilger]

I noticed that there was something landed in the http://review.whamcloud.com/14994 patch that I don't really like. Moving is_vmalloc_addr() into OBD_FREE() means that there is the overhead of this check for every call to OBD_FREE(), even though this is not needed for the large majority of allocations. It would be better to have the is_vmalloc_addr() check only in OBD_FREE_LARGE() and remove it from OBD_FREE(). The deprecation of OBD_FREE_LARGE() should also be removed from checkpatch.pl.

There are a couple of patches that have landed since then that need to be converted back to using OBD_FREE_LARGE(), at least:

af13cfff297d4882de35fb7c11bf5261293b8287
09141c0796802e7a3471c084ea5928674b3a1862

[Peter Jones]

It looks like this is landed for 2.8

[Yang Sheng]

Thanks for your comment, James. I think it is good idea to make a patch against stable code first.

[James A Simmons]

Looking at patches by their sublayer the areas under going the most changes are llite, osp, and target. I would recommend doing a patch set with that combination that could perhaps be landed after feature freeze. The rest of the OBD_FREE_LARGE users appear to be safe to update. I noticed ptlrpc and obdclass are large users of OBD_FREE_LARGE so if you might wanted to do a patch set covering those regions.

[Yang Sheng]

The next step of this ticket is replacing OBD_FREE_LARGE to OBD_FREE. Either all in one or one by one as subsystem. But i think we need a grace period for every one know that. So could we close this ticket first?

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch http://review.whamcloud.com/14994/
Subject: LU-6587 mem: refactor OBD_ALLOC_LARGE marco
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 919b85d796f845ec3e6633702dda1639a3d86176