> -----Original Message----- > From: EXT Maxim Uvarov [mailto:maxim.uva...@linaro.org] > Sent: Friday, September 11, 2015 12:47 PM > To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer > Cc: LNG ODP Mailman List > Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count > support > > On 09/11/15 12:32, Savolainen, Petri (Nokia - FI/Espoo) wrote: > > > >> -----Original Message----- > >> From: EXT Maxim Uvarov [mailto:maxim.uva...@linaro.org] > >> Sent: Friday, September 11, 2015 11:30 AM > >> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer > >> Cc: LNG ODP Mailman List > >> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference count > >> support > >> > >> On 09/11/15 10:48, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >>>> -----Original Message----- > >>>> From: EXT Maxim Uvarov [mailto:maxim.uva...@linaro.org] > >>>> Sent: Thursday, September 10, 2015 7:11 PM > >>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer > >>>> Cc: LNG ODP Mailman List > >>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference > count > >>>> support > >>>> > >>>> On 09/10/15 15:54, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >>>>>> -----Original Message----- > >>>>>> From: EXT Maxim Uvarov [mailto:maxim.uva...@linaro.org] > >>>>>> Sent: Thursday, September 10, 2015 2:20 PM > >>>>>> To: Savolainen, Petri (Nokia - FI/Espoo); EXT Bill Fischofer > >>>>>> Cc: LNG ODP Mailman List > >>>>>> Subject: Re: [lng-odp] [API-NEXT PATCH] api: packet reference > >> count > >>>>>> support > >>>>>> > >>>>>> On 09/10/15 12:00, Savolainen, Petri (Nokia - FI/Espoo) wrote: > >>>>>>> The problem to solve is: how to enable multiple references to a > >>>>>> packet > >>>>>>> and more generally to a packet segment. It would be better to > >>>> create > >>>>>>> new handles, instead of increment a 32bit count. Currently, the > >>>>>>> ownership of a handle is lost during enq/free/… operations. > That > >>>>>>> semantic is worth maintaining also with multiple references. > >>>>>>> > >>>>>>> odp_packet_t pkt = current_packet; > >>>>>>> > >>>>>>> odp_packet_t my_ref; > >>>>>>> > >>>>>>> // Create new reference to a packet > >>>>>>> > >>>>>>> my_ref = odp_packet_create_ref(pkt); > >>>>>>> > >>>>>>> if (odp_packet_ref_count(pkt) > 1) > >>>>>>> > >>>>>>> // reference count is now 2 > >>>>>>> > >>>>>>> foo(); > >>>>>>> > >>>>>>> odp_packet_free(pkt); > >>>>>>> > >>>>>>> // After free, user does not own handle ‘pkt’ anymore and > cannot > >>>> it. > >>>>>>> if (odp_packet_ref_count(my_ref) > 1) > >>>>>>> > >>>>>>> // reference count is now 1 > >>>>>>> > >>>>>>> foo(); > >>>>>>> > >>>>>>> odp_packet_free(my_ref); > >>>>>>> > >>>>>>> // Packet is now freed and user does not have any reference to > it > >>>>>> anymore. > >>>>>> I think that uses case is not close to real life. If you do in > one > >>>>>> thread: > >>>>>> pkt = <get packet or alloc>; > >>>>>> odp_packet_refcount_set(pkt, 2); > >>>>>> foo(pkt); > >>>>>> odp_packet_free(pkt); <- ref is 1 > >>>>>> foo(pkt); > >>>>>> odp_packet_free(pkt); <- ref 0, free > >>>>>> > >>>>>> Then you know what pkt is on each line. Instead of having 2 > >>>> reference > >>>>>> as > >>>>>> in your example my_ref and pkt, you > >>>>>> use just one pkt. > >>>>> The point of the example is clear ownership of a reference to the > >>>> packet. > >>>>> These are *bugs* currently, and API should stay clear on > ownership > >>>> issues also multiple references: > >>>>> // double free a packet (handle or reference) > >>>>> odp_packet_free(pkt); > >>>>> odp_packet_free(pkt); > >>>>> > >>>>> > >>>>> // double enqueue a packet (handle or reference) > >>>>> ev = odp_packet_from_event(pkt); > >>>>> odp_queue_enq(queue1, ev); > >>>>> odp_queue_enq(queue2, ev); > >>>>> > >>>>> > >>>>> With two different handles (references) it's clear how owns the > >>>> handle and implementation can either return the same or different > >> value > >>>> as the handle. > >>>>> my_ref = odp_packet_create_ref(pkt); > >>>>> > >>>>> ev1 = odp_packet_from_event(pkt); > >>>>> odp_queue_enq(queue1, ev1); > >>>>> > >>>>> ev2 = odp_packet_from_event(my_ref); > >>>>> odp_queue_enq(queue2, ev2); > >>>>> > >>>> In my case it's easy to check, inside odp_packet_free(pkt): > >>>> if (!odp_packet_refcount(pkt)) > >>>> ODP_ABORT("illegal free"); > >>>> > >>>> That will capture bugs with double enqueue when you will try to > send > >>>> that packets. > >>>> > >>>> > >>>> If your case we should maintain somewhere array of packet handles > >> and > >>>> packet headers, > >>>> with unclear size depending how match reference we can have. And > >>>> handles > >>>> should not > >>>> overlap with current packet handles. That looks more complicated, > >>>> right? > >>>> > >>> Just like any other ODP API, this is a question of API definition > vs. > >> freedom of implementation. API spec defines two handles (one per > >> reference) and thus user must act as if those two handles are > unique, > >> BUT the implementation can choose if it reuses the handle value or > not. > >>> For example, > >>> > >>> typedef odp_packet_t soc_packet_descriptor_t *; > >>> > >>> > >>> odp_packet_t odp_packet_create_ref(odp_packet_t pkt) > >>> { > >>> soc_packet_descriptor_t *desc = (soc_packet_descriptor_t *) pkt; > >>> > >>> desc->ref_count++; > >>> return pkt; > >>> } > >> that variant is clear, but pkt on input and output is the same - > it's > >> pointer to desc somewhere. > >> But it's the same what I proposed. > >> > >>> OR > >>> > >>> odp_packet_t odp_packet_create_ref(odp_packet_t pkt) > >>> { > >>> soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt) > >> & PTR_MASK; > >>> desc->ref_count++; > >>> return (desc | > >> THIS_IS_A_REFERENCE_TO_A_PACKET_NOT_THE_ORIGINAL_PACKET_DESC); > >>> } > >> That variant modifies desc pointer. And there might be possible > issues: > >> > >> 1. it might not portable due to unable to restore original pointer > to > >> descr (if all bits of pointer address are used.). > >> 2. I assume some hw schedulers can unable to work with such modified > >> pointers. And it will be hard to implement in hw. > >> 3. On every function which works with packet you need clear > >> REFERENCE_BIT before accessing packet (access to *desc), > >> which is additional overhead. > > > > soc_packet_descriptor_t *desc = ((soc_packet_descriptor_t *) pkt) & > PTR_MASK; > > > > In this case: > > - handle = pointer + flags > > - pointer = handle - flags (the mask) > > - implementation passes the pointer to the HW, not the handle > > > > Handle can carry anything implementation needs, e.g. a flag which > distinguishes the original packet handle from additional references > (if that's import to the implementation). > > > > > > -Petri > > > > My point is - not everywhere you can do: > > - handle = pointer + flags > - pointer = handle - flags (the mask) > > > Let's say pointer uses 32 bits for address (PTR_MASK is 32 bit) handle > is also 32 bit. You just not have enough room > to save flags. > > Maxim.
It's OK since any implementation is not forced to save flags or return different handles for pkt and ref. Also in 32 bit systems you can - take advantage of descriptor alignment and/or memory map - and pack pointers to less than 32 bits - use indexes instead of pointers - define odp_packet_t as a structure of flags + pointer (e.g. sizeof struct can be 64 bits or more) Nothing special here, just use normal programming methods to pack information into a handle ==> into a structure that you can define. -Petri _______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
