NFD

Btw, we should move the Transport::EndpointId typedef to face.hpp as nfd::face::EndpointId (and possibly aliased as nfd::EndpointId, similar to FaceId) and then use EndpointId everywhere instead of uint64_t.

Btw, we should move the Transport::EndpointId typedef to face.hpp as nfd::face::EndpointId (and possibly aliased as nfd::EndpointId, similar to FaceId) and then use EndpointId everywhere instead of uint64_t.

Yes, I agree. Can we have this as a separate issue so that only this change can be done over there?

Separate commit, yes. I don't see the need for a separate redmine issue. This is a mechanical change.

Separate commit, yes. I don't see the need for a separate redmine issue. This is a mechanical change.

Yes. That can be done. But then which issue should the change refer to in the commit message?

You can refer to this issue, but I don't really care, it's unimportant.

Ok. If I move out the typedef for EndpointId from transport.hpp to face.hpp then should I rename the EndpointId remoteEndpoint to uint64_t remoteEndpoint in the Packet subclass under Transport class or keep the typedef uint64_t EndpointId in the transport.hpp too?

Moving the typedef is causing me error even though I did the aliasing in face.hpp, some examples:

../daemon/face/transport.hpp:137:5: error: ‘EndpointId’ does not name a type; did you mean ‘dprintf’?
     EndpointId remoteEndpoint;
.
.
../daemon/face/lp-reassembler.hpp:100:62: error: template argument 2 is invalid
   signal::Signal<LpReassembler, Transport::EndpointId, size_t> beforeTimeout;
.
.
../daemon/face/datagram-transport.hpp:197:6: error: ‘class nfd::face::Transport::Packet’ has no member named ‘remoteEndpoint’
   tp.remoteEndpoint = makeEndpointId(m_sender);

Am I doing something wrong here? If not then I think we should keep the EndpointId in Transport too and just change the description.

Is the new typedef visible in transport.hpp? if not, you can't expect it to work. Also you need to change all Transport::EndpointId to just EndpointId.
To avoid including face.hpp from transport.hpp, it's probably better to keep the EndpointId typedef in transport.hpp but outside the Transport class.

Thanks for the clarification.

In NFD primary thread (i.e. except RIB), replace all usage of IncomingFaceIdTag with IncomingFaceTag.

Please correct me if I'm wrong. Does this mean that in ndn-cxx/lp/tags.hpp,

typedef SimpleTag<uint64_t, 10> IncomingFaceIdTag;

becomes:

typedef SimpleTag<FaceEndpoint, 10> IncomingFaceTag;

Where FaceEndpoint is similar to #4849?

FaceEndpoint itself can be extended to be used as a tag. No need for SimpleTag wrapper.

Junxiao Shi wrote:

FaceEndpoint itself can be extended to be used as a tag. No need for SimpleTag wrapper.

Please elaborate, I don't understand what you want to do. NFD's Face object is meaningless outside NFD code, in library code and tools we use the face ID.

Davide Pesavento wrote:

Junxiao Shi wrote:

FaceEndpoint itself can be extended to be used as a tag. No need for SimpleTag wrapper.

Please elaborate, I don't understand what you want to do. NFD's Face object is meaningless outside NFD code, in library code and tools we use the face ID.

I'm guessing the question is for me. Yes, there's no need to keep Face outside NFD. I just asked to confirm if we want the FaceId and EndpointId together in one tag or have separate tags. From what Junxiao mentioned, we can have:

typedef SimpleTag<std::pair<uint64_t, uint64_t>, 10> IncomingFaceIdEndpointTag;

But to make it better representable, I think he's suggesting to use a class similar to FaceEndpoint. If we follow this, then for the tag, the class should be FaceIdEndpoint with members (FaceId,Endpoint,TypeId).

We still haven't addressed the issue of why other strategies raise segmentation fault error at runtime in unit tests. I saw the logs in here: http://jenkins.named-data.net/job/NFD/OS=Ubuntu-16.04-64bit/6857/console

Not fully understanding the reason. But what I can see, seems like buggy Address is being set.


Address 0x7f1dd52ef880 is located in stack of thread T0 at offset 0 in frame

SUMMARY: AddressSanitizer: stack-use-after-return /usr/include/boost/asio/detail/thread_info_base.hpp:45 boost::asio::detail::thread_info_base::allocate(boost::asio::detail::thread_info_base*, unsigned long)
Shadow bytes around the buggy address:
0x0fe43aa55ec0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
0x0fe43aa55ed0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
0x0fe43aa55ee0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
0x0fe43aa55ef0: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
0x0fe43aa55f00: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
=>0x0fe43aa55f10:[f5]f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
0x0fe43aa55f20: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe43aa55f30: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe43aa55f40: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5
0x0fe43aa55f50: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00
0x0fe43aa55f60: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 00 00 00 00

Junxiao Shi wrote:

For reliability, there are three choices:

• Create one instance of LpReliability for each endpointId.

This seems to be a good compromise for the time being, and shouldn't be too complicated to implement.

Davide Pesavento wrote:

Junxiao Shi wrote:

For reliability, there are three choices:

• Create one instance of LpReliability for each endpointId.

This seems to be a good compromise for the time being, and shouldn't be too complicated to implement.

Does this mean having something similar to a map container for m_reliability to maintain per-endpointId instance?

I am kind of lost here. I understand that an instance of LpReliability can be created with say an std::unordered_map. However, as the LpReliability sends out an Lp packet through the LinkService, with this line in LpReliability::onLpPacketLost(lp::Sequence txSeq) -

