Re: [dpdk-dev] Admin Queue ENA

[kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>]

Hi Michał,

Thanks for getting back on this.

In our design we are using multiple cores requesting for rte_eth_stats_get,
it is not from one process and hence not serialized. Since in our design
this is not serialized, and hence in get_comp_ctxt() checking for occupied
flag and comp_ctxt_release() are not done atomically which is causing this
issue. Please let me know if my understanding is correct, so that I will
fix the application in such a way that it is done from one process and not
multiple.

Thanks,
Param.

On Thu, Nov 28, 2019 at 6:44 PM Michał Krawczyk <mk@semihalf.com> wrote:

> Hi Param,
>
> first of all - you are using very old ena_com. This code comes from
> the DPDK version before v18.08. If you have any doubts, please check
> the newer version of the driver and DPDK as the potential bug could be
> already fixed there.
>
> Anyway, if you will look at the function get_comp_ctxt() which is
> called by __ena_com_submit_admin_cmd() to get the completion context,
> there is a check for the context if it's not occupied - in case it is
> (which will be true until comp_ctxt_release() will clear it), the new
> command using the same context cannot be used. So there shouldn't be
> two consumers using the same completion contexts.
>
> In addition, drivers that are using ena_com are sending admin commands
> one at a time during the init, so there shouldn't be even 2 commands
> at a time. The only exception is ena_com_get_dev_basic_stats(), which
> is called from rte_eth_stats_get() context - but if you consider DPDK
> application, it should use it on the management lcore after init, so
> it'll also be serialized.
>
> Thanks,
> Michal
>
>
>
> pt., 8 lis 2019 o 07:02 kumaraparameshwaran rathinavel
> <kumaraparamesh92@gmail.com> napisał(a):
> >
> > Hi Michał,
> >
> > Please look at the below function,
> >
> > static int
> > ena_com_wait_and_process_admin_cq_polling(
> >         struct ena_comp_ctx *comp_ctx,
> >         struct ena_com_admin_queue *admin_queue)
> > {
> >     unsigned long flags = 0;
> >     u64 start_time;
> >     int ret;
> >
> >     start_time = ENA_GET_SYSTEM_USECS();
> >
> >     while (comp_ctx->status == ENA_CMD_SUBMITTED) {
> >         if ((ENA_GET_SYSTEM_USECS() - start_time) >
> >             ADMIN_CMD_TIMEOUT_US) {
> >             ena_trc_err("Wait for completion (polling) timeout\n");
> >             /* ENA didn't have any completion */
> >             ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
> >             admin_queue->stats.no_completion++;
> >             admin_queue->running_state = false;
> >             ENA_SPINLOCK_UNLOCK(admin_queue->q_lock, flags);
> >
> >             ret = ENA_COM_TIMER_EXPIRED;
> >             goto err;
> >         }
> >
> >         ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
> >         ena_com_handle_admin_completion(admin_queue);
> >         ENA_SPINLOCK_UNLOCK(admin_queue->q_lock, flags);
> >     }
> >
> >     if (unlikely(comp_ctx->status == ENA_CMD_ABORTED)) {
> >         ena_trc_err("Command was aborted\n");
> >         ENA_SPINLOCK_LOCK(admin_queue->q_lock, flags);
> >         admin_queue->stats.aborted_cmd++;
> >         ENA_SPINLOCK_UNLOCK(admin_queue->q_lock, flags);
> >         ret = ENA_COM_NO_DEVICE;
> >         goto err;
> >     }
> >
> >     ENA_ASSERT(comp_ctx->status == ENA_CMD_COMPLETED,
> >            "Invalid comp status %d\n", comp_ctx->status);
> >
> >     ret = ena_com_comp_status_to_errno(comp_ctx->comp_status);
> > err:
> >     comp_ctxt_release(admin_queue, comp_ctx);
> >     return ret;
> > }
> >
> > This is a case where there are two threads executing admin commands.
> >
> > The occupied flag is set to false in the function comp_ctxt_release.
> Let us say there are two consumers of completion context and C1 has a
> completion context and the same completion context can be used by another
> consumer C2 even before the C1 is resetting the occupied flag.
> >
> > This is because the ena_com_handle_admin_completion is done under spin
> lock and comp_ctxt_release is not under this spin lock.
> >
> > Thanks,
> > Param
> >
> > On Thu, Oct 24, 2019 at 2:09 PM Michał Krawczyk <mk@semihalf.com> wrote:
> >>
> >> sob., 19 paź 2019 o 20:26 kumaraparameshwaran rathinavel
> >> <kumaraparamesh92@gmail.com> napisał(a):
> >> >
> >> > Hi All,
> >> >
> >> > In the ENA poll mode driver I see that every request in the admin
> queue is
> >> > associated with a completion context and this is preallocated during
> the
> >> > device initialisation. When the completion context is used we check
> for
> >> > occupied to be true in the 16.X version if the occupied flag is set
> to true
> >> > we assert and in the latest version I see that this is an error log.
> But
> >> > there is a time window where if the completion context would be
> available
> >> > to the other consumer but still the old consumer did not set the
> occupied
> >> > to false. The new consumer holds the admin queue lock to get the
> completion
> >> > context but the update by the old consumer to set the the occupied
> flag is
> >> > not done under lock. So should we make sure that the new consumer
> should
> >> > get the completion context only when the occupied flag is set to
> false. Any
> >> > thoughts on this?
> >>
> >> Hi Param,
> >>
> >> Both the producer and the consumer are holding the spinlock while
> >> getting the completion context. If you see any situation where it
> >> isn't (besides the release function), please let me know.
> >> As it is protected by the lock, returning error while completion
> >> context is occupied (and it shouldn't) it fine, as it will stop the
> >> admin queue and allow the DPDK user application to execute the reset
> >> of the device.
> >>
> >> Thanks,
> >> Michal
> >>
> >> > If required I can try to make a patch where the completion context
> would be
> >> > available only after setting the occupied flag to false.
> >> >
> >> > Thanks,
> >> > Param.
>