Bug in the error handling in TTMs pool implementation

[Christian König]

Am 27.11.23 um 13:24 schrieb Karolina Stolarek:
>
> On 24.11.2023 16:53, Christian König wrote:
>> @Karolina do you of hand know a way how we could exercise this in a 
>> TTM unit test? Basically we would need to redirect the 
>> alloc_pages_node() symbol to an unit test internal function and let 
>> it return an error (or something like that).
>
> Do I understand correctly that you want to mock NULL from 
> alloc_pages_node() in ttm_pool_alloc_page()? I had a quick look at it, 
> and found a(n ugly) way to do it.
>
> KUnit provides an API to redirect calls using 
> KUNIT_STATIC_STUB_REDIRECT that can be registered in the original 
> function and then overridden in the test. You can read more about the 
> mechanism in [1].
>
> The problem is that we'd need to introduce a wrapper call for 
> alloc_pages_node() in TTM and expose it, so the function can be 
> substituted during the test. We can't directly modify 
> alloc_pages_node() due to cyclic dependencies caused by 
> kunit/static_stub.h.
>
> For example, in ttm_pool.c, we'd have:
>
> +struct page *ttm_alloc_pages_node(int nid, gfp_t gfp_mask,
> +                                 unsigned int order)
> +{
> +       KUNIT_STATIC_STUB_REDIRECT(ttm_alloc_pages_node, nid, gfp_mask,
> order);
> +       return alloc_pages_node(nid, gfp_mask, order);
> +}
> +EXPORT_SYMBOL(ttm_alloc_pages_node);
> +
>  /* Allocate pages of size 1 << order with the given gfp_flags */
>  static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t
> gfp_flags,
>                                         unsigned int order)
> (...)
>         if (!pool->use_dma_alloc) {
> -               p = alloc_pages_node(pool->nid, gfp_flags, order);
> +               p = ttm_alloc_pages_node(pool->nid, gfp_flags, order);
>                 if (p)
>                         p->private = order;
>                 return p;
>
> And in the test we would say:
>
> +static struct page *fake_ttm_alloc_pages_node(int nid, gfp_t gfp_mask,
> +                                             unsigned int order)
> +{
> +       return NULL;
> +}
> +
> +static void ttm_pool_alloc_failed(struct kunit *test)
> +{
> (...)
> +       kunit_activate_static_stub(test, ttm_alloc_pages_node,
> +                                  fake_ttm_alloc_pages_node);
> +       err = ttm_pool_alloc(pool, tt, &simple_ctx);
> +       kunit_deactivate_static_stub(test, ttm_alloc_pages_node);
>
> Christian, what do you think?

Yes, that was exactly what I was looking for.

And you are right that is indeed quite a bit ugly, I was hoping more 
like adding debugging hooks or something like that.

Need to look into that in more detail, thanks for the pointer.

Regards,
Christian.

>
> All the best,
> Karolina
>
> ---------------------------------------------------------
> [1] - 
> https://lore.kernel.org/all/20230128074918.1180523-1-davidgow@google.com/
>
>