| Message ID | 1500573607-18783-10-git-send-email-odpbot@yandex.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/10] linux-generic: packet: restructure inline routines to use macros | expand |
On 07/20/17 21:00, Github ODP bot wrote: > From: Bill Fischofer <bill.fischofer@linaro.org> > > When attempting to send a reference via IPC, make a copy of the packet as > reference sharing is only supported within a single ODP instance. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > /** Email created from pull request 82 (Bill-Fischofer-Linaro:pktrefs) > ** https://github.com/Linaro/odp/pull/82 > ** Patch: https://github.com/Linaro/odp/pull/82.patch > ** Base sha: 95ba4b394009d92c29c2e22f0776e90bb4c6edec > ** Merge commit sha: 01d093d362af5a52e079c622a99ae3495f68b64c > **/ > platform/linux-generic/pktio/ipc.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/platform/linux-generic/pktio/ipc.c b/platform/linux-generic/pktio/ipc.c > index bc7d7564..8c7db84e 100644 > --- a/platform/linux-generic/pktio/ipc.c > +++ b/platform/linux-generic/pktio/ipc.c > @@ -592,7 +592,9 @@ static int ipc_pktio_send_lockless(pktio_entry_t *pktio_entry, > > _ipc_free_ring_packets(pktio_entry, pktio_entry->s.ipc.tx.free); > > - /* Copy packets to shm shared pool if they are in different */ > + /* Copy packets to shm shared pool if they are in different > + * pool, or if they are references (we can't share across IPC). > + */ In my understanding references have to work in following way. Lets say two processes A and B. A: send pkt: queue it to ring. B: recv: dequeue pkt from ring, inc pkt ref, create pkt handle. B: free or send: dec pkt ref, queue to 'free ring' to be freeing by A. A: dequeue 'free ring': finally destroys packet if no more references. Logic might be complex if B will also modify packet. But because they share in memory the same packet pool, phdrs also are common. So there is good chance that references will work in that case. Maxim. > for (i = 0; i < len; i++) { > odp_packet_t pkt = pkt_table[i]; > pool_t *ipc_pool = pool_entry_from_hdl(pktio_entry->s.ipc.pool); > @@ -602,7 +604,8 @@ static int ipc_pktio_send_lockless(pktio_entry_t *pktio_entry, > pkt_hdr = odp_packet_hdr(pkt); > pool = pkt_hdr->buf_hdr.pool_ptr; > > - if (pool->pool_idx != ipc_pool->pool_idx) { > + if (pool->pool_idx != ipc_pool->pool_idx || > + odp_packet_has_ref(pkt)) { > odp_packet_t newpkt; > > newpkt = odp_packet_copy(pkt, pktio_entry->s.ipc.pool); >
On Thu, Jul 20, 2017 at 2:32 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 07/20/17 21:00, Github ODP bot wrote: > > From: Bill Fischofer <bill.fischofer@linaro.org> > > > > When attempting to send a reference via IPC, make a copy of the packet as > > reference sharing is only supported within a single ODP instance. > > > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > > --- > > /** Email created from pull request 82 (Bill-Fischofer-Linaro:pktrefs) > > ** https://github.com/Linaro/odp/pull/82 > > ** Patch: https://github.com/Linaro/odp/pull/82.patch > > ** Base sha: 95ba4b394009d92c29c2e22f0776e90bb4c6edec > > ** Merge commit sha: 01d093d362af5a52e079c622a99ae3495f68b64c > > **/ > > platform/linux-generic/pktio/ipc.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/platform/linux-generic/pktio/ipc.c > b/platform/linux-generic/pktio/ipc.c > > index bc7d7564..8c7db84e 100644 > > --- a/platform/linux-generic/pktio/ipc.c > > +++ b/platform/linux-generic/pktio/ipc.c > > @@ -592,7 +592,9 @@ static int ipc_pktio_send_lockless(pktio_entry_t > *pktio_entry, > > > > _ipc_free_ring_packets(pktio_entry, pktio_entry->s.ipc.tx.free); > > > > - /* Copy packets to shm shared pool if they are in different */ > > + /* Copy packets to shm shared pool if they are in different > > + * pool, or if they are references (we can't share across IPC). > > + */ > > In my understanding references have to work in following way. Lets say > two processes A and B. > > A: send pkt: queue it to ring. > B: recv: dequeue pkt from ring, inc pkt ref, create pkt handle. > B: free or send: dec pkt ref, queue to 'free ring' to be freeing by A. > A: dequeue 'free ring': finally destroys packet if no more references. > > Logic might be complex if B will also modify packet. But because they > share in memory the same packet pool, phdrs also are common. So there is > good chance that references will work in that case. > The problem is that when a packet is referenced, it consists of a shared part, which must be treated as read/only, and an unshared part, which is private to that particular reference and hence may be freely extended or modified. These semantics cannot be maintained if it is sent across an IPC channel, so to be safe we make a copy of the whole packet. You're already doing this if the packet resides in another pool, so this seemed the most natural place to put this safeguard. The other issue is that the reference count associated with packet segments is shared metadata and hence must be modifiable by all holders of a reference (it uses a odp_atomic_u32_t variable). Again, it's not clear if we can maintain these semantics across IPC since the various sections of referenced packets are chained together with pointers, which are not sharable across address space boundaries. Since there is no clear use case for passing references across IPC interfaces, the simplest solution is to just do an implied copy whenever a reference is sent. That ensures that there is no confusion between the two parties. > > Maxim. > > > for (i = 0; i < len; i++) { > > odp_packet_t pkt = pkt_table[i]; > > pool_t *ipc_pool = pool_entry_from_hdl(pktio_ > entry->s.ipc.pool); > > @@ -602,7 +604,8 @@ static int ipc_pktio_send_lockless(pktio_entry_t > *pktio_entry, > > pkt_hdr = odp_packet_hdr(pkt); > > pool = pkt_hdr->buf_hdr.pool_ptr; > > > > - if (pool->pool_idx != ipc_pool->pool_idx) { > > + if (pool->pool_idx != ipc_pool->pool_idx || > > + odp_packet_has_ref(pkt)) { > > odp_packet_t newpkt; > > > > newpkt = odp_packet_copy(pkt, > pktio_entry->s.ipc.pool); > > > >
diff --git a/platform/linux-generic/pktio/ipc.c b/platform/linux-generic/pktio/ipc.c index bc7d7564..8c7db84e 100644 --- a/platform/linux-generic/pktio/ipc.c +++ b/platform/linux-generic/pktio/ipc.c @@ -592,7 +592,9 @@ static int ipc_pktio_send_lockless(pktio_entry_t *pktio_entry, _ipc_free_ring_packets(pktio_entry, pktio_entry->s.ipc.tx.free); - /* Copy packets to shm shared pool if they are in different */ + /* Copy packets to shm shared pool if they are in different + * pool, or if they are references (we can't share across IPC). + */ for (i = 0; i < len; i++) { odp_packet_t pkt = pkt_table[i]; pool_t *ipc_pool = pool_entry_from_hdl(pktio_entry->s.ipc.pool); @@ -602,7 +604,8 @@ static int ipc_pktio_send_lockless(pktio_entry_t *pktio_entry, pkt_hdr = odp_packet_hdr(pkt); pool = pkt_hdr->buf_hdr.pool_ptr; - if (pool->pool_idx != ipc_pool->pool_idx) { + if (pool->pool_idx != ipc_pool->pool_idx || + odp_packet_has_ref(pkt)) { odp_packet_t newpkt; newpkt = odp_packet_copy(pkt, pktio_entry->s.ipc.pool);