Re: Asynchronous crypto layer.

[Evgeniy Polyakov <johnpol@2ka.mipt.ru>]

[-- Attachment #1: Type: text/plain, Size: 5611 bytes --]

On Tue, 2004-12-14 at 01:56 -0500, James Morris wrote:
> On Mon, 1 Nov 2004, Evgeniy Polyakov wrote:
> 
> [Sorry for taking so long to get back to this].
> 
> > > c) Async engine: scheduling/batching/routing etc.
> > > 
> > > This seems to be the core of what you've developed so far.  I'm not sure 
> > > if we need pluggable load balancers.  How would the sysadmin select the 
> > > correct one?  The simpler this code is the better.
> > 
> > By following command:
> > echo -en "simple_lb" > /sys/class/crypto_lb/lbs
> > or by sending [still not implemented] connector's(netlink) command.
> 
> The real question was how would the sysadmin know which is the best load 
> balancer to configure.  Start simple, make one good load balancer, and 
> ponly create a pluggable framework only if a demonstratable need arises.

By reading documentation which will be written some day...
If it has only SW crypto it does not need scheduler at all - only very
simple switch,
but if system has diferent kinds of HW crypto cards, and sysadmin knows
about it's
capabilities it can be better to turn more advanced scheduler on - for
example one 
which can take packet size into account.
It will be a bit slower while searching needed device, but will have big
win 
when doing actual crypto processing.

> > > Overall I think it's a good start.  There are some chicken & egg type 
> > > problems when you don't have GPL drivers, hardware or an existing async 
> > > API, so I'd imagine that this will all continue to evolve:  with more 
> > > hardware we can write/incorporate more drivers, with more drivers we can 
> > > further refine the async support and API etc.
> > 
> > That is true, but I think not all parts of API should be exported as GPL
> > only.
> 
> Why do you think this?

In my current acrypto tree I export all symbols as GPL except
crypto_session_alloc().
Lets greedy scums use at least bits of new functionality. :)

> 
> > > Here are some issues that I noticed:
> > > 
> > > 
> > > What is main_crypto_device?  Is this a placeholder for when there are no 
> > > other devices registered?
> > 
> > main_crypto_device is just virtual device into which all sessions are
> > placed along with specific crypto device queue.
> > it is usefull for cases when some driver decides that HW is broken and
> > marks itself like not working, then scheduler _may_ (simple_lb does not
> > have such feature) and actually _should_ move all sessions that are
> > placed into broken device's queue into other devices or drop them(call
> > callback with special session flag).
> 
> No reason to drop them, just fall back to software.

SW is just one of the crypto devices, consider emebedded system without 
gigantic processor time which can be spent doing crypto processing.
Or if it is asymmetric crypto?
If SW exists and is loaded into acrypto it _will_ be called if needed.


> > > Async & sync drivers need to both be treated as crypto drivers.
> > > 
> > > 
> > > static inline void crypto_route_free(struct crypto_route *rt)
> > > {
> > >         crypto_device_put(rt->dev);
> > >         rt->dev = NULL;
> > >         kfree(rt);
> > > }
> > > 
> > > Why do you set rt->dev to NULL here?  It should not still be referenceable 
> > > at this point.
> > 
> > crypto_route_free() can be called only from crypto_route_del() which
> > unlinks given route thus noone can reference rt->dev(but of course
> > asnyone can have it's private copy, obtained under the lock).
> > Any route access is guarded by route list lock.
> 
> No, if you're about to kfree(rt), you cannot also be able to dereference 
> it.

I mean reference to device,  not route.
Crypto route in this point can be accessed only through crypto_route_del
().
Any routing _must_ be accessed _only_ by crypto_route_* functions from 
crypto_route.h which are always performed with proper locking.

> > > __crypto_route_del() etc:
> > > 
> > > Why are you rolling your own list management and not using the list.h 
> > > functions?
> > 
> > It is more convenient here to use queue like sk_buf does.
> 
> Use list.h or put what you need into list.h.

list.h will never have struct sk_buff_head or any other queueing
primitives.
crypto_route is not a list, it is a queue.

> > > +struct crypto_device_stat
> > > +{
> > > +       __u64                   scompleted;
> > > +       __u64                   sfinished;
> > > ...
> > > 
> > > Please see how networking stats are implemented (e.g. netif_rx_stats) and 
> > > previous netdev discussions on 64-bit counters.  Please only use __u64 and 
> > > similar when exporting structures to userspace.
> > 
> > I use __ prefix for any parameter that is exported to userspace.
> 
> But this structure is not.

struct crypto_device_stat
{
	__u64			scompleted;
	__u64			sfinished;
	__u64			sstarted;
	__u64			kmem_failed;
};

Probably version mismatch...

> > I've seen several attempts to convert network statistic to 64bit values,
> > with backward compatibility it is not an easy task. Let's do not
> > catch this again.
> 
> To clarifu: the best way to do this is to use per-cpu counters of unsigned
> long and add them together in userspace.

It is the fastest way sure, 
current acrypto statistic code is not fast, it takes a stat lock to
guarantee
that fields are changed atomically since some (per
session_crypto_initializer )
of them are used in scheduler.

> 
> - James
-- 
        Evgeniy Polyakov

Crash is better than data corruption -- Arthur Grabowski

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]