Re: [PATCH v2 10/21] net/ena: proxy AQ calls to primary process

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 2/22/2022 10:24 PM, Ferruh Yigit wrote:
> On 2/22/2022 6:11 PM, Michal Krawczyk wrote:
>> From: Stanislaw Kardach <kda@semihalf.com>
>>
>> Due to how the ena_com compatibility layer is written, all AQ commands
>> triggering functions use stack to save results of AQ and then copy them
>> to user given function.
>> Therefore to keep the compatibility layer common, introduce ENA_PROXY
>> macro. It either calls the wrapped function directly (in primary
>> process) or proxies it to the primary via DPDK IPC mechanism. Since all
>> proxied calls are taken under a lock share the result data through
>> shared memory (in struct ena_adapter) to work around 256B IPC parameter
>> size limit.
>>
>> New proxy calls can be added by
>> 1. Adding a new message type at the end of enum ena_mp_req
>> 2. Adding new message arguments to the struct ena_mp_body if needed
>> 3. Defining proxy request descriptor with ENA_PROXY_DESC. Its arguments
>>     include handlers for request preparation and response processing.
>>     Any of those may be empty (aside of marking arguments as used).
>> 4. Adding request handling logic to ena_mp_primary_handle()
>> 5. Replacing proxied function calls with ENA_PROXY(adapter, <func>, ...)
>>
>> Signed-off-by: Stanislaw Kardach <kda@semihalf.com>
>> Reviewed-by: Michal Krawczyk <mk@semihalf.com>
>> Reviewed-by: Dawid Gorecki <dgr@semihalf.com>
>> Reviewed-by: Shai Brandes <shaibran@amazon.com>
> 
> <...>
> 
>> + */
>> +#define ENA_PROXY(a, f, ...)                        \
>> +({                                    \
>> +    struct ena_adapter *_a = (a);                    \
>> +    struct timespec ts = { .tv_sec = ENA_MP_REQ_TMO };        \
>> +    struct ena_mp_body *req, *rsp;                    \
>> +    struct rte_mp_reply mp_rep;                    \
>> +    struct rte_mp_msg mp_req;                    \
>> +    int ret;                            \
>> +                                    \
>> +    if (rte_eal_process_type() == RTE_PROC_PRIMARY) {        \
>> +        ret = f(__VA_ARGS__);                    \
>> +    } else {                            \
>> +        /* Prepare and send request */                \
>> +        req = (struct ena_mp_body *)&mp_req.param;        \
>> +        mp_msg_init(&mp_req, mp_type_ ## f, _a->edev_data->port_id); \
>> +        mp_prep_ ## f(_a, req, ## __VA_ARGS__);            \
>> +                                    \
>> +        ret = rte_mp_request_sync(&mp_req, &mp_rep, &ts);    \
>> +        if (likely(!ret)) {                    \
>> +            RTE_ASSERT(mp_rsp.nb_received == 1);        \
> 
> 'mp_rsp' is not defined, I assume intention is to use 'mp_rep'.
> 
> Build error can be reproduced by enabling RTE_ASSERT (by setting
> 'RTE_ENABLE_ASSERT').
> 
>> +            rsp = (struct ena_mp_body *)&mp_rep.msgs[0].param; \
>> +            ret = rsp->result;                \
>> +            if (ret == 0) {                    \
>> +                mp_proc_##f(_a, rsp, ## __VA_ARGS__);    \
>> +            } else {                    \
>> +                PMD_DRV_LOG(ERR,            \
>> +                        "%s returned error: %d\n",    \
>> +                        mp_name_ ## f, rsp->result);\
>> +            }                        \
>> +            free(mp_rep.msgs);                \
>> +        } else if (rte_errno == ENOTSUP) {            \
>> +            PMD_DRV_LOG(ERR,                \
>> +                    "No IPC, can't proxy to primary\n");\
>> +            ret = -rte_errno;                \
>> +        } else {                        \
>> +            PMD_DRV_LOG(ERR, "Request %s failed: %s\n",    \
>> +                    mp_name_ ## f,            \
>> +                    rte_strerror(rte_errno));        \
>> +            ret = -ECOMM;                    \

Also 'ECOMM' seems only defined for Linux, so it breaks the
FreeBSD build:
http://mails.dpdk.org/archives/test-report/2022-February/263131.html

>> +        }                            \
>> +    }                                \
>> +    ret;                                \
>> +})
>>
> <...>