Thread: [RTnet-developers] Patches for SOCKET_RAW and ETH_P_ALL
Brought to you by:
bet-frogger,
kiszka
Menu
▾
▴
rtnet-developers
|
From: Jorge A. <j-a...@cr...> - 2006-09-11 10:25:02
|
JAN, In attach goes the patches for the SOCKET_RAW and ETH_P_ALL functionalities for RTnet. The only thing that was not done was the configurable option for the ETH_P_ALL. After some tests in the same computer, with two different cards, i find something interesting that i need to talk with you (or anybody that that wants to participate in the discution). I have two small test programs, one for sending, and another one to receive the message. In both there's one socket opened with the ETH_P_ALL feature. Making two different filedescriptors in the same machine. After i send the message it is sended for both sockets (REmenber that we are in the same machine), but i only read from one. This means, after some time the queue for the opened socket used to write only (not read) will fill. Wath is the behaviour for this problem? This queue should fill and start to give errors when no more memory is available? Or we should not send the message to one sockt thas is only writing? How can we know if the socket is only writting? Or we can set a maximum value to the queued message in one determined queue? (I think this is the best option) Please comment. Greetings. -- Jorge Almeida j-a...@cr... DISCLAIMER: This message may contain confidential information or privileged material and is intended only for the individual(s) named. If you are not a named addressee and mistakenly received this message you should not copy or otherwise disseminate it: please delete this e-mail from your system and notify the sender immediately. E-mail transmissions are not guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete or contain viruses. Therefore, the sender does not accept liability for any errors or omissions in the contents of this message that arise as a result of e-mail transmissions. Please request a hard copy version if verification is required. Critical Software, SA. |
|
From: Jorge A. <j-a...@cr...> - 2006-09-11 10:42:15
Attachments:
patches.tar.bz2
|
OPS Sorry :) There it goes :) JAN, In attach goes the patches for the SOCKET_RAW and ETH_P_ALL functionalities for RTnet. The only thing that was not done was the configurable option for the ETH_P_ALL. After some tests in the same computer, with two different cards, i find something interesting that i need to talk with you (or anybody that that wants to participate in the discution). I have two small test programs, one for sending, and another one to receive the message. In both there's one socket opened with the ETH_P_ALL feature. Making two different filedescriptors in the same machine. After i send the message it is sended for both sockets (REmenber that we are in the same machine), but i only read from one. This means, after some time the queue for the opened socket used to write only (not read) will fill. Wath is the behaviour for this problem? This queue should fill and start to give errors when no more memory is available? Or we should not send the message to one sockt thas is only writing? How can we know if the socket is only writting? Or we can set a maximum value to the queued message in one determined queue? (I think this is the best option) Please comment. Greetings. -- Jorge Almeida j-a...@cr... DISCLAIMER: This message may contain confidential information or privileged material and is intended only for the individual(s) named. If you are not a named addressee and mistakenly received this message you should not copy or otherwise disseminate it: please delete this e-mail from your system and notify the sender immediately. E-mail transmissions are not guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete or contain viruses. Therefore, the sender does not accept liability for any errors or omissions in the contents of this message that arise as a result of e-mail transmissions. Please request a hard copy version if verification is required. Critical Software, SA. |
|
From: Jan K. <jan...@we...> - 2006-09-11 10:51:59
Attachments:
signature.asc
|
Jorge Almeida wrote: > OPS Sorry :) >=20 > There it goes :) >=20 >=20 > JAN, >=20 > In attach goes the patches for the SOCKET_RAW and ETH_P_ALL functionali= ties for RTnet. > The only thing that was not done was the configurable option for the ET= H_P_ALL.=20 >=20 Thanks, will have a look at them later. >=20 > After some tests in the same computer, with two different cards, i find= something interesting that i need to talk with you (or anybody that that= wants to participate in the discution). Both NICs attached to the same network? > I have two small test programs, one for sending, and another one to rec= eive the message. In both there's one socket opened with the ETH_P_ALL fe= ature. Making two different filedescriptors in the same machine. > After i send the message it is sended for both sockets (REmenber that w= e are in the same machine), but i only read from one.=20 > This means, after some time the queue for the opened socket used to wri= te only (not read) will fill.=20 > Wath is the behaviour for this problem? Buffers for TX and RX are taken from the same pool. Thus, once incoming unhandled packets consumed all of them, TX will fail. >=20 > This queue should fill and start to give errors when no more memory is = available? > Or we should not send the message to one sockt thas is only writing? Ho= w can we know if the socket is only writting? > Or we can set a maximum value to the queued message in one determined q= ueue? (I think this is the best option) Simple solution: if you have a send-only socket, don't open it with ETH_P_ALL. Use some arbitrary unused protocol instead. This also helps to reduce the system load due to unnecessary packet delivery via costly cloning. Jan |
|
From: Jan K. <jan...@we...> - 2006-09-11 17:36:14
Attachments:
signature.asc
|
Jorge Almeida wrote: > OK. >=20 > I checkout the SVN and made the diff (it's in attach). Sigh, your patch reverts a bulk of changes I made over the last weeks. This will need some cleanup first. What about the test cases? Would be very helpful now to check if things work as expected after the refactoring. >=20 > The RAW_SOCKET and ETH_P_ALL features are already mixed in the code. I = must return to an early stage to generate two different patches. > But i send you a pach only for RAW_SOCKET some time ago (28-08-2006). T= hat code didn't change. Despite my remarks? Mmh... >=20 >=20 > Regarding the other question in the last e-mail, about the queue behavi= our, i can only send 16 messages, after that the send function return -10= 5 for error code for every message that i try to send. > Error 105 - No Buffer Space Available.=20 > Even using 0x1234 has protocol type to send the messages. > But using this type of protocol the messages are only sended to the rea= ding buffer, and not to the writting too. Let me check if I understood this correctly: Although there cannot arrive any packets to the send-only socket now, it still runs out of buffers? Can you reproduce such problem over the loopback device as well? I'm asking as the best thing to analyse this issue now is to have a free-standing test case. Jan |
|
From: Jan K. <jan...@we...> - 2006-09-11 19:56:18
Attachments:
signature.asc
|
Hi Jorge,
some review on your latest patch.
Jorge Almeida wrote:
> -----------------------------------------------------------------------=
-
>=20
> Index: stack/include/rtnet_socket.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stack/include/rtnet_socket.h (revision 1060)
> +++ stack/include/rtnet_socket.h (working copy)
> @@ -48,14 +48,14 @@
> =20
> rtdm_lock_t param_lock;
> =20
> - unsigned int priority;
> - nanosecs_rel_t timeout; /* receive timeout, 0 for infi=
nite */
> + volatile unsigned int priority;
> + nanosecs_t timeout; /* receive timeout, 0 for infi=
nite */
Please kill those downgrades to a pre-0.9.5 era.
> =20
> rtdm_sem_t pending_sem;
> #ifdef CONFIG_RTNET_RTDM_SELECT
> wait_queue_primitive_t *wakeup_select; /* for selecting calls - t=
his
> - SHOULD be the head of a=
wait
> - queue mechanism. */
> + SHOULD be the head of a wait
> + queue mechanism. */
Whitespace changes ought to be avoided.
> #endif /* CONFIG_RTNET_RTDM_SELECT*/
> =20
> void (*callback_func)(struct rtdm_dev_context *=
,
> @@ -78,7 +78,7 @@
> /* packet socket specific */
> struct {
> struct rtpacket_type packet_type;
> - int ifindex;
> + volatile int ifindex;
Please update.
> } packet;
> } prot;
> };
> @@ -104,4 +104,7 @@
> rtdm_user_info_t *user_info,
> int request, void *arg);
> =20
> +#define PACKET_SOCKET "PACKET_SOCKET"
> +#define PACKET_SOCKET_LENGTH 13
> +
Mmh, that's part of a story I do not fully understand yet.
> #endif /* __RTNET_SOCKET_H_ */
> Index: stack/include/rtskb.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stack/include/rtskb.h (revision 1060)
> +++ stack/include/rtskb.h (working copy)
> @@ -166,12 +166,12 @@
> struct rtsocket *sk; /* assigned socket */
> struct rtnet_device *rtdev; /* source or destination device */=
> =20
> - nanosecs_abs_t time_stamp; /* arrival or transmission (RTcap)=
time */
> + nanosecs_t time_stamp; /* arrival or transmission (RTcap)=
time */
> =20
> /* patch address of the transmission time stamp, can be NULL
> * calculation: *xmit_stamp =3D cpu_to_be64(time_in_ns + *xmit_sta=
mp)
> */
> - nanosecs_abs_t *xmit_stamp;
> + nanosecs_t *xmit_stamp;
> =20
> /* transport layer */
> union
> @@ -223,7 +223,7 @@
> struct rtskb *cap_next; /* used for capture queue =
*/
> unsigned char *cap_start; /* start offset for capturing =
*/
> unsigned int cap_len; /* capture length of this rtskb =
*/
> - nanosecs_abs_t cap_rtmac_stamp; /* RTmac enqueuing time =
*/
> + nanosecs_t cap_rtmac_stamp; /* RTmac enqueuing time =
*/
> #endif
> };
> =20
To be killed.
> @@ -728,6 +728,7 @@
> extern unsigned int rtskb_pool_shrink_rt(struct rtskb_queue *pool,
> unsigned int rem_rtskbs);
> extern int rtskb_acquire(struct rtskb *rtskb, struct rtskb_queue *comp=
_pool);
> +extern int rtskb_clone(struct rtskb *rtskb_1,struct rtskb *rtskb_2,str=
uct rtskb_queue *pool);
> =20
> extern int rtskb_pools_init(void);
> extern void rtskb_pools_release(void);
> Index: stack/stack_mgr.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stack/stack_mgr.c (revision 1060)
> +++ stack/stack_mgr.c (working copy)
> @@ -27,6 +27,7 @@
> #include <rtnet_internal.h>
> #include <rtskb_fifo.h>
> #include <stack_mgr.h>
> +#include <rtnet_socket.h>
See my comment on the SOCK_RAW patch (fatal layering violation...)
> =20
> =20
> static unsigned int stack_mgr_prio =3D RTNET_DEF_STACK_PRIORITY;
> @@ -54,15 +55,13 @@
> int ret =3D 0;
> rtdm_lockctx_t context;
> =20
> -
> - if (pt->type =3D=3D htons(ETH_P_ALL))
> + if (likely(pt->type =3D=3D htons(ETH_P_ALL) && (strncmp(pt->name,P=
ACKET_SOCKET,PACKET_SOCKET_LENGTH) !=3D 0 )))
> return -EINVAL;
Please explain: what's the idea of this string comparison?
The name field in rtpacket_type is probably something we should rather
kill at this chance - it's only set but doesn't seem to be read anymore
(or never was actually).
> =20
> INIT_LIST_HEAD(&pt->list_entry);
> pt->refcount =3D 0;
> =20
> hash =3D ntohs(pt->type) & RTPACKET_HASH_KEY_MASK;
> -
> rtdm_lock_get_irqsave(&rt_packets_lock, context);
> list_add_tail(&pt->list_entry, &rt_packets[hash]);
> rtdm_lock_put_irqrestore(&rt_packets_lock, context);
> @@ -83,8 +82,7 @@
> =20
> =20
> RTNET_ASSERT(pt !=3D NULL, return -EINVAL;);
> -
> - if (pt->type =3D=3D htons(ETH_P_ALL))
> + if (likely(pt->type =3D=3D htons(ETH_P_ALL) && (strncmp(pt->name,P=
ACKET_SOCKET,PACKET_SOCKET_LENGTH) !=3D 0 )))
> return -EINVAL;
See above.
> =20
> rtdm_lock_get_irqsave(&rt_packets_lock, context);
> @@ -119,7 +117,6 @@
> rtdev_reference(rtdev);
> =20
> if (unlikely(rtskb_fifo_insert_inirq(&rx.fifo, skb) < 0)) {
> - rtdm_printk("RTnet: dropping packet in %s()\n", __FUNCTION__);=
Why? Maybe we can create some warn-once, but we need an alarm that the
system was misdesigned (too few buffers or too high load).
> kfree_rtskb(skb);
> rtdev_dereference(rtdev);
> }
> @@ -153,7 +150,6 @@
> struct rtnet_device *rtdev;
> int err;
> =20
> -
> while (rtdm_event_wait(mgr_event) =3D=3D 0)
> while (1) {
> next_packet:
> @@ -161,33 +157,46 @@
> skb =3D __rtskb_fifo_remove(&rx.fifo);
> if (!skb)
> break;
> -
> rtdev =3D skb->rtdev;
> =20
> rtcap_report_incoming(skb);
> =20
> skb->nh.raw =3D skb->data;
> =20
> + /*hash for the ETH_P_ALL packets*/
> + hash =3D ETH_P_ALL & RTPACKET_HASH_KEY_MASK;
> + rtdm_lock_get_irqsave(&rt_packets_lock, context);
> + /*for cycle that runs over all list entries to check if an=
yone has ETH_P_ALL for protocol type*/
> + list_for_each_entry(pt_entry, &rt_packets[hash], list_entr=
y)
It's better to keep ETH_P_ALL separated from normal subscribers. If some
protocol matches the hash key of ETH_P_ALL, we but that subscriber
without any needs into this hotpath.
> + {
> + if ( likely(ntohs(pt_entry->type) =3D=3D hash &&=20
> + (strncmp(pt_entry->name,PACKET_SOCKET,PACKET_SOCK=
ET_LENGTH) =3D=3D 0 )))
> + {
> + pt_entry->refcount++;
> + rtdm_lock_put_irqrestore(&rt_packets_lock, context=
);
> + err =3D pt_entry->handler(skb, pt_entry);
> + rtdm_lock_get_irqsave(&rt_packets_lock, context);
> + pt_entry->refcount--;
> + rtdm_lock_put_irqrestore(&rt_packets_lock, context=
);
The last lock_put is bogus, remove it.
> + }
> + }
> + rtdm_lock_put_irqrestore(&rt_packets_lock, context);
> + =20
> hash =3D ntohs(skb->protocol) & RTPACKET_HASH_KEY_MASK;
> -
> rtdm_lock_get_irqsave(&rt_packets_lock, context);
Don't drop and reacquire the same lock here. [And even if we go for a
separate list of ETH_P_ALL listeners: we should use the same lock for it
as well.]
> -
> + /*for cycle that runs over all list entries*/
> list_for_each_entry(pt_entry, &rt_packets[hash], list_entr=
y)
> if (pt_entry->type =3D=3D skb->protocol) {
> pt_entry->refcount++;
> rtdm_lock_put_irqrestore(&rt_packets_lock, context=
);
> -
> err =3D pt_entry->handler(skb, pt_entry);
> -
> rtdm_lock_get_irqsave(&rt_packets_lock, context);
> pt_entry->refcount--;
> rtdm_lock_put_irqrestore(&rt_packets_lock, context=
);
> -
> rtdev_dereference(rtdev);
> if (!err)
> goto next_packet;
> }
> -
Please kill all those code layout changes like the above. They exist to
improve the readability.
> rtdm_lock_put_irqrestore(&rt_packets_lock, context);
> =20
> /* don't warn if running in promiscuous mode (RTcap...?) *=
/
> Index: stack/rtskb.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stack/rtskb.c (revision 1060)
> +++ stack/rtskb.c (working copy)
> @@ -483,8 +483,34 @@
> return 0;
> }
> =20
> +/** Clone a rtskb to another, allocating a rtskb for the copy */
> +int rtskb_clone(struct rtskb *rtskb_1,struct rtskb *rtskb_2,struct rts=
kb_queue *pool)
Please rename, e.g. rtskb_1 -> rtskb_new, rtskb_2 -> rtskb.
> +{
> + if(rtskb_1 =3D=3D NULL)
> + return -ENOMEM;
Mmh, wouldn't it make sense to also allocate the rtskb here and return
the pointer? That's what Linux does as well. The prototype would then
look like this:
struct rtskb *rtskb_clone(struct rtskb *rtskb, struct rtskb_queue *pool)
> =20
> + rtskb_1->chain_end =3D rtskb_1;
Done on allocation - unless you manage rtskbs in your own secrete queue
(which you don't).
> + memcpy(rtskb_1->data, rtskb_2->data, rtskb_2->len );
That looks wrong. The original rtskb may have already forwarded its data
pointer past the MAC header, and the same must happen to the clone. You
have to account for the current data vs. the mac.raw, copy the full
block to the target, and then move the data pointer of the target
accordingly.
> + rtskb_1->len =3D rtskb_2->len;
> + rtskb_1->time_stamp =3D rtskb_2->time_stamp;
> + rtskb_1->rtdev =3D rtskb_2->rtdev;
> + rtskb_1->protocol =3D rtskb_2->protocol;
> + rtskb_1->pool =3D pool;
rtskb_1->pool is already valid and points to pool, see alloc_rtskb.
> + rtskb_1->pkt_type =3D rtskb_2->pkt_type;
> + rtskb_1->priority =3D rtskb_2->priority;
> + rtskb_1->ip_summed =3D rtskb_2->ip_summed;
> + rtskb_1->csum =3D rtskb_2->csum;
> + =20
> + /*copy ethernet stuff*/
> + rtskb_1->mac.raw =3D rtskb_1->data;
Likely bogus, see above.
> + memcpy(rtskb_1->mac.ethernet->h_dest,rtskb_2->mac.ethernet->h_dest=
, ETH_ALEN);
> + memcpy(rtskb_1->mac.ethernet->h_source,rtskb_2->mac.ethernet->h_so=
urce, ETH_ALEN);
See above.
> + rtskb_1->mac.ethernet->h_proto =3D rtskb_2->mac.ethernet->h_proto;=
To be really generic, you would also have to clone rtskb.h and .nh -
even if the original rtskb contains garbage in this regard.
> + =20
> + return 0;
> +}
> =20
> +
> int rtskb_pools_init(void)
> {
> rtskb_slab_pool =3D kmem_cache_create("rtskb_slab_pool",
> @@ -547,6 +573,7 @@
> EXPORT_SYMBOL(global_pool);
> =20
> EXPORT_SYMBOL(rtskb_acquire);
> +EXPORT_SYMBOL(rtskb_clone);
> =20
> #ifdef CONFIG_RTNET_CHECKED
> EXPORT_SYMBOL(rtskb_over_panic);
> Index: stack/packet/af_packet.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stack/packet/af_packet.c (revision 1060)
> +++ stack/packet/af_packet.c (working copy)
> @@ -43,31 +43,53 @@
> void (*callback_func)(struct rtdm_dev_context *, void *=
);
> void *callback_arg;
> rtdm_lockctx_t context;
> -
> -
> + struct rtskb *clone_skb =3D NULL;
> + unsigned short hash =3D 0;
> + int err =3D -1;
> + =20
> if (unlikely((ifindex !=3D 0) && (ifindex !=3D skb->rtdev->ifindex=
)))
> return EUNATCH;
> + =20
> + /*hash for the ETH_P_ALL packets*/
> + hash =3D ETH_P_ALL & RTPACKET_HASH_KEY_MASK;
> + if(likely(ntohs(pt->type) =3D=3D hash &&=20
> + (strncmp(pt->name,PACKET_SOCKET,PACKET_SOCKET_LENGTH) =3D=3D 0 =
)))
> + {
> + clone_skb =3D alloc_rtskb(skb->len,&sock->skb_pool);
> + if(clone_skb =3D=3D NULL) return -ENOMEM;
> + rtdm_lock_get_irqsave(&rt_packets_lock, context);
> + err =3D rtskb_clone(clone_skb, skb, &sock->skb_pool);
> + rtdm_lock_put_irqrestore(&rt_packets_lock, context);
What's the lock protecting?
> + }
> + else
> + {
> + err =3D rtskb_acquire(skb, &sock->skb_pool);
> + clone_skb =3D skb;
> + }
> =20
> - if (unlikely(rtskb_acquire(skb, &sock->skb_pool) !=3D 0))
> - kfree_rtskb(skb);
> - else {
> - rtdev_reference(skb->rtdev);
> - rtskb_queue_tail(&sock->incoming, skb);
> + if ( err !=3D 0)
> + {
> + kfree_rtskb(clone_skb);
> + }
> + else=20
> + {
> + rtdev_reference(clone_skb->rtdev);
> + rtskb_queue_tail(&sock->incoming, clone_skb);
> + =20
> rtdm_sem_up(&sock->pending_sem);
> =20
> rtdm_lock_get_irqsave(&sock->param_lock, context);
> callback_func =3D sock->callback_func;
> callback_arg =3D sock->callback_arg;
> rtdm_lock_put_irqrestore(&sock->param_lock, context);
> -
> if (callback_func)
> + {
> callback_func(rt_socket_context(sock), callback_arg);
> + }
> }
> return 0;
> }
> =20
> -
> -
> /***
> * rt_packet_bind
> */
> @@ -89,7 +111,7 @@
> =20
> rtdm_lock_get_irqsave(&sock->param_lock, context);
> =20
> - /* release exisiting binding */
> + /* release existing binding */
That's an unrelated fix I accept. :)
> if ((pt->type !=3D 0) && ((ret =3D rtdev_remove_pack(pt)) < 0)) {
> rtdm_lock_put_irqrestore(&sock->param_lock, context);
> return ret;
> @@ -100,7 +122,7 @@
> =20
> /* if protocol is non-zero, register the packet type */
> if (sock->protocol !=3D 0) {
> - pt->name =3D "PACKET_SOCKET";
> + pt->name =3D PACKET_SOCKET;
> pt->handler =3D rt_packet_rcv;
> pt->err_handler =3D NULL;
> =20
> @@ -169,13 +191,13 @@
> =20
> if ((ret =3D rt_socket_init(sockctx)) !=3D 0)
> return ret;
> -
> + =20
> sock->prot.packet.packet_type.type =3D protocol;
> sock->prot.packet.ifindex =3D 0;
> =20
> /* if protocol is non-zero, register the packet type */
> if (protocol !=3D 0) {
> - sock->prot.packet.packet_type.name =3D "PACKET_SOCKET";=
> + sock->prot.packet.packet_type.name =3D PACKET_SOCKET;
> sock->prot.packet.packet_type.handler =3D rt_packet_rcv;
> sock->prot.packet.packet_type.err_handler =3D NULL;
> =20
> @@ -271,13 +293,12 @@
> struct ethhdr *eth;
> struct sockaddr_ll *sll;
> int ret;
> - nanosecs_abs_t timeout =3D sock->timeout;
> + nanosecs_t timeout =3D sock->timeout;
> =20
> -
> /* non-blocking receive? */
> if (testbits(msg_flags, MSG_DONTWAIT))
> timeout =3D -1;
> -
> + =20
> ret =3D rtdm_sem_timeddown(&sock->pending_sem, timeout, NULL);
> if (unlikely(ret < 0)) {
> if ((ret !=3D -EWOULDBLOCK) && (ret !=3D -ETIMEDOUT))
> @@ -287,9 +308,7 @@
> =20
> skb =3D rtskb_dequeue_chain(&sock->incoming);
> RTNET_ASSERT(skb !=3D NULL, return -EFAULT;);
> -
> eth =3D skb->mac.ethernet;
> -
> sll =3D msg->msg_name;
> =20
> /* copy the address */
> @@ -313,10 +332,38 @@
> copy_len =3D len;
> msg->msg_flags |=3D MSG_TRUNC;
> }
> + =20
> + if(sockctx->device->socket_type =3D=3D SOCK_DGRAM)
> + {
> + /* copy the data */
> + rt_memcpy_tokerneliovec(msg->msg_iov, skb->data, copy_len);
> + }
> + else if (sockctx->device->socket_type =3D=3D SOCK_RAW)
> + {
> + int nBufferLen =3D 0;
> + unsigned char puchBuffer[ETH_FRAME_LEN] =3D {0};
> + =20
> + memcpy((puchBuffer+nBufferLen), eth->h_dest,ETH_ALEN);
> + nBufferLen +=3D ETH_ALEN;
> =20
> - /* copy the data */
> - rt_memcpy_tokerneliovec(msg->msg_iov, skb->data, copy_len);
> + memcpy((puchBuffer+nBufferLen), eth->h_source,ETH_ALEN);
> + nBufferLen +=3D ETH_ALEN;
> =20
> + memcpy((puchBuffer+nBufferLen), &(eth->h_proto),2);
> + nBufferLen +=3D 2;
> +
> + memcpy((puchBuffer+nBufferLen), skb->data, copy_len);
> + nBufferLen +=3D copy_len;
> +
> + /* copy the data to user */ =20
> + rt_memcpy_tokerneliovec(msg->msg_iov, puchBuffer, nBufferLen);=
> + /* new buffer length*/
> + real_len =3D nBufferLen;
> + }
> + else
> + {
> + rtdm_printk("Packet: Socket type Not Supported\n");
> + }
> if ((msg_flags & MSG_PEEK) =3D=3D 0) {
> rtdev_dereference(skb->rtdev);
> kfree_rtskb(skb);
> @@ -324,7 +371,6 @@
> rtskb_queue_head(&sock->incoming, skb);
> rtdm_sem_up(&sock->pending_sem);
> }
> -
> return real_len;
> }
> =20
> @@ -383,13 +429,34 @@
> =20
> rtskb->rtdev =3D rtdev;
> rtskb->priority =3D sock->priority;
> -
> - if (rtdev->hard_header) {
> - ret =3D rtdev->hard_header(rtskb, rtdev, ntohs(sll->sll_protoc=
ol),
> - sll->sll_addr, rtdev->dev_addr, rtskb=
->len);
> - if (ret < 0)
> - goto err;
> + // SOCK_DGRAM
> + if(sockctx->device->socket_type =3D=3D SOCK_DGRAM)
> + {
> + if (rtdev->hard_header) {
> + ret =3D rtdev->hard_header(rtskb, rtdev, ntohs(sll->sll_pr=
otocol),
> + sll->sll_addr, rtdev->dev_addr, rt=
skb->len);
> + if (ret < 0)
> + goto err;
> + }
> }
> + // SOCK_RAW
> + else if (sockctx->device->socket_type =3D=3D SOCK_RAW)
> + {
> + unsigned char puchBuffer[ETH_FRAME_LEN];
> + if (rtdev->hard_header) {
> + ret =3D rtdev->hard_header(rtskb, rtdev, (unsigned short)*=
(rtskb->data+ETH_ALEN+ETH_ALEN),
> + sll->sll_addr, (rtskb->data+ETH_A=
LEN), (rtskb->len - ETH_HLEN));
> + if (ret < 0)
> + goto err;
> + }
> + memcpy(puchBuffer,rtskb->data,rtskb->len);
> + memcpy(rtskb->data,(puchBuffer+ETH_HLEN),(rtskb->len - ETH_HLE=
N));
> + rtskb->len =3D rtskb->len - ETH_HLEN;
> + }
> + else
> + {
> + rtdm_printk("Packet: Socket type Not Supported\n");
> + }
Regarding all this above: see earlier review.
> =20
> if ((rtdev->flags & IFF_UP) !=3D 0) {
> if ((ret =3D rtdev_xmit(rtskb)) =3D=3D 0)
> @@ -409,8 +476,6 @@
> return ret;
> }
> =20
> -
> -
> static struct rtdm_device packet_proto_dev =3D {
> struct_version: RTDM_DEVICE_STRUCT_VER,
> =20
> @@ -430,7 +495,7 @@
> ioctl_nrt: rt_packet_ioctl,
> recvmsg_rt: rt_packet_recvmsg,
> sendmsg_rt: rt_packet_sendmsg
> - },
> +},
> =20
> device_class: RTDM_CLASS_NETWORK,
> device_sub_class: RTDM_SUBCLASS_RTNET,
> @@ -443,15 +508,55 @@
> };
> =20
> =20
> +static struct rtdm_device raw_packet_proto_dev =3D {
> + struct_version: RTDM_DEVICE_STRUCT_VER,
> +
> + device_flags: RTDM_PROTOCOL_DEVICE,
> + context_size: sizeof(struct rtsocket),
> +
> + protocol_family: PF_PACKET,
> + socket_type: SOCK_RAW,
> +
> + socket_rt: rt_packet_socket,
> + socket_nrt: rt_packet_socket,
> +
> + ops: {
> + close_rt: rt_packet_close,
> + close_nrt: rt_packet_close,
> + ioctl_rt: rt_packet_ioctl,
> + ioctl_nrt: rt_packet_ioctl,
> + recvmsg_rt: rt_packet_recvmsg,
> + sendmsg_rt: rt_packet_sendmsg
> + },
> +
> + device_class: RTDM_CLASS_NETWORK,
> + device_sub_class: RTDM_SUBCLASS_RTNET,
> + driver_name: "rtpacket",
> + driver_version: RTNET_RTDM_VER,
> + peripheral_name: "Real-Time Packet Socket Interface",
> + provider_name: rtnet_rtdm_provider_name,
> +
> + proc_name: "PACKET_RAW"
> +};
> +
> +
> int __init rt_packet_proto_init(void)
> {
> - return rtdm_dev_register(&packet_proto_dev);
> + int nReturn =3D -1;
> + nReturn =3D rtdm_dev_register(&packet_proto_dev);
> + if(nReturn !=3D 0)
> + rtdm_printk("Error registing DGRAM socket. Return =3D %d\n",nRetu=
rn);
> + nReturn =3D rtdm_dev_register(&raw_packet_proto_dev);
> + if(nReturn !=3D 0)
> + rtdm_printk("Error registing RAW socket. Return =3D %d\n",nRet=
urn);
> + return 0;
See earlier review.
> }
> =20
> =20
> void rt_packet_proto_release(void)
> {
> rtdm_dev_unregister(&packet_proto_dev, 1000);
> + rtdm_dev_unregister(&raw_packet_proto_dev, 1000);
> }
> =20
> =20
Jan
|
|
From: Jorge A. <j-a...@cr...> - 2006-09-12 13:56:33
Attachments:
raw_example_rtnet.tar.bz2
|
Jan, In attach follows the test cases for the SOCK_RAW and ETH_P_ALL features. Its two files written in C code. One for sending messages, and other to read they using the ETH_P_ALL feature. Please test the patch i send to you, after the refactoring that you need to do, and comment. The problem about the sending messages still happen, i only can send 16 messages. Please check if you have the same problem in one of your machines. In the loopback device i can send several messages (my test send 1000) but none is received in the rcv program. The loopback device works in a different way? Em Segunda, 11 de Setembro de 2006 18:36, o Jan Kiszka escreveu: > Jorge Almeida wrote: > > OK. > > > > I checkout the SVN and made the diff (it's in attach). > > Sigh, your patch reverts a bulk of changes I made over the last weeks. > This will need some cleanup first. > > What about the test cases? Would be very helpful now to check if things > work as expected after the refactoring. > > > > > The RAW_SOCKET and ETH_P_ALL features are already mixed in the code. I must return to an early stage to generate two different patches. > > But i send you a pach only for RAW_SOCKET some time ago (28-08-2006). That code didn't change. > > Despite my remarks? Mmh... > > > > > > > Regarding the other question in the last e-mail, about the queue behaviour, i can only send 16 messages, after that the send function return -105 for error code for every message that i try to send. > > Error 105 - No Buffer Space Available. > > Even using 0x1234 has protocol type to send the messages. > > But using this type of protocol the messages are only sended to the reading buffer, and not to the writting too. > > Let me check if I understood this correctly: Although there cannot > arrive any packets to the send-only socket now, it still runs out of > buffers? Can you reproduce such problem over the loopback device as > well? I'm asking as the best thing to analyse this issue now is to have > a free-standing test case. > > Jan > > -- Jorge Almeida j-a...@cr... DISCLAIMER: This message may contain confidential information or privileged material and is intended only for the individual(s) named. If you are not a named addressee and mistakenly received this message you should not copy or otherwise disseminate it: please delete this e-mail from your system and notify the sender immediately. E-mail transmissions are not guaranteed to be secure or error-free as information could be intercepted, corrupted, lost, destroyed, arrive late or incomplete or contain viruses. Therefore, the sender does not accept liability for any errors or omissions in the contents of this message that arise as a result of e-mail transmissions. Please request a hard copy version if verification is required. Critical Software, SA. |
|
From: Jan K. <jan...@we...> - 2006-09-12 17:31:16
Attachments:
signature.asc
|
Jorge Almeida wrote: > Jan, >=20 > In attach follows the test cases for the SOCK_RAW and ETH_P_ALL feature= s. Its two files written in C code. > One for sending messages, and other to read they using the ETH_P_ALL fe= ature. Thanks, will have a look. >=20 > Please test the patch i send to you, after the refactoring that you nee= d to do, and comment. Well, if you read my comment, this is first of all about fixing your patch. Therefore I would appreciate some comments on my findings as well (disagree, ack, answers to my questions...). Even better would be an updated version that is at least correct wrt/ rtskb_clone and resolves the layering issue between stack_mgr.c and af_packet.c. >=20 > The problem about the sending messages still happen, i only can send 16= messages. Please check if you have the same problem in one of your machi= nes. >=20 > In the loopback device i can send several messages (my test send 1000) = but none is received in the rcv program.=20 > The loopback device works in a different way? Yeah, loopback device is likely no good idea as it by-passes the stack manager, see the code. That driver actually needs to be included in the ETH_P_ALL patch as well, but we could do this later by extracting the common features of the stack manager loop and the loopback driver into one common function. Jan |
|
From: Jan K. <jan...@we...> - 2006-09-14 16:20:35
Attachments:
signature.asc
|
Jorge Almeida wrote:
> Hi Jan
> Only now i have time to look at your sugestions.
>=20
> I make some comments below and send a new patch against SVN HEAD for yo=
u to look and fix some things that i comment below.
>=20
=2E..
>>> Index: stack/stack_mgr.c
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- stack/stack_mgr.c (revision 1060)
>>> +++ stack/stack_mgr.c (working copy)
>>> @@ -27,6 +27,7 @@
>>> #include <rtnet_internal.h>
>>> #include <rtskb_fifo.h>
>>> #include <stack_mgr.h>
>>> +#include <rtnet_socket.h>
>> See my comment on the SOCK_RAW patch (fatal layering violation...)
>>
>>> =20
>>> =20
>>> static unsigned int stack_mgr_prio =3D RTNET_DEF_STACK_PRIORITY;
>>> @@ -54,15 +55,13 @@
>>> int ret =3D 0;
>>> rtdm_lockctx_t context;
>>> =20
>>> -
>>> - if (pt->type =3D=3D htons(ETH_P_ALL))
>>> + if (likely(pt->type =3D=3D htons(ETH_P_ALL) && (strncmp(pt->name=
,PACKET_SOCKET,PACKET_SOCKET_LENGTH) !=3D 0 )))
>>> return -EINVAL;
>> Please explain: what's the idea of this string comparison?
> The string comparison is to protect the socket type. Only the PACKET_SO=
CKET is prepared to clone the rtskb and proceed with the correct way of t=
hings.
> I don't know any other kind of identifying without mistakes the PACKET_=
SOCKET with the available information. Maybe you do??!!
> Please comment.
Check the existing code (e.g. [1]): sockctx->device->socket_type
>>> @@ -161,33 +157,46 @@
>>> skb =3D __rtskb_fifo_remove(&rx.fifo);
>>> if (!skb)
>>> break;
>>> -
>>> rtdev =3D skb->rtdev;
>>> =20
>>> rtcap_report_incoming(skb);
>>> =20
>>> skb->nh.raw =3D skb->data;
>>> =20
>>> + /*hash for the ETH_P_ALL packets*/
>>> + hash =3D ETH_P_ALL & RTPACKET_HASH_KEY_MASK;
>>> + rtdm_lock_get_irqsave(&rt_packets_lock, context);
>>> + /*for cycle that runs over all list entries to check if =
anyone has ETH_P_ALL for protocol type*/
>>> + list_for_each_entry(pt_entry, &rt_packets[hash], list_en=
try)
>> It's better to keep ETH_P_ALL separated from normal subscribers. If so=
me
>> protocol matches the hash key of ETH_P_ALL, we but that subscriber
>> without any needs into this hotpath.
> I don't understand this comment.
> Another list only with ETH_P_ALL sockets????????!!!!!!!!!!!!!!!!!!!
Exactly.
>>> -
>>> + /*for cycle that runs over all list entries*/
>>> list_for_each_entry(pt_entry, &rt_packets[hash], list_en=
try)
>>> if (pt_entry->type =3D=3D skb->protocol) {
>>> pt_entry->refcount++;
>>> rtdm_lock_put_irqrestore(&rt_packets_lock, conte=
xt);
>>> -
>>> err =3D pt_entry->handler(skb, pt_entry);
>>> -
>>> rtdm_lock_get_irqsave(&rt_packets_lock, context)=
;
>>> pt_entry->refcount--;
>>> rtdm_lock_put_irqrestore(&rt_packets_lock, conte=
xt);
>>> -
>>> rtdev_dereference(rtdev);
>>> if (!err)
>>> goto next_packet;
>>> }
>>> -
>> Please kill all those code layout changes like the above. They exist t=
o
>> improve the readability.
> Until the last patch, stable version, i think is better to keep it.
Don't understand your argumentation. The larger your patch becomes, the
more functionally irrelevant changes it contains, the harder it is to
maintain (and to review).
>>> Index: stack/rtskb.c
>>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>>> --- stack/rtskb.c (revision 1060)
>>> +++ stack/rtskb.c (working copy)
>>> @@ -483,8 +483,34 @@
>>> return 0;
>>> }
>>> =20
>>> +/** Clone a rtskb to another, allocating a rtskb for the copy */
>>> +int rtskb_clone(struct rtskb *rtskb_1,struct rtskb *rtskb_2,struct r=
tskb_queue *pool)
>> Please rename, e.g. rtskb_1 -> rtskb_new, rtskb_2 -> rtskb.
> Renamed
>>> +{
>>> + if(rtskb_1 =3D=3D NULL)
>>> + return -ENOMEM;
>> Mmh, wouldn't it make sense to also allocate the rtskb here and return=
>> the pointer? That's what Linux does as well. The prototype would then
>> look like this:
>>
>> struct rtskb *rtskb_clone(struct rtskb *rtskb, struct rtskb_queue *poo=
l)
>>
> DONE
>=20
>>> =20
>>> + rtskb_1->chain_end =3D rtskb_1;
>> Done on allocation - unless you manage rtskbs in your own secrete queu=
e
>> (which you don't).
>>
> DONE
>>> + memcpy(rtskb_1->data, rtskb_2->data, rtskb_2->len );
>> That looks wrong. The original rtskb may have already forwarded its da=
ta
>> pointer past the MAC header, and the same must happen to the clone. Yo=
u
>> have to account for the current data vs. the mac.raw, copy the full
>> block to the target, and then move the data pointer of the target
>> accordingly.
>>
>=20
> I don't understand correctly this. The mac.raw pointer has the all mess=
age? I thought that it contains only the Ethernet header!
mac.raw always points to the beginning of a frame (because one assumes
the mac layer adds its stuff to the frame head - fairly common...).
mac.eth is just a typecast to the Ethernet header, see the code!
Now to the new one (already quite short, that's a good sign :)):
> -----------------------------------------------------------------------=
-
>=20
> Index: stack/include/rtskb.h
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stack/include/rtskb.h (revision 1063)
> +++ stack/include/rtskb.h (working copy)
> @@ -728,6 +728,7 @@
> extern unsigned int rtskb_pool_shrink_rt(struct rtskb_queue *pool,
> unsigned int rem_rtskbs);
> extern int rtskb_acquire(struct rtskb *rtskb, struct rtskb_queue *comp=
_pool);
> +extern struct rtskb* rtskb_clone(struct rtskb *rtskb,struct rtskb_queu=
e *pool);
> =20
> extern int rtskb_pools_init(void);
> extern void rtskb_pools_release(void);
> Index: stack/stack_mgr.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stack/stack_mgr.c (revision 1063)
> +++ stack/stack_mgr.c (working copy)
> @@ -55,7 +55,7 @@
> rtdm_lockctx_t context;
> =20
> =20
> - if (pt->type =3D=3D htons(ETH_P_ALL))
> + if (likely(pt->type =3D=3D htons(ETH_P_ALL) && (strncmp(pt->name,"=
PACKET_SOCKET",13) !=3D 0 )))
Why do you think this should be limited to packet sockets? Anyone
subscribing to ETH_P_ALL would, of course, have to take care of the
implication this has just like you already do in af_packet.c. But this
sanity check makes no sense here...
> return -EINVAL;
> =20
> INIT_LIST_HEAD(&pt->list_entry);
> @@ -84,7 +84,7 @@
> =20
> RTNET_ASSERT(pt !=3D NULL, return -EINVAL;);
> =20
> - if (pt->type =3D=3D htons(ETH_P_ALL))
> + if (likely(pt->type =3D=3D htons(ETH_P_ALL) && (strncmp(pt->name,"=
PACKET_SOCKET",13) !=3D 0 )))
=2E..and here.
> return -EINVAL;
> =20
> rtdm_lock_get_irqsave(&rt_packets_lock, context);
> @@ -167,11 +167,26 @@
> rtcap_report_incoming(skb);
> =20
> skb->nh.raw =3D skb->data;
> -
> +/* */ =20
> + /*hash for the ETH_P_ALL packets*/
> + hash =3D ETH_P_ALL & RTPACKET_HASH_KEY_MASK;
> + rtdm_lock_get_irqsave(&rt_packets_lock, context);
> + /*for cycle that runs over all list entries to check if an=
yone has ETH_P_ALL for protocol type*/
> + list_for_each_entry(pt_entry, &rt_packets[hash], list_entr=
y)
> + {
> + if ( likely(ntohs(pt_entry->type) =3D=3D hash &&=20
> + (strncmp(pt_entry->name,"PACKET_SOCKET",13) =3D=3D=
0 )))
> + {
> + pt_entry->refcount++;
> + rtdm_lock_put_irqrestore(&rt_packets_lock, context=
);
> + err =3D pt_entry->handler(skb, pt_entry);
> + rtdm_lock_get_irqsave(&rt_packets_lock, context);
> + pt_entry->refcount--;
> + }
> + }
> +/* */
TODO: separate ETH_P_ALL subscriber list.
BTW, I just realised that you will need more patching below to avoid
massive "RTnet: no one cared for packet..." warnings if there are
ETH_P_ALL listeners but no one on the regular list for that specific
protocol. So, track if there was at least one ETH_P_ALL listener
accepting the frame and skip the warning in that case.
> hash =3D ntohs(skb->protocol) & RTPACKET_HASH_KEY_MASK;
> =20
> - rtdm_lock_get_irqsave(&rt_packets_lock, context);
> -
> list_for_each_entry(pt_entry, &rt_packets[hash], list_entr=
y)
> if (pt_entry->type =3D=3D skb->protocol) {
> pt_entry->refcount++;
> Index: stack/rtskb.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stack/rtskb.c (revision 1063)
> +++ stack/rtskb.c (working copy)
> @@ -483,8 +483,39 @@
> return 0;
> }
> =20
> +/** Clone a rtskb to another, allocating a rtskb for the copy */
> +struct rtskb* rtskb_clone(struct rtskb *rtskb,struct rtskb_queue *pool=
)
> +{
> + struct rtskb* clone_rtskb =3D NULL;
> + clone_rtskb =3D alloc_rtskb(rtskb->len,pool);
> + if(clone_rtskb =3D=3D NULL)
> + return clone_rtskb;
> + /* Please check this */
> + memcpy(clone_rtskb->data, rtskb->data, rtskb->len );
memcpy(clone_rtskb->data, rtskb->mac.raw,
rtskb->len + rtskb->data - rtskb->mac.raw);
> + =20
> + clone_rtskb->len =3D rtskb->len;
> + clone_rtskb->time_stamp =3D rtskb->time_stamp;
> + clone_rtskb->rtdev =3D rtskb->rtdev;
> + clone_rtskb->protocol =3D rtskb->protocol;
> =20
> + clone_rtskb->pkt_type =3D rtskb->pkt_type;
> + clone_rtskb->priority =3D rtskb->priority;
> + clone_rtskb->ip_summed =3D rtskb->ip_summed;
> + clone_rtskb->csum =3D rtskb->csum;
> + =20
> + /* Please check this */
> + /*copy ethernet stuff*/
> + clone_rtskb->mac.raw =3D clone_rtskb->data;
Yes, but you also have to issue:
__rtskb_pull(clone_rtskb, rtskb->data - rtskb->mac.raw);
This forwards clone_rtskb->data to the same relative position like
rtskb->data.
> + memcpy(clone_rtskb->mac.ethernet->h_dest,rtskb->mac.ethernet->h_de=
st, ETH_ALEN);
> + memcpy(clone_rtskb->mac.ethernet->h_source,rtskb->mac.ethernet->h_=
source, ETH_ALEN);
> + clone_rtskb->mac.ethernet->h_proto =3D rtskb->mac.ethernet->h_prot=
o;
Not needed with the full copy above.
> + =20
> + return clone_rtskb;
> +}
[No guarantee that me quick review didn't oversee some detail. This
cloning is certainly not trivial.]
> =20
> +
> +
> +
> int rtskb_pools_init(void)
> {
> rtskb_slab_pool =3D kmem_cache_create("rtskb_slab_pool",
> @@ -547,6 +578,7 @@
> EXPORT_SYMBOL(global_pool);
> =20
> EXPORT_SYMBOL(rtskb_acquire);
> +EXPORT_SYMBOL(rtskb_clone);
> =20
> #ifdef CONFIG_RTNET_CHECKED
> EXPORT_SYMBOL(rtskb_over_panic);
> Index: stack/packet/af_packet.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- stack/packet/af_packet.c (revision 1063)
> +++ stack/packet/af_packet.c (working copy)
> @@ -43,16 +43,34 @@
> void (*callback_func)(struct rtdm_dev_context *, void *=
);
> void *callback_arg;
> rtdm_lockctx_t context;
> + struct rtskb *clone_skb =3D NULL;
> + unsigned short hash =3D 0;
> + int err =3D 0;
> =20
> =20
> if (unlikely((ifindex !=3D 0) && (ifindex !=3D skb->rtdev->ifindex=
)))
> return EUNATCH;
> -
> - if (unlikely(rtskb_acquire(skb, &sock->skb_pool) !=3D 0))
> - kfree_rtskb(skb);
> + /*hash for the ETH_P_ALL packets*/
> + hash =3D ETH_P_ALL & RTPACKET_HASH_KEY_MASK;
> + if(likely(ntohs(pt->type) =3D=3D hash &&=20
Why not (pt->type =3D=3D htons(ETH_P_ALL))?
> + (strncmp(pt->name,"PACKET_SOCKET",13) =3D=3D 0 )))
Redundant, we are already in af_packet.c.
> + {
> + clone_skb =3D rtskb_clone(skb, &sock->skb_pool);
> + if( clone_skb =3D=3D NULL)
> + return 0;
I would prefer
skb =3D clone_skb;
so that...
> + }
> + else
> + {
> + err =3D rtskb_acquire(skb, &sock->skb_pool);
> + clone_skb =3D skb;
> + }
> + if ( err !=3D 0)
> + {
> + kfree_rtskb(clone_skb);
> + }
> else {
> - rtdev_reference(skb->rtdev);
> - rtskb_queue_tail(&sock->incoming, skb);
> + rtdev_reference(clone_skb->rtdev);
> + rtskb_queue_tail(&sock->incoming, clone_skb);
=2E..such changes are not required.
> rtdm_sem_up(&sock->pending_sem);
> =20
> rtdm_lock_get_irqsave(&sock->param_lock, context);
We are getting closer, much closer. :)
Jan
|
|
From: Jan K. <jan...@we...> - 2006-09-14 17:16:09
Attachments:
signature.asc
|
Jorge Almeida wrote: > New Patch. >=20 > Please check if everything is now OK. Well, the thrilling question is now if it works... Did you test it alread= y? If it does, the rest is about introducing the ETH_P_ALL subscriber list and coding style cleanups (alignments with the RTnet style). Jan |
Thanks for helping keep SourceForge clean.
X