diff mbox

[1/2] linux-generic: packets: enable segmented packet support

Message ID 1418858186-25251-1-git-send-email-bill.fischofer@linaro.org
State Accepted
Commit 05b8dfc7e94ba26faeae79fa630e23419ee71504
Headers show

Commit Message

Bill Fischofer Dec. 17, 2014, 11:16 p.m. UTC 
Add segmented packet I/O support via sockets. RAW sockets limited to
single segment.

Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
---
 platform/linux-generic/odp_packet_socket.c | 33 +++++++++++++++++-------------
 1 file changed, 19 insertions(+), 14 deletions(-)

Comments

Taras Kondratiuk Dec. 18, 2014, 2:56 p.m. UTC | #1 
On 12/18/2014 01:16 AM, Bill Fischofer wrote:
> Add segmented packet I/O support via sockets. RAW sockets limited to
> single segment.
>
> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
> ---
>   platform/linux-generic/odp_packet_socket.c | 33 +++++++++++++++++-------------
>   1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/platform/linux-generic/odp_packet_socket.c b/platform/linux-generic/odp_packet_socket.c
> index 2849065..340da88 100644
> --- a/platform/linux-generic/odp_packet_socket.c
> +++ b/platform/linux-generic/odp_packet_socket.c
> @@ -321,7 +321,8 @@ int recv_pkt_sock_basic(pkt_sock_t *const pkt_sock,
>
>   	for (i = 0; i < len; i++) {
>   		if (odp_likely(pkt == ODP_PACKET_INVALID)) {
> -			pkt = _odp_packet_alloc(pkt_sock->pool);
> +			pkt = odp_packet_alloc(pkt_sock->pool,
> +					       pkt_sock->max_frame_len);

In seems only MMAP type supports segmented buffers as it uses
odp_packet_copydata_*() API. RAW and MMSG operate only with a first
segment. Should we add assert here to check that packet is unsegmented?

>   			if (odp_unlikely(pkt == ODP_PACKET_INVALID))
>   				break;
>   		}
Bill Fischofer Dec. 18, 2014, 9:27 p.m. UTC | #2 
The MMAP code here as far as I can tell is never called, so I'm not sure
how it gets used.  Perhaps Ciprian can comment.

For the RAW code, on RX the effect is we can never receive a packet larger
than one segment.  So there's no way to even notice we have a problem. If a
packet tries to create a large packet itself (without having first received
it) then only the first segment would TX, meaning it would be discarded by
whoever gets it as an incomplete/damaged packet.  I'd rather just accept
that as a restriction for now ("results are undefined") and figure out how
to address this properly.

On Thu, Dec 18, 2014 at 8:56 AM, Taras Kondratiuk <
taras.kondratiuk@linaro.org> wrote:
>
> On 12/18/2014 01:16 AM, Bill Fischofer wrote:
>
>> Add segmented packet I/O support via sockets. RAW sockets limited to
>> single segment.
>>
>> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
>> ---
>>   platform/linux-generic/odp_packet_socket.c | 33
>> +++++++++++++++++-------------
>>   1 file changed, 19 insertions(+), 14 deletions(-)
>>
>> diff --git a/platform/linux-generic/odp_packet_socket.c
>> b/platform/linux-generic/odp_packet_socket.c
>> index 2849065..340da88 100644
>> --- a/platform/linux-generic/odp_packet_socket.c
>> +++ b/platform/linux-generic/odp_packet_socket.c
>> @@ -321,7 +321,8 @@ int recv_pkt_sock_basic(pkt_sock_t *const pkt_sock,
>>
>>         for (i = 0; i < len; i++) {
>>                 if (odp_likely(pkt == ODP_PACKET_INVALID)) {
>> -                       pkt = _odp_packet_alloc(pkt_sock->pool);
>> +                       pkt = odp_packet_alloc(pkt_sock->pool,
>> +                                              pkt_sock->max_frame_len);
>>
>
> In seems only MMAP type supports segmented buffers as it uses
> odp_packet_copydata_*() API. RAW and MMSG operate only with a first
> segment. Should we add assert here to check that packet is unsegmented?
>
>
>                          if (odp_unlikely(pkt == ODP_PACKET_INVALID))
>>                                 break;
>>                 }
>>
>
>
Bill Fischofer Dec. 19, 2014, 1:37 p.m. UTC | #3 
OK, I didn't see anything in odp_packet_socket.c that would suggest that
the MMAP code is actually being used.  If it is, then we have proper
segmented packet I/O support in place in linux-generic and folks can play
around with it by simply making adjustments to
ODP_CONFIG_PACKET_BUF_LEN_MIN/MAX.

Regarding the MMAP frame size, that should also be controlled by
ODP_CONFIG_PACKET_BUF_LEN_MAX.  Stuart, is that something you'd like to
look into?

On Fri, Dec 19, 2014 at 4:00 AM, Stuart Haslam <stuart.haslam@arm.com>
wrote:
>
> On Wed, Dec 17, 2014 at 11:16:25PM +0000, Bill Fischofer wrote:
> > Add segmented packet I/O support via sockets. RAW sockets limited to
> > single segment.
> >
> > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org>
> > ---
> >  platform/linux-generic/odp_packet_socket.c | 33
> +++++++++++++++++-------------
> >  1 file changed, 19 insertions(+), 14 deletions(-)
> >
> > diff --git a/platform/linux-generic/odp_packet_socket.c
> b/platform/linux-generic/odp_packet_socket.c
> > index 2849065..340da88 100644
> > --- a/platform/linux-generic/odp_packet_socket.c
> > +++ b/platform/linux-generic/odp_packet_socket.c
> > @@ -321,7 +321,8 @@ int recv_pkt_sock_basic(pkt_sock_t *const pkt_sock,
> >
> >       for (i = 0; i < len; i++) {
> >               if (odp_likely(pkt == ODP_PACKET_INVALID)) {
> > -                     pkt = _odp_packet_alloc(pkt_sock->pool);
> > +                     pkt = odp_packet_alloc(pkt_sock->pool,
> > +                                            pkt_sock->max_frame_len);
> >                       if (odp_unlikely(pkt == ODP_PACKET_INVALID))
> >                               break;
> >               }
> > @@ -339,7 +340,7 @@ int recv_pkt_sock_basic(pkt_sock_t *const pkt_sock,
> >                       continue;
> >
> >               /* Parse and set packet header data */
> > -             packet_set_len(pkt, recv_bytes);
> > +             odp_packet_pull_tail(pkt, pkt_sock->max_frame_len -
> recv_bytes);
> >               _odp_packet_parse(pkt);
> >
> >               pkt_table[nb_rx] = pkt;
> > @@ -418,7 +419,8 @@ int recv_pkt_sock_mmsg(pkt_sock_t *const pkt_sock,
> >       memset(msgvec, 0, sizeof(msgvec));
> >
> >       for (i = 0; i < (int)len; i++) {
> > -             pkt_table[i] = _odp_packet_alloc(pkt_sock->pool);
> > +             pkt_table[i] = odp_packet_alloc(pkt_sock->pool,
> > +                                             pkt_sock->max_frame_len);
> >               if (odp_unlikely(pkt_table[i] == ODP_PACKET_INVALID))
> >                       break;
> >
> > @@ -445,7 +447,9 @@ int recv_pkt_sock_mmsg(pkt_sock_t *const pkt_sock,
> >               }
> >
> >               /* Parse and set packet header data */
> > -             packet_set_len(pkt_table[i], msgvec[i].msg_len);
> > +             odp_packet_pull_tail(pkt_table[i],
> > +                                  pkt_sock->max_frame_len -
> > +                                  msgvec[i].msg_len);
> >               _odp_packet_parse(pkt_table[i]);
> >
> >               pkt_table[nb_rx] = pkt_table[i];
> > @@ -566,7 +570,6 @@ static inline unsigned pkt_mmap_v2_rx(int sock,
> struct ring *ring,
> >       uint8_t *pkt_buf;
> >       int pkt_len;
> >       struct ethhdr *eth_hdr;
> > -     uint8_t *l2_hdr;
> >       unsigned i = 0;
> >
> >       (void)sock;
> > @@ -591,13 +594,15 @@ static inline unsigned pkt_mmap_v2_rx(int sock,
> struct ring *ring,
> >                               continue;
> >                       }
> >
> > -                     pkt_table[i] = _odp_packet_alloc(pool);
> > +                     pkt_table[i] = odp_packet_alloc(pool, pkt_len);
> >                       if (odp_unlikely(pkt_table[i] ==
> ODP_PACKET_INVALID))
> >                               break;
> >
> > -                     packet_set_len(pkt_table[i], pkt_len);
> > -                     l2_hdr = odp_packet_data(pkt_table[i]);
> > -                     memcpy(l2_hdr, pkt_buf, pkt_len);
> > +                     if (odp_packet_copydata_in(pkt_table[i], 0,
> > +                                                pkt_len, pkt_buf) != 0)
> {
> > +                             odp_packet_free(pkt_table[i]);
> > +                             break;
> > +                     }
>
> By the way I think this also resolves this issue;
>
> http://lists.linaro.org/pipermail/lng-odp/2014-December/005811.html
>
> Well at least it should avoid the crash.
>
> --
> Stuart.
>
>
diff mbox

Patch

diff --git a/platform/linux-generic/odp_packet_socket.c b/platform/linux-generic/odp_packet_socket.c
index 2849065..340da88 100644
--- a/platform/linux-generic/odp_packet_socket.c
+++ b/platform/linux-generic/odp_packet_socket.c
@@ -321,7 +321,8 @@  int recv_pkt_sock_basic(pkt_sock_t *const pkt_sock,
 
 	for (i = 0; i < len; i++) {
 		if (odp_likely(pkt == ODP_PACKET_INVALID)) {
-			pkt = _odp_packet_alloc(pkt_sock->pool);
+			pkt = odp_packet_alloc(pkt_sock->pool,
+					       pkt_sock->max_frame_len);
 			if (odp_unlikely(pkt == ODP_PACKET_INVALID))
 				break;
 		}
@@ -339,7 +340,7 @@  int recv_pkt_sock_basic(pkt_sock_t *const pkt_sock,
 			continue;
 
 		/* Parse and set packet header data */
-		packet_set_len(pkt, recv_bytes);
+		odp_packet_pull_tail(pkt, pkt_sock->max_frame_len - recv_bytes);
 		_odp_packet_parse(pkt);
 
 		pkt_table[nb_rx] = pkt;
@@ -418,7 +419,8 @@  int recv_pkt_sock_mmsg(pkt_sock_t *const pkt_sock,
 	memset(msgvec, 0, sizeof(msgvec));
 
 	for (i = 0; i < (int)len; i++) {
-		pkt_table[i] = _odp_packet_alloc(pkt_sock->pool);
+		pkt_table[i] = odp_packet_alloc(pkt_sock->pool,
+						pkt_sock->max_frame_len);
 		if (odp_unlikely(pkt_table[i] == ODP_PACKET_INVALID))
 			break;
 
@@ -445,7 +447,9 @@  int recv_pkt_sock_mmsg(pkt_sock_t *const pkt_sock,
 		}
 
 		/* Parse and set packet header data */
-		packet_set_len(pkt_table[i], msgvec[i].msg_len);
+		odp_packet_pull_tail(pkt_table[i],
+				     pkt_sock->max_frame_len -
+				     msgvec[i].msg_len);
 		_odp_packet_parse(pkt_table[i]);
 
 		pkt_table[nb_rx] = pkt_table[i];
@@ -566,7 +570,6 @@  static inline unsigned pkt_mmap_v2_rx(int sock, struct ring *ring,
 	uint8_t *pkt_buf;
 	int pkt_len;
 	struct ethhdr *eth_hdr;
-	uint8_t *l2_hdr;
 	unsigned i = 0;
 
 	(void)sock;
@@ -591,13 +594,15 @@  static inline unsigned pkt_mmap_v2_rx(int sock, struct ring *ring,
 				continue;
 			}
 
-			pkt_table[i] = _odp_packet_alloc(pool);
+			pkt_table[i] = odp_packet_alloc(pool, pkt_len);
 			if (odp_unlikely(pkt_table[i] == ODP_PACKET_INVALID))
 				break;
 
-			packet_set_len(pkt_table[i], pkt_len);
-			l2_hdr = odp_packet_data(pkt_table[i]);
-			memcpy(l2_hdr, pkt_buf, pkt_len);
+			if (odp_packet_copydata_in(pkt_table[i], 0,
+						   pkt_len, pkt_buf) != 0) {
+				odp_packet_free(pkt_table[i]);
+				break;
+			}
 
 			mmap_rx_user_ready(ppd.raw);
 
@@ -620,7 +625,6 @@  static inline unsigned pkt_mmap_v2_tx(int sock, struct ring *ring,
 				      odp_packet_t pkt_table[], unsigned len)
 {
 	union frame_map ppd;
-	uint8_t *pkt_buf;
 	uint32_t pkt_len;
 	unsigned frame_num, next_frame_num;
 	int ret;
@@ -634,13 +638,14 @@  static inline unsigned pkt_mmap_v2_tx(int sock, struct ring *ring,
 
 			next_frame_num = (frame_num + 1) % ring->rd_num;
 
-			pkt_buf = odp_packet_l2_ptr(pkt_table[i], &pkt_len);
-
+			pkt_len = odp_packet_len(pkt_table[i]);
 			ppd.v2->tp_h.tp_snaplen = pkt_len;
 			ppd.v2->tp_h.tp_len = pkt_len;
 
-			memcpy((uint8_t *)ppd.raw + TPACKET2_HDRLEN -
-			       sizeof(struct sockaddr_ll), pkt_buf, pkt_len);
+			odp_packet_copydata_out(pkt_table[i], 0, pkt_len,
+						(uint8_t *)ppd.raw +
+						TPACKET2_HDRLEN -
+						sizeof(struct sockaddr_ll));
 
 			mmap_tx_user_ready(ppd.raw);