Re: [1/1] connector/CBUS: new messaging subsystem. Revision number next.

[Dmitry Torokhov]

On 5/10/05, Evgeniy Polyakov <johnpol@xxxxxxxxxxx> wrote:
> On Tue, 10 May 2005 09:56:45 -0500
> Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> wrote:
> 
> > On 5/10/05, Evgeniy Polyakov <johnpol@xxxxxxxxxxx> wrote:
> > > On Tue, 10 May 2005 01:18:46 -0500
> > > Dmitry Torokhov <dtor_core@xxxxxxxxxxxxx> wrote:
> > >
> > > > - drop cn_dev - there is only one connector;
> > >
> > > There may be several connectors with different socket types.
> > > I use it in my environment for tests.
> > >
> >
> > Why do you need that? u32 ID space is not enough?
> 
> They use different netlink sockets.
> One of them can fail during initialisation, for example...

Are you saying that there more chances successfully allocating 2
netlink sockets than one?

> 
> > > > - simplify cn_notify_request to carry only one range - it simplifies code
> > > >   quite a bit - nothing stops clientd from sending several notification
> > > >   requests;
> > >
> > > ? Nothing stops clients from sending several notification requests now.
> > > I do not see how it simplifies the things?
> > > Feel free to set idx_num and val_num to 1 and you will have the same.
> >
> > Compare the implementaion - without variable-length notification
> > requests the code is much simplier and flexible.
> 
> Hmm, old request had a length, new one - does not.
> But instead of atomically addition of the whole set, one need to send
> couple of events.
> I do not see any profit here.

Smaller kernel code, simpler notification structure, ability to stop
listenig to a certain events without re-registering the rest of the
block.

> > > > - admit that SKBs are message medium and do not mess up with passing around
> > > >   destructor functions.
> > >
> > > You mess up layers.
> > > I do not see why you want it.
> >
> > What layers? Connector core is not an onion, with my patch it is 481
> > line .c file(down from 900+), comments probably take 1/3 of it. We
> > just need to keep it simple, that's all.
> 
> There is low-level medium layer - skbs.
> There is middle-level layer - logical handling attached data and appropriate callback invocation.
> There is high-level layer - various protocols over connector.
> 
> Why any of them should know that others exist?
> Your changes with destructor removing only saved < 10 lines of code, btw.

Yes, line here and line there - all in all couple hundred lines.

> > > Connector with your changes just does not work.
> >
> > As in "I tried to run it and adjusted the application for the new
> > cn_notify_req size and it does not work" or "I am saying it does not
> > work"?
> 
> Second, i.e. "I am saing it does not work".
> With your changes userspace just does not see any message being sent by kernelspace.

Would you care to elaborate on this please. As far as I know
kernel->userspace path was not changed at all.

> 
> > > Dmitry, I do not understand why do you change things in so radical manner,
> > > especially when there are no 1. new interesting features, 2. no performance
> > > improvements, 3. no right design changes.
> >
> > There is a right design change - it keeps the code simple. It peels
> > all the layers you invented for something that does not really need
> > layers - connector is just a simple wrapper over raw netlink.
> 
> That is your opinion.
> 
> Code is already as simple as possible, since it is layered and each
> piece of code is separated from different logical entities.
> 
> > When I look at the code I see the mess of unneeded refcounting, wrong
> > kinds of locking, over-engineering. Of course I want to change it.
> 
> There are no unneded reference counting now.
> Locking was not wrong - you only placed semaphore instead of notify BH lock
> just to have ability to sleep under the lock and send callback addition/removing
> notification with GFP_KERNEL mask,
> and it is probably right change from the first view, as I said,
> but it can be implemented in a different way.

That's what I was referring to as "wring locking". Notifications do
not need to stop bottom handlers, a spinlock or semaphore willl work
just fine. SInce I wanted to allocate memory only after I determined
that it is a new  notify request a semaphore was a better choise.

> > Keep it simple. Except for your testing environment why do you need to
> > have separate netlink sockets? You have an ample ID space. Why do you
> > need separate cn_dev and cn_queue_dev (and in the end have them at
> > all)? Why do you need "input" handler, do you expect to use different
> > handlers? If so which? And why? Do you expect to use other transports?
> > If so which? And why? You need to have justifiction for every
> > additional layer you putting on.
> 
> Sigh.
> It was already answered so many times, as far as I remember...
> Ok, let's do it again.
> 
> Several netlink sockets - if you can see, connector uses NETLINK_ULOG
> for it's notification, you can not use the same socket twice, so you
> may have new device with new socket, or several of them, and system
> may work if one initialisation failed.

Or it may not. Why do you think that having several sockets is safer
than using one? After all, it is just a matter of setting it up and
finally allocating a separate socket number for connector.

> cn_dev is connector device structure, cn_queue_dev - it's queueing part.
> input handler is needed for ability to receive messages.

You chose not to understand my question. Why do you need to have is as
a pointer? Do you expect to have different input handler?

> Couple of next questions can be answered in one reply:
> original connector's idea was to implement notification based
> not on top of netlink protocol, but using ioctls of char device,
> so there could be different connector device, input method,
> transport and so on. Probably I will not implement it thought.

IOW this all is remnants of an older design. You have no plans to
utilize the framework and therefore it is not needed anymore and can
be dropped.

-- 
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/