Re: [RTnet-developers] Patches for SOCKET_RAW and ETH_P_ALL
Brought to you by:
bet-frogger,
kiszka
Menu
▾
▴
Re: [RTnet-developers] Patches for SOCKET_RAW and ETH_P_ALL
|
From: Jan K. <jan...@we...> - 2006-09-14 16:20:35
|
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
|
