Re: [PATCH bpf-next v3] docs/bpf: Add documentation for BPF_MAP_TYPE_SK_STORAGE

[Yonghong Song <yhs@meta.com>]

On 12/8/22 8:35 AM, Donald Hunter wrote:
> Donald Hunter <donald.hunter@gmail.com> writes:
> 
>> David Vernet <void@manifault.com> writes:
>>
>>> On Wed, Dec 07, 2022 at 10:27:21AM +0000, Donald Hunter wrote:
>>>> +
>>>> +This snippet shows how to retrieve socket-local storage in a BPF program:
>>>> +
>>>> +.. code-block:: c
>>>> +
>>>> +    SEC("sockops")
>>>> +    int _sockops(struct bpf_sock_ops *ctx)
>>>> +    {
>>>> +            struct my_storage *storage;
>>>> +            struct bpf_sock *sk;
>>>> +
>>>> +            sk = ctx->sk;
>>>> +            if (!sk)
>>>> +                    return 1;
>>>
>>> Don't feel strongly about this one, but IMO it's nice for examples to
>>> illustrate code that's as close to real and pristine as possible. To
>>> that point, should this example perhaps be updated to return -ENOENT
>>> here, and -ENOMEM below?
>>
>> Will do.
>>
> 
> After digging into this a bit more I notice that the sockops programs in
> tools/testing/selftests/bpf/progs mostly return 1 in all cases.
> 
> I'm assuming that sockops programs should return valid values for
> some op types such as BPF_SOCK_OPS_TIMEOUT_INIT. Other than that I can't
> find a definitive list. Do you know if valid return values are
> enumerated anywhere, or do I need to dig some more?

It can return any integer.

static inline u32 tcp_timeout_init(struct sock *sk)
{
         int timeout;

         timeout = tcp_call_bpf(sk, BPF_SOCK_OPS_TIMEOUT_INIT, 0, NULL);

         if (timeout <= 0)
                 timeout = TCP_TIMEOUT_INIT;
         return min_t(int, timeout, TCP_RTO_MAX);
}

In uapi bpf.h,

         BPF_SOCK_OPS_TIMEOUT_INIT,      /* Should return SYN-RTO value 
to use or
                                          * -1 if default value should 
be used
                                          */

I think the above code is from selftests tcp_rtt.c. You can add a
reference to provide more context. If people are really interested,
they can go to selftest to find more.

> 
>>>> +
>>>> +            storage = bpf_sk_storage_get(&socket_storage, sk, 0,
>>>> +                                         BPF_LOCAL_STORAGE_GET_F_CREATE);
>>>> +            if (!storage)
>>>> +                    return 1;
>>>> +
>>>> +            /* Use 'storage' here */
>>>
>>> Let's return 0 at the end to make the example program technically
>>> correct.
>>
>> Will do.

In this case, 'return 0' probably not correct. I suggest keep the
code as is to sync with selftests. Add a reference to selftest
for more reference.

>>
>>>> +    }
>>>
>>> Thanks,
>>> David