m_linkService->sendLpPacket(lp::Packet(newTxFrag.pkt), 0);

This requires to know the EndpointId. If that's the case then LpReliability also needs to be aware of EndpointId. Then why create an LpReliability instance per EndpointId if LpReliability eventually needs to be aware of EndpointId anyway?

Md Ashiqur Rahman wrote:

This requires to know the EndpointId.

Yes, each instance of LpReliability knows its own EndpointId. It's effectively a constant within each LpReliability instance.

Alternatively, GenericLinkService can supply a callback to LpReliability's constructor that LpReliability uses instead of calling sendLpPacket(). In this case, the EndpointId is already bound to the callback, so LpReliability doesn't need to know its EndpointId at all. However, this approach is slightly less performant because it involves an std::function.

If that's the case then LpReliability also needs to be aware of EndpointId.

No, that doesn't mean "being aware of EndpointId". LpReliability blindly passes the provided EndpointId along to the functions that need it, but other than that, no LpReliability logic depends on EndpointId.

Of course you're always free to choose option 2 of note-18, if you think that's easier to implement for you.

I think this is more like a c++ question:
I'm using this for per EndpointId instance of LpReliability in GenericLinkService:

std::unordered_map<EndpointId, LpReliability> m_reliability;

But how should I initialize an instance of LpReliability for an EndpointId? Currently, I am looking to do this:

m_reliability[endpointId] = LpReliability(m_options.reliabilityOptions, this);

However, it's definitely not going to work as LpReliability and also LinkService is noncopyable.

Other than this initialization problem, I think I have a good draft implementation for the desired behavior.

Use unordered_map::emplace().

I tried using this:

m_reliability.emplace(endpointId, LpReliability(m_options.reliabilityOptions, this));

But getting this error:

/usr/include/c++/7/ext/new_allocator.h:136:4: error: no matching function for call to 
‘std::pair<const long unsigned int, nfd::face::LpReliability>::pair(long unsigned int&, nfd::face::LpReliability)’
  { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }

complete log: https://gist.github.com/ashiqopu/2849352e2d1a80e8c6d6496e4567b068

m_reliability.emplace(std::piecewise_construct,
                      std::forward_as_tuple(endpointId),
                      std::forward_as_tuple(m_options.reliabilityOptions, this));

Thanks. worked. I tried the one you provided earlier and was still getting error as I was simply trying to access element as m_reliability[endpointId].Foo() which does not work. Had to use `m_reliability.at(endpointId).Foo(). Compiles ok now. I will push the patch after I fix the unit test.

Md Ashiqur Rahman wrote:

I tried the one you provided earlier and was still getting error

It is the exact same function, you were just using it incorrectly.

I was simply trying to access element as m_reliability[endpointId].Foo() which does not work. Had to use `m_reliability.at(endpointId).Foo().

This is a different problem and not related to emplace.

One possible way to clear LpReliability instance tied to an EndpointId could be to keep the latest timestamp of the instance being used and clear after some amount of inactive time (e.g. 1s or 500ms).

Md Ashiqur Rahman wrote:

One possible way to clear LpReliability instance tied to an EndpointId could be to keep the latest timestamp of the instance being used and clear after some amount of inactive time (e.g. 1s or 500ms).

Why such a short timeout? It seems to me that it could easily be in the order of several seconds or even minutes...

Also, when you destruct and recreate LpReliability for a given endpoint, the sequence number will reset, right? How is the other peer going to react to that?

Davide Pesavento wrote:

Md Ashiqur Rahman wrote:

One possible way to clear LpReliability instance tied to an EndpointId could be to keep the latest timestamp of the instance being used and clear after some amount of inactive time (e.g. 1s or 500ms).

Why such a short timeout? It seems to me that it could easily be in the order of several seconds or even minutes...

This is just an examples. Of course the interval can be set much higher. But could this interval be related to how stable/unstable the link might be? For example, in wired networks where links are more stable, an interval in minutes makes sense. However, in a highly dynamic vehicular network, a higher interval might create/keep a large number of instances. What could be the overhead of having large number of LpReliability Instances? Any thoughts or suggestions?

Also, when you destruct and recreate LpReliability for a given endpoint, the sequence number will reset, right? How is the other peer going to react to that?

Yes. In this case, a naive choice would be to set a large interval before destroying an instance. But then the question of overhead comes again. I am not sure what to do exactly. Maybe attach some sort of signal from the Packet sender side and the peer who is responsible for sending ACKs reacts to those signals?

I have been trying to figure out what's causing the build to fail: http://jenkins.named-data.net/job/NFD/7083/
Looking into the logs, is it possible that std::unordered_map::erase is only removing the pointer to the list that only contains pointer to the actual <key, value> stored in the memory and not clearing the actual values right away?

I found this stackoverflow link as a reference but not sure.

I also looked at boost::unordered_map here. Can I try this instead of std::unordered_map?

Something I don't quite understand in the overall design: how does the link service know if a particular feature (e.g. LpReliability) applies to a given endpoint? For instance, LpReliability doesn't make sense for multi-access links. This used to be easy to check: if the underlying transport is not point-to-point, disable (or don't enable) the reliability feature. After the EndpointId refactoring, the underlying transport may use unicast or multicast depending on the EndpointId, so how can the link service know if LpReliability can be enabled for a given endpoint?

I'll be honest here, I don't know this either. But then again, the changes made are making everything "per endpoint" dependant decision. Maybe Junxiao can provide a better answer to this.