From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4775C43B61; Wed, 21 Feb 2024 10:31:35 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 163CF402B9; Wed, 21 Feb 2024 10:31:35 +0100 (CET) Received: from mail-qt1-f179.google.com (mail-qt1-f179.google.com [209.85.160.179]) by mails.dpdk.org (Postfix) with ESMTP id 717AF40284 for ; Wed, 21 Feb 2024 10:31:33 +0100 (CET) Received: by mail-qt1-f179.google.com with SMTP id d75a77b69052e-42a9f4935a6so51271001cf.1 for ; Wed, 21 Feb 2024 01:31:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708507893; x=1709112693; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=jtVZ6ie6oTV8Ji7LxhjRi6eao0Go7nz4wbn8ZWm/HhY=; b=V0tp8CmUDTBKTy++72nMEP25+T/5Ol4uiTIi1knfhowPDLgi9o9XELxKuunBa7cByw T1rcH8JBl3a+m8z8QlcjUd/TS4ZZnsJXnHocSCMoObSDmte3uETIiuQUvGoTxDXEI0k5 c8Y1OVUkBs7p1kK7EnhWTTgJpWQI/smfMfz1WZlhetRgGfFTsRnHqEVRBtTvE+9nRSlD qov6dAKzzqtMGKJMhNor74vsXj5FHamfJTHQyCE+w0ETGvr3mecP0GWCbeIjZRSB5HJd EC8kporoAUMkE2tztiN2FebYprY5jdHGmBCip0sO/IuA0ZcOb2ijRDyQMME+CpDvGHYK MSzQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708507893; x=1709112693; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=jtVZ6ie6oTV8Ji7LxhjRi6eao0Go7nz4wbn8ZWm/HhY=; b=su/qLpYbS9fHuubNk/9Xl/+HDL3MbpI7vzDQUh4Zl5xxe2oEUNK7dbPtQPMvNgHLTm QdZgkPJm54mprrwn+oWLTh11yaZ/Kx2YfUo60hJYlNcdsaelj0zwY88yVrb4YJ5rE1gY mrQQCKfsBkeWxeWEz6+Tg26JKzize9A64h56O/Pxx23X/JLNxtHzTfcL5wtdNGRDGUDC YkTzffAwUu1mC/X4XCwcz1nWXgjEeV9z1d6vRRys58BN9chHPvwoNv8+n4KYEB0uspuG 5rDIo6WTbOTFgsDl4iz1dZJjNZ1cbaKIb3S0pwra6Bc4DSLgEpUxlw5rm9yYZTYedU9q gp5g== X-Gm-Message-State: AOJu0YyQpU/4E/5/DehcO+1xBvzX5IevD2SP16Yzzs2f8o6Ydfm1/PcY fMbAjpvZ8xbpqePF4z96uSLVz9rWOOJHwxRaA3wesiHZE0+J/DJMVtUfjFc4oKwOYHcHdVasuo5 pCQIJmRnunKGoyTGDs5bLEt4TfUs= X-Google-Smtp-Source: AGHT+IGhjt6QbVQOZd1aJiO/JheOrWmmShWi1grLQ/EQdOdVAPigaeXMjds1mxT2MYDWrs2Oc2XS9sOw538m/jXy5c4= X-Received: by 2002:a05:622a:1821:b0:42d:d9f9:862f with SMTP id t33-20020a05622a182100b0042dd9f9862fmr23826847qtc.29.1708507892560; Wed, 21 Feb 2024 01:31:32 -0800 (PST) MIME-Version: 1.0 References: <20240119174346.108905-1-bruce.richardson@intel.com> <20240202123953.77166-1-bruce.richardson@intel.com> <20240202123953.77166-11-bruce.richardson@intel.com> In-Reply-To: From: Jerin Jacob Date: Wed, 21 Feb 2024 15:01:06 +0530 Message-ID: Subject: Re: [PATCH v3 10/11] eventdev: clarify docs on event object fields and op types To: Bruce Richardson Cc: dev@dpdk.org, jerinj@marvell.com, mattias.ronnblom@ericsson.com, abdullah.sevincer@intel.com, sachin.saxena@oss.nxp.com, hemant.agrawal@nxp.com, pbhagavatula@marvell.com, pravin.pathak@intel.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Feb 20, 2024 at 11:09=E2=80=AFPM Bruce Richardson wrote: > > On Fri, Feb 09, 2024 at 02:44:04PM +0530, Jerin Jacob wrote: > > On Fri, Feb 2, 2024 at 6:11=E2=80=AFPM Bruce Richardson > > wrote: > > > > > > Clarify the meaning of the NEW, FORWARD and RELEASE event types. > > > For the fields in "rte_event" struct, enhance the comments on each to > > > clarify the field's use, and whether it is preserved between enqueue = and > > > dequeue, and it's role, if any, in scheduling. > > > > > > Signed-off-by: Bruce Richardson > > > --- > > > V3: updates following review > > > --- > > > lib/eventdev/rte_eventdev.h | 161 +++++++++++++++++++++++++---------= -- > > > 1 file changed, 111 insertions(+), 50 deletions(-) > > > > > > diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.= h > > > index 8d72765ae7..58219e027e 100644 > > > --- a/lib/eventdev/rte_eventdev.h > > > +++ b/lib/eventdev/rte_eventdev.h > > > @@ -1463,47 +1463,54 @@ struct rte_event_vector { > > > > > > /* Event enqueue operations */ > > > #define RTE_EVENT_OP_NEW 0 > > > -/**< The event producers use this operation to inject a new event to= the > > > - * event device. > > > +/**< The @ref rte_event.op field must be set to this operation type = to inject a new event, > > > + * i.e. one not previously dequeued, into the event device, to be sc= heduled > > > + * for processing. > > > */ > > > #define RTE_EVENT_OP_FORWARD 1 > > > -/**< The CPU use this operation to forward the event to different ev= ent queue or > > > - * change to new application specific flow or schedule type to enabl= e > > > - * pipelining. > > > +/**< The application must set the @ref rte_event.op field to this op= eration type to return a > > > + * previously dequeued event to the event device to be scheduled for= further processing. > > > * > > > - * This operation must only be enqueued to the same port that the > > > + * This event *must* be enqueued to the same port that the > > > * event to be forwarded was dequeued from. > > > + * > > > + * The event's fields, including (but not limited to) flow_id, sched= uling type, > > > + * destination queue, and event payload e.g. mbuf pointer, may all b= e updated as > > > + * desired by the application, but the @ref rte_event.impl_opaque fi= eld must > > > + * be kept to the same value as was present when the event was deque= ued. > > > */ > > > #define RTE_EVENT_OP_RELEASE 2 > > > /**< Release the flow context associated with the schedule type. > > > * > > > - * If current flow's scheduler type method is *RTE_SCHED_TYPE_ATOMIC= * > > > - * then this function hints the scheduler that the user has complete= d critical > > > - * section processing in the current atomic context. > > > - * The scheduler is now allowed to schedule events from the same flo= w from > > > - * an event queue to another port. However, the context may be still= held > > > - * until the next rte_event_dequeue_burst() call, this call allows b= ut does not > > > - * force the scheduler to release the context early. > > > - * > > > - * Early atomic context release may increase parallelism and thus sy= stem > > > + * If current flow's scheduler type method is @ref RTE_SCHED_TYPE_AT= OMIC > > > + * then this operation type hints the scheduler that the user has co= mpleted critical > > > + * section processing for this event in the current atomic context, = and that the > > > + * scheduler may unlock any atomic locks held for this event. > > > + * If this is the last event from an atomic flow, i.e. all flow lock= s are released, > > > > > > Similar comment as other email > > [Jerin] When there are multiple atomic events dequeue from @ref > > rte_event_dequeue_burst() > > for the same event queue, and it has same flow id then the lock is .... > > > > [Mattias] > > Yes, or maybe describing the whole lock/unlock state. > > > > "The conceptual per-queue-per-flow lock is in a locked state as long > > (and only as long) as one or more events pertaining to that flow were > > scheduled to the port in question, but are not yet released." > > > > Maybe it needs to be more meaty, describing what released means. I don'= t > > have the full context of the documentation in my head when I'm writing = this. > > > > Will take a look to reword a bit > > > > > > + * the scheduler is now allowed to schedule events from that flow fr= om to another port. > > > + * However, the atomic locks may be still held until the next rte_ev= ent_dequeue_burst() > > > + * call; enqueuing an event with opt type @ref RTE_EVENT_OP_RELEASE = allows, > > > > Is ";" intended? > > > > > + * but does not force, the scheduler to release the atomic locks ear= ly. > > > > instead of "not force", can use the term _hint_ the driver and reword. > > Ok. > > > > > + * > > > + * Early atomic lock release may increase parallelism and thus syste= m > > > * performance, but the user needs to design carefully the split int= o critical > > > * vs non-critical sections. > > > * > > > - * If current flow's scheduler type method is *RTE_SCHED_TYPE_ORDERE= D* > > > - * then this function hints the scheduler that the user has done all= that need > > > - * to maintain event order in the current ordered context. > > > - * The scheduler is allowed to release the ordered context of this p= ort and > > > - * avoid reordering any following enqueues. > > > - * > > > - * Early ordered context release may increase parallelism and thus s= ystem > > > - * performance. > > > + * If current flow's scheduler type method is @ref RTE_SCHED_TYPE_OR= DERED > > > + * then this operation type informs the scheduler that the current e= vent has > > > + * completed processing and will not be returned to the scheduler, i= .e. > > > + * it has been dropped, and so the reordering context for that event > > > + * should be considered filled. > > > * > > > - * If current flow's scheduler type method is *RTE_SCHED_TYPE_PARALL= EL* > > > - * or no scheduling context is held then this function may be an NOO= P, > > > - * depending on the implementation. > > > + * Events with this operation type must only be enqueued to the same= port that the > > > + * event to be released was dequeued from. The @ref rte_event.impl_o= paque > > > + * field in the release event must have the same value as that in th= e original dequeued event. > > > * > > > - * This operation must only be enqueued to the same port that the > > > - * event to be released was dequeued from. > > > + * If a dequeued event is re-enqueued with operation type of @ref RT= E_EVENT_OP_RELEASE, > > > + * then any subsequent enqueue of that event - or a copy of it - mus= t be done as event of type > > > + * @ref RTE_EVENT_OP_NEW, not @ref RTE_EVENT_OP_FORWARD. This is bec= ause any context for > > > + * the originally dequeued event, i.e. atomic locks, or reorder buff= er entries, will have > > > + * been removed or invalidated by the release operation. > > > */ > > > > > > /** > > > @@ -1517,56 +1524,110 @@ struct rte_event { > > > /** Event attributes for dequeue or enqueue operation= */ > > > struct { > > > uint32_t flow_id:20; > > > - /**< Targeted flow identifier for the enqueue= and > > > - * dequeue operation. > > > - * The value must be in the range of > > > - * [0, nb_event_queue_flows - 1] which > > > - * previously supplied to rte_event_dev_confi= gure(). > > > + /**< Target flow identifier for the enqueue a= nd dequeue operation. > > > + * > > > + * For @ref RTE_SCHED_TYPE_ATOMIC, this field= is used to identify a > > > + * flow for atomicity within a queue & priori= ty level, such that events > > > + * from each individual flow will only be sch= eduled to one port at a time. > > > + * > > > + * This field is preserved between enqueue an= d dequeue when > > > + * a device reports the @ref RTE_EVENT_DEV_CA= P_CARRY_FLOW_ID > > > + * capability. Otherwise the value is impleme= ntation dependent > > > + * on dequeue. > > > */ > > > uint32_t sub_event_type:8; > > > /**< Sub-event types based on the event sourc= e. > > > + * > > > + * This field is preserved between enqueue an= d dequeue. > > > + * This field is for application or event ada= pter use, > > > + * and is not considered in scheduling decisi= ons. > > > > > > cnxk driver is considering this for scheduling decision to > > differentiate the producer i.e event adapters. > > If other drivers are not then we can change the language around it is > > implementation defined. > > > How does the event type influence the scheduling decision? I can drop the For cnxk, From HW POV, the flow ID is 32 bit which is divided between flow_id(20 bit), sub event type(8bit) and event type(4bit) > last line here Yes. Please > but it seems strange to me that the type of event could affect > things. I would have thought that with the eventdev API only the queue, > flow id, and priority would be factors in scheduling? > > > > > > + * > > > * @see RTE_EVENT_TYPE_CPU > > > */ > > > uint32_t event_type:4; > > > - /**< Event type to classify the event source. > > > - * @see RTE_EVENT_TYPE_ETHDEV, (RTE_EVENT_TYP= E_*) > > > + /**< Event type to classify the event source.= (RTE_EVENT_TYPE_*) > > > + * > > > + * This field is preserved between enqueue an= d dequeue > > > + * This field is for application or event ada= pter use, > > > + * and is not considered in scheduling decisi= ons. > > > > > > cnxk driver is considering this for scheduling decision to > > differentiate the producer i.e event adapters. > > If other drivers are not then we can change the language around it is > > implementation defined. > > > > > */ > > > uint8_t op:2; > > > - /**< The type of event enqueue operation - ne= w/forward/ > > > - * etc.This field is not preserved across an = instance > > > - * and is undefined on dequeue. > > > - * @see RTE_EVENT_OP_NEW, (RTE_EVENT_OP_*) > > > + /**< The type of event enqueue operation - ne= w/forward/ etc. > > > + * > > > + * This field is *not* preserved across an in= stance > > > + * and is implementation dependent on dequeue= . > > > + * > > > + * @see RTE_EVENT_OP_NEW > > > + * @see RTE_EVENT_OP_FORWARD > > > + * @see RTE_EVENT_OP_RELEASE > > > */ > > > uint8_t rsvd:4; > > > - /**< Reserved for future use */ > > > + /**< Reserved for future use. > > > + * > > > + * Should be set to zero on enqueue. > > > > I am worried about some application explicitly start setting this to > > zero on every enqueue. > > Instead, can we say application should not touch the field, Since every= eventdev > > operations starts with dequeue() driver can fill to the correct value. > > > > I'll set this to zero on "NEW", or untouched on FORWARD/RELEASE. OK > If we don't state that it should be zeroed on NEW or untouched > otherwise we cannot use the space in future without ABI break. > > > > + */ > > > uint8_t sched_type:2; > > > /**< Scheduler synchronization type (RTE_SCHE= D_TYPE_*) > > > * associated with flow id on a given event q= ueue > > > * for the enqueue and dequeue operation. > > > + * > > > + * This field is used to determine the schedu= ling type > > > + * for events sent to queues where @ref RTE_E= VENT_QUEUE_CFG_ALL_TYPES > > > + * is configured. > > > + * For queues where only a single scheduling = type is available, > > > + * this field must be set to match the config= ured scheduling type. > > > + * > > > + * This field is preserved between enqueue an= d dequeue. > > > + * > > > + * @see RTE_SCHED_TYPE_ORDERED > > > + * @see RTE_SCHED_TYPE_ATOMIC > > > + * @see RTE_SCHED_TYPE_PARALLEL > > > */ > > > uint8_t queue_id; > > > /**< Targeted event queue identifier for the = enqueue or > > > * dequeue operation. > > > - * The value must be in the range of > > > - * [0, nb_event_queues - 1] which previously = supplied to > > > - * rte_event_dev_configure(). > > > + * The value must be less than @ref rte_event= _dev_config.nb_event_queues > > > + * which was previously supplied to rte_event= _dev_configure(). > > > > Some reason, similar text got removed for flow_id. Please add the same. > > > > That was deliberate based on discussion on V2. See: > > http://inbox.dpdk.org/dev/Zby3nb4NGs8T5odL@bricha3-MOBL.ger.corp.intel.co= m/ > > and wider thread discussion starting here: > > http://inbox.dpdk.org/dev/ZbvOtAEpzja0gu7b@bricha3-MOBL.ger.corp.intel.co= m/ > > Basically, the comment is wrong based on what the code does now. No event > adapters or apps are limiting the flow-id, and nothing seems broken, so w= e > can remove the comment. OK >