Re: [Etherlab-dev] Rebase of Gavin Lambert patchset 20190904 to current stable-1.5

[Esben Haabendal]

The branches mentioned has been pushed to this repository:
https://gitlab.com/deif-as/ethercat/ethercat

On Tue, Jun 29, 2021 at 2:57 PM Esben Haabendal 
wrote:

> Hi all
>
> As part of a process of aligning a couple of branches based on old
> etherlabmaster versions, I have taken the Gavin Lambert patchset 20190904
> and
> rebased it to current stable-1.5.
>
> There are several changes made on the old default branch, and in the
> patchset that the products rely on, so for now, we cannot simply use
> stable-1.5 as-is.
>
> As the 20190904 patchset is based on a specific commit (hg revision 33b922)
> from the (now deprecated/unsupported) default branch, I rebased the
> patchset
> in 3 steps.
>
> 1. Rebase of the 33b922 revision to the new stable-1.5 branch as of
>20141028 (git commit c02e204fcb5b).  Pushed to branch
>hg-default-33b922-rebased-to-stable-1.5-20141028
>
> 2. Rebase of the branch created in step 1 to latest stable-1.5 commit
>(git commit 1fa5565aa028).  Pushed to branch
>hg-default-33b922-rebased-to-stable-1.5-20210609
>
> 3. Applied the 20190904 patchset (minus stable/*.patch), trying to
>resolve any conflicts found.  Pushed to
>gavin-patchset-20190904-stable-1.5-20210609
>
> All of the branches above are for all practical purposes only meant for
> inspiration/review.  They are not in any to be considered upstream, and
> no merge requests or anything like that will be considered.
>
> Also, the rebased branches have only been very lightly tested as of
> now.  Work on that will be started ASAP.
>
> The question is then.  What, if anything, can and should we do with all
> this?  I believe it will be in everybodys interest to get back to a
> situation where we have a common baseline to work on, or maybe a small
> number of aligned common baselines.
>
> What is acceptable for getting merged to stable-1.5 branch?  I
> assume that any backwards compatible bugfixes is fine.  But what about
> changes that modifies the ioctl() API/ABI?  And what about changes that
> modifies the user-space library API and/or ABI?
>
> If there are changes which cannot be merged to stable-1.5, can we create
> a new branch (something like development-1.6?) to have a common place to
> share this work?  And if we do this, we need to agree on to keep that
> branch aligned with stable-1.5, so we don't end up with another
> "default" branch to be abandoned a few years up the road.
>
> Best regards,
> Esben
>


-- 
Esben Haabendal
Ulstrupvej 7, 9500 Hobro, Denmark
-- 
Etherlab-dev mailing list
Etherlab-dev@etherlab.org
https://lists.etherlab.org/mailman/listinfo/etherlab-dev

[Etherlab-dev] Rebase of Gavin Lambert patchset 20190904 to current stable-1.5

[Esben Haabendal]

Hi all

As part of a process of aligning a couple of branches based on old
etherlabmaster versions, I have taken the Gavin Lambert patchset 20190904 and
rebased it to current stable-1.5.

There are several changes made on the old default branch, and in the
patchset that the products rely on, so for now, we cannot simply use
stable-1.5 as-is.

As the 20190904 patchset is based on a specific commit (hg revision 33b922)
from the (now deprecated/unsupported) default branch, I rebased the patchset
in 3 steps.

1. Rebase of the 33b922 revision to the new stable-1.5 branch as of
   20141028 (git commit c02e204fcb5b).  Pushed to branch
   hg-default-33b922-rebased-to-stable-1.5-20141028

2. Rebase of the branch created in step 1 to latest stable-1.5 commit
   (git commit 1fa5565aa028).  Pushed to branch
   hg-default-33b922-rebased-to-stable-1.5-20210609

3. Applied the 20190904 patchset (minus stable/*.patch), trying to
   resolve any conflicts found.  Pushed to
   gavin-patchset-20190904-stable-1.5-20210609

All of the branches above are for all practical purposes only meant for
inspiration/review.  They are not in any to be considered upstream, and
no merge requests or anything like that will be considered.

Also, the rebased branches have only been very lightly tested as of
now.  Work on that will be started ASAP.

The question is then.  What, if anything, can and should we do with all
this?  I believe it will be in everybodys interest to get back to a
situation where we have a common baseline to work on, or maybe a small
number of aligned common baselines.

What is acceptable for getting merged to stable-1.5 branch?  I
assume that any backwards compatible bugfixes is fine.  But what about
changes that modifies the ioctl() API/ABI?  And what about changes that
modifies the user-space library API and/or ABI?

If there are changes which cannot be merged to stable-1.5, can we create
a new branch (something like development-1.6?) to have a common place to
share this work?  And if we do this, we need to agree on to keep that
branch aligned with stable-1.5, so we don't end up with another
"default" branch to be abandoned a few years up the road.

Best regards,
Esben
-- 
Etherlab-dev mailing list
Etherlab-dev@etherlab.org
https://lists.etherlab.org/mailman/listinfo/etherlab-dev

Re: [etherlab-dev] ec_lock_* vs. ec_ioctl_lock in master/ioctl.c

[Esben Haabendal]

Hi Graeme

Need to get one thing straight first.

RTDM user-space is not the same as "plain" Linux user-space.
I know that you are 100% aware that, but your replies to me seems less
clear about it.

Your 0002-eoe-via-rtdm.patch does not seem usable for "plain" user-space
applications, so does not apply to the applications we support.

>> > These 4 functions are special.  The master should be written (and 
>> > mostly seems to be) in a fashion that between a ecrt_master_activate() 
>> > and ecrt_master_deactivate() the above calls do not require any locks 
>> > to synchronize with the master code, except for the EOE thread which 
>> > uses callbacks.
>> 
>> Well, mostly is the same as not in my world.  If they mostly do not require
>> any locks, they do require locks.
>
> When I say mostly, I mean the vanilla Etherlab version is ok, but the patch
> that added ec_ioctl_lock_up() and ec_ioctl_lock_down() broke this.  I do not
> use this patch.

Do you mean that the patches (and I guess you mean
0017-Master-locks-to-avoid-corrupted-datagram-queue.patch and
0018-Use-call-back-functions.patch) introduces the need for locking?

That does not sound right to me.  As I read them (and your writing in
this thread), the need for locking is there with and without these
patches.  Without them, locking needs to be handled at application
level.  With the patches, the master tries to handle the locking.

And that is a clearly a design change, and one which you clearly does
not agree with.

But if you really mean that these patches broke etherlabmaster (and not
"just" changed the design), would you please explain it a bit more
detailed?

>> As for the EOE thread, I might be overlooking something obvious.  But how
>> are you supposed to use callbacks when using the library API?
>> 
>> As far as I read the code, if you are using EoE and not RTDM, you will
>> always use the standard ec_master_internal_send_cb() and
>> ec_master_internal_receive_cb() callbacks.  See ec_ioctl_activate().
>
> If you do not set EoE callbacks (with ecrt_master_callbacks()) there will be
> NO callbacks once the master goes active.  (It will not use
> ec_master_internal_send_cb() and ec_master_internal_receive_cb().  It will not
> start an EoE processing thread.  You will get a "No EoE processing because of
> missing callbacks!" message in your system log.)

In ec_ioctl_activate() there is:

#ifndef EC_IOCTL_RTDM
ecrt_master_callbacks(master, ec_master_internal_send_cb,
ec_master_internal_receive_cb, master);
#endif

So at least for (non RTDM) user-space applications, that activate the
master with ioctl, EoE callbacks are always set to
ec_master_internal_send_cb() and ec_master_internal_receive_cb().

> If your application is in kernel space then you can specify callback functions
> and then use application level locking.  If your application is in user space,
> then you cannot specify callback functions as the kernel cannot call back into
> user space.  This is why I created my latest patch "0002-eoe-via-rtdm.patch".
> This patch lets you create your own EoE processing thread in your own
> application.  This of course also lets you use application level locking.

But only for RTDM.  The functions added are guarded by

#if !defined(__KERNEL__) && defined(EC_RTDM) && (EC_EOE)

which means that they do not apply when using non-RTDM user-space.

>> And with them, I don't see how you are supposed to do locking at the
>> application level.  I will not be able to specify application level locks
>> that are used by both application and eoe, unless they are implemented
>> directly in the master (kernel).
>> 
>> And with ec_master_eoe_thread() running inside master, you really need to do
>> locking to synchronize access to master->datagram_queue (i.e. the io_sem
>> lock).  If not, you will have race conditions between eoe and 3 of the 4
>> functions:
>
> The io_sem lock is only used by the master when it is idle.  Once you activate
> the master it is up to your application to provide the locking.

Ok.  But I dare to say that it is an open question if that is an design
for etherlabmaster.  I believe it has both pros and cons.

Pro: Application developers can implement synchronization as they find
most optimal for them.

Cons: Application developers need to figure out and implement
synchronization without much (any) help from the etherlabmaster code.
And getting synchronization is IMHO something that is often tricky to
get right (at least for many developers).

>>ecrt_master_send()
>>ecrt_master_receive()
>>ecrt_domain_queue()
>> 
>> So we really do not to do locking in at least these 3 when using EOE.
>> 
>> Or maybe the whole master->datagram_queue should be refactored to something
>> that can be synchronized safely in a lock-free way?
>
> The point of the callback functions is so that you can make sure your
> application does not call any of ecrt_master_receive(), ecrt_master_send(),
> ecrt_master_send_ext(), 

Re: [etherlab-dev] Community contribution

[Esben Haabendal]

Hi Titouan

Titouan Mesot  writes:

> Thanks for your email. I had the same concern when I started working with
> Etherlab. I've also some patch (mostly EoE and SoE) but ... well I don't
> want to add a patchset as it's done now. For me the main question remain how
> to manage starting a new project on github but also keeping the sourceforge
> branch ?

That naturally depends on how Florian decides to work.

Worst-case (i.e. IgH continues to focus on their own work), we can
cherry-pick changes from their repo if/when that benefits our work.

But unless the IgH managed repository starts to accept community
contribution again, or we limit size of our "patchset", and thus limit
the amount of work we allow ourselves to do, it will at some point in
time become less relevant or realistic to cherry-pick from one
repository to the other.  It will end up as two forks.

But by deciding against forking, when not being able to get changes
merged, is practically the same as deciding not to be able to improve
the code.

> Do main contributors (Florian Pose, Philipp Weyer, Gavin ...) have an
> opinion on that ?

I really hope they do, and hope they will participate in the discussion
here.

> Thanks to ask this question,

Thanks for voicing your opinion :)

/Esben
___
etherlab-dev mailing list
etherlab-dev@etherlab.org
http://lists.etherlab.org/mailman/listinfo/etherlab-dev

Re: [etherlab-dev] ec_lock_* vs. ec_ioctl_lock in master/ioctl.c

[Esben Haabendal]

Graeme Foot  writes:

>> Graeme Foot  writes:
>> 
>> > The ec_ioctl_lock_up() and ec_ioctl_lock_down_interruptible() calls 
>> > were added to protect the following functions when multiple 
>> > send/receive loops are
>> > running:
>> > - ecrt_master_send()
>> > - ecrt_master_receive()
>> > - ecrt_master_domain_process()
>> > - ecrt_master_domain_queue()
>> >
>> > In my opinion any locking on these functions should be at the 
>> > application level instead.
>> 
>> Well, I have a different opinion on that.
>> 
>> Implementation of locking is inherently difficult to get right.  You
>> both have to ensure against race conditions while avoiding deadlocks.
>> But you also do not want to block for too long.
>> 
>> While I do acknowledge that for trivial cases where there are only a
>> single application, it is not a big program for that single
>> application to maintain synchronization, I don't think that it is a
>> good solution to let each application developer in the more
>> complicated situations (like multiple independent processes) have to
>> do this without any help from etherlabmaster.  Forcing all (or at
>> least some) application developers to solve this same problem again
>> and again should not be the best we can do.
>
> These 4 functions are special.  The master should be written (and
> mostly seems to be) in a fashion that between a ecrt_master_activate()
> and ecrt_master_deactivate() the above calls do not require any locks
> to synchronize with the master code, except for the EOE thread which
> uses callbacks.

Well, mostly is the same as not in my world.  If they mostly do not
require any locks, they do require locks.

I guess some of the confusion I have is caused by the fact that it is
unclear exactly which functions are allowed to be called between
ecrt_master_activate() and ecrt_master_deactivate(), and which functions
are not.  Do we have such a list?  Or how can I create such a list
exactly?  And would it be possible to enforce this directly in the API,
so if you call a function you are not allowed to call, you get an error
back instead of introducing random breakage by modifying unsynchronized
shared data structures?

As for the EOE thread, I might be overlooking something obvious.  But
how are you supposed to use callbacks when using the library API?

As far as I read the code, if you are using EoE and not RTDM, you will
always use the standard ec_master_internal_send_cb() and
ec_master_internal_receive_cb() callbacks.  See ec_ioctl_activate().

And with them, I don't see how you are supposed to do locking at the
application level.  I will not be able to specify application level
locks that are used by both application and eoe, unless they are
implemented directly in the master (kernel).

And with ec_master_eoe_thread() running inside master, you really need
to do locking to synchronize access to master->datagram_queue (i.e. the
io_sem lock).  If not, you will have race conditions between eoe and 3
of the 4 functions:

   ecrt_master_send()
   ecrt_master_receive()
   ecrt_domain_queue()

So we really do not to do locking in at least these 3 when using EOE.

Or maybe the whole master->datagram_queue should be refactored to something
that can be synchronized safely in a lock-free way?

> And the reason it uses callbacks is because only your application
> knows if it is appropriate to allow the EOE thread to call
> ecrt_master_recieve() and ecrt_master_send_ext() etc at any particular
> time.

I see what you mean.  But I just don't see how it is possible to
accomplish this for user space applications.

> However, if your application has multiple send/receive loops then they need to
> be synchronized with each other (see next comment).  There are a few more
> functions such as the distributed clock calls that are also in the
> send/receive loops.  They also do not require ethercat level locks as they
> should be safe to call between the activate and deactivate.  They also do not
> need application level locking as they should really only be called by one
> send/receive loop per master.  All other ethercat function calls should not be
> in a hard realtime context.

Ok.  But you imply a struct rule for how to design applications doing
EtherCAT communication: "There can only be one send/receive loop per
master".

That might (is not in my case) accepted by all application developers.
I need to continue support for applications that use multiple
send/receive loops for multiple EtherCAT domains, each with different
cycle time.

One solution could be to create a single send/receive loop per master,
and then let the multiple application loops communicate with that
instead of directly with the master.  That might actually be a really
good idea.  But, if that is done, I think it should fit very well as a
general feature of the EtherCAT master.  It should be possible to
implement without any application specific code.



The final "problem" is as 

Re: [etherlab-dev] [PATCH] Fix kmalloc while holding read lock on dev_base_lock

[Esben Haabendal]

Hi

On Tue, 2010-02-16 at 16:12 +0100, Florian Pose wrote: 
 Hi,
 
 On Fri, Feb 12, 2010 at 10:09:12AM +0100, Esben Haabendal wrote:
  Using the ec_generic driver on a standard (ie. non RT_PREEMPT) 2.6.32
  kernel BUGs out on the kmalloc in ec_gen_init_module().
  It might be that this happens to work on UP RT_PREEMPT kernels, but
  AFAIK, kmalloc while holdning rwlock should be GFP_ATOMIC.
  
  /Esben
 
  Binary files etherlabmaster/.hg/dirstate and 
  etherlabmaster.patched/.hg/dirstate differ
  diff -urN etherlabmaster/devices/generic.c 
  etherlabmaster.patched/devices/generic.c
  --- etherlabmaster/devices/generic.c2010-02-11 14:42:46.0 
  +0100
  +++ etherlabmaster.patched/devices/generic.c2010-02-12 
  09:04:49.0 +0100
  @@ -408,7 +408,7 @@
   for_each_netdev(init_net, netdev) {
   if (netdev-type != ARPHRD_ETHER)
   continue;
  -desc = kmalloc(sizeof(ec_gen_interface_desc_t), GFP_KERNEL);
  +desc = kmalloc(sizeof(ec_gen_interface_desc_t), GFP_ATOMIC);
   if (!desc) {
   ret = -ENOMEM;
   read_unlock(dev_base_lock);
 
 many thanks, I will fix it.

Have you found a different way to fix it?  AFAICS, the patch is not
applied to the current tip yet...

/Esben


___
etherlab-dev mailing list
etherlab-dev@etherlab.org
http://lists.etherlab.org/mailman/listinfo/etherlab-dev