Kurt Van Dijck wrote: > On Thu, Sep 10, 2009 at 04:07:00PM +0200, Wolfgang Grandegger wrote: >> Date: Thu, 10 Sep 2009 16:07:00 +0200 >> Subject: Re: [PATCH v3] ems_usb: CAN/USB driver for EMS CPC-USB/ARM7 CAN >> interfaces >> From: Wolfgang Grandegger <[email protected]> >> To: Kurt Van Dijck <[email protected]> >> CC: SocketCAN Core Mailing List <[email protected]>, >> Oliver Hartkopp <[email protected]> >> >> Kurt Van Dijck wrote: >>> On Thu, Sep 10, 2009 at 02:29:37PM +0200, Wolfgang Grandegger wrote: >>> [...] >>>>>>> Index: include/linux/can/dev.h >>>>>>> =================================================================== >>>>>>> --- include/linux/can/dev.h (Revision 1048) >>>>>>> +++ include/linux/can/dev.h (Arbeitskopie) >>>>>>> @@ -32,7 +32,7 @@ >>>>>>> /* >>>>>>> * CAN common private data >>>>>>> */ >>>>>>> -#define CAN_ECHO_SKB_MAX 4 >>>>>>> +#define CAN_ECHO_SKB_MAX 10 >>>>>>> >>>>>>> struct can_priv { >>>>>>> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23) >>>>>>> Index: include/socketcan/can/dev.h >>>>>>> =================================================================== >>>>>>> --- include/socketcan/can/dev.h (Revision 1048) >>>>>>> +++ include/socketcan/can/dev.h (Arbeitskopie) >>>>>>> @@ -32,7 +32,7 @@ >>>>>>> /* >>>>>>> * CAN common private data >>>>>>> */ >>>>>>> -#define CAN_ECHO_SKB_MAX 4 >>>>>>> +#define CAN_ECHO_SKB_MAX 10 >>>>>>> >>>>>>> struct can_priv { >>>>>>> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,23) >>>>>> For the moment I want to keep the old settings. We need to discuss how >>>>>> to handle this in a generic way. Either by increasing it to 8, let's >>>>>> say, or by providing a kernel config. Other opinions? >>>>> with regard to CAN_ECHO_SKB_MAX, I had the same problem in the softing >>>>> driver. >>>>> would some interface like this: >>>> [...] >>>>> +struct net_device *alloc_candev(int sizeof_priv, int echo_skb_max); >>>> This is a good solution if we want the driver to set the number of echo >>>> skb's. I would actually put the echo_skbs at the end of the >>>> netdev_priv(). Would it also be useful that the user decides via kernel >>>> configuration option? >>> 1) 'behind netdev_priv' >>> so a layout in memory: >>> [[can_priv]driver_priv][echo_skb[]] >> Yep. >> >>> this is possible, not breaking things. But still, you'd need a pointer >>> in from within can_priv, as the exact size of the priv is lost after the >>> constructor. so, it justs saves the heap overhead. >> Of course. The address of the echo skbs should be *close* to the other >> addresses used by the CAN network device. >> >>> It must then be allocated during alloc_candev(), and requires an extra >>> argument. I'm not sure how easily such change gets accepted in mainline. >> I don't see a real problem here. Other opinions? I'm going to prepare a >> patch now. > I was in the process of preparing a patch with alternate approach.
I started to implement the alloc_candev(int sizeof_priv, int echo_skb_max), which is quit simple. Any alternate approach is welcome, of course. >>> 2) 'kernel config option' >>> I assume that every device will have the same echo_skb_max then? >>> >>> Before choosing a value, a user needs to be informed very well, but I >>> can imagine that a user does not even understand the consequences, ie. I >>> have difficulties understanding the relation between echo_skb_max & URB >>> traffic in a driver _I didn't write_. How would I choose a good value? >>> >>> IMO, such things are very driver specific, and thus better put const in >>> the driver. But this requires a variable echo_skb_max from can_priv >>> scope. That why I put both echo_skb & echo_skb_max as members. >>> This addressess the problematic choice of a static echo_skb_max that >>> would otherwise be left to the end user. >>> >>> Please mind this does not address how to malloc such things (ie. after >>> netdev_priv, or seperate block in heap). >> The user may want to restrict the number of messages queued in hardware >> for the sake of latency. But as TX messages are queued in software as >> well and as there is not yet a facility to handle prioritized messages >> it's currently not really a strong argument. > ??? > echo_skb is a cache for the skb's queued in hardware. The more you can > queue in hardware, the bigger the echo_skb cache, not? > So, it's hardware dependent. > The ems_usb driver proved that 10 is a viable number for that driver. > SJA1000 _can_ only use 1. why allocate 10 for sja1000? That's what I > found hard to accept. Currently we use 4 echo skbs and for that amount it was not worth to implement a more sophisticated approach. So far, nobody proved that there is a benefit of using a higher number and for that reason I was not willing to change it. For devices with slow queuing, like USB I see some benefit and Sebastian provided even figures. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
